GNOME Bugzilla – Bug 711619
Replace zenity "not responding" dialogue with native dialogue
Last modified: 2017-06-28 01:02:31 UTC
Instead of spawning a pretty bad looking zenity dialogue when an application is not responding, we could overlay the window with something of similar purpose. It would look better, and like a native gnome-shell dialogue, rather than something bolted on.
Also, it would be good to log instances of 'not responding' to the journal with enough data to identify the app
Design is needed for this. I think a gnome-shell-style dialog is wrong, and we can't pop up a GTK+ dialog in mutter itself, which is why we fork out to zenity.
As much as possible, we should make apps look and behave like they are self-contained functional units. I would say that my preference would be to make the app fail state look like it belongs to the app itself, rather than the system.
(In reply to comment #3) > As much as possible, we should make apps look and behave like they are > self-contained functional units. I would say that my preference would be to > make the app fail state look like it belongs to the app itself, rather than the > system. Yeah, this is mostly what happens now with the Zenity dialog. Maybe it should simply get its appearance improved a bit? Bastien, why do you find it so ugly? :-)
Created attachment 259247 [details] screenshot This doesn't look one bit good. Spacing is busted, it's using the window title instead of the application name. And mutter requiring 5 megs of dependency when it could package a 10kB helper...
Just cleaning up the dialog, keeping the same approach: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/00patterns/not-responding/not-responding.png When discussing this with allan, he made a good point about communicating the 'system' nature of the action/termination. However, a simple system modal would not be appropriate due to its workflow-interrupting nature. We'd need to figure out a more involved solution to indicate app misbehaving and the OS wanting to deal with it.
(In reply to comment #6) > When discussing this with allan, he made a good point about communicating the > 'system' nature of the action/termination. However, a simple system modal would > not be appropriate I don't think anyone was suggesting that - we'd obviously need to introduce "window/app modal dialogs"
(In reply to comment #7) > (In reply to comment #6) > > When discussing this with allan, he made a good point about communicating the > > 'system' nature of the action/termination. However, a simple system modal would > > not be appropriate > > I don't think anyone was suggesting that - we'd obviously need to introduce > "window/app modal dialogs" Yes, I wasn't suggesting to change the level of modality of the dialog. It pertains to an application/window, so I would expect the new dialog to stay at that level, and not block anything but the application itself. MacOS X uses the infamous beachball for blocked applications, not a dialogue.
I've been looking into this during the weekend. The gnome-shell patches are still kinda wip so I'm only attaching the mutter patches at the moment. As for the gnome-shell implementation, do we generally want the same UI on window-modal than on desktop-modal dialogs? (except the "spotlight" effect of course). I wonder how much can be detached/reused from js/ui/modalDialog.js
Created attachment 331205 [details] [review] core: Add MetaCloseDialog This is an interface that can be used to implement the "application is not responding" dialog. One instance is created per window, which is initially hidden, and can be shown/hidden on demand.
Created attachment 331206 [details] [review] core: Implement MetaCloseDialogDefault This is basically a copy of the implementation currently residing in src/core/delete.c, which will be eventually deleted in favor of this one.
Created attachment 331207 [details] [review] src: Add infrastructure to build marshalers The only marshaler currently in the list is meant for MetaDisplay:create-close-dialog, which has a signature not covered by the glib marshalers.
Created attachment 331208 [details] [review] core: Add MetaDisplay:create-close-dialog signal/vmethod The default behavior of this vmethod is returning a MetaCloseDialogDefault implementing the good ol' zenity dialog. This can be overriden though in order implement the close dialog otherwise.
Created attachment 331209 [details] [review] core: Replace close dialog implementation with MetaCloseDialog src/core/delete.c now entirely relies on MetaCloseDialog in order to handle the "Application is not responding" dialog.
There's some discussion about styling in bug 762083 ...
Created attachment 343834 [details] [review] core: Add MetaCloseDialog This is an interface that can be used to implement the "application is not responding" dialog. One instance is created per window, which is initially hidden, and can be shown/hidden on demand.
Created attachment 343835 [details] [review] core: Implement MetaCloseDialogDefault This is basically a copy of the implementation currently residing in src/core/delete.c, which will be eventually deleted in favor of this one.
Created attachment 343836 [details] [review] src: Add infrastructure to build marshalers The only marshaler currently in the list is meant for MetaDisplay:create-close-dialog, which has a signature not covered by the glib marshalers.
Created attachment 343837 [details] [review] core: Add MetaDisplay:create-close-dialog signal/vmethod The default behavior of this vmethod is returning a MetaCloseDialogDefault implementing the good ol' zenity dialog. This can be overriden though in order implement the close dialog otherwise.
Created attachment 343838 [details] [review] core: Replace close dialog implementation with MetaCloseDialog src/core/delete.c now entirely relies on MetaCloseDialog in order to handle the "Application is not responding" dialog.
Review of attachment 343834 [details] [review]: ::: src/meta/meta-close-dialog.h @@ +36,3 @@ +{ + META_CLOSE_DIALOG_RESPONSE_WAIT, + META_CLOSE_DIALOG_RESPONSE_CLOSE, nit: FORCE_CLOSE? (not that it matters much) @@ +39,3 @@ +} MetaCloseDialogResponse; + +struct _MetaCloseDialogInterface { nit: { on its own line @@ +46,3 @@ + + void (* response) (MetaCloseDialog *dialog, + MetaCloseDialogResponse response); Doesn't seem to be used.
Review of attachment 343836 [details] [review]: I'm not very familiar with marshalers etc, but can't find anything standing out; except that splinter review shows the meta-marshalers.list as empty, but seems to be non-empty when opening the actual patch.
Review of attachment 343837 [details] [review]: ::: src/core/display.c @@ +423,3 @@ + _meta_marshal_OBJECT__OBJECT, + META_TYPE_CLOSE_DIALOG, + 1, META_TYPE_WINDOW); Shouldn't we rather put this in meta-plugin.h? Seems a bit obscure to semi-hide this in a MetaDisplay signal.
Review of attachment 343838 [details] [review]: Looks good to me.
Created attachment 344346 [details] [review] core: Add MetaCloseDialog This is an interface that can be used to implement the "application is not responding" dialog. One instance is created per window, which is initially hidden, and can be shown/hidden on demand.
Created attachment 344348 [details] [review] core: Implement MetaCloseDialogDefault This is basically a copy of the implementation currently residing in src/core/delete.c, which will be eventually deleted in favor of this one.
Created attachment 344349 [details] [review] compositor: Expose MetaPlugin vmethod to create a MetaCloseDialog So the actual close dialog can be overridden by MetaPlugin implementations.
Created attachment 344350 [details] [review] core: Replace close dialog implementation with MetaCloseDialog src/core/delete.c now entirely relies on MetaCloseDialog in order to handle the "Application is not responding" dialog.
Review of attachment 344346 [details] [review]: Looks good. Just a nit. ::: src/meta/meta-close-dialog.h @@ +28,3 @@ +#define META_TYPE_CLOSE_DIALOG (meta_close_dialog_get_type ()) + +typedef struct _MetaCloseDialog MetaCloseDialog; Not needed.
Review of attachment 344348 [details] [review]: lgtm.
Review of attachment 344349 [details] [review]: coding style nits, but otherwise lgtm. ::: src/compositor/meta-plugin-manager.c @@ +381,3 @@ +MetaCloseDialog * +meta_plugin_manager_create_close_dialog (MetaPluginManager *plugin_mgr, + MetaWindow *window) nit: coding style ::: src/compositor/meta-plugin-manager.h @@ +93,3 @@ +MetaCloseDialog * meta_plugin_manager_create_close_dialog (MetaPluginManager *plugin_mgr, + MetaWindow *window); nit: coding style
Review of attachment 344350 [details] [review]: style nit, otherwise lgtm. ::: src/core/delete.c @@ +60,3 @@ { + if (is_alive && window->close_dialog) + meta_close_dialog_hide (window->close_dialog); style nit: all or none branches with {}
Thanks for the review! Fixed those nits and pushed to master. The gnome-shell follow-up will happen on bug #762083. Attachment 344346 [details] pushed as b47de58 - core: Add MetaCloseDialog Attachment 344348 [details] pushed as 020e0bb - core: Implement MetaCloseDialogDefault Attachment 344349 [details] pushed as 68a9675 - compositor: Expose MetaPlugin vmethod to create a MetaCloseDialog Attachment 344350 [details] pushed as fe5138d - core: Replace close dialog implementation with MetaCloseDialog