GNOME Bugzilla – Bug 674952
Provide an app menu
Last modified: 2012-05-16 07:53:47 UTC
See https://live.gnome.org/GnomeGoals/PortToGMenu You can replace the entire menu bar with the app menu. Recommended app menu format: View by: o Script Unicode Block -- Show only glyphs from this font -- Zoom In Zoom Out Normal Size -- Find... -- Help About Character Map Quit 'Snap Columns to Power of Two' seems to be somewhat reproducing the zoom functionality, plus it's a terrible label!
Created attachment 214085 [details] [review] main: Port to GtkApplication Make gucharmap single-window using the GtkApplication API; this is a prerequisite for recent desktop integration features.
Created attachment 214086 [details] [review] window: Inherit from GtkApplicationWindow instead of GtkWindow This is necessary to make use of the new GMenu API.
Created attachment 214087 [details] [review] ui: Use GResource instead of string Using resources for UI descriptions shares the advantages of using embedded strings, with the additional benefit of being translatable.
Created attachment 214088 [details] [review] Port to the new GMenu API Replace the current GtkUIManager/GtkAction based menus with a GMenu/GAction based menubar.
Created attachment 214089 [details] [review] Move menu actions to a single app menu Replace the in-window menubar with a single application menu. Actions that are no longer shown in the menu may still be triggered by their shortcuts.
Comment on attachment 214085 [details] [review] main: Port to GtkApplication (In reply to comment #1) > Created an attachment (id=214085) [details] [review] > main: Port to GtkApplication > > Make gucharmap single-window using the GtkApplication API; this is > a prerequisite for recent desktop integration features. I don't like the manual handling of --help*. Why not just keep the gtk_init_with_args() call in main before creating the application? And then use gtk_application_run (app, 0, NULL); + application = gtk_application_new ("org.gnome.gucharmap", + G_APPLICATION_HANDLES_COMMAND_LINE); Also use G_APPLICATION_NON_UNIQUE.
Comment on attachment 214087 [details] [review] ui: Use GResource instead of string (In reply to comment #3) > Created an attachment (id=214087) [details] [review] > ui: Use GResource instead of string > > Using resources for UI descriptions shares the advantages of using > embedded strings, with the additional benefit of being translatable. The in-source string does too, and there's no translation needed for the uimanager string... but I see this is just a prereq for later changes. +gucharmap-resources.c: gucharmap.gresource.xml $(UI_FILES) + glib-compile-resources --target=$@ --sourcedir=$(srcdir) \ + --generate-source --c-name gucharmap $< + +gucharmap-resources.h: gucharmap.gresource.xml + glib-compile-resources --target=$@ --sourcedir=$(srcdir) \ + --generate-header --c-name gucharmap $< Use a common rule, and use the --generate-dependencies option instead of hardcoding the deps. (See e.g. aisleriot/src/Makefile.am for an example how this works). +UI_FILES = gucharmap-ui.xml @@ -207,6 +221,8 @@ EXTRA_DIST = \ + $(UI_FILES) \ Use dist_DATA = ... instead. @@ -123,6 +125,8 @@ gucharmap_SOURCES = \ gucharmap-mini-fontsel.h \ gucharmap-print-operation.c \ gucharmap-print-operation.h \ + gucharmap-resources.c \ + gucharmap-resources.h \ Put these into nodist_gucharmap_SOURCES. +#define UI_RESOURCE "/org/gnome/gucharmap/gucharmap-ui.xml" /org/gnome/Charmap/... (and same change in the .gresource.xml file).
Comment on attachment 214088 [details] [review] Port to the new GMenu API (In reply to comment #4) > Created an attachment (id=214088) [details] [review] > Port to the new GMenu API > > Replace the current GtkUIManager/GtkAction based menus with a > GMenu/GAction based menubar. + <attribute name="accel"><Primary>w</attribute> Why change the accel? <control> is standard here. +#define UI_RESOURCE "/org/gnome/gucharmap/gucharmap-menus.ui" /org/gnome/Charmap/... and same change in the .gresource.xml file. Also I noticed in the first patch: + window = gucharmap_window_new (); + gtk_window_set_application (GTK_WINDOW (window), + GTK_APPLICATION (application)); I'd prefer if you'd change gucharmap_window_new() to the the GApplication as a param. + <file>gucharmap-menus.ui</file> Use preprocess="xml-stripblanks", add a configure check for XMLLINT and pass XMLLINT=$(XMLLINT) to the glib-compile-resources invocation. (See aisleriot/src/Makefile.am and aisleriot/configure.ac for an example how to do this.)
Comment on attachment 214089 [details] [review] Move menu actions to a single app menu (In reply to comment #5) > Created an attachment (id=214089) [details] [review] > Move menu actions to a single app menu > > Replace the in-window menubar with a single application menu. Actions > that are no longer shown in the menu may still be triggered by their > shortcuts. Now we come to the most problematic patch. I don't like this one bit. I'm ok with using the app menu when the shell shows it, but gucharmap should retain its traditional menubar when NOT using the shell. Ie., check for gtk-shell-shows-app-menu gtksetting, and only do the app menu stuff when it's TRUE. On the code side, the move of the code that belongs in GucharmapWindow to main.c is bad. If it really needs to be called from outside, add public API to gucharmap-window.h instead of moving internal code outside gucharmap-window.c.
BTW, you forgot to add your © to all files you nontrivially changed. Plus, the .gresource.xml file needs a full copyright header (see aisleriot/src/aisleriot.gresource.xml.in for an example).
Sorry, I mean /org/gnome/charmap/... , not Charmap.
(In reply to comment #6) > + application = gtk_application_new ("org.gnome.gucharmap", > + G_APPLICATION_HANDLES_COMMAND_LINE); > > Also use G_APPLICATION_NON_UNIQUE. I understand there are applications which want to opt out of the default single-instance behavior, but to be honest, I don't quite see how that applies to utility applications like gucharmap. (In reply to comment #8) > + <attribute name="accel"><Primary>w</attribute> > > Why change the accel? <control> is standard here. As I understand it, the idea behind <Primary> is that it resolves to the "common" platform modifier (which is control here, but e.g. command on Mac OS).
(In reply to comment #12) > (In reply to comment #6) > > + application = gtk_application_new ("org.gnome.gucharmap", > > + G_APPLICATION_HANDLES_COMMAND_LINE); > > > > Also use G_APPLICATION_NON_UNIQUE. > > I understand there are applications which want to opt out of the default > single-instance behavior, but to be honest, I don't quite see how that applies > to utility applications like gucharmap. Because there's no reason to limit this to one charmap window, nor to one process (no shared profile / resources or anything). > (In reply to comment #8) > > + <attribute name="accel"><Primary>w</attribute> > > > > Why change the accel? <control> is standard here. > > As I understand it, the idea behind <Primary> is that it resolves to the > "common" platform modifier (which is control here, but e.g. command on Mac OS). OK.
(In reply to comment #6) > I don't like the manual handling of --help*. Why not just keep the > gtk_init_with_args() call in main before creating the application? Because (unique) GtkApplications and GOption handling don't go very well together. For `gucharmap & gucharmap --help`, there are two options: (1) use the default --help handling, and have *both* instances exit (2) handle --help manually, and have the first instance show the output from the second invocation Neither of the options is too nice, with (2) being the lesser evil. Of course none of this applies to a non-unique application, so I reverted that part.
Created attachment 214158 [details] [review] main: Port to GtkApplication Make gucharmap a non-unique GtkApplication; this is a prerequisite for recent desktop integration features.
Created attachment 214159 [details] [review] window: Inherit from GtkApplicationWindow instead of GtkWindow This is necessary to make use of the new GMenu API.
Created attachment 214160 [details] [review] ui: Use GResource instead of string (In reply to comment #7) > @@ -207,6 +221,8 @@ EXTRA_DIST = \ > + $(UI_FILES) \ > > Use dist_DATA = ... instead. I've assumed that you meant dist_noinst_DATA.
Created attachment 214161 [details] [review] Port to the new GMenu API Replace the current GtkUIManager/GtkAction based menus with a GMenu/GAction based menubar.
Created attachment 214162 [details] [review] Add an application menu Merge "File" and "Help" into the application menu. When the menu is displayed by the Shell, items from "View" are also shown in the app menu and the in-window menubar is hidden. (In reply to comment #9) > I don't like this one bit. I'm ok with using the app menu when the shell shows > it, but gucharmap should retain its traditional menubar when NOT using the > shell. Ie., check for gtk-shell-shows-app-menu gtksetting, and only do the app > menu stuff when it's TRUE. Are you OK with a minimal (File+Help) app menu as first item in the menubar? The app menu cannot be set/unset or shown/hidden, so other solutions are likely to get hackish (though of course possible if desired). > On the code side, the move of the code that belongs in GucharmapWindow to > main.c is bad. If it really needs to be called from outside, add public API to > gucharmap-window.h instead of moving internal code outside gucharmap-window.c. OK, I just mapped from app actions to the corresponding window actions ...
Comment on attachment 214158 [details] [review] main: Port to GtkApplication + application = gtk_application_new ("org.gnome.gucharmap", "org.gnome.Charmap" since that's what our gsettings schema uses too. With that changed, ok to commit.
Comment on attachment 214159 [details] [review] window: Inherit from GtkApplicationWindow instead of GtkWindow This one is ok. However you need to invert he order of this patch and the "main: Port to GtkApplication" one ,because that one uses the "application" property on GucharmapWindow which only exists once it's a GtkApplicationWindow.
Comment on attachment 214160 [details] [review] ui: Use GResource instead of string +AC_PATH_PROG([GLIB_COMPILE_RESOURCES],[glib-compile-resources],[false]) +if test "$GLIB_COMPILE_RESOURCES" = "false"; then + AC_MSG_ERROR([glib-compile-resources not found]) +fi Need to add +AC_ARG_VAR([GLIB_COMPILE_RESOURCES],[the glib-compile-resources programme])
(In reply to comment #21) > (From update of attachment 214159 [details] [review]) > However you need to invert he order of this patch and the > "main: Port to GtkApplication" one ,because that one uses the "application" > property on GucharmapWindow which only exists once it's a GtkApplicationWindow. No, it's a property of GtkWindow[0] [0] http://git.gnome.org/browse/gtk+/tree/gtk/gtkwindow.c#n1026
Comment on attachment 214161 [details] [review] Port to the new GMenu API +AC_PATH_PROG([XMLLINT],[xmllint],[false]) +if test "$XMLLINT" = "false"; then + AC_MSG_ERROR([xmllint not found]) +fi Need to add +AC_ARG_VAR([XMLLINT],[the xmllint programme]) before the AC_PATH_PROG.
(In reply to comment #23) > (In reply to comment #21) > > (From update of attachment 214159 [details] [review] [details]) > > However you need to invert he order of this patch and the > > "main: Port to GtkApplication" one ,because that one uses the "application" > > property on GucharmapWindow which only exists once it's a GtkApplicationWindow. > > No, it's a property of GtkWindow[0] > > [0] http://git.gnome.org/browse/gtk+/tree/gtk/gtkwindow.c#n1026 Hmm you're right. This is weird... anyway so there's no change required in the patch order.
Comment on attachment 214162 [details] [review] Add an application menu (In reply to comment #19) > Created an attachment (id=214162) [details] [review] > Add an application menu > > Merge "File" and "Help" into the application menu. When the menu is > displayed by the Shell, items from "View" are also shown in the app > menu and the in-window menubar is hidden. > > Are you OK with a minimal (File+Help) app menu as first item in the menubar? > The app menu cannot be set/unset or shown/hidden, so other solutions are likely > to get hackish (though of course possible if desired). Since the File menu was just Close window, I think it's ok to have this 'app' menu contain that and the Help + About actions. If the shell is not running, this is shown as the first menu item in the app window's menubar, right? + <attribute name="label" translatable="yes">Quit</attribute> + <attribute name="action">app.quit</attribute> + <attribute name="accel"><Primary>q</attribute> + </item> Should be Close, not Quit; with <primary>W as accel, not Q. Also, all the 'app menu' items seem to be missing accels? I guess the shell app menu doesn't show accels, but they should be there for the non-shell in-menubar appmenu case, right? So should be _Help, _About, _Close. + g_object_bind_property (gtk_settings_get_default (), Just as a matter of style I prefer to use gtk_widget_get_settings (GTK_WIDGET (window)) here. + if (show_app_menu) + { + g_menu_append (menu, _("Script"), "app.group-by::script"); + g_menu_append (menu, _("Unicode Block"), "app.group-by::block"); + } Couldn't these items be always present, but just invisible in the non-shell case? + g_signal_connect (gtk_settings_get_default (), + "notify::gtk-shell-shows-app-menu", + G_CALLBACK (update_shell_app_menu), application); + update_shell_app_menu (gtk_settings_get_default (), NULL, application); Can this setting actually change during the app lifetime ? Ok to commit after fixing the nits above; the rest are just questions out of curiosity. Thanks for the patches!
(In reply to comment #26) > Since the File menu was just Close window, I think it's ok to have this 'app' > menu contain that and the Help + About actions. If the shell is not running, > this is shown as the first menu item in the app window's menubar, right? Yup (or as first item in Unity's global menu). > Should be Close, not Quit; with <primary>W as accel, not Q. Hmm, OK to do "Quit" in the shell and "Close" otherwise? > Also, all the 'app menu' items seem to be missing accels? I guess the shell app > menu doesn't show accels, but they should be there for the non-shell in-menubar > appmenu case, right? So should be _Help, _About, _Close. Right, I'll add that. > + if (show_app_menu) > + { > + g_menu_append (menu, _("Script"), "app.group-by::script"); > + g_menu_append (menu, _("Unicode Block"), "app.group-by::block"); > + } > > Couldn't these items be always present, but just invisible in the non-shell > case? Unfortunately no, GMenu doesn't support the concept of visibility. > + g_signal_connect (gtk_settings_get_default (), > + "notify::gtk-shell-shows-app-menu", > + G_CALLBACK (update_shell_app_menu), application); > + update_shell_app_menu (gtk_settings_get_default (), NULL, application); > > Can this setting actually change during the app lifetime ? Yes, but it's a fringe case: metacity --replace gnome-shell --replace
(In reply to comment #27) > Hmm, OK to do "Quit" in the shell and "Close" otherwise? Fine with me. > > + if (show_app_menu) > > + { > > + g_menu_append (menu, _("Script"), "app.group-by::script"); > > + g_menu_append (menu, _("Unicode Block"), "app.group-by::block"); > > + } > > > > Couldn't these items be always present, but just invisible in the non-shell > > case? > > Unfortunately no, GMenu doesn't support the concept of visibility. Ok :-( — [makes me rather wary of porting my other modules to GMenu when it's clearly an inferior solution compared to GtkUIManager...] > > + g_signal_connect (gtk_settings_get_default (), > > + "notify::gtk-shell-shows-app-menu", > > + G_CALLBACK (update_shell_app_menu), application); > > + update_shell_app_menu (gtk_settings_get_default (), NULL, application); > > > > Can this setting actually change during the app lifetime ? > > Yes, but it's a fringe case: > > metacity --replace > gnome-shell --replace I see. So IMHO we don't have to support that case, but since you already coded it, it's up to you if you want to simplify this or not :-)
Attachment 214158 [details] pushed as ae0abb2 - main: Port to GtkApplication Attachment 214159 [details] pushed as 5291ecc - window: Inherit from GtkApplicationWindow instead of GtkWindow Attachment 214160 [details] pushed as af84d39 - ui: Use GResource instead of string Attachment 214161 [details] pushed as 9375ce0 - Port to the new GMenu API Attachment 214162 [details] pushed as 6e2a438 - Add an application menu
Thanks Florian!