*** Bug 597352 has been marked as a duplicate of this bug. ***

Created attachment 186908 [details] [review]
display: clean up focus_window vs expected_focus_window

Mutter previously defined display->focus_window as the window that the
server says is focused, but kept display->expected_focus_window to
indicate the window that we have requested to be focused. But it turns
out that "expected_focus_window" was almost always what we wanted.

Make MetaDisplay do a better job of tracking focus-related requests
and events, and change display->focus_window to be our best guess of
the "currently" focused window (ie, the window that will be focused at
the time when the server processes the next request we send it).

Created attachment 186909 [details] [review]
display: clean up focus_window vs expected_focus_window

Mutter previously defined display->focus_window as the window that the
server says is focused, but kept display->expected_focus_window to
indicate the window that we have requested to be focused. But it turns
out that "expected_focus_window" was almost always what we wanted.

Make MetaDisplay do a better job of tracking focus-related requests
and events, and change display->focus_window to be our best guess of
the "currently" focused window (ie, the window that will be focused at
the time when the server processes the next request we send it).

Comment on attachment 186909 [details] [review]
display: clean up focus_window vs expected_focus_window

mrh

Created attachment 186910 [details] [review]
display: clean up focus_window vs expected_focus_window

Mutter previously defined display->focus_window as the window that the
server says is focused, but kept display->expected_focus_window to
indicate the window that we have requested to be focused. But it turns
out that "expected_focus_window" was almost always what we wanted.

Make MetaDisplay do a better job of tracking focus-related requests
and events, and change display->focus_window to be our best guess of
the "currently" focused window (ie, the window that will be focused at
the time when the server processes the next request we send it).

OK, this implements basically what's in bug 597352 comment 1. It passes at least basic testing.

Examining all remaining uses of focus_window and window->has_focus suggests that the new semantics should be as good or better than the old semantics in most places.

One slight change: if we try to focus a WM_TAKE_FOCUS window, and then that window closes without taking focus, we will be left with the no_focus_window focused, instead of having focus revert back to the previously-focused window as in current mutter/metacity. This is in fact what expected_focus_window was originally added for (bug 84564, though the bigger problem there was that None was getting focused rather than no_focus_window). So... we could bring expected_focus_window back and use it in just that case, if we wanted...

poke

Review of attachment 186910 [details] [review]:

Mostly looks good as far as I can tell. Few concerns about particular aspects.

::: src/core/display.c
@@ +1591,3 @@
+handle_window_focus_event (MetaDisplay *display,
+                           MetaWindow  *window,
+                           XEvent      *event)

This function looks like it was written to handle UnmapNotify events, but it doesn't appear to be ever called for UnmapNotify events. (Meaning that server_focus_window can end up pointing to an unmanaged window at least briefly)

@@ +2195,3 @@
                               "Unsetting focus from %s due to mouse entering "
                               "the DESKTOP window\n",
+                              display->focus_window->desc);

You removed (not replaced) the display->expected_focus_window != NULL check, but couldn't display->focus_window be NULL here in some weird corner case?

@@ +5526,3 @@
+  if (window != display->focus_window)
+    {
+      /* We can't just set display->focus_window to @window, since we

I think it might be good to start this off with:
 
 This is the rare "globally active" case.

to clarify the scope of the problem.

@@ +5528,3 @@
+      /* We can't just set display->focus_window to @window, since we
+       * we don't know when (or even if) the window will actually take
+       * focus, so we could end up being wrong for arbitrarily long.

I think the way you've done it here has the right user behavior:

 - As soon as you click away, the window that you clicked away from loses the focus indication and stops getting keystrokes
 - If the app you've clicked on is slow to take care of it's "globally active" responsibilities it doesn't get focus indication until it actually gets focused and starts getting keystrokes.

I don't think dealing with the case where globally active windows handle WM_TAKE_FOCUS  by closing is worth extra code unless it leaves things entirely broken.

@@ +5635,3 @@
+ * Get the window that we currently consider to be focused. This is
+ * the window for which we have most recently either received a
+ * FocusIn or sent an XSetInputFocus.

mixture of Xlib and protocol terminology - "sent a SetInputFocus request"

::: src/core/window.c
@@ +1514,3 @@
     }
 
+  g_assert (window->display->focus_window != window);

Do you also want to assert the same about ->server_focus_window here as well? Or should you rather have clearing logic for server_focus_window in case there are some cases where it woudn't get unset? (Does it get unset if a window is destroyed without being withdrawn first?)

@@ +6413,3 @@
+void
+meta_window_set_focused (MetaWindow *window,
+                         gboolean    focused)

This name seems a little too simple for something that's an implementation detail and doesn't take care of the display-> properties and system state. Yes, it's not in the public header, so it shouldn't confuse users of the library, but I'd back happier with _internal tacked on to the name.

(In reply to comment #8)
> +  g_assert (window->display->focus_window != window);
> 
> Do you also want to assert the same about ->server_focus_window here as well?
> Or should you rather have clearing logic for server_focus_window in case there
> are some cases where it woudn't get unset? (Does it get unset if a window is
> destroyed without being withdrawn first?)

XDestroyWindow(3) says "If the window specified by the w argument is mapped, it is unmapped automatically" which seems to suggest that that can't happen.

However, there was at least one broken case: if a call to XSetInputFocus() fails due to someone else having already called it with a newer timestamp, then once we notice this, we revert focus_window back to server_focus_window. So if server_focus_window had been freed, then, boom. So now, when a window is unmanaged, if it's the server_focus_window, we set that to NULL. This means that in the case above, we'd end up setting focus_window to NULL as well, which is correct, since we have no clue what window is focused at that point, until we receive another FocusIn. (And we know that there has to be a FocusIn coming, or else our XSetInputFocus() wouldn't have failed, so we don't want to call focus_the_no_focus_window() or anything.)

Created attachment 192062 [details] [review]
display: clean up focus_window vs expected_focus_window

Mutter previously defined display->focus_window as the window that the
server says is focused, but kept display->expected_focus_window to
indicate the window that we have requested to be focused. But it turns
out that "expected_focus_window" was almost always what we wanted.

Make MetaDisplay do a better job of tracking focus-related requests
and events, and change display->focus_window to be our best guess of
the "currently" focused window (ie, the window that will be focused at
the time when the server processes the next request we send it).

(In reply to comment #9)
> (In reply to comment #8)
> > +  g_assert (window->display->focus_window != window);
> > 
> > Do you also want to assert the same about ->server_focus_window here as well?
> > Or should you rather have clearing logic for server_focus_window in case there
> > are some cases where it woudn't get unset? (Does it get unset if a window is
> > destroyed without being withdrawn first?)
> 
> XDestroyWindow(3) says "If the window specified by the w argument is mapped, it
> is unmapped automatically" which seems to suggest that that can't happen.
> 
> However, there was at least one broken case: if a call to XSetInputFocus()
> fails due to someone else having already called it with a newer timestamp, then
> once we notice this, we revert focus_window back to server_focus_window. So if
> server_focus_window had been freed, then, boom. So now, when a window is
> unmanaged, if it's the server_focus_window, we set that to NULL. This means
> that in the case above, we'd end up setting focus_window to NULL as well, which
> is correct, since we have no clue what window is focused at that point, until
> we receive another FocusIn. (And we know that there has to be a FocusIn coming,
> or else our XSetInputFocus() wouldn't have failed, so we don't want to call
> focus_the_no_focus_window() or anything.)

I don't think it's true about having to be a FocusIn coming for the failed XSetInputFocus case - we can get a FocusIn *before* we call XSetInputFocus and still try to XSetInputFocus from an older timestamp from a button press or something that will get denied. (And we can't even fix this because FocusIn doesn't contain the focus timestap.)

But more generically, an UnmapNotify for the server focus window is never the final thing we see - even if that causes the focus to go None, we'll then get a FocusIn on the root window with  NotifyDetailNone and that will cause us to focus the no focus window. So, I think we're OK.

Review of attachment 192062 [details] [review]:

Little uncertain about a couple aspects of how the call to handle_window_focus_event was added for Unmap events

::: src/core/display.c
@@ +2383,3 @@
                               "Received pending unmap, %d now pending\n",
                               window->unmaps_pending);
+                  handle_window_focus_event (display, window, event);

Is it right to add this in the !frame_was_receiver block? Under certain circumstances we focus a window by focusing the frame (shaded windows, windows set not to accept keyboard input); in that case, unmapping the frame implies a focus-out. And in fact, in all case, unmapping the frame implies a focus-out. (if live_hidden_windows is true, we never unmap frames, so it's not normal, but it's at least accurate)

Should we only call handle_widow_focus_event if the window is the server focus window? The code seems to produce confuse debug spew at a minimum if called for unfocused windows on unmap.

(In reply to comment #8)
> I think it might be good to start this off with:
> 
>  This is the rare "globally active" case.

As it happens, this is not actually rare: Gdk sets the Input hint on a window according to the accept-focus property, but includes WM_TAKE_FOCUS in WM_PROTOCOLS unconditionally. And when it receives a WM_TAKE_FOCUS request on a window, it ignores it if accept-focus is FALSE. So, every time you click on an accept-focus=FALSE GtkWindow, you hit this "rare" "edge case". Good thing we decided to make it work. :)

(In reply to comment #12)
> Little uncertain about a couple aspects of how the call to
> handle_window_focus_event was added for Unmap events

> +                  handle_window_focus_event (display, window, event);
> 
> Is it right to add this in the !frame_was_receiver block?

No

> Should we only call handle_widow_focus_event if the window is the server focus
> window? The code seems to produce confuse debug spew at a minimum if called for
> unfocused windows on unmap.

Indeed, although that was a pre-existing bug.

Created attachment 195450 [details] [review]
test-focus: test program for focus window management

Created attachment 195451 [details] [review]
display: clean up focus_window vs expected_focus_window

====

new-and-improved. We now keep server_focus_window as a Window rather
than as a MetaWindow, and so we can track all focus events, not just
those with an associated MetaWindow, and so we don't need to pretend
that UnmapNotify events are FocusOut events.

Created attachment 218766 [details] [review]
test-focus: test program for focus window management

Created attachment 218767 [details] [review]
display: clean up focus_window vs expected_focus_window

Mutter previously defined display->focus_window as the window that the
server says is focused, but kept display->expected_focus_window to
indicate the window that we have requested to be focused. But it turns
out that "expected_focus_window" was almost always what we wanted.

Make MetaDisplay do a better job of tracking focus-related requests
and events, and change display->focus_window to be our best guess of
the "currently" focused window (ie, the window that will be focused at
the time when the server processes the next request we send it).



Rebased onto master. I'm picking this up now :)

Created attachment 218768 [details] [review]
workspace: Respect the not_this_one parameter passed in

This fixes a crash when closing a window with the previous cleanup

Created attachment 219461 [details] [review]
display: clean up focus_window vs expected_focus_window

Mutter previously defined display->focus_window as the window that the
server says is focused, but kept display->expected_focus_window to
indicate the window that we have requested to be focused. But it turns
out that "expected_focus_window" was almost always what we wanted.

Make MetaDisplay do a better job of tracking focus-related requests
and events, and change display->focus_window to be our best guess of
the "currently" focused window (ie, the window that will be focused at
the time when the server processes the next request we send it).



Obsoleting workspace patch because it's been pushed.

Attaching new patch that corrects a bug where a new window will not appear to have focus, even if it does.

Created attachment 244662 [details] [review]
test-focus: test program for focus window management

Created attachment 244663 [details] [review]
window: Refactor "got focus" code

Just move this out to a separate function.

Created attachment 244664 [details] [review]
window: Merge got_focus/lost_focus to a new function

Make it a static function for now, but this will be a private
function soon, replacing meta_window_lost_focus. This should
contain no functional changes, only cosmetic indentation changes,
so best viewed with ignorews=1 or -w or -b, you know the drill.

Created attachment 244665 [details] [review]
window: Clean up the set_focused_internal function

Move things out of an indentation layer, and reshuffle
things around.

Created attachment 244666 [details] [review]
display: clean up focus_window vs expected_focus_window

Mutter previously defined display->focus_window as the window that the
server says is focused, but kept display->expected_focus_window to
indicate the window that we have requested to be focused. But it turns
out that "expected_focus_window" was almost always what we wanted.

Make MetaDisplay do a better job of tracking focus-related requests
and events, and change display->focus_window to be our best guess of
the "currently" focused window (ie, the window that will be focused at
the time when the server processes the next request we send it).

Created attachment 244667 [details] [review]
display: Use XI2 constants for mode/detail focus event values

This makes no functional difference, except conceptual clarity.

Created attachment 244668 [details] [review]
window: Properly handle focusing override redirect windows

If an app pops up an OR window and sets input focus to it, like
Steam does, we'll think the focus window is null, causing us to
think the app is not focused.

OR windows should not be special if they get input focus, where
the input focus would be set to NULL. Instead, the window should
be marked as focused.

Review of attachment 244666 [details] [review]:

Mostly looks good, few comments and comments about comments, and one thing about the code (the change to meta_window_unmanage()) where it seems like we might get into trouble in some edge cases.

::: src/core/display.c
@@ +1874,3 @@
+      /* Make sure that signals handlers invoked by
+       * meta_window_set_focused_internal() don't see
+       * display->focus_window->has_focus == FALSE

Is the "don't" here stray, meaning that in the handlers, we want  window->has_focus iff. display->focus_window == window to hold? (Text would still need some improvement other than remove "don't")

And then again, the main signal handler is the property notification which is in this function? Overall, I'm just confused :-)

@@ +1980,3 @@
+    {
+      display->server_focus_window = event->event;
+      display->server_focus_serial = serial;

Just a note from our conversation about short-circuiting, since we don't exclude NotifyAncestor, we *can* certainly get a FocusIn on a window which already has focus. In which case, because we do have the short-circuiting in set_focus_window, we'll update the display->focus_serial, and do nothing more, which is fine.

@@ +1983,3 @@
+
+      if (window && window->override_redirect)
+        focus_window = NULL;

I'm confused why I don't see this removed in the later O-R patch

@@ +2054,3 @@
   display->monitor_cache_invalidated = TRUE;
+
+  if (event->xany.serial > display->focus_serial &&

Hmm, the > here is a little unexpected - events with a specified serial refer to only the request we made and *later* events. So we'll have to make another request before we found out that the focus failed, and simply seeing incoming events won't do that. But thinking about it some more, I think that's fine, since using >= would be tricky - we have to wait for *all* focus events generated  by our SetInputFocus to be seen before a >= serial indicates failure - and the complexity of 

 event->xany.serial > display->focus_serial ||
 (<event is not a core or XI focus event> && event->xany.serial >= display->focus_serial)

seems unwarranted. So leave as is.

@@ -2377,3 @@
-
-              meta_topic (META_DEBUG_FOCUS,
-                          "Focus %s event received on root window 0x%lx "

This log was lost, but I guess that's fine, since we do log when we actually do something for root window focus events. If someone is debugging a problem with the code paths, they'll probably have to add it back...

::: src/core/window.c
@@ -1796,3 @@
-  if (window->display->focus_window == window)
-    {
-      window->display->focus_window = NULL;

I'm confused by this removal - we can meta_window_unmanage() a window before the X window is destroyed in a few cases (like having the parent dialog of an attached transient window destroyed), in which I *think* we'll free the MetaWindow and leave window->display->focus_window pointing to junk.

Review of attachment 244662 [details] [review]:

Looking at this test case, I have two thoughts:

 - This is pretty dependent on the details of GTK+ (not much we can do about that)
 - How would we actually know if Mutter got confused? The main thing that you'll see is the "backdrop" theme state, but that comes from Mutter's idea of what window is focused. Maybe we should pack some widget into all the windows (including the toplevel) that is tied to notify::active on the toplevel and shows "FOCUSED" or "NOT FOCUSED" ?

::: src/wm-tester/test-focus.c
@@ +35,3 @@
+      if (protocols[i] == wm_take_focus)
+	{
+	  protocols[i] = protocols[--n_protocols];

Just test code, but this still deserves to be called out as pointlessly tricky code. The compression of:

 protocols[i] = protocols[n_protocols - 1];
 n_protocols--;

To this is in no way actually meaningful, it's just trying to save writing some characters.

@@ +48,3 @@
+clear_input_hint (GtkWidget *window)
+{
+  GdkWindow *gdkwindow = gtk_widget_get_window (window);

I think this is different from disable_take_focus() and doens't realize because it has to be called after show() or GTK+ will overwrite it? If so it would be helpful to have a comment.

@@ +56,3 @@
+  XSetWMHints (GDK_DISPLAY_XDISPLAY (gdk_window_get_display (gdkwindow)),
+	       GDK_WINDOW_XID (gdkwindow),
+	       &wm_hints);

Is it intentional that you are wiping out the rest of WMHints? The flags field doesn't mean "change only this" it means "only this is set"

@@ +157,3 @@
+
+static guint32
+get_current_time_roundtrip (void)

Why not use gdk_x11_get_server_time() ?

Review of attachment 244663 [details] [review]:

Going to assume this is OK

Review of attachment 244664 [details] [review]:

Going to assume this is OK

Review of attachment 244665 [details] [review]:

This removes the short-circuits which is wrong, but doesn't matter a lot, since they *should* be removed in a subsequent patch. Otherwise seems OK.

Review of attachment 244667 [details] [review]:

Looks good

Review of attachment 244668 [details] [review]:

As mentioned in a earlier review, it looks like this patch doesn't actually change things to include O-R windows in tracking focus, it just makes the code in window.c handle that. Can we also extend the test case to test this, so we don't have to test with Steam or whatever? The blinking of the gnome-shell App menu is probably stil visible even for a no-.desktop-file test program.

(Answering the questions about the test case since I wrote that code.)

(In reply to comment #28)
> +clear_input_hint (GtkWidget *window)
> +{
> +  GdkWindow *gdkwindow = gtk_widget_get_window (window);
> 
> I think this is different from disable_take_focus() and doens't realize because
> it has to be called after show() or GTK+ will overwrite it? If so it would be
> helpful to have a comment.

I think that's correct, yes.

> +  XSetWMHints (GDK_DISPLAY_XDISPLAY (gdk_window_get_display (gdkwindow)),
> +           GDK_WINDOW_XID (gdkwindow),
> +           &wm_hints);
> 
> Is it intentional that you are wiping out the rest of WMHints?

No, but it doesn't affect the behavior of the tests either way.

> +static guint32
> +get_current_time_roundtrip (void)
> 
> Why not use gdk_x11_get_server_time() ?

I didn't know that existed and so just copied the code from mutter.

Attachment 244663 [details] pushed as f6dd081 - window: Refactor "got focus" code
Attachment 244664 [details] pushed as 696d9d2 - window: Merge got_focus/lost_focus to a new function
Attachment 244665 [details] pushed as e430e05 - window: Clean up the set_focused_internal function


Gonna push these preliminary cleanups for now, with short-circuit
guards re-added.

(In reply to comment #27)
> Review of attachment 244666 [details] [review]:
> 
> Mostly looks good, few comments and comments about comments, and one thing
> about the code (the change to meta_window_unmanage()) where it seems like we
> might get into trouble in some edge cases.
> 
> ::: src/core/display.c
> @@ +1874,3 @@
> +      /* Make sure that signals handlers invoked by
> +       * meta_window_set_focused_internal() don't see
> +       * display->focus_window->has_focus == FALSE
> 
> Is the "don't" here stray, meaning that in the handlers, we want 
> window->has_focus iff. display->focus_window == window to hold? (Text would
> still need some improvement other than remove "don't")
> 
> And then again, the main signal handler is the property notification which is
> in this function? Overall, I'm just confused :-)

It reads well to me. From a preliminary reading, the only signal handler that can be invoked by meta_window_set_focused_internal(FALSE); is notify::appears-focused, but it's possible that we want more to be.

What it's saying is that the signal handler may want to check display->focus_window, which without the NULL set, will get set to the window, so we'll have this weird state of display->focus_window->has_focus being FALSE. Setting focus_window to NULL before calling the signal handler makes it so that those signal handlers believe that there is no focused window, which is correct at that point.

> @@ +1980,3 @@
> +    {
> +      display->server_focus_window = event->event;
> +      display->server_focus_serial = serial;
> 
> Just a note from our conversation about short-circuiting, since we don't
> exclude NotifyAncestor, we *can* certainly get a FocusIn on a window which
> already has focus. In which case, because we do have the short-circuiting in
> set_focus_window, we'll update the display->focus_serial, and do nothing more,
> which is fine.

Ah, OK, that makes sense.

> @@ +1983,3 @@
> +
> +      if (window && window->override_redirect)
> +        focus_window = NULL;
> 
> I'm confused why I don't see this removed in the later O-R patch

I do remember removing it, and I tested the patch a lot, so I'm going to take the convenient "blame-it-on-rebase" excuse.

> ::: src/core/window.c
> @@ -1796,3 @@
> -  if (window->display->focus_window == window)
> -    {
> -      window->display->focus_window = NULL;
> 
> I'm confused by this removal - we can meta_window_unmanage() a window before
> the X window is destroyed in a few cases (like having the parent dialog of an
> attached transient window destroyed), in which I *think* we'll free the
> MetaWindow and leave window->display->focus_window pointing to junk.

Immediately before that is:

+  g_assert (window->display->focus_window != window);

Which implements the same guarantee, but more strictly. The reason being is that directly above *that* is:

  if (window->has_focus)
    {
      meta_topic (META_DEBUG_FOCUS,
                  "Focusing default window since we're unmanaging %s\n",
                  window->desc);
      meta_workspace_focus_default_window (window->screen->active_workspace,
                                           window,
                                           timestamp);
    }
  else
    {
      meta_topic (META_DEBUG_FOCUS,
                  "Unmanaging window %s which doesn't currently have focus\n",
                  window->desc);
    }

Which I'm quite sure will select the right window, or none, in that case.

Created attachment 245067 [details] [review]
test-focus: test program for focus window management

Created attachment 245068 [details] [review]
window: Properly handle focusing override redirect windows

If an app pops up an OR window and sets input focus to it, like
Steam does, we'll think the focus window is null, causing us to
think the app is not focused.

OR windows should not be special if they get input focus, where
the input focus would be set to NULL. Instead, the window should
be marked as focused.



Properly remove the obviously broken override-redirect case.

Review of attachment 245067 [details] [review]:

Looks good

Review of attachment 245068 [details] [review]:

OK

(In reply to comment #36)
> (In reply to comment #27)
> > Review of attachment 244666 [details] [review] [details]:
> > 
> > Mostly looks good, few comments and comments about comments, and one thing
> > about the code (the change to meta_window_unmanage()) where it seems like we
> > might get into trouble in some edge cases.
> > 
> > ::: src/core/display.c
> > @@ +1874,3 @@
> > +      /* Make sure that signals handlers invoked by
> > +       * meta_window_set_focused_internal() don't see
> > +       * display->focus_window->has_focus == FALSE
> > 
> > Is the "don't" here stray, meaning that in the handlers, we want 
> > window->has_focus iff. display->focus_window == window to hold? (Text would
> > still need some improvement other than remove "don't")
> > 
> > And then again, the main signal handler is the property notification which is
> > in this function? Overall, I'm just confused :-)
> 
> It reads well to me. From a preliminary reading, the only signal handler that
> can be invoked by meta_window_set_focused_internal(FALSE); is
> notify::appears-focused, but it's possible that we want more to be.
> 
> What it's saying is that the signal handler may want to check
> display->focus_window, which without the NULL set, will get set to the window,
> so we'll have this weird state of display->focus_window->has_focus being FALSE.
> Setting focus_window to NULL before calling the signal handler makes it so that
> those signal handlers believe that there is no focused window, which is correct
> at that point.

Ah, OK, I missread it as 'window->has_focus == FALSE' rather than display->focus_window->has_focus == FALSE. Good as is.

> > ::: src/core/window.c
> > @@ -1796,3 @@
> > -  if (window->display->focus_window == window)
> > -    {
> > -      window->display->focus_window = NULL;
> > 
> > I'm confused by this removal - we can meta_window_unmanage() a window before
> > the X window is destroyed in a few cases (like having the parent dialog of an
> > attached transient window destroyed), in which I *think* we'll free the
> > MetaWindow and leave window->display->focus_window pointing to junk.
> 
> Immediately before that is:
> 
> +  g_assert (window->display->focus_window != window);
> 
> Which implements the same guarantee, but more strictly. The reason being is
> that directly above *that* is:
> 
>   if (window->has_focus)
>     {
>       meta_topic (META_DEBUG_FOCUS,
>                   "Focusing default window since we're unmanaging %s\n",
>                   window->desc);
>       meta_workspace_focus_default_window (window->screen->active_workspace,
>                                            window,
>                                            timestamp);
>     }
>   else
>     {
>       meta_topic (META_DEBUG_FOCUS,
>                   "Unmanaging window %s which doesn't currently have focus\n",
>                   window->desc);
>     }
> 
> Which I'm quite sure will select the right window, or none, in that case.

OK, make sense.

Attachment 244666 [details] pushed as 7a4c808 - display: clean up focus_window vs expected_focus_window
Attachment 244667 [details] pushed as d03ffd8 - display: Use XI2 constants for mode/detail focus event values
Attachment 245067 [details] pushed as 4f1d621 - test-focus: test program for focus window management
Attachment 245068 [details] pushed as df8234c - window: Properly handle focusing override redirect windows