GNOME Bugzilla – Bug 656747
GNOME Shell Extension Integration
Last modified: 2011-08-26 23:01:36 UTC
Add support for disabling/enabling GNOME Shell Extensions from the fail whale dialog.
Created attachment 194041 [details] [review] gsm: Add a flag to show the fail whale dialog on request
Created attachment 194042 [details] [review] gsm: Add debug mode to fail whale dialog The debug mode lets you crudely debug the fail whale dialog in a window
Created attachment 194043 [details] [review] gsm: Add a new system to manage and get information about Shell Extensions This functionality will be used to allow the user to enable and disable GNOME Shell Extensions in the fail whale dialog after a crash. WIP: This has a bunch of debugging code because gsettings is screwing up. I don't know why.
Created attachment 194044 [details] [review] gsm: Add a simple tool to dump the properties of Shell Extensions
Created attachment 194045 [details] [review] gsm: Add a UI to the fail whale for enabling and disabling Shell Extensions When the session puts up the fail whale after a failed restart, the user now has the opportunity to disable/enable extensions.
Created attachment 194049 [details] [review] gsm: Add a new system to manage and get information about Shell Extensions This functionality will be used to allow the user to enable and disable GNOME Shell Extensions in the fail whale dialog after a crash. Remove debugging code, the problem I was having was local to my environment.
Created attachment 194115 [details] [review] gsm: Add a UI to the fail whale for enabling and disabling Shell Extensions When the session puts up the fail whale after a failed restart, the user now has the opportunity to disable/enable extensions. s/uncompatible/incompatible/
Created attachment 194136 [details] [review] gsm: Add a new system to get information and frob Shell Extensions This functionality will be used to allow the user to enable and disable GNOME Shell Extensions in the fail whale dialog after a crash. Use explicit whitelist, see: bug #654770
Review of attachment 194041 [details] [review]: You should update doc/man/gnome-session.1 and document the new argument there. ::: gnome-session/main.c @@ +281,3 @@ { "failsafe", 'f', 0, G_OPTION_ARG_NONE, &failsafe, N_("Do not load user-specified applications"), NULL }, { "version", 0, 0, G_OPTION_ARG_NONE, &show_version, N_("Version of this application"), NULL }, + { "i-love-whale", 0, 0, G_OPTION_ARG_NONE, &please_fail, N_("Show the fail whale dialog for testing"), NULL }, --i-love-whale seems to be a little on the 'overly cute' side. Maybe just --whale ? But not a big deal
Review of attachment 194042 [details] [review]: When I try this, and hit 'Logout', I get Gtk-Message: Failed to load module "canberra-gtk-module" ** (gnome-session:5788): DEBUG: GsmManager: Logout called ** (gnome-session:5788): CRITICAL **: gsm_manager_logout: assertion `GSM_IS_MANAGER (manager)' failed (gnome-session:5788): GLib-GObject-WARNING **: invalid unclassed pointer in cast to `GtkWindow' Bus error We should just not try to call logout in debug mode, and just gtk_main_quit() in that case ::: gnome-session/gsm-fail-whale-dialog.c @@ +136,3 @@ + g_object_unref (priv->extensions); + priv->extensions = NULL; + } This hunk should be in a later patch - it breaks the build here, since there is no priv->extensions.
Review of attachment 194044 [details] [review]: It feels wierd to add code for dumping _gnome-shell_ extensions to _gnome-session_. Why should this not live in the shell ? Also, gsm-shell-extensions.h is missing here.
Review of attachment 194136 [details] [review]: This patch needs to be a few steps up, since earlier patches already include gsm-shell-extensions.h
Created attachment 194842 [details] [review] gsm: Add a flag to show the fail whale dialog on request
Created attachment 194843 [details] [review] gsm: Add debug mode to fail whale dialog The debug mode lets you crudely debug the fail whale dialog in a window
Created attachment 194844 [details] [review] gsm: Add a new system to get information and frob Shell Extensions This functionality will be used to allow the user to enable and disable GNOME Shell Extensions in the fail whale dialog after a crash.
Created attachment 194846 [details] [review] gsm: Add a simple tool to dump the properties of Shell Extensions This is just a simple test for the GsmShellExtensions code while I was developing it -- we don't have to land it.
Created attachment 194847 [details] [review] gsm: Add a UI to the fail whale for enabling and disabling Shell Extensions When the session puts up the fail whale after a failed restart, the user now has the opportunity to disable/enable extensions.
Looking at the dialog itself, it needs some fine-tuning: 'A problem has occurred and the system can't recover. Some extensions may be incompatible with GNOME. Please log out and try again.' This needs to be clearer about a) What extensions are we talking about ? b) It should explain that you can turn off extensions before hitting the log out button to try again
Oh, and total nit pick: the icon shows up low-res in alt-tab
And while we are at it, we should figure out the theming problem with the prelight of the log out button.
Review of attachment 194844 [details] [review]: ::: gnome-session/gsm-shell-extensions.c @@ +46,3 @@ + + if (extension->name != NULL) + g_free (extension->name); g_free is NULL-safe, so do away with those ifs @@ +57,3 @@ + * + * Returns: (transfer none) + */ I think we don't want those empty doc comments here. They just clutter things up, and its not like this code is getting used from bindings where the annotations might justify it. @@ +277,3 @@ +{ + self->priv = SHELL_EXTENSIONS_PRIVATE (self); + self->priv->settings = g_settings_new (SHELL_SCHEMA); Uh oh. This silently adds a hard gnome-shell dep to gnome-session. I think that is going to be problematic for a bit. If we have to poke into the shells settings space, we should do it in a way that doesn't crash your session manager if the shell schema is not installed. ::: gnome-session/gsm-shell-extensions.h @@ +79,3 @@ +GsmShellExtension * gsm_shell_extensions_get_for_uuid (GsmShellExtensions *self, + gchar *uuid); + It looks like the general gnome-session style is to line up headers, so please do that here.
Created attachment 194857 [details] [review] gsm: Add a new system to get information and frob Shell Extensions This functionality will be used to allow the user to enable and disable GNOME Shell Extensions in the fail whale dialog after a crash. > Review of attachment 194844 [details] [review]: > --> (https://bugzilla.gnome.org/review?bug=656747&attachment=194844) > > ::: gnome-session/gsm-shell-extensions.c > @@ +46,3 @@ > + > + if (extension->name != NULL) > + g_free (extension->name); > > g_free is NULL-safe, so do away with those ifs easy enough. > @@ +57,3 @@ > + * > + * Returns: (transfer none) > + */ > > I think we don't want those empty doc comments here. They just clutter things > up, and its not like this code is getting used from bindings where the > annotations might justify it. sure. > @@ +277,3 @@ > +{ > + self->priv = SHELL_EXTENSIONS_PRIVATE (self); > + self->priv->settings = g_settings_new (SHELL_SCHEMA); > > Uh oh. This silently adds a hard gnome-shell dep to gnome-session. I think that > is going to be problematic for a bit. > If we have to poke into the shells settings space, we should do it in a way > that doesn't crash your session manager if the shell schema is not installed. I asked desrt about this, and he just told to scan list_schemas for the existence of the schema. So... done. > ::: gnome-session/gsm-shell-extensions.h > @@ +79,3 @@ > +GsmShellExtension * gsm_shell_extensions_get_for_uuid (GsmShellExtensions > *self, > + gchar > *uuid); > + > > It looks like the general gnome-session style is to line up headers, so please > do that here. M-x align-regexp ( easy enough.
Review of attachment 194846 [details] [review]: I must say that this feels really out of place in gnome-session. If it is wanted, put it in gnome-shell somewhere, or ship a small cmdline utility for it. It is also unrelated to the fail whale functionality, from what I can see ?
Review of attachment 194857 [details] [review]: This patch still needs to be reordered to before the first use of gsm-shell-extensions.h
Review of attachment 194847 [details] [review]: Comment 20 still needs addressing (clarify the text a bit). The only other thing that came up in irc is that we should not show the extensions unless it was actually gnome-shell that crashed. At the very minimum, don't show them if we are in fallback mode.
Review of attachment 194842 [details] [review]: This looks fine now, being able to trigger the fail whale easily is actually very nice.
Review of attachment 194843 [details] [review]: Looks fine now.
Created attachment 194878 [details] [review] gsm: Add a UI to the fail whale for enabling and disabling Shell Extensions When the session puts up the fail whale after a failed restart, the user now has the opportunity to disable/enable extensions. Improved the text in the label a bit.
Created attachment 194879 [details] [review] gsm: Only show the extensions toggle when a session component asks for it Session components can ask for the extension toggle with the "X-GNOME-ShowExtensionsOnFail" key in their desktop file. welp
Review of attachment 194879 [details] [review]: Hmm. This feels unnecessarily invasive to me. I mean, why do we have to do the plumbing to get this all through gsm-app and gsm-autostart-app, when we know that this is only about gnome-shell anyway ?
Review of attachment 194878 [details] [review]: ::: gnome-session/gsm-fail-whale-dialog.c @@ +380,3 @@ + message_label = gtk_label_new (_("A problem has occurred and the system can't recover. Some of the extensions below may have caused this.\nPlease try disabling some of the extensions below, and then log out and try again.")); + else + message_label = gtk_label_new (_("A problem has occurred and the system can't recover.\nPlease log out and try again.")); Better. It would be nice to avoid the repetition of 'extensions below', though. Maybe just 'these' for the second instance ?
Created attachment 194882 [details] [review] gsm: Only show extensions UI when gnome-shell crashes If another required component fails, it's quite useless here.
(In reply to comment #32) > Review of attachment 194879 [details] [review]: > > Hmm. > > This feels unnecessarily invasive to me. I mean, why do we have to do the > plumbing to get this all through gsm-app and gsm-autostart-app, when we know > that this is only about gnome-shell anyway ? Because I couldn't find something as simple as g_str_equal (app_name, "gnome-shell"); as dumb as it sounds. Of course, now that I take a second glance, I find what I'm looking for.
Created attachment 194883 [details] [review] gsm: Add a UI to the fail whale for enabling and disabling Shell Extensions When the session puts up the fail whale after a failed restart, the user now has the opportunity to disable/enable extensions. > ... these ... sure.
Review of attachment 194882 [details] [review]: Yeah, that is better.
Review of attachment 194883 [details] [review]: Ok, good enough. This is all one big 'We suck!' dialog anyway :)
Review of attachment 194857 [details] [review]: I guess you are going to reorder things in the right way when committing this...
Attachment 194842 [details] pushed as edbf310 - gsm: Add a flag to show the fail whale dialog on request Attachment 194843 [details] pushed as f41c07f - gsm: Add debug mode to fail whale dialog Attachment 194857 [details] pushed as aadad23 - gsm: Add a new system to get information and frob Shell Extensions Attachment 194882 [details] pushed as 14fa54c - gsm: Only show extensions UI when gnome-shell crashes Attachment 194883 [details] pushed as 0b80e1a - gsm: Add a UI to the fail whale for enabling and disabling Shell Extensions