An attempt at fixing a bug involving DGA and mouse buttons

[Steven Elliott]

On Fri, 2012-08-24 at 12:29 +1000, Peter Hutterer wrote:
> On Sat, 2012-08-18 at 20:46 -0500, Steven Elliott wrote:>
> > I have not yet been successful in writing a standalone test program to
> > recreate the problem (to recreate it without MCEdit), but I can try
> > things and report back.
> 
> you need to do the following: 
> issue a button grab with GrabModeSync for the pointer, then on receiving the
> button press event call XDGASetMode/XDGASelectInput before calling
> XAllowEvents.
> 
> I suspect that MCEdit uses GrabModeAsync which makes the window of the
> trigger hard to hit, especially for short clicks.

Yes, that seems likely.  It's hard for me to analyze it because of the
various layers of abstraction between MCEdit and Xlib, but from my notes
when I attempted to trace it around when point where the problem happens
and then use that trace to write a code snippet that does the same thing
this is what I came up with:
    Window focusReturn = 0;
    int revertToReturn = 0;
    XGetInputFocus(dpy, &focusReturn, &revertToReturn);

    XGrabPointer(dpy, win, True,   /* owner_events */
                 VisibilityChangeMask | OwnerGrabButtonMask,
                 GrabModeAsync, GrabModeAsync, win,
                 None, CurrentTime);

    XGrabKeyboard(dpy, win, True,  /* owner_events */
              GrabModeAsync, GrabModeAsync, CurrentTime);

    XWindowChanges change;
    change.stack_mode = Above;
    XConfigureWindow(dpy, win, CWStackMode, &change);
That's probably incomplete and some of it's probably irrelevant, but
those were things that stood out to me in the trace at the time.  I can
see that I didn't capture any DGA activity.

> I've attached a sample program that reproduces the issue here, please
> confirm this is what you're seeing.

I've tried the sample program here on my Fedora 16 64 bit system (X.org
version 1.11.4-3).  It consistently fails with an error in XDGASetMode()
and exits:
----
X Error of failed request:  BadValue (integer parameter out of range for
operation)
  Major opcode of failed request:  130 (XFree86-DGA)
  Minor opcode of failed request:  13 (XDGASetMode)
  Value in failed request:  0x15d
  Serial number of failed request:  15
  Current serial number in output stream:  15
Current button mask: 0
Waiting for button press + release
event 4
Button press received
----

Although it is still possible to move the mouse pointer mouse button and
keyboard events no longer have any effect.  In fact, the display is no
updated - even though the sample program has exited I don't see the
prompt return in bash.

One of my thoughts was trying a different index in "modes" for the call
to XDGASetMode().  It was passing index 0 which for me is
"nvidia-auto-select".  I tried index 1 which for me is
"1280x1024_75" (correct resolution, but incorrect refresh rate), but it
failed in the same way.

The sample program fails in the same way regardless of whether the patch
is applied or not.

I don't know if it's helpful to continue debugging the sample program
given the success I've had with the patch with the feedback I've
received form you and Ville, which I'll elaborate on later in this
email, but we can try additional things if you want.

> we can't drop this block, it is central to button handling with multiple
> devices. right now, if you hold a button on the mouse and the touchpad, the
> logical button will only be released once _both_ buttons are up. this is
> behaviour we had since 1.7 (at least), I don't really want to change it.
> 
> A better fix is what Ville suggested, that sounds like the right thing to
> do.

The attached patch does that.  It no longer removes the block of code in
question.  It moves only the 
    if (!IsMaster(keybd))
       return;
from inside DGAHandleEvent() to inside DGAProcessKeyboardEvent() and
DGAProcessPointerEvent() after the call to UpdateDeviceState() in those
functions like Ville suggested.  

I've attached an updated patch which fixes the problem for me.  This
time I generated the patch with the latest from git with
git-format-patch in the interest of following the patch submission
guidelines a bit more than I did last time.

-- 
------------------------------------------------------------------------
|  Steven Elliott  |  http://selliott.org  |  selliott4 at austin.rr.com  |
------------------------------------------------------------------------
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dga-button-stuck-down-2.diff
Type: text/x-patch
Size: 1573 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20120825/d938f057/attachment.bin>