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 700735 - Rework focus handling; remove the input mode
Rework focus handling; remove the input mode
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2013-05-20 17:18 UTC by Jasper St. Pierre (not reading bugmail)
Modified: 2013-07-08 21:15 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
layout: Remove affectsInputRegion (2.94 KB, patch)
2013-05-20 17:18 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
global: Remove support for the NONREACTIVE input mode (4.59 KB, patch)
2013-05-20 17:18 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
Rework window / actor focus handling (20.22 KB, patch)
2013-05-20 17:18 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
global: Clean up the code that actually sets the shape on the stage (2.81 KB, patch)
2013-05-20 17:18 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
global: Automatically unshape the stage X window when we take a modal (2.64 KB, patch)
2013-05-20 17:18 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
overview: Add the overview as chrome (5.08 KB, patch)
2013-05-20 17:18 UTC, Jasper St. Pierre (not reading bugmail)
reviewed Details | Review
layout: Don't use the input mode to block events to windows (7.95 KB, patch)
2013-05-20 17:18 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
layout: Remove use of skip_paint (1.25 KB, patch)
2013-05-21 15:38 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
Rework window / actor focus handling (20.58 KB, patch)
2013-05-21 16:21 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
Overview: don't use the overlay_group (4.98 KB, patch)
2013-05-21 20:15 UTC, Giovanni Campagna
committed Details | Review
overview: Add the overview as chrome (3.13 KB, patch)
2013-05-21 22:03 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
layout: Fix overviewGroup (939 bytes, patch)
2013-05-21 22:49 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
overview: Add the overview as chrome (4.91 KB, patch)
2013-05-21 22:49 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
Overview: use a normal chrome actor to handle events during XDND (6.39 KB, patch)
2013-05-22 16:17 UTC, Giovanni Campagna
committed Details | Review
Finish removing the overlay_group concept (2.53 KB, patch)
2013-05-22 16:26 UTC, Giovanni Campagna
committed Details | Review
compositor: remove the overlay_group concept (3.46 KB, patch)
2013-05-22 16:26 UTC, Giovanni Campagna
committed Details | Review
display: Consolidate code calling XSetInputFocus into a new function (9.07 KB, patch)
2013-05-23 21:43 UTC, Jasper St. Pierre (not reading bugmail)
reviewed Details | Review
compositor: Add an API to focus the stage X window (5.13 KB, patch)
2013-05-23 21:43 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
grabHelper: Rewrite documentation for GrabHelper.grab() (3.94 KB, patch)
2013-05-23 21:44 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
messageTray: Don't use focus grabs (10.87 KB, patch)
2013-05-23 21:44 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
grabHelper: Remove explicitly having to select modal (4.55 KB, patch)
2013-05-23 21:44 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
global: Use the new mutter API for focusing the stage window (1.44 KB, patch)
2013-05-23 21:44 UTC, Jasper St. Pierre (not reading bugmail)
accepted-commit_now Details | Review
global: Make it an error to try and begin a modal twice (1.49 KB, patch)
2013-05-23 21:44 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
Rework window / actor focus handling (9.59 KB, patch)
2013-05-23 21:44 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
grabHelper: Remove explicitly having to select modal (4.64 KB, patch)
2013-05-23 21:54 UTC, Jasper St. Pierre (not reading bugmail)
accepted-commit_now Details | Review
compositor: Add an API to focus the stage X window (4.48 KB, patch)
2013-05-24 16:04 UTC, Jasper St. Pierre (not reading bugmail)
accepted-commit_now Details | Review
display: Consolidate code calling XSetInputFocus into a new function (8.78 KB, patch)
2013-05-24 19:58 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
compositor: Add an API to focus the stage X window (3.80 KB, patch)
2013-05-24 19:58 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
global: Use the new mutter API for focusing the stage window (1.44 KB, patch)
2013-05-24 19:58 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
grabHelper: Rewrite documentation for GrabHelper.grab() (4.89 KB, patch)
2013-05-24 19:58 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
messageTray: Don't use focus grabs (11.15 KB, patch)
2013-05-24 19:58 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
grabHelper: Remove explicitly having to select modal (4.68 KB, patch)
2013-05-24 19:59 UTC, Jasper St. Pierre (not reading bugmail)
accepted-commit_now Details | Review
Rework window / actor focus handling (10.34 KB, patch)
2013-05-24 19:59 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
compositor: Add an API to query if the stage is focused (5.25 KB, patch)
2013-06-03 03:15 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
grabHelper: Rewrite documentation for GrabHelper.grab() (4.89 KB, patch)
2013-06-03 03:16 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
messageTray: Don't use focus grabs (11.15 KB, patch)
2013-06-03 03:16 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
grabHelper: Remove explicitly having to select modal (4.68 KB, patch)
2013-06-03 03:16 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
Rework window / actor focus handling (8.92 KB, patch)
2013-06-03 03:16 UTC, Jasper St. Pierre (not reading bugmail)
reviewed Details | Review
messageTray: Don't use focus grabs (10.72 KB, patch)
2013-06-21 19:01 UTC, Jasper St. Pierre (not reading bugmail)
reviewed Details | Review
messageTray: Don't use focus grabs (10.73 KB, patch)
2013-07-08 20:54 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
Rework window / actor focus handling (9.56 KB, patch)
2013-07-08 20:54 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review

Description Jasper St. Pierre (not reading bugmail) 2013-05-20 17:18:24 UTC
While working on the message tray menu, Giovanni spotted a large number
of bugs related to how modals work, along with the grab helper. While
working on merging the two, I ended up reworking how focus management
works in the shell, so that the modal is a lot simpler for consumers to
use.

This removes the need for anybody to explicitly set the input mode to
FULLSCREEN or FOCUSED, and acually removes the input mode entirely,
instead opting to put a lot of "smarts" in the underlying shell system.

This fixes a few known focus-related bugs, the most annoying being the
"flashing" input focus after hitting Ctrl+Alt+Tab to panel navigate and
going back to windows.

I hope the resulting JS code is cleaner, even if the C code is a bit
more complex, to make the system more "automatic".
Comment 1 Jasper St. Pierre (not reading bugmail) 2013-05-20 17:18:28 UTC
Created attachment 244840 [details] [review]
layout: Remove affectsInputRegion

This can easy be worked around by adding things to the uiGroup
instead, so there's really no reason to have it here still.
Comment 2 Jasper St. Pierre (not reading bugmail) 2013-05-20 17:18:31 UTC
Created attachment 244841 [details] [review]
global: Remove support for the NONREACTIVE input mode

As it's unused, this is a quick cleanup before we can go onto
more important things.
Comment 3 Jasper St. Pierre (not reading bugmail) 2013-05-20 17:18:34 UTC
Created attachment 244842 [details] [review]
Rework window / actor focus handling

The duality of the Clutter's key focus and mutter's window focus
has long been a problem for us in lots of case, and caused us to
create large and complicated hacks to get around the issue,
including GrabHelper's focus grab model.

Instead of doing this, tie basic focus management into the core
of gnome-shell, instead of requiring complex "application-level"
management to get it done right.

Do this by making sure that only one of an actor or window can
be focused at the same time, and apply the appropriate logic to
drop one or the other, reactively.

Modals are considered a special case, so be very careful to track
modals and stop tracking changes to the focused thing when a modal
is going on, and resync after the modal is dropped. This ensures
that the existing modal system won't interfere.

At the same time, drop support for GrabHelper's grabFocus model,
and remove some parts that explicitly care about the FOCUSED input
mode, which is now removed.
Comment 4 Jasper St. Pierre (not reading bugmail) 2013-05-20 17:18:37 UTC
Created attachment 244843 [details] [review]
global: Clean up the code that actually sets the shape on the stage

Instead of having "dummy" setters that do work, split out the parts
that do work into their own function.
Comment 5 Jasper St. Pierre (not reading bugmail) 2013-05-20 17:18:40 UTC
Created attachment 244844 [details] [review]
global: Automatically unshape the stage X window when we take a modal

This prevents the "client" from having to do it, and removes one part
of the FULLSCREEN input mode.
Comment 6 Jasper St. Pierre (not reading bugmail) 2013-05-20 17:18:43 UTC
Created attachment 244845 [details] [review]
overview: Add the overview as chrome

When in the overview, we set the input mode to FULLSCREEN,
but this was sort of a hack that took into account the fact
that the overview takes up the entire stage. Simply add
the overview as a chrome actor for proper tracking instead.

Additionally, remove the restriction that actors passed to
trackChrome() must be a descendent of a chrome actor. In the
future we'll remove the overlay group, but for now, just do it.
Comment 7 Jasper St. Pierre (not reading bugmail) 2013-05-20 17:18:47 UTC
Created attachment 244846 [details] [review]
layout: Don't use the input mode to block events to windows

Instead, use the standard Clutter scene graph and our Chrome system.

This also removes our last use of the input mode, so remove that as
well.
Comment 8 Giovanni Campagna 2013-05-21 15:38:30 UTC
Review of attachment 244840 [details] [review]:

We should not be adding stuff to the uiGroup from random points of code, but I'll concede if this is part of a larger cleanup.
Comment 9 Jasper St. Pierre (not reading bugmail) 2013-05-21 15:38:59 UTC
Created attachment 244936 [details] [review]
layout: Remove use of skip_paint

We no longer use this on the uiGroup.
Comment 10 Giovanni Campagna 2013-05-21 15:46:11 UTC
Review of attachment 244841 [details] [review]:

Yes.
Comment 11 Jasper St. Pierre (not reading bugmail) 2013-05-21 16:21:05 UTC
Created attachment 244961 [details] [review]
Rework window / actor focus handling

The duality of the Clutter's key focus and mutter's window focus
has long been a problem for us in lots of case, and caused us to
create large and complicated hacks to get around the issue,
including GrabHelper's focus grab model.

Instead of doing this, tie basic focus management into the core
of gnome-shell, instead of requiring complex "application-level"
management to get it done right.

Do this by making sure that only one of an actor or window can
be focused at the same time, and apply the appropriate logic to
drop one or the other, reactively.

Modals are considered a special case, so be very careful to track
modals and stop tracking changes to the focused thing when a modal
is going on, and resync after the modal is dropped. This ensures
that the existing modal system won't interfere.

At the same time, drop support for GrabHelper's grabFocus model,
and remove some parts that explicitly care about the FOCUSED input
mode, which is now removed.



Fix some CurrentTime spam and a dumb typo in the grab helper causing
stacked grabs to leak the captured-event handler.
Comment 12 Giovanni Campagna 2013-05-21 19:32:40 UTC
Review of attachment 244961 [details] [review]:

So, I read this, and I personally don't like the call to XSetInputFocus behind mutter's and clutter's back. I believe the whole focus interaction between clutter actors and meta windows should happen in mutter.
This said, I'm probably not the most qualified to review input stuff, but if we decide to land this in the shell, it looks good to me.
Comment 13 Giovanni Campagna 2013-05-21 19:33:28 UTC
Review of attachment 244843 [details] [review]:

Yes.
Comment 14 Giovanni Campagna 2013-05-21 19:34:03 UTC
Review of attachment 244844 [details] [review]:

Ok
Comment 15 Giovanni Campagna 2013-05-21 19:35:15 UTC
Review of attachment 244845 [details] [review]:

::: js/ui/overview.js
@@ +132,3 @@
         this._overview._delegate = this;
 
+        Main.layoutManager.trackChrome(this._overview);

We're modal in the overview.
Why do we need this?

Or do you want to remove modality in the overview?
Comment 16 Jasper St. Pierre (not reading bugmail) 2013-05-21 19:37:40 UTC
(In reply to comment #15)
> Review of attachment 244845 [details] [review]:
> 
> ::: js/ui/overview.js
> @@ +132,3 @@
>          this._overview._delegate = this;
> 
> +        Main.layoutManager.trackChrome(this._overview);
> 
> We're modal in the overview.
> Why do we need this?

We're not always modal in the overview. The one case we're not is during XDnD drags where another client has the pointer, so we can't take a pointer grab.
Comment 17 Giovanni Campagna 2013-05-21 19:41:32 UTC
Review of attachment 244846 [details] [review]:

::: js/ui/layout.js
@@ +608,3 @@
+                                              height: global.screen_height,
+                                              reactive: true });
+        this.addChrome(this._coverPane);

I don't like this.
Can't we have a check inside the chrome code, that bypasses actor traversal and asks for a full input region?
Comment 18 Giovanni Campagna 2013-05-21 19:42:27 UTC
Review of attachment 244936 [details] [review]:

Yes.
Comment 19 Jasper St. Pierre (not reading bugmail) 2013-05-21 19:47:18 UTC
(In reply to comment #17)
> Review of attachment 244846 [details] [review]:
> 
> ::: js/ui/layout.js
> @@ +608,3 @@
> +                                              height: global.screen_height,
> +                                              reactive: true });
> +        this.addChrome(this._coverPane);
> 
> I don't like this.
> Can't we have a check inside the chrome code, that bypasses actor traversal and
> asks for a full input region?

So, I'm planning on removing the input region and replace it with pure InputOnly X windows, so I'd like the input region to be based on actors as much as possible.

Additionally, the whole coverPane thing was lifted straight from the overview code. Would it be more comfortable if there was one coverPane shared between the overview and the startup code?
Comment 20 Giovanni Campagna 2013-05-21 20:15:56 UTC
Created attachment 244994 [details] [review]
Overview: don't use the overlay_group

It's a deprecated concept, and we want to have our own actor
that we can add to the chrome to handle the input region.
Comment 21 Jasper St. Pierre (not reading bugmail) 2013-05-21 20:17:04 UTC
Review of attachment 244994 [details] [review]:

OK.
Comment 22 Giovanni Campagna 2013-05-21 20:27:11 UTC
Comment on attachment 244994 [details] [review]
Overview: don't use the overlay_group

Attachment 244994 [details] pushed as d0310bd - Overview: don't use the overlay_group
Comment 23 Jasper St. Pierre (not reading bugmail) 2013-05-21 22:03:15 UTC
Created attachment 245001 [details] [review]
overview: Add the overview as chrome

When in the overview, we set the input mode to FULLSCREEN,
but this was sort of a hack that took into account the fact
that the overview takes up the entire stage. Simply add
the overview as a chrome actor for proper tracking instead.

Additionally, remove the restriction that actors passed to
trackChrome() must be a descendent of a chrome actor. In the
future we'll remove the overlay group, but for now, just do it.
Comment 24 Jasper St. Pierre (not reading bugmail) 2013-05-21 22:49:37 UTC
Created attachment 245003 [details] [review]
layout: Fix overviewGroup

Make it a tracked actor
Comment 25 Jasper St. Pierre (not reading bugmail) 2013-05-21 22:49:48 UTC
Created attachment 245004 [details] [review]
overview: Add the overview as chrome

When in the overview, we set the input mode to FULLSCREEN,
but this was sort of a hack that took into account the fact
that the overview takes up the entire stage. Simply add
the overview as a chrome actor for proper tracking instead.

Additionally, remove the restriction that actors passed to
trackChrome() must be a descendent of a chrome actor. In the
future we'll remove the overlay group, but for now, just do it.
Comment 26 Jasper St. Pierre (not reading bugmail) 2013-05-22 16:11:49 UTC
Attachment 244840 [details] pushed as e62d22a - layout: Remove affectsInputRegion
Attachment 244841 [details] pushed as 9a79c71 - global: Remove support for the NONREACTIVE input mode
Attachment 244936 [details] pushed as 402f2d9 - layout: Remove use of skip_paint


Let's push these out of the way for now.
Comment 27 Giovanni Campagna 2013-05-22 16:17:26 UTC
Created attachment 245059 [details] [review]
Overview: use a normal chrome actor to handle events during XDND

Instead of using the input mode, when the overview is not modal
it should use a Chrome-tracked actor, that is added to the input
region. Because the overview always takes pointer input when
visible, the actor is added at startup, and it is shown and hidden
as needed.
Comment 28 Jasper St. Pierre (not reading bugmail) 2013-05-22 16:19:43 UTC
Review of attachment 245059 [details] [review]:

OK.
Comment 29 Giovanni Campagna 2013-05-22 16:26:30 UTC
Created attachment 245063 [details] [review]
Finish removing the overlay_group concept

In preparation for removing from mutter too.
Comment 30 Giovanni Campagna 2013-05-22 16:26:57 UTC
Created attachment 245064 [details] [review]
compositor: remove the overlay_group concept

The hierarchy handling is handled in the shell by adding stuff
directly to the uiGroup, and we have a dedicated actor for
the overview there, so we don't need this anymore.
Comment 31 Jasper St. Pierre (not reading bugmail) 2013-05-22 16:30:13 UTC
Review of attachment 245063 [details] [review]:

OK.
Comment 32 Jasper St. Pierre (not reading bugmail) 2013-05-22 16:30:28 UTC
Review of attachment 245064 [details] [review]:

OK.
Comment 33 Giovanni Campagna 2013-05-22 16:31:38 UTC
Comment on attachment 245059 [details] [review]
Overview: use a normal chrome actor to handle events during XDND

Attachment 245059 [details] pushed as f0113c5 - Overview: use a normal chrome actor to handle events during XDND
Comment 34 Giovanni Campagna 2013-05-22 16:34:34 UTC
Attachment 245063 [details] pushed as 976166a - Finish removing the overlay_group concept
Comment 35 Giovanni Campagna 2013-05-22 16:36:22 UTC
Comment on attachment 245064 [details] [review]
compositor: remove the overlay_group concept

Attachment 245064 [details] pushed as e108047 - compositor: remove the overlay_group concept
Comment 36 Owen Taylor 2013-05-23 17:47:16 UTC
Review of attachment 244961 [details] [review]:

::: js/ui/grabHelper.js
@@ +129,3 @@
+    // or @params.forceFocus is true. If you attempt to grab an already
+    // focused actor, the request to be focused will be ignored. The
+    // actor will not be added to the grab stack, so do not call a paired

In general, this doc comment and the behavior it is documenting is *really* hard to understand*. 

    // grab:
    // @params: A bunch of parameters, see below
    //
    // Grabs the mouse and keyboard, according to the GrabHelper's
    // parameters. If @newFocus is not %null, then the keyboard focus

What does "Grab" mean? I believe it means that "keyboard and mouse events are delivered as normal to widgets inside @params.actor, other keyboard and mouse events are ignored, except for possibly ending the grab, as described below"

    // is moved to the first #StWidget:can-focus widget inside it.

@newFocus doesn't exist. I'm not sure what it was meant to be - probably '@params.actor', but @params.actor cannot be %null. (OK, from IRC maybe it was meant to allow %null, and maybe it even works, but the code isn't really written like it expects it and the meaning is obscure - block all events?)

    //
    // The grab will automatically be dropped if:
    //   - The user clicks outside the grabbed actors
    //   - The user types Escape
    //   - The keyboard focus is moved outside the grabbed actors
    //   - A window is focused
    //
    // If @params.actor is not null, then it will be focused as the
    // new actor, as long as the stage already had a key focused actor,

Sounds like there are "focused" and "key focused" and those are two different things.

    // or @params.forceFocus is true.

Again, the code doesn't allow @params.actor to be %null. What does "as the new actor", mean? params.actor is not focused, the first  #StWidget:can-focus widget inside it is focused, but this can be overridden by params.actor.

       If you attempt to grab an already
    // focused actor, the request to be focused will be ignored. The
    // actor will not be added to the grab stack, so do not call a paired
    // ungrab().
   
Meant to be "grab an already *grabbed* actor". Since the function returns %true in this case, I'm unclear about how the caller is supposed to detect this and react. Probably either this function should treat that as a fatal error (throw a exception) or just handle it gracefully. Returning false is an option too, but that would change "Non-modal grabs can never fail."

BTW, only 1/5 calls to grabHelper.grab checks the return value from .grab(), even though any call too grabhelper.grab() with modal == true, which is most of the calls, can fail.)

    // If @params contains { modal: true }, then grab() will push a modal
    // on the owner of the GrabHelper. 

I don't think "push a modal" is a thing. Maybe just say call "Main.pushModal()"

    // As long as there is at least one
    // { modal: true } actor on the grab stack, the grab will be kept.

You don't mean "grab will be kept' you mean the "modal" - the text as it is currently here made me thing that there was some weird logic that as soon as all modal grabs were gone, all *other* grabs dropped too.

    // When the last { modal: true } actor is ungrabbed, then the modal
    // will be dropped. A modal grab can fail if there is already a grab
    // in effect from aother application; in this case the function returns
    // false and nothing happens. Non-modal grabs can never fail.

Moving on to ungrab:

    // ungrab:
    // @params: The parameters for the grab; see below.
    //
    // Pops an actor from the grab stack, potentially dropping the grab.
    //
    // If the actor that was popped from the grab stack was not the actor
    // That was passed in, this call is ignored.

Paradox alert! If this call is ignored, then no actor is popped from the grab stack. Maybe "would be popped"? But in any case, this has no relationship to what the function *actually* does, which is to find the actor in the grab stack and pop it and *all subsequent grabs* off the grab stacks.

My suggestion here would be to do a preliminary cleanup pass that rewrites the docs to be accurate and make sense for the old code, then rebase your patch on top of that. The reverse is possible, but it makes this patch hard to understand since it's diffing docs that are just wrong.

As a doc style thing, I'd suggest doing an overall description of the behavior without getting into the effect of the parameters, then document the parameters one by one, rather than trying to fold them into the docs.
Comment 37 Owen Taylor 2013-05-23 18:42:53 UTC
Review of attachment 244961 [details] [review]:

::: js/ui/ctrlAltTab.js
@@ +134,2 @@
     _focusWindows: function(timestamp) {
+        global.stage.set_key_focus(null);

In the "unified focus" world, this doesn't make a ton of sense to me conceptually  ... what's the "focus thing" after we unfocus all actors why does? - I see that the code in shell-global.c focuses the default window in that case, but if it was much clearer if:

 global.screen.focus_default_window(timestamp);

worked - presumably we want it so that whatever API is called to make a window window unfocuses the stage actor. If we can't make that work using the Mutter API's we should have wrappers.

::: js/ui/grabHelper.js
@@ +176,3 @@
                 return false;
+
+            this._capturedEventId = global.stage.connect('captured-event', Lang.bind(this, this._onCapturedEvent));

If we only now capture events when there is a modal grab in effect, I'm confused about what non-modal grabs mean.

@@ -216,3 @@
-
-            this._keyFocusNotifyId = global.stage.connect('notify::key-focus', Lang.bind(this, this._onKeyFocusChanged));
-            this._focusWindowChangedId = metaDisplay.connect('notify::focus-window', Lang.bind(this, this._focusWindowChanged));

Since you removed this, it appears:

"The grab will automatically be dropped if ...The keyboard focus is moved outside the grabbed actors" is no longer true?

@@ +244,2 @@
             if (poppedGrab.savedFocus)
                 poppedGrab.savedFocus.grab_key_focus();

Won't this mean that if a notification is focused, then the user hits escape, the focus will stay on the notification, or someplace random, rather than returning to the window? 

(Maybe it works because the notification is destroyed or hidden, clutter moves the focus to the stage, and you catch that and move the focus to the windows but that's working by accident, not working by an expected mechanism. :-) )

Do we want savedFocus to be the "focus thing" rather the stage focus actor?

Oh - looking ahead, maybe it's working because you've sort of added saving of focus to shell-global.c *as well*? So we have three places saving the focus - here, main.js and shell-global.c - can we reduce that in some way? If we have these three places saving focus, you'll need to be very explicit in comments about how that works and interacts, but I'm not sure that I see the need for shell-global.c to add that high-level behavior.

::: src/shell-global.c
@@ +542,3 @@
+  XSetInputFocus (global->xdisplay, global->stage_xwindow,
+                  RevertToPointerRoot,
+                  get_current_time_maybe_roundtrip (global));

I concur with Giovanni - this is very much behind Mutter's back and means that we will temporarily have two different ideas about what is focused - we'll have the shell-global.c idea and the Mutter idea, and they will only sync up when we get events back from X. This is going to cause a very obscure bug someday. Let's add Mutter API to do this that can properly integrate with the Mutter focus prediction.

(On the other hand, I don't really see a need to move *all* of this to Mutter - I'm fine if we just wrap this one call.)

@@ +546,3 @@
+
+static void
+sync_input_focus (ShellGlobal *global)

sync_input_focus is a rather unclear name, because it doesn't make it clear what direction it's syncing. Something like:

 set_current_focus_state_from_focused_thing()

would be better.

@@ +588,3 @@
+   */
+  if (global->has_modal)
+    return;

This return can leave global->focused_thing pointing to garbage - if sometimes you don't track focus moving off the focused thing, then you have to handle the focused thing being destroyed/unmanaged. My suggestion (as mentioned earlier in this review) is to leave saving focus to a higher level and make this code just track what is actually focused.

@@ +595,3 @@
+  /* Only attempt to resync when transitioning from
+   * ClutterActor => MetaWindow or MetaWindow => ClutterActor,
+   * or when there's NULLs in there somewhere.

This indicates that you are using sync_input_focus() for two different things - one is moving focus back and forth between the stage window and the root window and the other is to set which actor or window is focused. Split those two functions up and this complex logic will drop out.

@@ +619,2 @@
+  if (global->is_syncing_focus)
+    return;

This seems to work, but I generally prefer the pattern where you simply write:

 if (new_focused_thing == global->focused_thing)
    return;

in set_focused_thing() and that takes care of any recursion (but then again, if you split sync_focus_thing and/or drop the focus saving during modal, you may not need this at all).

@@ +1111,3 @@
                           MetaModalOptions  options)
 {
+  global->has_modal = meta_plugin_begin_modal (global->plugin, global->stage_xwindow, None, options, timestamp);

This tramples on global->has_modal if begin_modal() was previously called.
Comment 38 Owen Taylor 2013-05-23 18:48:54 UTC
Review of attachment 244846 [details] [review]:

Seems OK to me

::: js/ui/layout.js
@@ +608,3 @@
+                                              height: global.screen_height,
+                                              reactive: true });
+        this.addChrome(this._coverPane);

The wip/wayland patches for Mutter use Clutter picking for event delivery to Mutter windows, so using a cover pane is more appropriate in that case then just assuming that if we have a fullscreen input region we won't get events on windows.

(The Wayland code can at least conceptually, and perhaps practically, also manage events to transformed windows, but I'm not sure we'd want that during startup, since any click would become a crazy drag.)
Comment 39 Jasper St. Pierre (not reading bugmail) 2013-05-23 21:30:10 UTC
(In reply to comment #37)
> In the "unified focus" world, this doesn't make a ton of sense to me
> conceptually  ... what's the "focus thing" after we unfocus all actors why
> does? - I see that the code in shell-global.c focuses the default window in
> that case, but if it was much clearer if:
> 
>  global.screen.focus_default_window(timestamp);
> 
> worked - presumably we want it so that whatever API is called to make a window
> window unfocuses the stage actor. If we can't make that work using the Mutter
> API's we should have wrappers.

Yeah, this is probably a cleanup too far. I was hoping to not explicitly call focus_default_window anywhere, but maybe that's the right thing to do.

> If we only now capture events when there is a modal grab in effect, I'm
> confused about what non-modal grabs mean.

There is no such thing, after grabFocus was removed. The call in messageTray is mostly just a wrapper around _navigateFocus now.

> Since you removed this, it appears:
> 
> "The grab will automatically be dropped if ...The keyboard focus is moved
> outside the grabbed actors" is no longer true?

This was never implemented for modals, and I don't think it's correct for it to be. Removed in the doc rewrite.

> Oh - looking ahead, maybe it's working because you've sort of added saving of
> focus to shell-global.c *as well*? So we have three places saving the focus -
> here, main.js and shell-global.c - can we reduce that in some way? If we have
> these three places saving focus, you'll need to be very explicit in comments
> about how that works and interacts, but I'm not sure that I see the need for
> shell-global.c to add that high-level behavior.

I have a plan to remove grabHelper and hopefully track things in only one place. That's a future bug, though.

> I concur with Giovanni - this is very much behind Mutter's back and means that
> we will temporarily have two different ideas about what is focused - we'll have
> the shell-global.c idea and the Mutter idea, and they will only sync up when we
> get events back from X. This is going to cause a very obscure bug someday.
> Let's add Mutter API to do this that can properly integrate with the Mutter
> focus prediction.

I want to point out that the XSetInputFocus call was already there, I was just moving it around, so let's make this a "preliminary" patch that "fixes" the very obscure bug which we'll maybe see one day.

> sync_input_focus is a rather unclear name, because it doesn't make it clear
> what direction it's syncing. Something like:
> 
>  set_current_focus_state_from_focused_thing()

I'm not sure that your function name is more clear, and I was hoping the comment would make things more clear, but I came up with a new name that I'm happy with.

> This indicates that you are using sync_input_focus() for two different things -
> one is moving focus back and forth between the stage window and the root window
> and the other is to set which actor or window is focused. Split those two
> functions up and this complex logic will drop out.

Sure. I figured it was easier to track it all here and provide a convenience API, but at the shell-global.c level, all we actually care about is the type of thing focused. Reworked to keep track of the thing.

> @@ +619,2 @@
> +  if (global->is_syncing_focus)
> +    return;
> 
> This seems to work, but I generally prefer the pattern where you simply write:
> 
>  if (new_focused_thing == global->focused_thing)
>     return;
> 
> in set_focused_thing() and that takes care of any recursion (but then again, if
> you split sync_focus_thing and/or drop the focus saving during modal, you may
> not need this at all).

The issue here is that we're trying to synchronize two different things. When we synchronize and focus a window, we call clutter_stage_set_key_focus (NULL), and want to prevent that function from trying to think that the user dropped key focus on their own accord. I moved this to be explicit transition logic, as you'll see.
Comment 40 Jasper St. Pierre (not reading bugmail) 2013-05-23 21:43:54 UTC
Created attachment 245188 [details] [review]
display: Consolidate code calling XSetInputFocus into a new function

At the same time, rename set_focus_window and add a comment so we're
not confused about which function does what.
Comment 41 Jasper St. Pierre (not reading bugmail) 2013-05-23 21:43:58 UTC
Created attachment 245189 [details] [review]
compositor: Add an API to focus the stage X window

gnome-shell has traditionally just called XSetInputFocus when wanting to
set the input focus to the stage window, but this might cause strange,
hard-to-reproduce bugs because of an interference with mutter's focus
prediction. Add API to allow gnome-shell to focus the stage window that
also updates mutter's internal focus prediction state.
Comment 42 Jasper St. Pierre (not reading bugmail) 2013-05-23 21:44:26 UTC
Created attachment 245190 [details] [review]
grabHelper: Rewrite documentation for GrabHelper.grab()

The previous docs were badly maintained. This does not mention
grabFocus grabs, as they'll be removed shortly.
Comment 43 Jasper St. Pierre (not reading bugmail) 2013-05-23 21:44:30 UTC
Created attachment 245191 [details] [review]
messageTray: Don't use focus grabs

We can easily implement much of the same behavior ourselves by
keeping track of Clutter's focus events.

This will temporarily break when the user selects a window until
we can make gnome-shell automatically set the stage focus.

This also removes our only use of focus grabs, so remove those
as well.
Comment 44 Jasper St. Pierre (not reading bugmail) 2013-05-23 21:44:35 UTC
Created attachment 245192 [details] [review]
grabHelper: Remove explicitly having to select modal
Comment 45 Jasper St. Pierre (not reading bugmail) 2013-05-23 21:44:40 UTC
Created attachment 245193 [details] [review]
global: Use the new mutter API for focusing the stage window

This way, we aren't "going behind mutter's back" about what the current
focused window is.
Comment 46 Jasper St. Pierre (not reading bugmail) 2013-05-23 21:44:45 UTC
Created attachment 245194 [details] [review]
global: Make it an error to try and begin a modal twice

Ideally, this should never happen, but it's easy to track, so
do it.
Comment 47 Jasper St. Pierre (not reading bugmail) 2013-05-23 21:44:50 UTC
Created attachment 245195 [details] [review]
Rework window / actor focus handling

The duality of the Clutter's key focus and mutter's window focus has long been
a problem for us in lots of case, and caused us to create large and complicated
hacks to get around the issue, including GrabHelper's focus grab model.

Instead of doing this, tie basic focus management into the core of gnome-shell,
instead of requiring complex "application-level" management to get it done
right.

Do this by making sure that only one of an actor or window can be focused at
the same time, and apply the appropriate logic to drop one or the other,
reactively.

Modals are considered a special case, as we implicitly have the stage focused,
but at the X level, it still keeps track of the X window that was focused
before the modal, so be very careful to track modals and stop tracking focus
changes, to make sure that after the modal is dropped, we're still correct
about which X window has focus.

At the same time, remove the FOCUSED input mode, as it's not longer necessary.
Comment 48 Jasper St. Pierre (not reading bugmail) 2013-05-23 21:54:35 UTC
Created attachment 245196 [details] [review]
grabHelper: Remove explicitly having to select modal

Fix an issue with us not properly unpopping the modal.
Comment 49 Owen Taylor 2013-05-24 15:07:22 UTC
Review of attachment 245189 [details] [review]:

This patch misses my point - it's not that I'm worried that you'll mess up Mutter's logic - the logic there is robust against anybody calling XSetInputFocus at any time. The concern is that Mutter and GNOME Shell have a consistent view of what's focused - that meta_display_get_focus_window() never returns a window when GNOME Shell thinks an actor is focused. This patch needs to update display->focus_window to help with that.
Comment 50 Jasper St. Pierre (not reading bugmail) 2013-05-24 16:04:44 UTC
Created attachment 245253 [details] [review]
compositor: Add an API to focus the stage X window

gnome-shell has traditionally just called XSetInputFocus when wanting to
set the input focus to the stage window, but this might cause strange,
hard-to-reproduce bugs because of an interference with mutter's focus
prediction. Add API to allow gnome-shell to focus the stage window that
also updates mutter's internal focus prediction state.
Comment 51 Owen Taylor 2013-05-24 17:03:01 UTC
Review of attachment 245191 [details] [review]:

Not quite right yet, but I'm OK with implementing the notification focus behavior separate from grabHelper.js since it seems to be pretty separate from menu-style grabs at this point. If it's short, it should be fine directly in messageTray.js

::: js/ui/components/telepathyClient.js
@@ +758,3 @@
         this._responseEntry.clutter_text.connect('activate', Lang.bind(this, this._onEntryActivated));
         this._responseEntry.clutter_text.connect('text-changed', Lang.bind(this, this._onEntryChanged));
+        this._responseEntry.clutter_text.connect('key-focus-out', Lang.bind(this, this._onKeyFocusOut));

I think this is very ugly and *really* hard on people, say, doing extensions who want rich notifications. As we discussed on IRC, it seems better to rely on notify::key-focus then to try and hack up and pretend key-focus-out is emitted recursively X-style.
Comment 52 Owen Taylor 2013-05-24 17:03:44 UTC
Review of attachment 245193 [details] [review]:

OK
Comment 53 Owen Taylor 2013-05-24 17:24:42 UTC
Review of attachment 245188 [details] [review]:

::: src/core/display.c
@@ +1859,3 @@
+ *
+ * set_input_focus_xwindow() attempts to set the currently focused
+ * window through XSetInputFocus()

To focus a bit too much on small details, I think that this names imply that the difference between these functions is that one works on MetaWindow and one works on XWindow, and that's really not the case.

The first one probably should be "update_focus_window()" because the precise definition of focus_window (see display-private.h) is the key concept of this code.

The second would be better named "request_xserver_input_focus_change()" or something like that.

@@ +1943,3 @@
+
+  meta_error_trap_push (display);
+  display->focus_serial = XNextRequest (display->xdisplay);

It's probably better to pass in the MetaWindow as well here, and make this function call update_focus_window() itself, since the structure of the code is that when you set display->focus_serial you *must* update display->focus_window to the MetaWindow corresponding to that X window or to NULL if there is no such window.

(Or just rebase the next patch without this patch since you no longer need it, up to you.)
Comment 54 Owen Taylor 2013-05-24 17:26:39 UTC
Review of attachment 245253 [details] [review]:

OK
Comment 55 Owen Taylor 2013-05-24 17:31:27 UTC
Review of attachment 245194 [details] [review]:

meta_begin_modal_for_plugin() already has:

  if (compositor->modal_plugin != NULL || display->grab_op != META_GRAB_OP_NONE)
    return FALSE;

So this patch actually changes nothing at all, it was already an "error" (I don't like that term really - it sounds like it is a programmer error, while it's just disallowed in the same way that any modal/grab_op can fail.) This patch is just adding has_modal with a confusing commit message :-) IMO just squash this with the main patch.
Comment 56 Owen Taylor 2013-05-24 19:03:43 UTC
Review of attachment 245195 [details] [review]:

> Modals are considered a special case, as we implicitly have the stage focused,
> but at the X level, it still keeps track of the X window that was focused
> before the modal, so be very careful to track modals and stop tracking focus
> changes, to make sure that after the modal is dropped, we're still correct
> about which X window has focus.

Is this slightly out of date in terms of "about which X window has focus"? 

I think what you are saying is that Mutter is tracking
NotifyWhileGrabbed, and if we get one of those while we have a modal, we don't
want to steal the focus away from the current actor.

::: js/ui/panel.js
@@ +525,3 @@
             // nothing happened; otherwise you can't keynav to the
             // app menu.
+            if (global.display.focus_window == null)

This is going to break when the last window on a workspace is closed, I think (see comments in shell-global.c). Maybe checking if there is a focused actor would work?

::: src/shell-global.c
@@ +601,3 @@
+    {
+      /* Don't update focused_thing or sync focus.
+       * When the window is focused, we'll get a property notify. */

It might be slightly more accurate to say "If a window is focused" rather than "When"

The no-focus-window adds some oddity to the states here - if an actor is focused, then we reliably force the stage window to be focused, but the state where no actor is focused and no window is focused can either have focus on the stage or on the no-focus window.

I don't have any real suggestions that make sense to me about how to fix that, so let's just not worry about it too much.

@@ +1073,3 @@
   meta_plugin_end_modal (global->plugin, timestamp);
+  global->has_modal = FALSE;
+  sync_input_focus (global);

If the focused actor is destroyed or unmapped when there is not a grab, we focus the default window instead of whatever Clutter does by default (focus the stage?) . If the focused actor is destroyed or unmapped *during* a grab, then at that point and when the grab ends we leave the stage focused. If the non-modal behavior is that when no actor is focused, we focus the windows, then you should enforce that at this point.
Comment 57 Owen Taylor 2013-05-24 19:05:20 UTC
Review of attachment 245196 [details] [review]:

Seems OK
Comment 58 Owen Taylor 2013-05-24 19:21:56 UTC
Review of attachment 245190 [details] [review]:

Can you fix the docs for ungrab too?

::: js/ui/grabHelper.js
@@ +141,3 @@
+    //
+    // When a grab is popped, if you pass a callback as @params.onUnlock,
+    // it will be called with { isUser: true }.

As of the code you are documenting, ungrab() can definitely be called not by the user, and there's no object passed in - it's  poppedGrab.onUngrab(params.isUser).
Comment 59 Jasper St. Pierre (not reading bugmail) 2013-05-24 19:58:15 UTC
Created attachment 245268 [details] [review]
display: Consolidate code calling XSetInputFocus into a new function

At the same time, rename set_focus_window and add a comment so we're
not confused about which function does what.
Comment 60 Jasper St. Pierre (not reading bugmail) 2013-05-24 19:58:20 UTC
Created attachment 245269 [details] [review]
compositor: Add an API to focus the stage X window

gnome-shell has traditionally just called XSetInputFocus when wanting to
set the input focus to the stage window, but this might cause strange,
hard-to-reproduce bugs because of an interference with mutter's focus
prediction. Add API to allow gnome-shell to focus the stage window that
also updates mutter's internal focus prediction state.
Comment 61 Jasper St. Pierre (not reading bugmail) 2013-05-24 19:58:49 UTC
Created attachment 245270 [details] [review]
global: Use the new mutter API for focusing the stage window

This way, we aren't "going behind mutter's back" about what the current
focused window is.
Comment 62 Jasper St. Pierre (not reading bugmail) 2013-05-24 19:58:54 UTC
Created attachment 245271 [details] [review]
grabHelper: Rewrite documentation for GrabHelper.grab()

The previous docs were badly maintained. This does not mention
grabFocus grabs, as they'll be removed shortly.
Comment 63 Jasper St. Pierre (not reading bugmail) 2013-05-24 19:58:58 UTC
Created attachment 245272 [details] [review]
messageTray: Don't use focus grabs

We can easily implement much of the same behavior ourselves by
keeping track of Clutter's focus events. Reintroduce heavily
modified FocusGrabber to do the work for us.

This will temporarily break when the user selects a window until
we can make gnome-shell automatically set the stage focus.

This also removes our only use of focus grabs, so remove those
as well.
Comment 64 Jasper St. Pierre (not reading bugmail) 2013-05-24 19:59:02 UTC
Created attachment 245273 [details] [review]
grabHelper: Remove explicitly having to select modal
Comment 65 Jasper St. Pierre (not reading bugmail) 2013-05-24 19:59:07 UTC
Created attachment 245274 [details] [review]
Rework window / actor focus handling

The duality of the Clutter's key focus and mutter's window focus has long been
a problem for us in lots of case, and caused us to create large and complicated
hacks to get around the issue, including GrabHelper's focus grab model.

Instead of doing this, tie basic focus management into the core of gnome-shell,
instead of requiring complex "application-level" management to get it done
right.

Do this by making sure that only one of an actor or window can be focused at
the same time, and apply the appropriate logic to drop one or the other,
reactively.

Modals are considered a special case, as we grab all keyboard events, but at
the X level, the client window still has focus. Make sure to not do any input
synchronization when we have a modal.

At the same time, remove the FOCUSED input mode, as it's no longer necessary.
Comment 66 Owen Taylor 2013-05-24 20:12:27 UTC
Review of attachment 245268 [details] [review]:

One thing, otherwise OK to commit

::: src/core/display.c
@@ +1954,3 @@
+
+  if (meta_window != NULL && meta_window != display->autoraise_window)
+    meta_display_remove_autoraise_callback (display);

The old focus_the_nofocus_window called remove_autoraise_callback unconditionally - Iyou want to remove the meta_window != NULL here.
Comment 67 Owen Taylor 2013-05-24 20:13:31 UTC
Review of attachment 245269 [details] [review]:

OK
Comment 68 Owen Taylor 2013-05-24 20:15:24 UTC
Review of attachment 245270 [details] [review]:

OK
Comment 69 Owen Taylor 2013-05-24 20:21:44 UTC
Review of attachment 245273 [details] [review]:

Still OK
Comment 70 Owen Taylor 2013-05-24 20:55:17 UTC
Review of attachment 245274 [details] [review]:

So, I was trying to understand the shell-global.c changes again, and figure out what *additional* information was encoded in global->focused_thing in addition to what was already present in the state of Mutter and Clutter, and it seems to me that focused_thing is basically being for one thing:

 Is the stage window focused?

[ Prior to the last revision, it was also used so that if a window was focused prior to a modal, and an actor was focused during the modal, to set the focus actor back to NULL again, but the current patch set goes the other way and moves the focus to the stage window in that case ]

But it actually doesn't do this correctly - consider the following sequence:

 * Focus is on the stage
 * meta_display_focus_the_nofocus_window() is called - focus moves to the no-focus window - focus_thing stays SHELL_FOCUSED_THING_ACTOR
 * Another actor is focused - focus_actor_changed() does *not* call sync_input_focus - keyboard events are lost

Suggest adding API to mutter to query whether the stage window is focused and have the logic:

 * If focus changes to an actor and the stage window is not focused, focus it (unless in a modal)
 * If the focus changes from an actor to NULL, and the stage window is focused, focus the default window (unless in a modal)
 * If the stage window loses focus, and an actor is focused, unfocus it (unless in a modal)
 * At the end of the modal
   - if the stage is not focused, unfocus any focused actor
   - if the stage is focused, and no actor is focused, focus the default window

I think that's basically what you need. (Note that you'll need to make sure that mutter notifies the property for the focus window when it moves between the two options for NULL)
Comment 71 Jasper St. Pierre (not reading bugmail) 2013-05-24 21:44:08 UTC
Attachment 245268 [details] pushed as 2ca2838 - display: Consolidate code calling XSetInputFocus into a new function
Attachment 245269 [details] pushed as bd19de9 - compositor: Add an API to focus the stage X window


Let's push these cleanups for now, with suggested fixes.
Comment 72 Jasper St. Pierre (not reading bugmail) 2013-05-24 21:44:52 UTC
Comment on attachment 245270 [details] [review]
global: Use the new mutter API for focusing the stage window

Attachment 245270 [details] pushed as 634ce34 - global: Use the new mutter API for focusing the stage window
Comment 73 Jasper St. Pierre (not reading bugmail) 2013-06-03 03:15:44 UTC
Created attachment 245888 [details] [review]
compositor: Add an API to query if the stage is focused

gnome-shell needs to know whether the stage window is focused so
it can synchronize between stage window focus and Clutter key actor
focus. Track all X windows, even those without MetaWindows, when
tracking the focus window, and add a compositor-level API to determine
when the stage is focused.
Comment 74 Jasper St. Pierre (not reading bugmail) 2013-06-03 03:16:08 UTC
Created attachment 245889 [details] [review]
grabHelper: Rewrite documentation for GrabHelper.grab()

The previous docs were badly maintained. This does not mention
grabFocus grabs, as they'll be removed shortly.
Comment 75 Jasper St. Pierre (not reading bugmail) 2013-06-03 03:16:12 UTC
Created attachment 245890 [details] [review]
messageTray: Don't use focus grabs

We can easily implement much of the same behavior ourselves by
keeping track of Clutter's focus events. Reintroduce heavily
modified FocusGrabber to do the work for us.

This will temporarily break when the user selects a window until
we can make gnome-shell automatically set the stage focus.

This also removes our only use of focus grabs, so remove those
as well.
Comment 76 Jasper St. Pierre (not reading bugmail) 2013-06-03 03:16:17 UTC
Created attachment 245891 [details] [review]
grabHelper: Remove explicitly having to select modal
Comment 77 Jasper St. Pierre (not reading bugmail) 2013-06-03 03:16:21 UTC
Created attachment 245892 [details] [review]
Rework window / actor focus handling

The duality of the Clutter's key focus and mutter's window focus has long been
a problem for us in lots of case, and caused us to create large and complicated
hacks to get around the issue, including GrabHelper's focus grab model.

Instead of doing this, tie basic focus management into the core of gnome-shell,
instead of requiring complex "application-level" management to get it done
right.

Do this by making sure that only one of an actor or window can be focused at
the same time, and apply the appropriate logic to drop one or the other,
reactively.

Modals are considered a special case, as we grab all keyboard events, but at
the X level, the client window still has focus. Make sure to not do any input
synchronization when we have a modal.

At the same time, remove the FOCUSED input mode, as it's no longer necessary.
Comment 78 Jasper St. Pierre (not reading bugmail) 2013-06-03 03:30:09 UTC
(whoops, I meant to use git bz edit to obsolete these patches,
but accidentally set them to the commited status instead)
Comment 79 Owen Taylor 2013-06-20 18:12:55 UTC
Review of attachment 245888 [details] [review]:

Looks fine to me.

::: src/core/display.c
@@ +1473,3 @@
                           XPointer  arg)
 {
+  MetaDisplay *display = (MetaDisplay *) arg;

Don't understand this change quite - we should be safe assuming XPointer is void, and void * doesn't need a cast. But sure fine...
Comment 80 Owen Taylor 2013-06-20 18:15:34 UTC
Review of attachment 245889 [details] [review]:

Looks good
Comment 81 Owen Taylor 2013-06-20 18:42:55 UTC
Review of attachment 245890 [details] [review]:

What testing have you done on this patch? What behaviors does the code you are changing handle, and have those been tested to still work?

::: js/ui/grabHelper.js
@@ -343,3 @@
 
-        if (!button && this._modalCount == 0)
-            return false;

There's an oddity in the code where we only remove this._onCapturedEvent() when the grab is *completely* dropped, not when the last modal grab is dropped - so it's actually possible for this code to be called with a _modalCount != 0.

In light of "grabHelper: Remove explicitly having to select modal" further down in this stack, I think it's OK if we just ignore that possiblity in this patch.

::: js/ui/messageTray.js
@@ +126,3 @@
+        let focusedActor = global.stage.get_key_focus();
+        if (!focusedActor || !this._actor.contains(focusedActor))
+            this.ungrabFocus();

I don't think we want the restoring focus behavior here - if focus is navigated by the user away from the grabbed actor, we should respect that - not force the focus back to what it was before?

@@ +150,3 @@
+    }
+});
+Signals.addSignalMethods(FocusGrabber.prototype);

You don't have any signals on FocusGrabber - but don't you need one? It seems that if focus is navigated away by the user, we need to update the state of the message tray?

@@ -2423,3 @@
                                                                  Lang.bind(this, this._escapeTray));
-        this._notificationUnfocusedId = this._notification.connect('unfocused', Lang.bind(this, function() {
-            this._updateState();

I'd think this needs to be preserved in some form?
Comment 82 Owen Taylor 2013-06-20 18:51:04 UTC
Review of attachment 245891 [details] [review]:

The removal in grabHelper.js of explicit modality seems a bit half-hearted - I'd have expected modalCount / takeModalGrab / releaseModalGrab to vanish. (modalCount is especially confusing to have as a leftover) Otherwise seems fine.
Comment 83 Jasper St. Pierre (not reading bugmail) 2013-06-20 21:23:50 UTC
Comment on attachment 245888 [details] [review]
compositor: Add an API to query if the stage is focused

Attachment 245888 [details] pushed as 96221e6 - compositor: Add an API to query if the stage is focused


Pushing the mutter side of things
Comment 84 Jasper St. Pierre (not reading bugmail) 2013-06-20 21:59:19 UTC
(In reply to comment #81)
> Review of attachment 245890 [details] [review]:
> 
> What testing have you done on this patch? What behaviors does the code you are
> changing handle, and have those been tested to still work?

It should not intentionally change any behavior. I have tested the full stack  quite a lot and it seems to work just fine. It's possible that I am accidentally changing behavior in a later patch and that this one is possibly broken.

> ::: js/ui/messageTray.js
> I don't think we want the restoring focus behavior here - if focus is navigated
> by the user away from the grabbed actor, we should respect that - not force the
> focus back to what it was before?

This was a quirk of the FocusGrabber as it existed before GrabHelper. I assumed it worked and didn't pay much attention to it. I'll fix it.

> +Signals.addSignalMethods(FocusGrabber.prototype);
> 
> You don't have any signals on FocusGrabber - but don't you need one?

The code beforehand that uses the GrabHelper doesn't specify any callback to call when the focused window changes. We already handle the "user pressed escape" case ourselves (_onNotificationKeyPress).

I'm assuming that between that and pointer tracking logic, that takes care most of it. If we want to change behavior, that's probably a good idea, but not for this patch.

> Lang.bind(this, this._escapeTray));
> -        this._notificationUnfocusedId =
> this._notification.connect('unfocused', Lang.bind(this, function() {
> -            this._updateState();
> 
> I'd think this needs to be preserved in some form?

It seems I prematurely removed this. The point is that if we have the chat entry focused, the notification should never close on the user, even after a timeout.

I remember that at one time, I had a signal on FocusGrabber to emit when it got unfocused to call _updateState, so this wouldn't be necessary, as that _updateState would take care of it. I removed it after deciding to retain existing behavior as much as possible.
Comment 85 Jasper St. Pierre (not reading bugmail) 2013-06-21 19:01:08 UTC
Created attachment 247485 [details] [review]
messageTray: Don't use focus grabs

We can easily implement much of the same behavior ourselves by
keeping track of Clutter's focus events. Reintroduce heavily
modified FocusGrabber to do the work for us.

This will temporarily break when the user selects a window until
we can make gnome-shell automatically set the stage focus.

This also removes our only use of focus grabs, so remove those
as well.
Comment 86 Owen Taylor 2013-06-27 19:58:27 UTC
(In reply to comment #84)
> (In reply to comment #81)
> > Review of attachment 245890 [details] [review] [details]:
> > 
> > What testing have you done on this patch? What behaviors does the code you are
> > changing handle, and have those been tested to still work?
> 
> It should not intentionally change any behavior. I have tested the full stack 
> quite a lot and it seems to work just fine. It's possible that I am
> accidentally changing behavior in a later patch and that this one is possibly
> broken.

These weren't rhetorical questions, and I certainly didn't mean to imply that you hadn't tested - I just wanted to to get an idea of what code is involved here and what you are looking at.

It seems to me that ensureNotificationFocused gets called in two cases:

 When the user mouses into an expanding notification
 When the user hits Super-n

So I guess the primary test would be that in both those cases, something reasonable happens when you:

 Try to alt-tab out of the notification (if nothing happens, that would be OK)
 Hit escape
 Click on a chrome UI element like the application menu
 Click on a window
 
We don't need to test partial-stack patches - if things are working fine with the full stack, that's good enough.

> > +Signals.addSignalMethods(FocusGrabber.prototype);
> > 
> > You don't have any signals on FocusGrabber - but don't you need one?
> 
> The code beforehand that uses the GrabHelper doesn't specify any callback to
> call when the focused window changes. We already handle the "user pressed
> escape" case ourselves (_onNotificationKeyPress).
> 
> I'm assuming that between that and pointer tracking logic, that takes care most
> of it. If we want to change behavior, that's probably a good idea, but not for
> this patch.

If behavior is "reasonable" for some definition of reasonable, then that's good enough for now... it does appear to me that it's possible that if there are cracks in the logic - , they could have been "coincidentally" working before (say due to some side effects causing updateState to be run) and not working now, so I think thorough testing is a good idea, but if things do working in testing, that's likely good enough.
Comment 87 Owen Taylor 2013-07-08 19:59:25 UTC
Review of attachment 247485 [details] [review]:

Basically, I don't feel I understand the intended behaviors of the message tray well enough to thoroughly say whether this patch preserves them, but if you've tested thoroughly that nothing obviously bad happens, then it's probably fine. Few small things below.

::: js/ui/messageTray.js
@@ +144,3 @@
+    ungrabFocus: function() {
+        if (this._prevKeyFocusActor) {
+            global.stage.set_key_focus(this._prevKeyFocusActor);

What if this._prevKeyFocusActor is inside this.actor? Seems like ungrabbing will fail in that case. Which is mostly harmless - but maybe just call _fcusUngrabbed explicitely at the end of the function - if it was already implicitly called it will be a no-op.

@@ +153,3 @@
+    }
+});
+Signals.addSignalMethods(FocusGrabber.prototype);

Did you want to leave this?
Comment 88 Owen Taylor 2013-07-08 20:11:39 UTC
Review of attachment 245892 [details] [review]:

The behavior at the end of modal is backwards from what I suggested - it drives whether the stage is focused or not by whether an actor is focused. The reason I suggested the other way is:

 - During modality, the shell is moving the actor focus around
 - However, tracking of whether the stage is focused or not is entirely unaffected by modality - in most cases it will have it's pre-modality value, unless, e.g., the shell code activates a meta window - and if the code activates a meta window, we don't want to move the focus back to the stage

So driving the "re-consistency" by whether the focus is on the stage or not, rather then by whether an actor has the focus inherently seems better to me. But if you have reasons to do it this way, then feel free to commit. (If the driver code that is doing shell_global_begin/end_modal saves and restores the focus itself after calling end_modal(), then this doesn't matter a whole lot.)


If the driver code is saving/restoring the focus itself, then this doesn't matter.
Comment 89 Jasper St. Pierre (not reading bugmail) 2013-07-08 20:54:13 UTC
Created attachment 248662 [details] [review]
messageTray: Don't use focus grabs

We can easily implement much of the same behavior ourselves by
keeping track of Clutter's focus events. Reintroduce heavily
modified FocusGrabber to do the work for us.

This will temporarily break when the user selects a window until
we can make gnome-shell automatically set the stage focus.

This also removes our only use of focus grabs, so remove those
as well.
Comment 90 Jasper St. Pierre (not reading bugmail) 2013-07-08 20:54:38 UTC
Created attachment 248663 [details] [review]
Rework window / actor focus handling

The duality of the Clutter's key focus and mutter's window focus has long been
a problem for us in lots of case, and caused us to create large and complicated
hacks to get around the issue, including GrabHelper's focus grab model.

Instead of doing this, tie basic focus management into the core of gnome-shell,
instead of requiring complex "application-level" management to get it done
right.

Do this by making sure that only one of an actor or window can be focused at
the same time, and apply the appropriate logic to drop one or the other,
reactively.

Modals are considered a special case, as we grab all keyboard events, but at
the X level, the client window still has focus. Make sure to not do any input
synchronization when we have a modal.

At the same time, remove the FOCUSED input mode, as it's no longer necessary.
Comment 91 Owen Taylor 2013-07-08 21:01:03 UTC
Review of attachment 248663 [details] [review]:

Seems OK

::: src/shell-global.c
@@ +1075,3 @@
+
+  /* If the stage window became unfocused, drop the key focus
+   * on Clutter's side. */

There's no necessary "became" here, right? (since you unconditionally call set_key_focus(), even when it might already be unset, and let Clutter deal)
Comment 92 Owen Taylor 2013-07-08 21:12:29 UTC
Review of attachment 248662 [details] [review]:

Looks good
Comment 93 Jasper St. Pierre (not reading bugmail) 2013-07-08 21:15:22 UTC
Attachment 244843 [details] pushed as 9c8c282 - global: Clean up the code that actually sets the shape on the stage
Attachment 244844 [details] pushed as 985d0c7 - global: Automatically unshape the stage X window when we take a modal
Attachment 244846 [details] pushed as 3582ba0 - layout: Don't use the input mode to block events to windows
Attachment 245889 [details] pushed as 3790e92 - grabHelper: Rewrite documentation for GrabHelper.grab()
Attachment 245891 [details] pushed as 393577e - grabHelper: Remove explicitly having to select modal
Attachment 248662 [details] pushed as eef593a - messageTray: Don't use focus grabs
Attachment 248663 [details] pushed as 93dc7a5 - Rework window / actor focus handling


Pushed with a fix for the copy/pasted comment.