GNOME Bugzilla – Bug 668118
the big appmenu switcheroo
Last modified: 2012-01-18 22:26:40 UTC
We need 4 changes: - finish dropping the support from menubars from GLib - add them into Gtk, migrating to xproperties - wire the xproperties into mutter - fix the shell The move to xprops is for two main reasons: 1) mutter can load a whole whack of x properties very quickly using the pipelining tricks it has, so my initial aversion to making a lot of properties was misplaced. 2) using DBus becomes more difficult because it would involve splitting the definition of the dbus interface between glib and gtk and gdbus doesn't really work like that.
Created attachment 205466 [details] [review] GApplication: drop support for appmenu/menubars This has been moved over to GtkApplication now.
Created attachment 205467 [details] [review] move menus over from GLib App menu and menubar are now properties of GtkApplication and their bus location is exported using X window properties.
Created attachment 205468 [details] [review] adjust to new Gtk properties for app menu
Created attachment 205484 [details] [review] Adjust to gtk/mutter changes for Application API
this patch series sometimes causes the shell to freeze up for a few seconds at a time when opening windows that have app menus on them. i'm not entirely sure why and i'm not in a position to debug it at this very moment.
No objection in principle to moving the entire menu business to gtkapplication. Using X properties more means that other dbus-using backends (wayland...) will need a more standalone implementation for menu support - but that is likely the case anyway. We should of course debug the freeze...
Review of attachment 205466 [details] [review]: Sure
Review of attachment 205468 [details] [review]: ::: src/meta/window.h @@ +104,1 @@ Is it really right to introduce _gtk_ in the api here ? We still hope for this to get wider adoption, right ?
Review of attachment 205467 [details] [review]: ::: gtk/gtkapplication.c @@ +207,3 @@ + /* keep trying until we get a working name... */ + for (i = 0; *id == 0; i++) + { Do we need to do something in publish_menu to make sure that the window properties get updated ? Or are there usage restrictions that prevent this from ever happening while the window is 'live' ? If so, a small comment somewhere would be nice
Review of attachment 205468 [details] [review]: ::: src/meta/window.h @@ +104,1 @@ There are two reasons for using 'gtk' here. First is that we probably should have *some* namespace for these properties on X. I considered (for probably too long) the various namespaces we could use and after some discussion on IRC the feeling was probably just that I should go with 'gtk' for now and switch to something else later if we can standardise (requiring a lockstep upgrade). If mutter has API guarantees (does it?) then perhaps we should be more careful about naming these public header APIs than we are about naming the X properties. Second reason: the DBus protocol is a private feature of GLib and GLib is in the 'org.gtk.' world. You absolutely have to use GLib in order for any of these calls to have any meaning at all (except perhaps the application id) -- so it's a bit of an advertisement of that fact.
Review of attachment 205467 [details] [review]: ::: gtk/gtkapplication.c @@ +207,3 @@ + /* keep trying until we get a working name... */ + for (i = 0; *id == 0; i++) + { 6 properties are in play here and I have some patches coming that will restrict when they can be changed: - app id: the app-id is construct-only on the application and it will not be possible to change the application after the window is realized - unique bus name: this will not change as long as the session bus is held open (as it is) - application path: ditto (effectively) - window path: it's determined here and won't change while realized - app menu path: it will not be permitted to change this on the app while the app has any windows associated - menubar path: ditto
Review of attachment 205484 [details] [review]: Only one minor comment; the code size reduction is definitely nice. ::: src/shell-app.c @@ -188,3 @@ -const char * -shell_app_get_dbus_id (ShellApp *app) Hm, was that really unused? git grep thinks so...but I swear something in the JS was using it. Maybe we removed it.
Review of attachment 205484 [details] [review]: ::: src/shell-app.c @@ -188,3 @@ -const char * -shell_app_get_dbus_id (ShellApp *app) I went through more or less the same thing... it may make sense to export (and use this) but then it should probably just be called "application ID". In general, I think it would be pretty cool if the shell moved toward using the namespace application identifiers, where available, in preference to the wmclass/desktopfile/argv[0] approach.
Some testing confirms that the shell freezes happen for me without my patch applied.
Review of attachment 205466 [details] [review]: ok then
Review of attachment 205467 [details] [review]: ok, getting these rules in a comment somewhere (or even the docs) would be nice, but I understand that there's followup patches to that effect
Comment on attachment 205466 [details] [review] GApplication: drop support for appmenu/menubars Attachment 205466 [details] pushed as 54b986d - GApplication: drop support for appmenu/menubars
Comment on attachment 205467 [details] [review] move menus over from GLib Attachment 205467 [details] pushed as 60317cb - move menus over from GLib
(In reply to comment #16) > nice, but I understand that there's followup patches to that effect bug 668203 for that.
Review of attachment 205468 [details] [review]: Looks fine.
Review of attachment 205484 [details] [review]: Looks fine then.
Attachment 205468 [details] pushed as 1772a2a - adjust to new Gtk properties for app menu