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 599181 - Titlebar double-click causes confusion between windows
Titlebar double-click causes confusion between windows
Status: RESOLVED FIXED
Product: metacity
Classification: Other
Component: Focus
2.28.x
Other Linux
: High critical
: ---
Assigned To: Thomas Thurman
Metacity maintainers list
: 600227 608139 608144 608296 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2009-10-21 12:42 UTC by Anthony Baire
Modified: 2010-10-28 20:09 UTC
See Also:
GNOME target: ---
GNOME version: 2.27/2.28


Attachments
Stop confusing GDK's grab tracking (6.64 KB, patch)
2010-06-10 00:01 UTC, Owen Taylor
reviewed Details | Review
Stop confusing GDK's grab tracking (6.86 KB, patch)
2010-06-17 18:20 UTC, Owen Taylor
reviewed Details | Review

Description Anthony Baire 2009-10-21 12:42:07 UTC
When a window is maximised by a double-click on the title bar, the window manager gets into an unconsistent state. Then the next time another window is selected, the focus is given to the maximised window (instead of the second window).

It is important to note that this bug occurs only if the window is maximised by a double-click on its title bar. It is not reproductible if the 'maximise' button is used. 

To reproduce the bug:
1. double click on the title bar of a window to maximise it
2. select another window (alt+TAB)
3. click on the title bar of this second window
   ---> the maximised window gets the focus


This may be related to bug #568547 (though I did not experience this problem with metacity 2.25.144 or it could be caused by another component...) It is especially disturbing on a dual-head configuration.
Comment 1 Anthony Baire 2009-10-31 23:04:30 UTC
short update: the bug occurs only when linked with gtk+ 2.18. I could not reproduce it with gtk 2.16.1
Comment 2 Anthony Baire 2009-11-01 00:03:00 UTC
I could trace the bug back to the following commit in gtk+. Unfortunately it is a huge commit.

http://git.gnome.org./cgit/gtk+/commit/?id=eabac453e652d5aa2e535d957057f9c84803eea9
Comment 3 Alex Smith 2009-11-28 12:45:55 UTC
This bug also occurs for me. Experienced it on both Arch Linux and Fedora 12.
Comment 4 Adalbert Dawid 2009-11-29 21:04:15 UTC
Same here on debian testing.
Comment 5 Rob 2010-04-07 16:30:39 UTC
I am also having this issue on ubuntu 9.10 (metacity 2.28.0) and I just booted up ubuntu 10.04 beta (4/6/10) (metacity 2.28.1) and It is happening there also.

Please fix this bug! it's awful with dual monitor.
Comment 6 mmonaco27 2010-05-09 13:12:21 UTC
Arch Linux, Metacity 2.30.1. I can't say for sure how long this has been going on for, but it has been a while.
Comment 7 mmonaco27 2010-05-24 04:16:08 UTC
*** Bug 600227 has been marked as a duplicate of this bug. ***
Comment 8 Owen Taylor 2010-06-09 16:23:07 UTC
*** Bug 608139 has been marked as a duplicate of this bug. ***
Comment 9 Owen Taylor 2010-06-09 16:23:25 UTC
*** Bug 608144 has been marked as a duplicate of this bug. ***
Comment 10 Owen Taylor 2010-06-09 16:24:56 UTC
*** Bug 608296 has been marked as a duplicate of this bug. ***
Comment 11 Owen Taylor 2010-06-09 16:35:14 UTC
Also:
 
 https://bugzilla.redhat.com/show_bug.cgi?id=533066
 https://bugs.launchpad.net/ubuntu/+source/metacity/+bug/494096

In bug 608296, the reporter git bisected and found that this was introduced by:

===
commit 38faa8fe109071dcb9c49fdbc13774d13f4691ee
Author: Thomas James Alexander Thurman <tthurman@src.gnome.org>
Date:   Fri Dec 19 12:47:11 2008 +0000

    when the user double-clicks the title bar, end the grab op. 
    Closes bug 401028.
    
diff --git a/src/ui/frames.c b/src/ui/frames.c
index 093af92..700e747 100644
--- a/src/ui/frames.c
+++ b/src/ui/frames.c
@@ -1388,6 +1388,7 @@ meta_frames_button_press_event (GtkWidget      *widget,
       event->button == 1 &&
       event->type == GDK_2BUTTON_PRESS)
     {
+      meta_core_end_grab_op (gdk_display, event->time);
       return meta_frame_double_click_event (frame, event);
     }
===

Launchpad bug has pretty good instructions:

===
From an empty desktop freshly started, with a gnome terminal launcher in the upper panel :
- launch gnome terminal
- double click on title bar -> it's now maximized
- launch a new gnome terminal -> there's now the old maximized gnome terminal in the background and the new windowed gnome terminal in the foreground
- grab-and-move the title bar of the *new* windowed gnome terminal -> the *old* maximized one is unmaximized and moved instead of the new one
====

With the addendum that you have to double-click *slowly* to reproduce the bug, which isn't natural for me. (With that realization, probably all the other reproduction instructions would work fine for me as well.) I was able to reproduce the bug reliably once I raised the double click time in mouse preferences so I could deliberately click twice instead of doing a normal double click.

I'll look into this soon (though maybe not today) and figure out why this is happening and see if we need a different approach for the original bug 401028, or whether there is some other fix.
Comment 12 Luis Menina 2010-06-09 20:57:05 UTC
I think that it has nothing to do with the double click speed. I've just set my mouse double click speed to the fastest available in gnome-mouse-properties, and I'm still able to reproduce it on my Mandriva 2010/GNOME 2.28.
Comment 13 Owen Taylor 2010-06-09 21:02:19 UTC
(In reply to comment #12)
> I think that it has nothing to do with the double click speed. I've just set my
> mouse double click speed to the fastest available in gnome-mouse-properties,
> and I'm still able to reproduce it on my Mandriva 2010/GNOME 2.28.

I'll know after I track down the root cause. 

For now, all I can say is that *I* couldn't reproduce it reliably with my normal style of double clicking the mouse, and could if I intentionally double-clicked slowly.
Comment 14 Luis Menina 2010-06-09 21:33:41 UTC
Following the reasonning on http://linuxfr.org/comments/1134304.html#1134304 (in french):
it seems that the gconf key /apps/metacity/general/action_double_click_titlebar has an influence on wether the bug will trigger or not.

If the value is "toggle_maximize_vertically", the bug triggers
If the value is "toggle_maximize_horizontally", the bug *doesn't* trigger
If the value is "toggle_maximize" (which seems to be a combination of both of the previous modes), the bug triggers

So the bug seems to be in the "toggle_maximize_vertically" part, which is the only action that changes the title bar position relatively to the mouse.
Comment 15 Owen Taylor 2010-06-09 22:10:36 UTC
OK, I now know exactly what is going on and why reproducing it involves all of:

 A) GTK+ 2.18 or later with client side windows
 B) The change from bug 401028
 C) A "slow" double click

And also a factor D:

 D) the window has to be positioned so that the maximization when 
    you double click moves the title bar away from the mouse pointer -
    in other words, the window can't be exactly at the top of the screen.

The basic thing that you have to understand is the concept of an "implicit grab" in X. Any time you click on a window and X sends a button press to a window, it automatically starts a "pointer grab" without any application intervention, and all pointer events are sent to the window until the button is released.

The idea of this is that when you get a button press event, you are guaranteed to get a button release event even if the user moves the pointer out of the window in between.

This is a lot like an explicit pointer grab using XGrabPointer/XUngrabPointer, except that the application doesn't have to call XGrabPointer/XUngrabPointer. In fact, it's so much like so that you can use XUngrabPointer() to ungrab an implicit grab.

With GTK+-2.18 and client side windows, GTK+ suddenly gets involved here because there can be many GdkWindows for each X window. So, GTK+ has to track implicit pointer grabs itself - it has to know which GdkWindow it should be sending events to. So, when GDK sees a button press event on a window, it records that window as the "grab window", and until it sees a button release event or a call to gdk_pointer_ungrab() it keeps on sending all pointer events to that window.

Now, with the patch in bug 401028, when Metacity gets a double click event - the second button press in:

 ButtonPress/ButtonRelease/*ButtonPress*/ButtonRelease

it explicitly calls not gdk_pointer_ungrab() but XUngrabPointer(); so unbeknownst to GDK, it's no longer guaranteed to get a ButtonRelease event. And if the maximization moves the title bar away from the pointer so the release doesn't occur over the titlebar, GDK *won't* get a button release. It still thinks the original window is the grab window and sends all the events there.

The reason why the "slow" double click matters is that if the second ButtonRelease:

 ButtonPress/ButtonRelease/ButtonPress/*ButtonRelease*

happen *before* Metacity reacts to the second ButtonPress and calls XUngrabPointer, then the XUngrabPointer call doesn't matter - the ButtonRelease event has already been sent to the title bar window. Whether Metacity reacts before the ButtonRelease is going to depend on both the speed you can Press/Release a mouse button and the speed of your computer. If your computer is particularly fast, you'll probably see the bug no matter fast you click.

So, that's a full explanation. Will think about fixes and comment separately.
Comment 16 Owen Taylor 2010-06-10 00:01:10 UTC
Created attachment 163253 [details] [review]
Stop confusing GDK's grab tracking

Here's a patch that seems to work for me. Testing to make sure it doesn't
break something else much appreciated - it's more complicated than I
would like.

Basic reasoning for this fix:

 * Just removing the one call to XUngrabPointer introduced by bug 401028
   is not a good fix; there are likely other cases where Metacity ends
   the grab operation before the button release event. [...thinks...]
   In fact, it's not only "likely" - there *are* other such cases.
   I was able to easily trigger the same bug by hitting Esc while
   dragging a window.

 * Trying to switch Metacity to use GDK grabs is going to be very hard;
   Metacity uses all sorts of advanced X mechanisms like XGrabButton()
   that have no equivalent in GDK.

 * I don't want to depend on some sort of new API to GDK to "fix up"
   the grab state.

 * I didn't want to do any major surgery on event handling inside
   Metacity, since MetaFrames is also exposed in libmetacity-private,
   which isn't *really* a private library, despite the name.

So the approach I took here was to simply hide mouse events from GDK -
to directly convert the mouse events myself and send them onto GTK+
bypassing the lower levels of GDK where it does grab tracking.

I had to make an exception for menu handling, since menus have client
side windows and need GDK to get involved. This and the frames are,
to my knowledge, the only uses of GTK+ event handling inside
Metacity.
Comment 17 Luis Menina 2010-06-10 00:36:54 UTC
Thank you for your analysis + patch :-).
Comment 18 Hernando Torque 2010-06-10 12:41:39 UTC
Wildly opened, closed, resized, minimized, maximized, unminimized, and moved around windows - seems to work fine, thanks! :-)
Comment 19 Foldarn 2010-06-10 19:09:13 UTC
See also: https://bugs.launchpad.net/ubuntu/+source/metacity/+bug/494096

Duplicate of this bug (I'm original submitter of above bug)
Comment 20 Dan Winship 2010-06-15 18:25:57 UTC
Comment on attachment 163253 [details] [review]
Stop confusing GDK's grab tracking

>+  /* If GDK already things it has a grab, we better let it see events; this

typo ("things")

There's nothing obviously wrong with the code, but there's nothing obviously right with it either. Bypassing gdk's internals seems like a bad plan because you're relying on the fact that they don't do anything important besides the grab tracking. But I guess metacity's use of gtk is just a giant pile of relying on random details of gtk's internals, so...
Comment 21 Owen Taylor 2010-06-15 19:20:51 UTC
(In reply to comment #20)
> (From update of attachment 163253 [details] [review])
> >+  /* If GDK already things it has a grab, we better let it see events; this
> 
> typo ("things")
> 
> There's nothing obviously wrong with the code, but there's nothing obviously
> right with it either. Bypassing gdk's internals seems like a bad plan because
> you're relying on the fact that they don't do anything important besides the
> grab tracking. But I guess metacity's use of gtk is just a giant pile of
> relying on random details of gtk's internals, so...

Basically, exactly so. 

You are right that if, say, GDK had a "gdk_display_get_current_down_button()" call, then sending the events directly to libgtk bypassing GDK wouldn't work out well. But, I think we're *reasonably* safe because:

 - We are only touching mouse events
 - We basically do everything in bulk consistently for all mouse events
   and don't encode a bunch of special cases.
   (The during-a-grab exception is the part I'm least comfortable with)
 - We don't run arbitrary GTK+ widgets within Metacity

Since I don't think I'm going to get a more enthusiastic review from anybody :-), and I don't know any alternative approaches, I'm going to go ahead and commit this to Metacity master (but not the 2.30 branch) and to Mutter.
Comment 22 Ray Strode [halfline] 2010-06-16 13:54:53 UTC
Review of attachment 163253 [details] [review]:

::: src/core/display.c
@@ +78,3 @@
 #endif
 #include <string.h>
+#include <gtk/gtk.h>

Note there's a rule in HACKING (that you're credited for helping make!) that you're not supposed to add gtk headers to stuff in the core directory. I don't know if this rule is still relevant or applicable here, but this should probably either be reworked, or the HACKING file updated to either remove the rule or to note this exception
Comment 23 Owen Taylor 2010-06-17 18:20:29 UTC
Created attachment 163949 [details] [review]
Stop confusing GDK's grab tracking

Here's a new version that moves everything into ui/ui.c and doesn't touch
the core/ directory. As far as I can determine, it should otherwise be
identical to the previous version.

Ray: Since you made me feel guilty about messy ui/ <=> core/ dependencies,
maybe you can give the part outside of maybe_redirect_mouse_event() a
look-over.
Comment 24 Hernando Torque 2010-07-21 05:59:06 UTC
May I ask what's keeping you from committing this change?
Comment 25 Thomas Thurman 2010-09-04 16:33:49 UTC
Review of attachment 163949 [details] [review]:

It all looks sane to me, and I'm happy to commit it.  The only minor thing I would suggest is that

+ /* For double-click tracking */

should probably say something like "For double-click tracking with redirect mouse events", since we don't use the fields for double-click tracking *in general*.
Comment 26 Rob 2010-10-12 18:28:51 UTC
This is fixed in ubuntu 10.10 and let me tell you it is wonderful.
Comment 27 Hernando Torque 2010-10-13 00:50:53 UTC
Will this also find its way into mutter?
Comment 28 Owen Taylor 2010-10-28 20:09:24 UTC
(In reply to comment #25)
> Review of attachment 163949 [details] [review]:
> 
> It all looks sane to me, and I'm happy to commit it.  The only minor thing I
> would suggest is that
> 
> + /* For double-click tracking */
> 
> should probably say something like "For double-click tracking with redirect
> mouse events", since we don't use the fields for double-click tracking *in
> general*.

You are probably right, maybe /* For double-click tracking for events we forward to GTK+ */ but since you pushed the patch literally into Metacity, I decided to go with the same thing in Mutter.

(In reply to comment #27)
> Will this also find its way into mutter?

Forgot about it but now pushed.

This seems to be upstream in Metacity, so I'm going to close this bug. Not sure why it wasn't closed earlier - feel free to reopen if there's some reason for that.