GNOME Bugzilla – Bug 135786
Middleclick should lose focus
Last modified: 2004-12-22 21:47:04 UTC
When a window is middleclicked, it should loose the focus to the toplevel window. Often I want to re-focus a window (I just middleclicked) with the panel and then it minimizes (because it has the focus). Sometimes I start to type something, until I notice, that I'm typing in the wrong window. This is really annoying, IMO
What if the window you middle-click on *is* the toplevel window? What about applications that already have a behavior defined for a middle-click (which the vast majority of the ones I interact with do); are you proposing to force the developers of all those applications to rewrite their program so that you can have yet another way to choose which window has the focus? Or am I not understanding your request? (I admit, I had to read it a couple times to try to understand what you were requesting and why and I may have misunderstood) Anyway, I'm tacking the bugsquad keyword on.
the only sense I can make of this is that he's talking about the problem solved by #118372. *** This bug has been marked as a duplicate of 118372 ***
Sorry, when I wasn't specific enough. When you middleclick on a window_title_ (not into the window) it moves to the background, but doesn't looses its focus. So the window is invisible, but still active. Sometimes this is really confusing to me. When the window was the toplevel window, it should give the focus to the window, which is now the toplevel one. I reopen this bug. If it still makes no sense to you, I'm sorry for wasting your time ;)
OK I see what you mean now. Hmmmm. If you use sloppy or mouse focus the window under your pointer will end up focused because of an EnterNotify. Should we explicitly implement that for click? Or should it focus the first window in the MRU list that's not the window itself? That could seem rather weird and inconsistent. Not really sure what the best thing here is.
Well, here's my $0.02 (which is worth even less than normal, considering the decline of the dollar): Basically, what piniator is asking for amounts to an alternate form of minimization. The exact way that the minimization is carried out (hidden & taken to the taskbar vs. lowered and defocused) is slightly different, but the basic effect is the same--the window is removed from the user's attention so that they can work in a different window. This also means removing the ability to make a window become lowered while still working with it. Now, this is of questionable utility to many but for some of those that work with sloppy/mouse focus modes with autoraise turned off, the means removing the only way to do a certain task that they may be used to doing--with the only reason being to obtain an alternate way to perform some other task. My opinion is that both behaviors are edge cases, but that making piniator's change involved making something formerly possible become impossible, while only gaining a different visual method for an already existing behavior. But I'm not a usability expert (though I make attempts to be at least an amateur), so I'll cc the usability people to get their opinion.
If you middle-click on the titlebar of a window, it gets moved to the back, but it still has focus. That's weird. Then, if you click in the window (not on its titlebar), it doesn't come to the front. You have to either click on something else (to un-focus it) and then click on the window again, or click on its titlebar. That's weird, too. What I expect to happen: If I middle-click on the titlebar of the focused window, it goes to the back, and becomes un-focused (i.e., no window is now focused, as if I'd clicked on the desktop). When focus_mode is 'click', no window can be focused when not on top, so the current behavior -- putting the focused window on the bottom without un-focusing it -- feels very inconsistent. This bug doesn't affect people using mouse or sloppy focus. With mouse/sloppy focus, when you middle-click a titlebar, the correct window automatically gets focused (either the same window, or the new window that's topmost at that point).
Created attachment 31278 [details] [review] In click-to-focus mode, when lowering a window, un-focus it. In click-to-focus mode, when lowering a window, un-focus it. This makes it easier to re-raise it (you can click in the window). It also removes the inconsistency of click-to-focus: "focused windows are always on top, except when they aren't". It doesn't do anything to mouse or sloppy focus, because they already act consistently on middle-click. This patch changes the meaning of meta_core_user_lower(). It works (that function is only called once), but it may not be the right level of abstraction for it. I'm not really a Metacity hacker...
Your last paragraph in comment 6 makes a lot of sense to me. Its totally consistent with the policy specified in bug 135810 too. I only looked at your patch briefly and haven't looked at the surrounding code, but I do have a couple comments regarding your patch (note that I'm not the maintainer, so this is merely my opinion; Rob or Havoc will have to give a more official word): It is easier to review if you use the -p flag. The patch should not use XSetInputFocus; meta_workspace_focus_default_window is the function that should be used to make a different window gain focus.
This patch isn't quite right, I think here we want to maybe have meta_core_user_lower_and_unfocus or something which calls focus_default_window after lowering. Unfocusing does make sense to me, and yeah I'd only do it in click to focus mode since in the mouse modes it's just likely to mess things up.
I just realized that I had made an assumption about how this patch would be implemented, and that this assumption may not have occurred to others. In click-to-focus, the invariant that the mru window is equal to the toplevel window is maintained. I see no reason to break that invariant here (if we did, there would almost certainly be breakage of some kind; for example, we'd probably have to change meta_workspace_focus_default_window to call meta_workspace_focus_toplevel_window instead of meta_workspace_focus_mru_window for click-to-focus), so if we move a window to the bottom of the stack, we should probably also move it to the bottom of the mru list.
Yeah, I know my patch isn't the right way to do it, but IME it's easier to get from a bad patch to a good one than from no patch to a good one. :-) I saw ...focus_*_window(), but they didn't appear to have any way to un-focus all windows, which is what my suggestion called for. (Elsewhere in the code, XSetInputFocus() was used for this purpose.) So I guess the first question is: when you un-focus this window, do you even want to focus something else? I thought that would be weird, so I decided to simply un-focus everything. (I'm not against focusing some other window, if it can be done in a way that always makes sense.) I tried simply replacing my XSetInputFocus() call with meta_workspace_focus_*_window(window->screen->active_workspace, window). All 3 variants (default, top, mru) yield strange behavior. Basically, if you have a big pile of windows, you can't ever get to the bottom by middle-clicking their titlebars (as you can now), because the ones you've just lowered pop back to the top when you lower another one. (I think this is what Elijah is saying about moving it to the tail of the mru list. If you did that, focus_mru would probably work fine.) For unfocus-everything, I think it should be something like meta_core_user_focus(xdisplay, display->no_focus_window, CurrentTime), but that doesn't work. (I think I've just demonstrated the limit of my X11/Metacity knowledge...)
Absolutely, see bug 150615 for an example where I submitted several crappy patches before I got it right. :-) For click to focus (and sloppy focus) we currently always have some window focused; it would seem odd to me to change that. focus_mru_window and focus_toplevel_window shouldn't be used outside of workspace.c; we should make them static functions and remove them from workspace.h (there have been several bugs related to this with choosing the wrong focus window in sloppy or mouse focus modes; focus_default_window is designed to do the right thing for all focus modes). But, for click-to-focus, all three functions currently do the exact same thing (except that maybe focus_toplevel_window doesn't filter panels to the bottom of the list?)... And yes, the fact that the first lowered window pops back to the top when you lower a second window is exactly because the window hasn't been shuffled to the bottom of the mru list. (You will probably also find that if you use alt-tabbing to switch windows that the first lowered window isn't where you expect it in the list) You can modify the mru_list (window->screen->active_workspace->mru_list) with the g_list_remove and g_list_append functions.
"For click to focus (and sloppy focus) we currently always have some window focused; it would seem odd to me to change that." If you click on the desktop, it doesn't *look* like any window is focused. That's all I meant. (I guess in truth the desktop is focused, or maybe no_focus_window is focused, or something.)
Yes, the desktop is a window (with a specialized type, window->type == META_WINDOW_DESKTOP), as is the panel (which has window->type == META_WINDOW_DOCK). But in both cases, we make the user explicitly click on those windows to give them focus. And for both of those focus methods, we try to make sure we never have no focus window, and thus we never let the no_focus_window be the focused one. Personally, I view the lowering operation similar to minimizing or closing a window--you're doing something to dismiss the window so that you can work with others. For both minimizing and closing, we focus the new default window. Why should this case be different?
ok that's driving me nuts. Carry on.
Created attachment 31340 [details] [review] Focus the new topmost window Elijah: OK, I'm sold. Here's a patch that sucks less. It still isn't quite right, so I'm apparently still missing something, but it's closer. Example of how to make it not work quite right: Open 3 windows, A, B, and C, and overlap them. Click on A, then B, then C (so C is on top). Now middle-click B's titlebar, then middle-click A's titlebar, then middle-click C's titlebar. The lowering works fine, but after the third middle-click, the panel gets focus (!). (I call ...focus_default_window(), which calls ...focus_mru_window(), which finds the panel. I don't see any panel-avoidance logic in any of the focus_*_window() functions. But it works ok if you only ever middle-click the focused window. Hmm...)
Looks pretty good so far, a couple comments: 1) You should only focus a new window if the one you lower had focus. 2) I'm worried about potential race conditions in the code. Perhaps I'm being paranoid, but similar code found in meta_window_notify_focus in the "if (event->type == FocusIn)" section also has code to shuffle the mru list and caused a large headache in a hard to track down bug (see bug 122016). Could you also add g_assert's like that code has, to make me feel safer if nothing else? 3) You must be using a version of Metacity from before August 2nd. Bug 120100 introduced panel avoidance logic in the focus_mru_window function. Upgrading should fix that, but you could also make use of the shuffle_docks_to_end function in window.c--the code in meta_window_notify_focus to shuffle the mru list (which was written before August 2nd) does that.
Created attachment 31347 [details] [review] Only focus new window if the lowered window had focus 1. OK, fixed, I think. (Is checking window->has_focus OK?) 2. (Reading all 74 comments...) Ouch. I'm all for adding asserts if it helps avoid a mess like that again. You're talking about adding code like window.c, lines 4330 - 4357 (big block comment), right? Basically, make sure it's on the workspace, and make sure it's in the MRU list. I don't know if we need shuffle_docks_to_end() here -- docks have no titlebar, so you wouldn't see them in this codepath, anyway, but I don't know if some other windows could make it into the dock group. (Do we? It's private to window.c right now.) 3. True, I was lazy and just grabbed the source for the version in Debian (2.8.1). :-) I've now upgraded to 2.8.4, and it does work fine now.
Created attachment 31348 [details] [review] With safety nets Version using the paranoia code from window.c. (This is what you meant, right, Elijah?) Only changes I made to it: - use append(), instead of prepend() (because we're lowering it) - removed last if-statement (is_in_dock_group() -> shuffle_docks_to_end()) The second one I'm not sure about. Both the condition and statement use functions private to window.c, and I'm afraid to mess with that. I'm not sure they're needed in this case, but then, nobody's sure any of this is needed -- it's just there to be on the safe side, at this point. (It's working fine for me, but the bugs we're trying to avoid are apparently hard to reproduce.) Does that if-statement help us be any safer?
Yes, checking window->has_focus is sufficient. Yeah, that's what I meant as far as the sanity checks. However, this isn't a FocusIn event but rather a middle-click button event, so the comment needs to change slightly. You could probably simplify it too, maybe just something like "move the window to the bottom of the MRU list. Do some sanity checks just to avoid possible race conditions." I'm not sure if the g_assert's will really be needed (this is a different circumstance), but the mru shuffling just reminded me of that bug. Rob, who actually tracked down and fixed 122016, or Havoc would probably know better about whether this should be needed. The shuffle_docks_to_end() isn't needed. If it weren't for the filtering of DOCK windows in focus_mru_window introduced in bug 120100 it would be. But, now that I think about it, since we do have that filtering in place, we should probably remove shuffle_docks_to_end() from window.c as well--it shouldn't be needed any more. The only remaining issue I see is that you should probably rename meta_core_user_lower to meta_core_user_lower_and_unfocus as per what Havoc said in comment 9. (I'm guessing you should rename the function instead of just adding a new one, because there is no other call to meta_core_user_lower anywhere). Anyway, I don't see any other problems so after you make that change you'll just need to wait for Havoc to review.
Created attachment 31349 [details] [review] More cleanups: fixed comment, named function appropriately OK, done and done. How's it look? Elijah gets about 90% of the credit for this one. As you can see, he told me what to do; I just typed it in. Thanks. :-)
Hehe, no. You were able to argue convincingly for what the correct behavior was and why. You may not realize this yet, but being able to do that is often worth a thousand patches in Metacity bugs. To get an idea of why that is, just check out some of the bugs listed in the rationales.txt file in the source code, such as the minimized windows in Alt+Tab or keeping the panel on top. You have a skill that is extremely useful to Metacity, and I hope you stick around to continue working on bugs. You were also able to identify the correct region of code that needed changing; the other changes (meta_workspace_focus_default_window instead of XSetInputFocus, shuffling the mru list, and assertion sanity checks) are all things that would require familiarity with Metacity to know about, which, by definition, a new contributor couldn't be aware of. So your coding was great too. /me crosses his fingers and hopes that he didn't lead you astray with any part of the patch, because otherwise he'll feel dumb when Havoc reviews it
Comment on attachment 31349 [details] [review] More cleanups: fixed comment, named function appropriately Patch looks reasonable to me, one "no space before parens" to fix. Thanks!
Created attachment 31439 [details] [review] Going for 10/10 on style Darn. I was doing the humility thing really well, too. (I'm usually pretty good at it. I bet I could out-humble Linus! Er, wait...) I don't know that I'll stick around Metacity much now. It's unannoying and unobtrusive, for the most part; the patches for the 2 bugs I've submitted are about all I can think of that's wrong with Metacity. So unless somebody wants to pay me to hack Metacity (which would be fun), I'll probably go find some other program I use that's annoying me more. I've got plenty... :-) Here's a final (maybe!) patch. Fixed the paren, and changed tabs to spaces, which it seems Metacity usually (but not always) uses.
Ken: Do you have cvs write access, or should I commit this for you?
Oops, sorry. No, I don't have access. (I just toss patches over the wall.)
Committed: 2004-09-15 Elijah Newren <newren@math.utah.edu> Patch from Ken Harris in #135786 to focus a new default window when lowering via middle-click on the frame. * src/core.[hc], src/frames.c: rename meta_core_user_lower to meta_core_user_lower_and_unfocus * src/core.c (meta_core_user_lower_and_unfocus): if in click-to-focus mode then also move the window to the back of the mru list and focus the new default window for the active workspace