[PATCH v2 2/2] Populate touch emulated button state separately

[Peter Hutterer]

On Thu, Jul 12, 2012 at 01:24:18PM -0700, Chase Douglas wrote:
> On 07/11/2012 11:34 PM, Peter Hutterer wrote:
> >On Mon, Jul 09, 2012 at 05:12:44PM -0700, Chase Douglas wrote:
> >>When a touch begin is pointer emulated, do not include touch state when
> >>calculating the button state for the initial motion and button press
> >>events. This change ensures the logical state of the buttons prior to the
> >>events is set properly for emulated events.
> >>
> >>Signed-off-by: Chase Douglas <chase.douglas at canonical.com>
> >>---
> >>  Xi/exevents.c    |   15 +++++++++++++++
> >>  dix/events.c     |    2 ++
> >>  dix/inpututils.c |    4 ----
> >>  3 files changed, 17 insertions(+), 4 deletions(-)
> >>
> >>diff --git a/Xi/exevents.c b/Xi/exevents.c
> >>index c1fcc55..0341ad5 100644
> >>--- a/Xi/exevents.c
> >>+++ b/Xi/exevents.c
> >>@@ -1445,6 +1445,13 @@ DeliverTouchEmulatedEvent(DeviceIntPtr dev, TouchPointInfoPtr ti,
> >>      event_set_state(dev, kbd, &ptrev->device_event);
> >>      ptrev->device_event.corestate = event_get_corestate(dev, kbd);
> >>
> >>+    /* Add button 1 emulated state only for touch update and touch end events.
> >>+     * The state confers the *previous* state of the button. */
> >>+    if (ev->any.type != ET_TouchBegin) {
> >>+        SetBit(ptrev->device_event.buttons, dev->button->map[1]);
> >>+        ptrev->device_event.corestate |= (Button1Mask >> 1) << dev->button->map[1];
> >>+    }
> >>+
> >
> >can we move this into event_get_corestate?
> 
> In order to do that in a way that satisfies both blocks above and
> below, we would have to extend the function like this:
> 
> event_get_corestate(DeviceIntPtr dev, DeviceIntPtr kbd,
>                     bool force_primary_button);
> 
> Which would be called like this for the above and below blocks,
> respectively:
> 
> event_get_corestate(dev, kbd, ev->any.type != ET_TouchBegin);
> 
> event_get_corestate(dev, kbd,
>                     (event->flags & XIPointerEmulated) && mouse &&
>                     mouse->touch && mouse->touch->buttonsDown > 0);
> 
> The same would apply for event_set_state(), and everywhere other
> than these two blocks the forcing parameter would be passed in as
> FALSE.
> 
> Does that work for you?

yikes. how about splitting event_set_state() etc. instead?  the code would
end up as this:

    event_set_state();
    corestate = event_get_corestate();

    if (type == TouchBegin)
       event_clear_btn1_state();
       event_clear_btn1_corestate(&corestate);

which is a bit more readable and more reliable than manual bithandling.
still not ideal but give then conditions here there's no real room for 
something more sensible.

coincidentally, don't we see the same problem for the emulated ButtonRelease
event too? TouchEnd should (currently) clear the state but the ButtonRelease
still needs that bit set. did you notice that at all?

> >>      if (grab) {
> >>          /* this side-steps the usual activation mechansims, but... */
> >>          if (ev->any.type == ET_TouchBegin && !dev->deviceGrab.grab)
> >>@@ -1701,6 +1708,14 @@ ProcessDeviceEvent(InternalEvent *ev, DeviceIntPtr device)
> >>      corestate = event_get_corestate(mouse, kbd);
> >>      event_set_state(mouse, kbd, event);
> >>
> >>+    /* Touch button state is already set for pointer emulated events. */
> >>+    if (!(event->flags & XIPointerEmulated)) {

one other question here: if the event isn't a pointer emulated event, do we
even include the button state? a non-emulated event can only ever be
delivered as touch events and those don't include touch state in button
state.

Cheers,
  Peter

>
> >>+        if (mouse && mouse->touch && mouse->touch->buttonsDown > 0) {
> >>+            SetBit(event->buttons, mouse->button->map[1]);
> >>+            event->corestate |= (Button1Mask >> 1) << mouse->button->map[1];
> >>+        }
> >>+    }
> >>+
> >
> >same here, let's move this into event_set_state()? i feel a bit queasy about
> >having a function that returns us the core state, only to have that state
> >messed with on returning.
> >
> >Cheers,
> >   Peter
> >
> >>      ret = UpdateDeviceState(device, event);
> >>      if (ret == DONT_PROCESS)
> >>          return;
> >>diff --git a/dix/events.c b/dix/events.c
> >>index 86336fe..00ae8a9 100644
> >>--- a/dix/events.c
> >>+++ b/dix/events.c
> >>@@ -5135,6 +5135,8 @@ ProcQueryPointer(ClientPtr client)
> >>      rep.type = X_Reply;
> >>      rep.sequenceNumber = client->sequence;
> >>      rep.mask = event_get_corestate(mouse, keyboard);
> >>+    if (mouse && mouse->touch)
> >>+        rep.mask |= mouse->touch->state;
> >>      rep.length = 0;
> >>      rep.root = (GetCurrentRootWindow(mouse))->drawable.id;
> >>      rep.rootX = pSprite->hot.x;
> >>diff --git a/dix/inpututils.c b/dix/inpututils.c
> >>index 223d547..3ae41cc 100644
> >>--- a/dix/inpututils.c
> >>+++ b/dix/inpututils.c
> >>@@ -671,7 +671,6 @@ event_get_corestate(DeviceIntPtr mouse, DeviceIntPtr kbd)
> >>                   kbd->key) ? XkbStateFieldFromRec(&kbd->key->xkbInfo->
> >>                                                    state) : 0;
> >>      corestate |= (mouse && mouse->button) ? (mouse->button->state) : 0;
> >>-    corestate |= (mouse && mouse->touch) ? (mouse->touch->state) : 0;
> >>
> >>      return corestate;
> >>  }
> >>@@ -685,9 +684,6 @@ event_set_state(DeviceIntPtr mouse, DeviceIntPtr kbd, DeviceEvent *event)
> >>          if (BitIsOn(mouse->button->down, i))
> >>              SetBit(event->buttons, mouse->button->map[i]);
> >>
> >>-    if (mouse && mouse->touch && mouse->touch->buttonsDown > 0)
> >>-        SetBit(event->buttons, mouse->button->map[1]);
> >>-
> >>      if (kbd && kbd->key) {
> >>          XkbStatePtr state;
> >>
> >>--
> >>1.7.10.4
> >>
> >
>