GNOME Bugzilla – Bug 304430
metacity sometimes ignores ButtonRelease events
Last modified: 2009-07-17 02:15:01 UTC
If the system is bogged down and a user clicks a window frame very quickly, Metacity will sometimes ignore the ButtonRelease event associated with the click. This is because Metacity only acts on ButtonRelease events if they occur after the relevent grab operation has been started (display.c, line 1758): case ButtonRelease: if (display->grab_window == window && event->xany.serial >= display->grab_start_serial && grab_op_is_mouse (display->grab_op)) meta_window_handle_mouse_grab_op_event (window, event); break; If a user clicks so fast that a ButtonRelease event is received and queued up before the ButtonPress callback is invoked (and thus before the grab op is started) then there is no gaurantee that "event->xany.serial >= display->grab_start_serial" will be true. It's not really clear why that condition was added. The log message (which is buried deep in the metacity-reduced-mode branch) is just: * src/display.h (struct _MetaDisplay): add grab_start_serial used to avoid responding to events that occurred prior to the grab initialization. Without knowing why responding to ButtonRelease events that occur prior to grab initialization is a bad idea, it's hard to say what the right fix is. Owen suggested a low risk workaround--query the X server for button press state in the ButtonPress event's callback to know if ButtonRelease event came before the callback was invoked.
Created attachment 46513 [details] [review] End grab op early if ButtonRelease occured before grab op was started. This patch implements Owen's suggestion. If the mouse button is pressed down when the ButtonPress event callback is invoked then end the grab op early.
Elijah do you remember the point of that grab_start_serial stuff?
I didn't actually ever know what the point of it was, so there's no way for me to have remembered it. ;-) You introduced the grab_start_serial stuff on the reduced resources branch before I was involved with Metacity much. There doesn't seem to be any bug number about it and the ChangeLog entry doesn't provide enough info for me; it appears that you had forgotten the reason even a few months after the change (see https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=107149#c11; though part of that comment might be misleading[1]). You and Keith discussed the grab_start_serial stuff some in bug 126871, and then I later found that '>=' prevented easily reproducable bugs caused by '>' (see bug 136587), but there appeared to possibly have been further issues as discussed in this bug as well as in comments 22 through 28 of https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=107149. [1] You took several ChangeLog comments from the reduced_resources branch and just included them all as one long ChangeLog comment when merging to HEAD; this resulted in a slightly misleading entry, as the following text: * src/display.h (struct _MetaDisplay): add grab_start_serial used to avoid responding to events that occurred prior to the grab initialization. 2003-07-20 Havoc Pennington <hp@pobox.com> Still broken in various ways, specifically EnterNotify that occurred prior to XGrabPointer is processed as if it occurred after. became * src/display.h (struct _MetaDisplay): add grab_start_serial used to avoid responding to events that occurred prior to the grab initialization. Still broken in various ways, specifically EnterNotify that occurred prior to XGrabPointer is processed as if it occurred after. meaning that people reading the ChangeLog on HEAD think that the second paragraph refers to stuff in the first, when it may not.
Thanks Elijah - this gives me the clue that maybe I was trying to fix EnterNotify/LeaveNotify moving the focus around. I bet mouse/sloppy focus has a couple funky bugs if we remove the grab serial thing. But we might have a simple fix - just drop the grab serial check for button presses?
If you were just trying to fix EnterNotify/LeaveNotify moving the focus around, then it sounds like you may have just been tackling a subset of bug 152000 (see attachment 31672 [details] in particular) as well as perhaps bug 154598/bug 154601. In other words, although I wouldn't be surprised if mouse/sloppy focus have a couple funny bugs if we remove the grab serial thing as you suggest, I wouldn't be surprised either if we remove it and things still work just fine. Maybe we should try it, while it's still early part of development for 2.11, and then investigate further from there. Thoughts?
No harm in trying it. I would definitely test reduced resources and also what happens when you open menus (metacity's or the app's)
Hi, I'd the problem related to this, ButtonRelease event is not fired. I've posted my problem on comp.windows.x group in the above link. http://groups.google.com/group/comp.windows.x/browse_frm/thread/bfe8d4e856d6a592 Thanks Ashish Shukla
Hey Elijah, Did you ever end up trying what you proposed in comment 5 (dropping the grab serial checks) ?
Nope, I forgot about it. We can try it out after branching for 2.17; reminders welcome. :-)
just looking at my patch list and noticing this one still hanging around. Let's drop it for 2.19 after we branch.
Created attachment 86041 [details] [review] Try dropping the grab_start_serial checks Okay, let's try it out. I just committed this to trunk.
Since the point of the original patch was to work around the grab_start_serial checking (which no longer exists in trunk), I'm marking the patch as obsolete to pull it off the radar. Might have to undo that after we see how this experiment works...
So we should close this guy out...