After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 671693 - fail whale extension list looks bad
fail whale extension list looks bad
Status: RESOLVED FIXED
Product: gnome-session
Classification: Core
Component: gnome-session
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Session Maintainers
Session Maintainers
Depends on:
Blocks:
 
 
Reported: 2012-03-09 04:57 UTC by Matthias Clasen
Modified: 2012-03-23 09:50 UTC
See Also:
GNOME target: 3.4
GNOME version: ---


Attachments
shell-extensions: Fix memory leak (1.06 KB, patch)
2012-03-09 18:54 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
shell-extensions: Don't show toggle switches for extensions (14.45 KB, patch)
2012-03-09 18:54 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
shell-extensions: Don't show toggle switches for extensions (17.96 KB, patch)
2012-03-17 08:40 UTC, Jasper St. Pierre (not reading bugmail)
reviewed Details | Review
shell-extensions: Don't show toggle switches for extensions (20.08 KB, patch)
2012-03-20 00:23 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review

Description Matthias Clasen 2012-03-09 04:57:49 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.
Comment 1 William Jon McCann 2012-03-09 05:07:57 UTC
Wait, we do what? What does it look like?
Comment 2 Matthias Clasen 2012-03-09 18:20:11 UTC
http://mclasen.fedorapeople.org/fail-whale-fail.png
Comment 3 Jasper St. Pierre (not reading bugmail) 2012-03-09 18:54:44 UTC
Created attachment 209347 [details] [review]
shell-extensions: Fix memory leak
Comment 4 Jasper St. Pierre (not reading bugmail) 2012-03-09 18:54:48 UTC
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.
Comment 5 Vincent Untz 2012-03-09 19:21:54 UTC
(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 6 Vincent Untz 2012-03-09 19:22:06 UTC
Comment on attachment 209347 [details] [review]
shell-extensions: Fix memory leak

Please commit
Comment 7 Jasper St. Pierre (not reading bugmail) 2012-03-09 19:41:02 UTC
(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.
Comment 8 William Jon McCann 2012-03-09 19:42:01 UTC
You and Owen should be talking that over with designers.
Comment 9 drago01 2012-03-10 19:13:16 UTC
(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?
Comment 10 Jasper St. Pierre (not reading bugmail) 2012-03-17 08:08:38 UTC
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.
Comment 11 Jasper St. Pierre (not reading bugmail) 2012-03-17 08:40:45 UTC
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.
Comment 12 Matthias Clasen 2012-03-18 01:46:22 UTC
Can we restrict the disabling of extensions to actual shell crashes ? And possibly also pop up a notification that says 'extensions have been disabled' ?
Comment 13 Jasper St. Pierre (not reading bugmail) 2012-03-18 01:57:33 UTC
(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.
Comment 14 drago01 2012-03-18 08:57:27 UTC
(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.
Comment 15 Vincent Untz 2012-03-19 15:11:37 UTC
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.
Comment 16 Jasper St. Pierre (not reading bugmail) 2012-03-20 00:23:14 UTC
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?
Comment 17 Vincent Untz 2012-03-21 10:49:26 UTC
Review of attachment 210126 [details] [review]:

Thanks, looks good. I guess we need to ask for a hard code freeze exception?
Comment 18 Jasper St. Pierre (not reading bugmail) 2012-03-21 11:11:07 UTC
(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.
Comment 19 Jasper St. Pierre (not reading bugmail) 2012-03-23 09:50:53 UTC
Attachment 210126 [details] pushed as 5d4e07d - shell-extensions: Don't show toggle switches for extensions