Created attachment 321259 [details]
screenshot

Attaching a screenshot - you can see that there are quite a lot of alignment issues in 3.19.x.

So, please know that our current "unresponsive" heuristics are tricky, and users have identified several places where they break. Loading big files in Blender, for instance, can take seconds of unresponsiveness. Games often trigger it when loading levels, because they don't have any way of responding to the ping.

The workflow of the unresponsive dialog isn't great right now, it's a bit too obtrusive. I would propose that we only show the dialog once the user tries to click on the app *after* it is already focused. We never show it automatically after a timeout.

Using a shell system modal dialog would be a huge step backwards since now I have to dismiss the prompt right way before I can interact with my other applications.

(In reply to Jasper St. Pierre from comment #2)
> So, please know that our current "unresponsive" heuristics are tricky, and
> users have identified several places where they break. Loading big files in
> Blender, for instance, can take seconds of unresponsiveness. Games often
> trigger it when loading levels, because they don't have any way of
> responding to the ping.
> 
> The workflow of the unresponsive dialog isn't great right now, it's a bit
> too obtrusive. I would propose that we only show the dialog once the user
> tries to click on the app *after* it is already focused. We never show it
> automatically after a timeout.

This bug is only about the visuals of the dialog, as far as I'm concerned.

> Using a shell system modal dialog would be a huge step backwards since now I
> have to dismiss the prompt right way before I can interact with my other
> applications.

If you look at the mockup I attached, you'll see that the shell dialog isn't system modal.

Yes, I did notice that, but your initial comment did say system modal, but I figured I'd clear it up, just in case.

Created attachment 343839 [details] [review]
ui: Refactor modal dialog into separate Dialog class

This is the basic dialog actor implementation, which will allow us to
use the same implementation on the session-global modal dialogs. The
ModalDialog class now uses it underneath, and so do all users of it.

Created attachment 343840 [details] [review]
ui: Implement "window doesn't respond" dialog on gnome-shell

This does allow us to use ClutterActors instead of lousy legacy tools
meant for shell scripting.

Created attachment 343841 [details]
screenshot with the patches

The patches rely on the mutter ones in bug #711619

(In reply to Carlos Garnacho from comment #7)
> Created attachment 343841 [details]
> screenshot with the patches
> 
> The patches rely on the mutter ones in bug #711619

Any chance you also tested when the window itself would be smaller than the dialogue it pertains to?

Created attachment 343894 [details]
Screenshot with small window

Just did, and the dialog appears centered over the overshrunk window.

One thing I realized doesn't work yet is enter/esc keybindings on the CloseDialog, it doesn't push modality on purpose, and wayland key event delivery is quite out-of-band with the actual clutter focus.

Other edge case would be how the window appears when in overview mode. Is the dialogue shown on top, or hidden? Does the window appear grayed out?

The dialog in that case is hidden, just like subsurfaces :/. The saturation effect is also gone, since the overview actors are just clones, and clutter seems to bypass the effect(s) in the original actor.

Review of attachment 343839 [details] [review]:

This breaks at least:
  - polkit dialog/keyring dialog/wireless selection (don't come up at all)
  - end session dialog (fullscreen from the 2nd invocation onward)
  - NM authentication dialog (no line wrapping)

Run dialog and display confirmation are fine, I didn't test the other ones.

Review of attachment 343840 [details] [review]:

Needs updating for the signal->plugin vfunc change

Created attachment 352677 [details] [review]
ui: Refactor modal dialog into separate Dialog class

This is the basic dialog actor implementation, which will allow us to
use the same implementation on the session-global modal dialogs. The
ModalDialog class now uses it underneath, and so do all users of it.

Created attachment 352678 [details] [review]
ui: Implement "window doesn't respond" dialog on gnome-shell

This does allow us to use ClutterActors instead of lousy legacy tools
meant for shell scripting.

These ones should be fixed now. With these patches I still see 2 issues though:

- Keyboard handling is broken: We connect to the parent actor events (in this case the frozen window), but if we don't push modal events won't get there at all. I guess I can add selective keyboard grabbing as long as the window is focused, or connect to stage events, but meh...

- The desaturation effect applied on the frozen window also affects the close dialog, because the actor is part of the hierarchy. Was conveniently hidden by the dialog being pretty much on the grayscale :(

Created attachment 352695 [details] [review]
ui: Refactor modal dialog into separate Dialog class

This is the basic dialog actor implementation, which will allow us to
use the same implementation on the session-global modal dialogs. The
ModalDialog class now uses it underneath, and so do all users of it.

Created attachment 352696 [details] [review]
ui: Implement "window doesn't respond" dialog on gnome-shell

This does allow us to use ClutterActors instead of lousy legacy tools
meant for shell scripting.

Created attachment 352697 [details] [review]
core: Forward events meant for non-alive windows to clutter

Otherwise the ClutterEventFilter will consider these handled, and not
forward these to Clutter. This gets necessary for key handling if we
mean to implement the close dialog with Clutter UI.

These patches fix all issues. I'm not specially happy with the keyboard handling bits though. Also changed the window effect to reduced brightness as per the mockups.

Created attachment 352717 [details] [review]
core: Forward events meant for non-alive windows to clutter

Otherwise the ClutterEventFilter will consider these handled, and not
forward these to Clutter. This gets necessary for key handling if we
mean to implement the close dialog with Clutter UI.

Review of attachment 352695 [details] [review]:

Overall this looks OK to me, needs-work just for the log errors due to the missing buttonLayout property

::: js/ui/dialog.js
@@ +35,3 @@
+    },
+
+    _init: function (parentActor, styleClass) {

We have a strong convention of the constructor being the first method of a class. We are generally quite good at breaking our conventions, but in this case it's so strong that this would be a first ;-)

@@ +53,3 @@
+
+    _onDestroy: function () {
+        this._parentActor.disconnect(this._eventId);

Maybe play it safe and do

  if (this._eventId)
      this._parentActor.disconnect(this._eventId);
  this._eventId = 0;

We shouldn't get multiple :destroy emissions, but IIRC I've seen it happen occasionally

@@ +72,3 @@
+
+            let button = buttonInfo['button'];
+            let action = buttonInfo['action'];

While moving code, we can modernize it a bit:

    let { button, action } = buttonInfo;

@@ +91,3 @@
+        let action = buttonInfo['action'];
+        let key = buttonInfo['key'];
+        let isDefault = buttonInfo['default'];

Unfortunately 'default' is a keywords, so we can't just destructure the whole bit. Still,

  let { label, action, key } = buttonInfo;
  let isDefault = buttonInfo.default;

is at least a bit of a cleanup

::: js/ui/modalDialog.js
@@ +19,3 @@
 const Main = imports.ui.main;
 const Tweener = imports.ui.tweener;
+const Dialog = imports.ui.dialog;

Please try to keep the list sorted alphabetically

@@ +70,3 @@
         this._group.add_actor(this._backgroundBin);
 
+        this.dialogLayout = new Dialog.Dialog(this.backgroundStack, params.styleClass);

This looks a bit risky. Changing the type from StBoxLayout to StWidget breaks this.dialogLayout.add(), and  any additional children will be lay out in a surprising way (stack instead of box).

But maybe it's fine, at least in gnome-shell itself and gnome-shell-extensions we don't use this, so there's a chance that the change won't break too much elsewhere ...

@@ -107,3 @@
-                                y_align: St.Align.START });
-
-        this.buttonLayout = new St.Widget ({ layout_manager: new Clutter.BoxLayout ({ homogeneous:true }) });

We miss this public property now, that's used for instance by RestartMessage in main.js

Review of attachment 352696 [details] [review]:

Except for the key forwarding that sometimes works unexpectedly and keynav, this works reasonably well in testing on wayland. Unfortunately it's fairly broken on X11 - the dialog freezes when clicking a button, and keys don't work at all (because the event doesn't even get to the stage in that case).

::: js/js-resources.gresource.xml
@@ +45,3 @@
     <file>ui/calendar.js</file>
     <file>ui/checkBox.js</file>
+    <file>ui/closeDialog.js</file>

The file contains translatable strings, so should be added to po/POTFILES.in as well

::: js/ui/closeDialog.js
@@ +7,3 @@
+const Pango = imports.gi.Pango;
+const Lang = imports.lang;
+const Signals = imports.signals;

Unused import

@@ +8,3 @@
+const Lang = imports.lang;
+const Signals = imports.signals;
+const Dialog = imports.ui.dialog;

Style nit: Empty line between 'system' and 'app' imports

@@ +29,3 @@
+
+    _createDialogContent: function () {
+        let scaleFactor = St.ThemeContext.get_for_stage(global.stage).scale_factor;

Better: Set the corresponding values via CSS and get them scaled for free

@@ +37,3 @@
+        box.add_child(errorIcon);
+
+        let windowTitle = this._window.get_title();

Does it make sense to use the application name instead?

  let app = Shell.WindowTracker.get_default().get_window_app(this._window);
  let title = app.get_name();

@@ +44,3 @@
+            primaryText = _("“%s” is not responding.").format(windowTitle);
+        } else {
+            primaryText = _("Application is not responding.");

If we do want to primarily use the window title, then we should at least use the app name as fallback

@@ +49,3 @@
+        let secondaryText = _("You may choose to wait a short while for it to " +
+                              "continue or force the application to quit entirely.");
+        let dialogText = "<big><b>%s</b></big>\n\n%s".format(primaryText, secondaryText);

Eeeks. Use two labels, style via CSS.

@@ +79,3 @@
+    },
+
+    _init: function (window) {

Please move to top, see previous comment

@@ +105,3 @@
+    },
+
+    onWait: function () {

Any reasons for those to be public?

@@ +115,3 @@
+    _forwardKeyEvents: function (actor, event) {
+        // Forward key events to our window/dialog, but only if
+        // it happens to be the focused one.

That's not enough. For instance if the user enters the overview, then escape should exit the overview, not *also* discard the dialog. Can we check for the focus actor?

@@ +129,3 @@
+
+    vfunc_show: function () {
+        if (this._dialog == null) {

Feel free to ignore, but my personal preference would be
  if (this._dialog != null)
      return;

  this._addWindowEffect();
  ...

@@ +131,3 @@
+        if (this._dialog == null) {
+            this._addWindowEffect();
+            this._initDialog();

Would be nice to animate this - regular attached modals animate scale-y on map, which sounds like the right effect here as well

Review of attachment 352717 [details] [review]:

::: src/core/delete.c
@@ +36,3 @@
                           MetaWindow              *window)
 {
+  g_clear_object (&window->close_dialog);

Doesn't this defeat the whole ensure/cache dialog code a bit? Would it make sense to set a visible member from show/hide so we can do
  if (window->close_dialog &&
      meta_close_dialog_is_visible (window->close_dialog)) {
      ...
   }

Thanks for the review! I agree to the code change suggestions, a few comments:

(In reply to Florian Müllner from comment #22)
> Review of attachment 352695 [details] [review] [review]:
> 
> Overall this looks OK to me, needs-work just for the log errors due to the
> missing buttonLayout property
> 
> ::: js/ui/dialog.js
> @@ +35,3 @@
> +    },
> +
> +    _init: function (parentActor, styleClass) {
> 
> We have a strong convention of the constructor being the first method of a
> class. We are generally quite good at breaking our conventions, but in this
> case it's so strong that this would be a first ;-)

Yeah, I'm that good at breaking conventions :p. Agree.

> @@ +70,3 @@
>          this._group.add_actor(this._backgroundBin);
>  
> +        this.dialogLayout = new Dialog.Dialog(this.backgroundStack,
> params.styleClass);
> 
> This looks a bit risky. Changing the type from StBoxLayout to StWidget
> breaks this.dialogLayout.add(), and  any additional children will be lay out
> in a surprising way (stack instead of box).
> 
> But maybe it's fine, at least in gnome-shell itself and
> gnome-shell-extensions we don't use this, so there's a chance that the
> change won't break too much elsewhere ...

Hmm, true, and fair point about extensions. The StWidget here is actually the parent of the dialog widget, IIRC it's basically used so the dialog has something to be centered around. Perhaps I can get rid of it and have the dialog force x/y_align on itself?

(In reply to Florian Müllner from comment #23)
> Review of attachment 352696 [details] [review] [review]:
> 
> Except for the key forwarding that sometimes works unexpectedly and keynav,
> this works reasonably well in testing on wayland. Unfortunately it's fairly
> broken on X11 - the dialog freezes when clicking a button, and keys don't
> work at all (because the event doesn't even get to the stage in that case).

Yeah I know it's all fairly broken on X :(. I had the impression it gets no pointing events because it's actually seeing the frozen window below, that surely is fixable, not too different from notification banners or other shell chrome.

But the key delivery will probably require passive key grabs on the window for enter/esc while it's frozen. Doable, but yikes.

> @@ +37,3 @@
> +        box.add_child(errorIcon);
> +
> +        let windowTitle = this._window.get_title();
> 
> Does it make sense to use the application name instead?

Yeah, probably... I went too much for copying the current message. Now that I double check the attached mockup, it does use the application name.

> @@ +105,3 @@
> +    },
> +
> +    onWait: function () {
> 
> Any reasons for those to be public?

Uh, not really.

> 
> @@ +115,3 @@
> +    _forwardKeyEvents: function (actor, event) {
> +        // Forward key events to our window/dialog, but only if
> +        // it happens to be the focused one.
> 
> That's not enough. For instance if the user enters the overview, then escape
> should exit the overview, not *also* discard the dialog. Can we check for
> the focus actor?

Right, should be possible.

> 
> @@ +129,3 @@
> +
> +    vfunc_show: function () {
> +        if (this._dialog == null) {
> 
> Feel free to ignore, but my personal preference would be
>   if (this._dialog != null)
>       return;
> 
>   this._addWindowEffect();
>   ...
> 
> @@ +131,3 @@
> +        if (this._dialog == null) {
> +            this._addWindowEffect();
> +            this._initDialog();
> 
> Would be nice to animate this - regular attached modals animate scale-y on
> map, which sounds like the right effect here as well

Ah, sure, will have a look. It could also be nice to animate the window effect as well, but that seems down to using Tweener AFAICS.

(In reply to Florian Müllner from comment #24)
> Review of attachment 352717 [details] [review] [review]:
> 
> ::: src/core/delete.c
> @@ +36,3 @@
>                            MetaWindow              *window)
>  {
> +  g_clear_object (&window->close_dialog);
> 
> Doesn't this defeat the whole ensure/cache dialog code a bit? Would it make
> sense to set a visible member from show/hide so we can do
>   if (window->close_dialog &&
>       meta_close_dialog_is_visible (window->close_dialog)) {
>       ...
>    }

Hmm, yeah. That sounds like a better way to do that.

Created attachment 355657 [details] [review]
core: Add meta_close_dialog_is_visible() property

Since the last show/hide call should be effective, implement
it without requiring an extra interface property.

Created attachment 355658 [details] [review]
core: Forward events meant for non-alive windows to clutter

Otherwise the ClutterEventFilter will consider these handled, and not
forward these to Clutter. This gets necessary for key handling if we
mean to implement the close dialog with Clutter UI.

Created attachment 355659 [details] [review]
ui: Refactor modal dialog into separate Dialog class

This is the basic dialog actor implementation, which will allow us to
use the same implementation on the session-global modal dialogs. The
ModalDialog class now uses it underneath, and so do all users of it.

Created attachment 355660 [details] [review]
ui: Implement "window doesn't respond" dialog on gnome-shell

This does allow us to use ClutterActors instead of lousy legacy tools
meant for shell scripting.

Created attachment 355661 [details] [review]
layout: Allow adding already parented actors for chrome tracking

It is the case of the MetaCloseDialog in gnome-shell, the dialog is
already included in the MetaWindowGroup, but needs to be tracked as
chrome in order to have the dialog receive pointer events on X11.

Created attachment 355663 [details] [review]
core: Add meta_close_dialog_focus() vmethod

This is used to request key focus on the close dialog whenever
a window that is frozen would receive key focus.

Created attachment 355664 [details] [review]
ui: Implement "window doesn't respond" dialog on gnome-shell

This does allow us to use ClutterActors instead of lousy legacy tools
meant for shell scripting.

Review of attachment 355657 [details] [review]:

LGTM

Review of attachment 355658 [details] [review]:

Dto.

Review of attachment 355663 [details] [review]:

As mentioned on IRC, we may need to call the new method from meta_close_dialog_show() (if the unresponsive window has focus and there's no compositor grab)

Review of attachment 355661 [details] [review]:

::: js/ui/layout.js
@@ +760,3 @@
     // and shown otherwise)
     addChrome: function(actor, params) {
+        if (!actor.get_parent()) {

I don't like this:
 - "add" implies that the actor is added somewhere
 - it breaks the symmetry of addChrome/removeChrome
   (the latter will fail for non-uiGroup-children)

Let's instead lift the restriction on trackChrome() to only allow descendants of actors added with addChrome() - besides documentation updates, something like:

  let ancestorData = ancestor ? this._trackedActor[index]
                              : defaultParams;

should be enough I think ...

Created attachment 355679 [details] [review]
core: Add meta_close_dialog_focus() vmethod

This is used to request key focus on the close dialog whenever
a window that is frozen would receive key focus. Also, ensure
that the dialog gets focus when first shown if the window was
meant to receive input.

Created attachment 355680 [details] [review]
ui: Implement "window doesn't respond" dialog on gnome-shell

This does allow us to use ClutterActors instead of lousy legacy tools
meant for shell scripting.

Created attachment 355681 [details] [review]
layout: Allow adding already parented actors for chrome tracking

let trackChrome accept actors that are not children of chrome actors.
this will be useful for the MetaCloseDialog in gnome-shell, which
is already included in the MetaWindowGroup, but needs to be tracked
as chrome for the dialog to receive pointer events on X11.

Created attachment 355682 [details] [review]
theme: Add minimal theming for close dialogs

Set proper spacing and font size on the primary label.

(In reply to Florian Müllner from comment #35)
> Review of attachment 355663 [details] [review] [review]:
> 
> As mentioned on IRC, we may need to call the new method from
> meta_close_dialog_show() (if the unresponsive window has focus and there's
> no compositor grab)

I instead made this in the only place where meta_close_dialog_show() is called. If I do it in the MetaCloseDialog method I need to use g_object_get() to fetch the window, and things seem to go very wrong on gjs :(. Admittedly a workaround, I want to do a testcase for this to file a bug.

(In reply to Florian Müllner from comment #36)
> Review of attachment 355661 [details] [review] [review]:
> 
> ::: js/ui/layout.js
> @@ +760,3 @@
>      // and shown otherwise)
>      addChrome: function(actor, params) {
> +        if (!actor.get_parent()) {
> 
> I don't like this:
>  - "add" implies that the actor is added somewhere
>  - it breaks the symmetry of addChrome/removeChrome
>    (the latter will fail for non-uiGroup-children)

All true...

> 
> Let's instead lift the restriction on trackChrome() to only allow
> descendants of actors added with addChrome() - besides documentation
> updates, something like:
> 
>   let ancestorData = ancestor ? this._trackedActor[index]
>                               : defaultParams;
> 
> should be enough I think ...

It indeed is :). Updated the patch(es) with that approach. Also added some css so the dialog looks nicer.

Review of attachment 355679 [details] [review]:

OK

Review of attachment 355659 [details] [review]:

LGTM

Review of attachment 355681 [details] [review]:

LGTM

(In reply to Carlos Garnacho from comment #41)
> Also added some css so the dialog looks nicer.

About that ... while reviewing those patches yesterday, I finally got annoyed enough about most of the dialogs implementing the same UI pattern with mostly identical styles to split out a shared widget. See patches in bug 784985 (on top of the split-out Dialog patch from here) - if you update the close-dialog patch to use that, it should look decent without any additional styling.

Review of attachment 355680 [details] [review]:

::: js/ui/closeDialog.js
@@ +5,3 @@
+const Lang = imports.lang;
+const Meta = imports.gi.Meta;
+const Pango = imports.gi.Pango;

Unused import

@@ +7,3 @@
+const Pango = imports.gi.Pango;
+const Shell = imports.gi.Shell;
+const St = imports.gi.St;

Dto.

@@ +28,3 @@
+        this._window = window;
+        this._dialog = null;
+        this._focusChangeId = 0;

Unused.

@@ +82,3 @@
+        this._dialog.addButton({ label:  _('Wait'),
+                                 action: Lang.bind(this, this._onWait),
+                                 key:    Clutter.Escape });

Add

  global.focus_manager.add_group(this._dialog);

to fix keynav as well \o/

@@ +149,3 @@
+
+    vfunc_focus: function () {
+        if (this._dialog && this._dialog != global.stage.get_key_focus())

I'm unsure about the second check - stage.key_focus is the actor that will receive keyboard events *if* Clutter is receiving events from the backend. On X, grabbing the focus has the side-effect of focusing the stage's xwindow, so can we be sure that we won't miss that and end up without keyboard events again? And if we keep the check, would dialog.contains(stageFocus) make sense?

Created attachment 355690 [details] [review]
closeDialog: Use MessageDialogContent

(In reply to Florian Müllner from comment #45)
> if you update the close-dialog patch to use that, it should look decent
> without any additional styling.

Attached patch does this.

Created attachment 355705 [details] [review]
ui: Implement "window doesn't respond" dialog on gnome-shell

This does allow us to use ClutterActors instead of lousy legacy tools
meant for shell scripting.

Review of attachment 355705 [details] [review]:

All nit from comment #46 still apply ...

Created attachment 355716 [details] [review]
ui: Implement "window doesn't respond" dialog on gnome-shell

This does allow us to use ClutterActors instead of lousy legacy tools
meant for shell scripting.

--

Doh. Managed to miss all updates here, your way to create dialog contents seemed
more concise, so I folded it in. The key focus check is also gone, was there to
avoid extra signaling on multiple calls, but clutter seems to take care of that
already.

Attachment 355659 [details] pushed as 1c7a3ee - ui: Refactor modal dialog into separate Dialog class
Attachment 355681 [details] pushed as 74bd009 - layout: Allow adding already parented actors for chrome tracking

Review of attachment 355716 [details] [review]:

LGTM, let's get this in \o/

Attachment 355657 [details] pushed as aa47374 - core: Add meta_close_dialog_is_visible() property
Attachment 355658 [details] pushed as f38c909 - core: Forward events meant for non-alive windows to clutter
Attachment 355679 [details] pushed as 4082929 - core: Add meta_close_dialog_focus() vmethod

Yay!

Attachment 355716 [details] pushed as d220e35 - ui: Implement "window doesn't respond" dialog on gnome-shell