GNOME Bugzilla – Bug 671693
fail whale extension list looks bad
Last modified: 2012-03-23 09:50:58 UTC
Theme changes, probably - it looks really bad now. Nobody wants to fiddle with extensions on that screen anyway. We should reconsider this and instead just turn off all extensions if we detect that the shell crashed, and possibly bring up a browser with e.g.o opened to some crash page.
Wait, we do what? What does it look like?
http://mclasen.fedorapeople.org/fail-whale-fail.png
Created attachment 209347 [details] [review] shell-extensions: Fix memory leak
Created attachment 209348 [details] [review] shell-extensions: Don't show toggle switches for extensions The UI for extension toggle switches was never really thought out that well, and it looks awful. Remove it and replace it with code that simply disables all extensions.
(In reply to comment #4) > Created an attachment (id=209348) [details] [review] > shell-extensions: Don't show toggle switches for extensions > > The UI for extension toggle switches was never really thought out > that well, and it looks awful. Remove it and replace it with code > that simply disables all extensions. Thanks. I'm wondering, though: it's probably a pain for users to disable all extensions blindly. They'd have to manually re-enable all of those. Could we instead some setting like "disable_all_extensions_because_of_crash" in gnome-shell, and when the gnome-shell browser plugin sees this setting is set to true when we're on extensions.gnome.org, we'd get a special page?
Comment on attachment 209347 [details] [review] shell-extensions: Fix memory leak Please commit
(In reply to comment #5) > Thanks. I'm wondering, though: it's probably a pain for users to disable all > extensions blindly. They'd have to manually re-enable all of those. > > Could we instead some setting like "disable_all_extensions_because_of_crash" in > gnome-shell, and when the gnome-shell browser plugin sees this setting is set > to true when we're on extensions.gnome.org, we'd get a special page? The main plan is to open up a notification leading them to http://extensions.gnome.org/local/#gnome-crashed or something like that. Talking it over with Owen, we both decided that we'd need some mechanism of determining when the user is back on the network so we can pop up a notification. I'm not sure we can fit that in for 3.4.
You and Owen should be talking that over with designers.
(In reply to comment #7) > (In reply to comment #5) > > Thanks. I'm wondering, though: it's probably a pain for users to disable all > > extensions blindly. They'd have to manually re-enable all of those. > > > > Could we instead some setting like "disable_all_extensions_because_of_crash" in > > gnome-shell, and when the gnome-shell browser plugin sees this setting is set > > to true when we're on extensions.gnome.org, we'd get a special page? > > The main plan is to open up a notification leading them to > http://extensions.gnome.org/local/#gnome-crashed or something like that. > Talking it over with Owen, we both decided that we'd need some mechanism of > determining when the user is back on the network so we can pop up a > notification. I'm not sure we can fit that in for 3.4. Can't we just show a notification "All extensions have been disabled due to the crash" and show a link / action button that opens the site in the notification?
OK, so there was a big discussion about this in #gnome-design about what to do going forward into the future, and the result was just undecided. I'll talk about it with Jon next week, but until then could we decide on a behaviour for 3.4? I'm fine with any of the three approaches: * Fix style issues with switches. * Disable extensions without warning or notice. * Do nothing and possibly put the user into a crash loop.
Created attachment 209992 [details] [review] shell-extensions: Don't show toggle switches for extensions The UI for extension toggle switches was never really thought out that well, and it looks awful. Remove it and replace it with code that simply disables all extensions. My patch never compiled because I'm a moron. Here's a working one.
Can we restrict the disabling of extensions to actual shell crashes ? And possibly also pop up a notification that says 'extensions have been disabled' ?
(In reply to comment #12) > Can we restrict the disabling of extensions to actual shell crashes ? This patch already does that, since it's based on the same code. See: http://git.gnome.org/browse/gnome-session/commit/?id=14fa54c0f2b86b93e00c53fe931cc81769112e05 > And possibly also pop up a notification that says 'extensions have been disabled' ? We already add something in the fail whale dialog, but if you don't want that, I can try to add a notification. I'd need to pull a hack, like creating a file like "~/.config/gnome-session/extensions-disabled" and create a notification if it exists after we log in, since I'm unsure of any other way to store state after a crash.
(In reply to comment #13) > (In reply to comment #12) > > Can we restrict the disabling of extensions to actual shell crashes ? > > This patch already does that, since it's based on the same code. See: > > > http://git.gnome.org/browse/gnome-session/commit/?id=14fa54c0f2b86b93e00c53fe931cc81769112e05 > > > And possibly also pop up a notification that says 'extensions have been disabled' ? > > We already add something in the fail whale dialog, but if you don't want that, > I can try to add a notification. > > I'd need to pull a hack, like creating a file like > "~/.config/gnome-session/extensions-disabled" and create a notification if it > exists after we log in, since I'm unsure of any other way to store state after > a crash. You could create a property on the root window instead of accessing the file system. Also only do that iff the user actually had extensions installed.
Review of attachment 209992 [details] [review]: ::: gnome-session/gsm-fail-whale-dialog.c @@ +46,2 @@ GsmShellExtensions *extensions; GtkWidget *extensions_box; You can drop extensions_box. @@ +327,3 @@ message_label = gtk_label_new (_("A problem has occurred and the system can't recover. Please contact a system administrator")); + else if (priv->disable_extensions && gsm_shell_extensions_n_extensions (priv->extensions) > 0) + message_label = gtk_label_new (_("A problem has occurred and the system can't recover. All of your extensions have been disabled as a precaution.")); s/All of your extensions/All extensions/ (especially as it disables system-wide extensions) @@ +379,3 @@ + + if (disable_extensions) { + gsm_shell_extensions_disable_all (fail_dialog->priv->extensions); I'd actually do this in gsm-manager.c; the fail whale dialog should just display something, not disable stuff. This means you'd pass a "extensions_disabled" argument to we_failed, so we can know which message to display, but that'd be all that would be done here. ::: gnome-session/gsm-shell-extensions.c @@ -39,3 @@ - gboolean enabled; -}; - You need to remove everything related to this from .h too. @@ +129,3 @@ } + self->priv->num_extensions ++; No space before ++ ::: gnome-session/gsm-shell.c @@ +564,3 @@ shell->priv->end_session_dialog_proxy = NULL; } } This is unrelated, you can commit this, but separately.
Created attachment 210126 [details] [review] shell-extensions: Don't show toggle switches for extensions The UI for extension toggle switches was never really thought out that well, and it looks awful. Remove it and replace it with code that simply disables all extensions. This better?
Review of attachment 210126 [details] [review]: Thanks, looks good. I guess we need to ask for a hard code freeze exception?
(In reply to comment #17) > Review of attachment 210126 [details] [review]: > > Thanks, looks good. I guess we need to ask for a hard code freeze exception? String and UI freeze break too.
Attachment 210126 [details] pushed as 5d4e07d - shell-extensions: Don't show toggle switches for extensions