GNOME Bugzilla – Bug 129779
[ui-review] remove separators from dialogs in gedit-plugins
Last modified: 2019-03-23 20:32:19 UTC
According to the HIG dialogs should not have separators. The separator needs to be removed from the following dialogs: (main app) - Open Location - Print - Find - Replace - Go To Line - Preferences - Configure Plugin (plugins) - Sort - Set Language - Document Statistics - Run Command
Here it is a first round... remove separator from - open location - find - replace - go to line more patches when I'm bored again ;-) Note that the patch also contains an ortogonal cleanup: gedit_button_with_stock_image is removed since there is an analougus function in eel, beside in the only two places where that function was used, using gedit_dialog_add_button is nicer. Ok to commit (with changelog) or do you prefer to wait for a patch with all the dialogs?
Created attachment 22800 [details] [review] remove separators
While at it, Paolo Maggi suggested to convert warnings to gedit_warning(). The following patch includes such suggestion. Note that gedit_warning was changed to take varargs, so all the callers are also fixed up in the patch.
Created attachment 22825 [details] [review] updated
Thanks for the patch. I have some comments: > + window = GTK_WINDOW (bonobo_mdi_get_active_window > + (BONOBO_MDI (gedit_mdi))); > + Use the gedit_get_active_window () function here. Part of the current code is using bonobo_mdi_get_active_window since it was written before bonobo_mdi_get_active_window utility function was introduced. > + if (!gui) > + { > + /* Translators: %s is a file path */ > + gedit_warning (window, > + _("Could not find \"%s\", reinstall gedit.\n"), > + GEDIT_GLADEDIR "goto-line.glade2"); Please, remove the \n at the end of the string. > Index: gedit/dialogs/gedit-dialog-replace.c > =================================================================== > RCS file: /cvs/gnome/gedit/gedit/dialogs/gedit-dialog-replace.c,v > retrieving revision 1.31 > diff -u -p -r1.31 gedit-dialog-replace.c > --- gedit/dialogs/gedit-dialog-replace.c 29 Dec 2003 23:06:14 -0000 1.31 > +++ gedit/dialogs/gedit-dialog-replace.c 1 Jan 2004 20:22:42 -0000 > @@ -238,13 +238,17 @@ dialog_replace_get_dialog (void) > GladeXML *gui; > GtkWindow *window; > GtkWidget *content; > - GtkWidget *button; > GtkWidget *replace_with_label; > > gedit_debug (DEBUG_SEARCH, ""); > > + window = GTK_WINDOW (bonobo_mdi_get_active_window > + (BONOBO_MDI (gedit_mdi))); > + Use the gedit_get_active_window () function here. > + /* Translators: %s is a file path */ > + gedit_warning (window, > + _("Could not find \"%s\", reinstall gedit.\n"), > + GEDIT_GLADEDIR "replace.glade2"); Remove \n at the end of the string > + /* Translators: %s is a file path */ > + gedit_warning (window, > + _("Could not find the required widgets inside \"%s\".\n"), > + GEDIT_GLADEDIR "replace.glade2"); Remove \n at the end of the string > @@ -381,8 +378,13 @@ dialog_find_get_dialog (void) > > gedit_debug (DEBUG_SEARCH, ""); > > + window = GTK_WINDOW (bonobo_mdi_get_active_window > + (BONOBO_MDI (gedit_mdi))); > + Use the gedit_get_active_window () function here. > + gedit_warning (window, > + _("Could not find \"%s\", reinstall gedit.\n"), > + GEDIT_GLADEDIR "replace.glade2"); Remove \n at the end of the string > + /* Translators: %s is a file path */ > + gedit_warning (window, > + _("Could not find the required widgets inside \"%s\".\n"), > + GEDIT_GLADEDIR "replace.glade2"); Remove \n at the end of the string > Index: gedit/dialogs/gedit-dialog-uri.c > =================================================================== > RCS file: /cvs/gnome/gedit/gedit/dialogs/gedit-dialog-uri.c,v > retrieving revision 1.11 > diff -u -p -r1.11 gedit-dialog-uri.c > --- gedit/dialogs/gedit-dialog-uri.c 6 Jun 2003 16:26:08 -0000 1.11 > +++ gedit/dialogs/gedit-dialog-uri.c 1 Jan 2004 20:22:42 -0000 > @@ -70,18 +70,20 @@ dialog_open_uri_get_dialog (void) > if (dialog != NULL) > return dialog; > > > + window = GTK_WINDOW (bonobo_mdi_get_active_window > + (BONOBO_MDI (gedit_mdi))); Use the gedit_get_active_window () function here. OK... as you probably have understood... always use gedit_get_active_window instead of bonobo_mdi_get_active_window (BONOBO_MDI (gedit_mdi)) and don't put \n in the warning strings. hmmm... I should introduce a couple of #defines as #define MISSING_FILE N_("Could not find \"%s\". Please, reinstall gedit."), #define MISSING_WIDGETS N_("Could not find the required widgets inside \"%s\". Please, reinstall gedit."), or #define MISSING_FILE N_("<bold>Could not find \"%s\".</bold>\n\nPlease, reinstall gedit."), #define MISSING_WIDGETS N_("<bold>Could not find the required widgets inside \"%s\".</bold>\n\nPlease, reinstall gedit."), @@ -686,9 +668,7 @@ gedit_show_page_setup_dialog (GtkWindow g_return_if_fail (parent != NULL); dialog = get_page_setup_dialog (parent); - - if (dialog == NULL) - return; + g_return_if_fail (dialog != NULL); hmm... I'm not sure if using g_return_if_fail in this case is right. It should be used only to catch programming errors and not error conditions like in this case. You have made the same "error" in other places of the patch like for example: > gedit_dialog_goto_line (void) > { > @@ -227,17 +225,7 @@ gedit_dialog_goto_line (void) > gedit_debug (DEBUG_SEARCH, ""); > > dialog = dialog_goto_line_get_dialog (); > - if (dialog == NULL) { > - g_warning ("Could not create the Goto Line dialog"); > - return; > - } > - > - gtk_window_set_transient_for (GTK_WINDOW (dialog->dialog), > - GTK_WINDOW > - (bonobo_mdi_get_active_window > - (BONOBO_MDI (gedit_mdi)))); > - > - gtk_widget_grab_focus (dialog->entry); > + g_return_if_fail (dialog); > if (!GTK_WIDGET_VISIBLE (dialog->dialog)) > gtk_widget_show_all (dialog->dialog); Please, commit the patch when you will have fixed the little problems I have pointed out. Thanks again for your great work. It is really appreciated.
Thanks for the suggestions. Where would be the best place to put the #define MISSING_FILE? With regard to the last comment (the one about g_return_if_fail), note the the warning is already displayed in the called function (*_get_dialog), that's way I removed > - if (dialog == NULL) { > - g_warning ("Could not create the Goto Line dialog"); > - return; While in the case where I did - if (dialog == NULL) - return; + g_return_if_fail (dialog != NULL); is because of consistency with all the other similar functions in the other dialogs; actually using if (dialog == NULL) return; everywhere would also be ok because as I said the warning is displayed in the called function, but to stay on the safe side (I don't like to blindly rely on the called function) I preferred using g_return_if_fail. Agreed?
The MISSING_FILE constants may be put in gedit-utils.h or in a new ad-hoc header file. g_return_*_if_fail must be used only to check the state consistence of the program and not to display error messages to the user. Since in our case the error message is displayed by the called function and the errors can only be caused by missing or corrupted files, then if (dialog == NULL) return; should be used.
Committed patch below.
Created attachment 22838 [details] [review] committed patch
Great! Closing
well, reopening. There are still dilogs with the separator.
Here it is the remaing patch: should fix all the dialogs in gedit, except the ones I missed ;-) (e.g. there may still be some error dialogs somewhere) Note that the print dialog comes with libgnomeprintui and is fixed in HEAD. Once this is applied this bug can either be closed or moved against gedit-plugins.
Created attachment 22937 [details] [review] remaining dialogs
Please, commit the patch and change the summary to "Remove separators from dialogs in gedit-plugins" and components to Plugins Thanks.
Patch committed, moving to gedit-plugins
Given the current state of gedit plugins, I don't think there is much point keeping this open.