GNOME Bugzilla – Bug 131582
Race Condition: Choosing window to focus
Last modified: 2004-12-22 21:47:04 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).
Created attachment 23390 [details] Screenshot of my setup to test the focus behavior on window minimize/close
Created attachment 23391 [details] [review] Fix the race condition by focusing the window under the mouse in sloppy/mouse focus modes
Created attachment 23392 [details] [review] *Really* ugly patch which handles the caveat case; meant for informational purposes only
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).
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.
Ok, I'll try to figure it out. :)
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?
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.
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 ***
Reopening due to Havoc's comments in bug 135810.
Created attachment 25150 [details] [review] Fix the race condition using _METACITY_SENTINEL
We've branched; is this okay to apply?
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.
Committed.
*** Bug 147947 has been marked as a duplicate of this bug. ***