GNOME Bugzilla – Bug 135810
Make choice of focus window be consistent
Last modified: 2004-12-22 21:47:04 UTC
Basically, there are three circumstances--besides mouse movement/clicking--under which the current window is no longer the one we want focused. These are: switching workspaces (without bringing a window along), minimizing the focused window, and closing the focused window. For any given focus mode, it would aid usability if the rule for all three could be consistent. Currently, Metacity is consistent for click-to-focus, but it has multiple inconsistencies for both sloppy & mouse focus modes. I will list the problems I perceive and provide a suggested solution & attach a patch. I'm sure there's probably other methods to fix the current problems and I know that window focusing is one of those big opinion areas--so if you dislike anything I suggest or have any better ideas, let me know and I'll adjust my patch accordingly. Here's what I think are the current problems: (1) In sloppy focus, one can have a window open on one desktop which is focused, then switch workspaces away and back--and then have the original window not be focused. (See bug 133120) (2) In mouse focus, the decision to focus the mru window upon minimization or closing of the focused window is highly non-intuitive; I believe it's clear that the window under the mouse should be focused. (3) Similar to (2), if another window is under the mouse in sloppy focus, the user would believe that the mouse had "entered" the window and thus it should be focused. Combined with (1), it seems that a "focus the window under the pointer if it exists, otherwise focus the mru window" policy would make the most sense for sloppy focus. (4) There is a race condition upon closing or minimizing windows in both sloppy & mouse focus. This results in the mru window being focused around 90% of the time for minimization (the other 10% is the mouse under the window), and results in the window under the mouse being focused around 90% of the time for closing of the window (the other 10% being the mru window). See bug 131582. So, I'm proposing the rule be: focus-on-click : focus mru window (same as toplevel window) sloppy focus : focus window under pointer if there is such a window, otherwise focus the mru window mouse focus : focus window under pointer if there is one, otherwise don't focus any window. This wouldn't alter anything for focus-on-click, but does require some small changes to sloppy & mouse focus modes. Does this seem sane? Any other ideas/suggestions?
Created attachment 24960 [details] [review] Make sloppy & mouse focusing consistent
*** Bug 131582 has been marked as a duplicate of this bug. ***
I think that the usuability rationale for focusing the MRU window applies equally to sloppy and mouse focus as it does for click to focus. It seems that the best way to fix this is simply to fix the race condition. The focus sentinel really will work for this -- what you have to do is 1) unmap the window existing window, 2) focus the MRU window, 3) increment the focus sentinel, in that order. This way you know that the focus sentinel won't be cleared until after the window has been unmapped _and_ after the MRU window has been focused. This way the EnterNotify events will come in before the focus sentinel PropertyNotify. The focus sentinel is kind of confusing I agree, but if you first understand the asynchronous nature of the metacity connection to the Xserver, and that the server will process events in the order that it receives them, you can understand it.
Created attachment 24983 [details] [review] Just fix the race condition
I've never seen a usability rationale for focusing the MRU window as opposed to the window under the mouse. Bug 108643 (in which focusing the mru window was introduced) provided the rationale for MRU window versus toplevel window, but never considered the window under the mouse. Is there another bug or file that contains this information? Also, the focus sentinel makes sense to me, but it did confuse me for a while because incrementing it immediately after focusing the mru window wouldn't work (the PropertyNotify's on the _METACITY_SENTINEL property would occur before the EnterNotifies for some reason). However, delaying the increment until after some other calls works nicely and I have attached a patch that does so.
Did it not work when you put the increment_focus_sentinel call just after the call to focus_mru_window in meta_window_free? I was about to say that we shouldn't increment the sentinel unless the window had focus but that's not strictly true, since you could have your mouse in the window and have it not be focused.
Yes. I placed the increment_focus_sentinel call in the line (well, lines because there's both the really focused and the expected focus cases) immediately after the focus_mru_window call and the race condition persisted. I moved it to the end of the meta_window_free function, and found that it worked. I didn't bother trying to figure out at which intermediate point(s) it would work and which it wouldn't. I thought about making the increment_focus_sentinel call conditional, but the PropertyNotify's occur so early that I thought it wasn't a big deal (in fact, putting an unconditional increment_focus_sentinel in idle_calc_showing also worked great for the minimization case with no ill side effects that I could see) Do you want me to attach a version of the patch that has the debugging information that makes this easier to view (but only for the race condition part)? Also, to follow up on what I said earlier: I can understand the use of the MRU window for the focus choice for sloppy focus (though in that case I think that switching workspaces should be made to do the same thing for consistency). It's not my favorite choice, but it does make sense. However, focusing the MRU window for mouse focus doesn't make any sense to me. I was wondering if you could explain your viewpoint so that I might understand. Perhaps it does make perfect sense viewed from the right angle, but any angle I can think of doesn't make it click for me.
The bugfix and the deliberate behavior change should be split cvs-commit-wise (and ideally but too late bugzilla-wise) just so we don't confuse ourselves. The race condition fix looks reasonable. I kind of agree with Elijah on the behavior, though I'd phrase it in a more biased way ;-): - click to focus - behave properly - mouse - enforce variant "window focused iff contains mouse" so keyboard navigation and other stuff is totally broken - sloppy - make crackass heuristic guesses at what to focus so it's semi-usable Not sure any of this can go in at current phase of release.
I'm going to have to take back what I said earlier about understanding the focus sentinel. I realized that if I were to try to explain it to someone I couldn't, because I only have a superficial understanding of it. (...and I don't even have a superficial understanding of when and how the PropertyNotify's occur relative to the EnterNotifies in windows. I don't understand why they occur too early (i.e. before the EnterNotify's) in some cases but not in others; I just found out that it does). However, I seem to remember that the unmap (step 1 Rob talked about) occurred after both steps 2 and 3 that Rob mentioned and may have been the reason that the increment_sentinel needed to be delayed. Now that I've said all this... :) Nah, it's not too late to split this bugzilla-wise, though I guess I shouldn't have marked bug 131582 as a duplicate of this one (I only did so because I was thinking of it in terms of "the race condition is one of the things causing inconsistencies"). I'll post the patch to fix the race-condition to that bug and reopen. And I'll shortly post a patch with everything but the race condition... I'm incrementing the version to version 2.7.x, and bumping the keyword to GNOMEVER2.7 (to reflect that we can't commit such changes at this stage)...
Created attachment 25151 [details] [review] Make focusing consistent (but doesn't correctly fix race condition)
We've branched; is this okay to apply?
Comment on attachment 25151 [details] [review] Make focusing consistent (but doesn't correctly fix race condition) OK I think that this is fairly sane for the most part. We do have some weird inconsistencies that this would fix, so I'm willing to give it a shot.
Committed.
*** Bug 123717 has been marked as a duplicate of this bug. ***
For the sake of completeness (since I often refer to this report when explaining choice of focus window in a bug), let me note that there is a case not covered above. That case is having a new window appear on a desktop (via launching a new application, opening a dialog of an existing app, unminimizing some window, etc.). In these cases, the choice is between keeping the current focus window or focusing the new one that appears. To do this, timestamps are used (see bug 118372).