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.

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.

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.

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.

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.

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.

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.

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.

Created attachment 244936 [details] [review]
layout: Remove use of skip_paint

We no longer use this on the uiGroup.

Review of attachment 244841 [details] [review]:

Yes.

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.

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.

Review of attachment 244843 [details] [review]:

Yes.

Review of attachment 244844 [details] [review]:

Ok

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?

(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.

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?

Review of attachment 244936 [details] [review]:

Yes.

(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?

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.

Review of attachment 244994 [details] [review]:

OK.

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

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.

Created attachment 245003 [details] [review]
layout: Fix overviewGroup

Make it a tracked actor

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.

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.

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.

Review of attachment 245059 [details] [review]:

OK.

Created attachment 245063 [details] [review]
Finish removing the overlay_group concept

In preparation for removing from mutter too.

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.

Review of attachment 245063 [details] [review]:

OK.

Review of attachment 245064 [details] [review]:

OK.

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

Attachment 245063 [details] pushed as 976166a - Finish removing the overlay_group concept

Comment on attachment 245064 [details] [review]
compositor: remove the overlay_group concept

Attachment 245064 [details] pushed as e108047 - compositor: remove the overlay_group concept

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.

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.

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.)

(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.

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.

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.

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.

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.

Created attachment 245192 [details] [review]
grabHelper: Remove explicitly having to select modal

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.

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.

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.

Created attachment 245196 [details] [review]
grabHelper: Remove explicitly having to select modal

Fix an issue with us not properly unpopping the modal.

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.

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.

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.

Review of attachment 245193 [details] [review]:

OK

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.)

Review of attachment 245253 [details] [review]:

OK

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.

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.

Review of attachment 245196 [details] [review]:

Seems OK

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).

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.

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.

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.

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.

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.

Created attachment 245273 [details] [review]
grabHelper: Remove explicitly having to select modal

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.

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.

Review of attachment 245269 [details] [review]:

OK

Review of attachment 245270 [details] [review]:

OK

Review of attachment 245273 [details] [review]:

Still OK

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)

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 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

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.

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.

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.

Created attachment 245891 [details] [review]
grabHelper: Remove explicitly having to select modal

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.

(whoops, I meant to use git bz edit to obsolete these patches,
but accidentally set them to the commited status instead)

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...

Review of attachment 245889 [details] [review]:

Looks good

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?

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 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

(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.

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.

(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.

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?

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.

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.

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.

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)

Review of attachment 248662 [details] [review]:

Looks good

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.