Created attachment 183464 [details] [review]
modalDialog: grab focus immediately, not after fade-in

If the user types Alt+F2 and then immediately starts typing, some keys
can get lost. Fix that by grabbing focus sooner.

Created attachment 183465 [details] [review]
altTab: popModal before fading out

This lets the user start typing in the newly-selected window right
away, without any characters possibly getting eaten during the
animation.

Created attachment 183729 [details] [review]
runDialog: popModal before running the command

If you run a command from Alt+F2 that tries to get a server grab (eg,
xmag), it will fail if it starts up before the run dialog is finished
hiding.

Add a ModalDialog.popModal method, and call that before running the
RunDialog command. If the command succeeds, close the dialog as
before. If it fails, call ModalDialog.pushModal() to put things back
to normal before displaying the error.

Review of attachment 183464 [details] [review]:

This has been bugging me every time I use St.set_slow_down_factor()!

I think I objected to subclasses modifying _members in an early version of the modalDialog patch and wanted setInitialKeyFocus() instead (in general I'm not a believer in "protected" - subclasses are just more ways to use a public interface), but ray found some way to avoid the need to modify it at all. But if it seems OK to you, go ahead with this way... don't feel that I'm the most authoritative guardian of our JS style.

Review of attachment 183465 [details] [review]:

Looks fine. (obvious some cut-and-paste-sync-up going on here between altTab and ctrlAltTab, but doesn't seem like worth trying to factor out a baseclass.)

Review of attachment 183729 [details] [review]:

You need a gdk_display_sync() in here somewhere for this to work reliably, right?

::: js/ui/modalDialog.js
@@ +195,3 @@
+    // dialog unusable as well (by pushing it under the lightbox), so
+    // it needs to be followed shortly by either a close() or a
+    // pushModal()

Not sure I understand the reason for this. In the runDialog case it doesn't seem it would matter, since we don't seem to return to the main loop, but if we did, then this seems like "noisy" interface - to dim temporarily then either undim and show an error or vanish. So, if we don't need it, and it isn't the right effect, why do it?

(In reply to comment #6)
> Not sure I understand the reason for this. In the runDialog case it doesn't
> seem it would matter, since we don't seem to return to the main loop, but if we
> did, then this seems like "noisy" interface - to dim temporarily then either
> undim and show an error or vanish. So, if we don't need it, and it isn't the
> right effect, why do it?

runDialog doesn't re-pushModal in the success case, and we return to the main loop for the fade-out animation in that case.

But the code is still wrong; we should be popModal()ing before the animation in *all* cases, because otherwise the dialog is still sensitive while it's fading out. (Type something in the run dialog and then hit Return twice very quickly and you'll launch two copies.)

As for the noisiness, I did it that way (as opposed to putting a new reactive actor over everything) because it was easy, and it turned out to be totally unnoticeable at normal speeds. (The dialog is fading at that point anyway.) Even slowed down, it's not really very noticeable.

(In reply to comment #7)
> 
> But the code is still wrong; we should be popModal()ing before the animation in
> *all* cases, because otherwise the dialog is still sensitive while it's fading
> out. (Type something in the run dialog and then hit Return twice very quickly
> and you'll launch two copies.)

Ohh...I wonder if *that's* the bug; people who have sticky Enter keys, or are holding it down or something.

(In reply to comment #8)
> (In reply to comment #7)
> > 
> > But the code is still wrong; we should be popModal()ing before the animation in
> > *all* cases, because otherwise the dialog is still sensitive while it's fading
> > out. (Type something in the run dialog and then hit Return twice very quickly
> > and you'll launch two copies.)
> 
> Ohh...I wonder if *that's* the bug; people who have sticky Enter keys, or are
> holding it down or something.

seems unlikely that someone would have an enter key that 100% reliably activates twice.

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > 
> > > But the code is still wrong; we should be popModal()ing before the animation in
> > > *all* cases, because otherwise the dialog is still sensitive while it's fading
> > > out. (Type something in the run dialog and then hit Return twice very quickly
> > > and you'll launch two copies.)
> > 
> > Ohh...I wonder if *that's* the bug; people who have sticky Enter keys, or are
> > holding it down or something.
> 
> seems unlikely that someone would have an enter key that 100% reliably
> activates twice.

What if it's set to fast repeat in some keymaps?

(In reply to comment #7)
> (In reply to comment #6)
> > Not sure I understand the reason for this. In the runDialog case it doesn't
> > seem it would matter, since we don't seem to return to the main loop, but if we
> > did, then this seems like "noisy" interface - to dim temporarily then either
> > undim and show an error or vanish. So, if we don't need it, and it isn't the
> > right effect, why do it?
> 
> runDialog doesn't re-pushModal in the success case, and we return to the main
> loop for the fade-out animation in that case.
> 
> But the code is still wrong; we should be popModal()ing before the animation in
> *all* cases, because otherwise the dialog is still sensitive while it's fading
> out. (Type something in the run dialog and then hit Return twice very quickly
> and you'll launch two copies.)

A different one-line make-insensitive is St.util_set_hidden-from_pick() (though you have to deal with reshow in that case) Should fade-out logic be moved into modalDialog? - seems that with your patch there's some messy runDialog/modalDialog stream-crossing.

> As for the noisiness, I did it that way (as opposed to putting a new reactive
> actor over everything) because it was easy, and it turned out to be totally
> unnoticeable at normal speeds. (The dialog is fading at that point anyway.)
> Even slowed down, it's not really very noticeable.

It's pretty hard to notice run-dialog fade-out as a fade-out at all at normal speeds, though maybe there's some subconcious effect.

Created attachment 183942 [details] [review]
runDialog: popModal before running the command

updated to do gdk_display_sync() and to use a separate event-blocking
actor rather than pushing the dialog under the lightbox.

Review of attachment 183942 [details] [review]:

Strikes me as a little inconsistent that during close keystrokes are let through but click are blocked, but doubt anyone will notice. (The fade out stuff seems to be weirder - I've had times when things have been left lightboxed, but I can click on them. Unrelated.)

One issue seen in review

::: js/ui/runDialog.js
@@ +241,3 @@
             let symbol = e.get_key_symbol();
             if (symbol == Clutter.Return || symbol == Clutter.KP_Enter) {
+                this.popModal(true);

Doesn't match signature of popModal - as discussed on irc would be possible to make popModal always sync - not sure if that's better or not.

(In reply to comment #13)
> Strikes me as a little inconsistent that during close keystrokes are let
> through but click are blocked

popModal() will revert the focus to somewhere outside of the dialog though, and since clicks are blocked, there's no way to get back in.

Attachment 183464 [details] pushed as bad8dbc - modalDialog: grab focus immediately, not after fade-in
Attachment 183465 [details] pushed as 1e366aa - altTab: popModal before fading out
Attachment 183942 [details] pushed as a40daa3 - runDialog: popModal before running the command