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 135786 - Middleclick should lose focus
Middleclick should lose focus
Status: RESOLVED FIXED
Product: metacity
Classification: Other
Component: general
2.8.x
Other All
: Normal normal
: ---
Assigned To: Metacity maintainers list
Metacity maintainers list
Depends on:
Blocks:
 
 
Reported: 2004-02-29 21:46 UTC by piniator
Modified: 2004-12-22 21:47 UTC
See Also:
GNOME target: 2.8.x
GNOME version: 2.7/2.8


Attachments
In click-to-focus mode, when lowering a window, un-focus it. (470 bytes, patch)
2004-09-05 01:07 UTC, Ken Harris
none Details | Review
Focus the new topmost window (795 bytes, patch)
2004-09-06 21:31 UTC, Ken Harris
none Details | Review
Only focus new window if the lowered window had focus (820 bytes, patch)
2004-09-07 01:03 UTC, Ken Harris
none Details | Review
With safety nets (1.50 KB, patch)
2004-09-07 01:14 UTC, Ken Harris
none Details | Review
More cleanups: fixed comment, named function appropriately (2.49 KB, patch)
2004-09-07 02:11 UTC, Ken Harris
accepted-commit_after_freeze Details | Review
Going for 10/10 on style (2.73 KB, patch)
2004-09-09 10:21 UTC, Ken Harris
accepted-commit_after_freeze Details | Review

Description piniator 2004-02-29 21:46:58 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
Comment 1 Elijah Newren 2004-02-29 22:51:58 UTC
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.
Comment 2 Rob Adams 2004-03-01 00:59:34 UTC
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 ***
Comment 3 piniator 2004-03-01 12:47:43 UTC
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 ;)
Comment 4 Rob Adams 2004-03-02 03:02:20 UTC
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.
Comment 5 Elijah Newren 2004-03-02 04:29:51 UTC
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.
Comment 6 Ken Harris 2004-09-05 01:02:16 UTC
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).
Comment 7 Ken Harris 2004-09-05 01:07:29 UTC
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...
Comment 8 Elijah Newren 2004-09-05 01:22:37 UTC
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.
Comment 9 Havoc Pennington 2004-09-05 01:56:25 UTC
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.
Comment 10 Elijah Newren 2004-09-05 02:59:24 UTC
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.
Comment 11 Ken Harris 2004-09-05 05:11:18 UTC
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...)
Comment 12 Elijah Newren 2004-09-05 14:39:57 UTC
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.
Comment 13 Ken Harris 2004-09-05 21:47:14 UTC
"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.)
Comment 14 Elijah Newren 2004-09-05 22:13:34 UTC
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?
Comment 15 Rob Adams 2004-09-05 22:39:49 UTC
ok that's driving me nuts.  Carry on.
Comment 16 Ken Harris 2004-09-06 21:31:14 UTC
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...)
Comment 17 Elijah Newren 2004-09-06 22:57:11 UTC
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.
Comment 18 Ken Harris 2004-09-07 01:03:01 UTC
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.
Comment 19 Ken Harris 2004-09-07 01:14:37 UTC
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?
Comment 20 Elijah Newren 2004-09-07 01:33:19 UTC
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.
Comment 21 Ken Harris 2004-09-07 02:11:15 UTC
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.  :-)
Comment 22 Elijah Newren 2004-09-07 04:02:54 UTC
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 23 Havoc Pennington 2004-09-07 14:08:28 UTC
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!
Comment 24 Ken Harris 2004-09-09 10:21:33 UTC
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.
Comment 25 Elijah Newren 2004-09-15 16:55:48 UTC
Ken: Do you have cvs write access, or should I commit this for you?
Comment 26 Ken Harris 2004-09-15 23:39:55 UTC
Oops, sorry.  No, I don't have access.  (I just toss patches over the wall.)
Comment 27 Elijah Newren 2004-09-16 00:07:06 UTC
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