Comment 19 for bug 298894

Revision history for this message
Steven Sheehy (steven-sheehy) wrote :

Patch review:
1) This should've been a new bug since you basically rewrote the fav hub dialog.
2) Not sure what was "un-GTK" about the old dialog, but the new one still needs some improvement. Please revert to the old style of having a GtkTable inside the GtkFrame. If you don't like the shadow around the GtkFrame you can turn it off, no need to replace the entire thing with one big table. With your approach it leaves a bunch of placeholders in the glade and using an entire row as a section label is not a pretty solution. You can still can still align the text box labels on the left using the old style.
3) First time you open fav hub dialog "Override default encoding" is not checked yet the encoding box is still editable. Checking and un-checking it becomes un-editable. Value inside box should be the "Default hub encoding".
4) When "Override default encoding" is not checked and the box is not editable, if you change the "Default hub encoding" in the preferences it does not get changed in the fab hubs dialog until you check "Override default encoding" again.
5) What is the purpose of putting N_() instead of _()? Can these strings not be translated until runtime?
6) Not an issue with your code since it was missing before, but can you please add params["Password"] to saveParams using the same logic to populate it from getFavHubParams_client? Without it the treeview doesn't get updated after editing.