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 131582 - Race Condition: Choosing window to focus
Race Condition: Choosing window to focus
Status: RESOLVED FIXED
Product: metacity
Classification: Other
Component: general
2.7.x
Other Linux
: High major
: ---
Assigned To: Metacity maintainers list
Metacity maintainers list
Depends on:
Blocks:
 
 
Reported: 2004-01-15 17:35 UTC by Elijah Newren
Modified: 2004-12-22 21:47 UTC
See Also:
GNOME target: ---
GNOME version: 2.7/2.8


Attachments
Screenshot of my setup to test the focus behavior on window minimize/close (151.71 KB, image/png)
2004-01-15 17:36 UTC, Elijah Newren
  Details
Fix the race condition by focusing the window under the mouse in sloppy/mouse focus modes (2.09 KB, patch)
2004-01-15 17:37 UTC, Elijah Newren
none Details | Review
*Really* ugly patch which handles the caveat case; meant for informational purposes only (2.54 KB, patch)
2004-01-15 17:39 UTC, Elijah Newren
none Details | Review
Fix the race condition using _METACITY_SENTINEL (1.67 KB, patch)
2004-03-04 06:09 UTC, Elijah Newren
accepted-commit_now Details | Review

Description Elijah Newren 2004-01-15 17:35:30 UTC
Disclaimer: I may not be correct that this is a race condition or be
understanding correctly why this occurs.  However, I can definitely state
that the behavior for the window chosen to be focused is NOT deterministic.
 See my description below...

The problem: When one window is below the window being closed or minimized,
X believes that the mouse has entered the window below.  Metacity tries to
focus the mru window (which may be a different window than the one that
will contain the mouse after the closing/minimization) upon window
close/minimize, but it also receives an X event say the mouse has entered
the window below so Metacity will also try to focus it.  The result is that
part of the time the MRU window is focused, and part of the time the window
that contains the mouse is focused (and of course, there can also be
flickering where one of the two windows gets focus long enough for the user
to notice but it quickly changes to a different window).  I will attach a
screenshot showing the setup I used to duplicate this (namely a lowered
mozilla window with two gnome-terminals, where the two gnome-terminals
cover parts of the bottom of the Mozilla windows, and where I move the
mouse from one gnome-terminal to the next and then close that gnome-terminal).

Some history: Before last August, Metacity used
meta_workspace_focus_top_window to choose which window should be focused. 
This could result in the window both furthest away from the mouse and least
recently used being focus.  It was then switched to the MRU window, which
at least made some more sense.  However, it still seems to be suboptimal. 
I think if the mouse has entered another window due to a window being
closed or minimized, then the window the mouse has entered (at least in
sloppy/mouse focus modes) should be the one that is focused.  This isn't
because X thinks the mouse has entered the window (although it's nice that
X seems to agree with me), but because that's what makes sense to me as a
user.  I think meta_workspace_focus_mru_window should be replaced by
meta_workspace_focus_default_window on both the grounds that it is more
intuitive and the fact that it gets rid of the race condition.  I have a
working patch that does that and I will attach it in a moment.

A caveat: When no window is below the one being closed or minimized, then
no window will be focused in either mouse or sloppy focus modes.  This
makes perfect sense for mouse focus.  I think it makes sense for sloppy
focus (the mouse hasn't entered any of the other windows).  However, others
may disagree with me on the grounds that they would consider the mouse
would have still been considered to have come from a recently used window
and thus argue that if no window is below the mouse then the most recently
used window should be focused.  While I don't like behavior (not only
because it adds more complexity to the code), I do have an ugly patch to
handle that case as well (but if it's applied in all its ugliness, then the
other patch should not be applied).
Comment 1 Elijah Newren 2004-01-15 17:36:19 UTC
Created attachment 23390 [details]
Screenshot of my setup to test the focus behavior on window minimize/close
Comment 2 Elijah Newren 2004-01-15 17:37:15 UTC
Created attachment 23391 [details] [review]
Fix the race condition by focusing the window under the mouse in sloppy/mouse focus modes
Comment 3 Elijah Newren 2004-01-15 17:39:31 UTC
Created attachment 23392 [details] [review]
*Really* ugly patch which handles the caveat case; meant for informational purposes only
Comment 4 Elijah Newren 2004-01-15 17:41:54 UTC
Setting priority to high because of the patch.  Let me repeat: I don't
like the second patch and think the first one should be applied
instead.  I attach the second one just in case someone really thinks
that the mru window should be focused if no window is under the
pointer in sloppy focus modes.  If the maintainers agree with that,
the second patch probably needs some cleaning up (by making a new
function or renaming it or something).
Comment 5 Rob Adams 2004-01-15 17:48:32 UTC
The correct way to fix this is to use the focus sentinel.  Look at the
way it works for moving a window to a different workspace.  Basically
we need to call the meta_display_increment_focus_sentinel function
right before we do the minimize.  Metacity will then ignore window
crossing events until we receive the PropertyNotify on the
_METACITY_SENTINEL property.  That will get rid of the race condition.
 Should be a pretty simple patch since the focus sentinel machinery is
already in place.
Comment 6 Elijah Newren 2004-01-15 18:13:19 UTC
Ok, I'll try to figure it out.  :)
Comment 7 Elijah Newren 2004-01-15 20:00:27 UTC
Uh, I must not be understanding.  I grepped through the src directory
for sentinel and found the meta_display*focus_sentinel* functions and
the _METACITY_SENTINEL property.  But the only place I could see it
used, besides the EnterNotify and PropertyNotify callback events in
display.c, was in the idle_calc_showing function of window.c.  I tried
looking through the meta_window_change_workspace function but couldn't
see this stuff being used.

Despite that, I tried to do both
  meta_display_increment_focus_sentinel(window->display);
and
  meta_display_increment_focus_sentinel(window->screen->display);

both immediately after the 'window->minimized = TRUE' line in
meta_window_minimize and immediately before the
meta_workspace_focus_mru_window call.  None of the four things worked.
   Am I going about this all wrong?
Comment 8 Elijah Newren 2004-02-29 03:57:13 UTC
I started looking at this again.  Apparently incrementing the focus
sentinel in meta_window_minimize and meta_window_free does not work
because the PropertyNotify's on the _METACITY_SENTINEL will occur
*before* the EnterNotify for the window below the one being dismissed
(which defeats the whole purpose).  Adding an unconditional
meta_display_increment_focus_sentinel in the idle_calc_showing does
fix the race for minimization, but not for closing of windows.  I'll
dig again when I get a little time.
Comment 9 Elijah Newren 2004-03-01 15:59:28 UTC
I figured it out and made a patch and tested it.  However, part of the
point of removing the race condition was to make things consistent and
predictable; however, there were still a few other problems with
choosing the focus window in mouse/sloppy focus modes so I filed bug
135810.  I'm going to mark this as a duplicate of that bug (note that
I incorporated the changes from this bug into the patch in that bug).
 If you'd like me to submit just the part of the patch that fixes the
race condition, though, let me know.

*** This bug has been marked as a duplicate of 135810 ***
Comment 10 Elijah Newren 2004-03-04 06:05:03 UTC
Reopening due to Havoc's comments in bug 135810.
Comment 11 Elijah Newren 2004-03-04 06:09:18 UTC
Created attachment 25150 [details] [review]
Fix the race condition using _METACITY_SENTINEL
Comment 12 Elijah Newren 2004-06-24 17:11:34 UTC
We've branched; is this okay to apply?
Comment 13 Rob Adams 2004-06-24 17:19:30 UTC
Comment on attachment 25150 [details] [review]
Fix the race condition using _METACITY_SENTINEL

If you've tested it and it works it looks good to me.  Stuff like this should
be committed sooner rather than later.

It looks like you figured out that what I said earlier is wrong; you want to
increment the sentinel right _after_ you do the minimize so that the unmap
event comes before the PropertyNotify.

So yea, go ahead and commit.
Comment 14 Elijah Newren 2004-06-24 19:53:44 UTC
Committed.
Comment 15 Elijah Newren 2004-07-20 23:52:49 UTC
*** Bug 147947 has been marked as a duplicate of this bug. ***