button->down used inconsistently

[Keith Packard]

On Sun, 2008-06-15 at 03:18 +0200, Daniel Stone wrote:

> I'd be happier with a usage count, e.g. as done with modifiers, since
> they can trigger XKB actions, et al.

Here's a patch which avoids counting by just having the master release
code look at the slave devices and release the button when all of the
slaves have released theirs.

From dfb622f991ae95f7d56931d898dbc012cf91ad78 Mon Sep 17 00:00:00 2001
From: Keith Packard <keithp at keithp.com>
Date: Sat, 14 Jun 2008 13:40:53 -0700
Subject: [PATCH] Make button down state a bitmask. Master buttons track union of slave buttons

Mixing usage where some parts of the code treated this field as a bitmask
and other parts as an array of card8 was wrong, and as the wire protocol
wanted bitmasks, it was less invasive to switch the newer counting code use
booleans.

Master devices track slave buttons by waiting for all slave buttons to be
released before delivering the release event to the client.

This also removes the state merging code in DeepCopyDeviceClasses -- that
code was changing master device state without delivering any events,
violating protocol invariants. The result will be that existing slave
button state which does not match the master will not be visible through the
master device. Fixing this would require that we synthesize events in this
function, which seems like a bad idea. Note that keyboards have the same
issue.

diff --git a/Xi/exevents.c b/Xi/exevents.c
index 57fb65b..3235964 100644
--- a/Xi/exevents.c
+++ b/Xi/exevents.c
@@ -587,8 +587,6 @@ DeepCopyDeviceClasses(DeviceIntPtr from, DeviceIntPtr to)
 
     if (from->button)
     {
-        int i;
-        DeviceIntPtr sd;
         if (!to->button)
         {
             classes = dixLookupPrivate(&to->devPrivates,
@@ -603,20 +601,6 @@ DeepCopyDeviceClasses(DeviceIntPtr from, DeviceIntPtr to)
                 classes->button = NULL;
         }
 
-        to->button->buttonsDown = 0;
-        memset(to->button->down, 0, MAP_LENGTH);
-        /* merge button states from all attached devices */
-        for (sd = inputInfo.devices; sd; sd = sd->next)
-        {
-            if (sd->isMaster || sd->u.master != to)
-                continue;
-
-            for (i = 0; i < MAP_LENGTH; i++)
-            {
-                to->button->down[i] += sd->button->down[i];
-                to->button->buttonsDown++;
-            }
-        }
 #ifdef XKB
         if (from->button->xkb_acts)
         {
@@ -923,8 +907,10 @@ UpdateDeviceState(DeviceIntPtr device, xEvent* xE, int count)
         if (!b)
             return DONT_PROCESS;
 
-        if (b->down[key]++ > 0)
+	kptr = &b->down[key >> 3];
+        if ((*kptr & bit) != 0)
             return DONT_PROCESS;
+	*kptr |= bit;
 	if (device->valuator)
 	    device->valuator->motionHintWindow = NullWindow;
         b->buttonsDown++;
@@ -938,10 +924,25 @@ UpdateDeviceState(DeviceIntPtr device, xEvent* xE, int count)
         if (!b)
             return DONT_PROCESS;
 
-        if (b->down[key] == 0)
-            return DONT_PROCESS;
-        if (--b->down[key] > 0)
+	kptr = &b->down[key>>3];
+        if (!(*kptr & bit))
             return DONT_PROCESS;
+	if (device->isMaster) {
+	    DeviceIntPtr sd;
+
+	    /*
+	     * Leave the button down if any slave has the
+	     * button still down. Note that this depends on the
+	     * event being delivered through the slave first
+	     */
+	    for (sd = inputInfo.devices; sd; sd = sd->next) {
+		if (sd->isMaster || sd->u.master != device)
+		    continue;
+		if ((sd->button->down[key>>3] & bit) != 0)
+		    return DONT_PROCESS;
+	    }
+	}
+	*kptr &= ~bit;
 	if (device->valuator)
 	    device->valuator->motionHintWindow = NullWindow;
         if (b->buttonsDown >= 1 && !--b->buttonsDown)
diff --git a/dix/devices.c b/dix/devices.c
index b88d856..8eb6c25 100644
--- a/dix/devices.c
+++ b/dix/devices.c
@@ -1153,7 +1153,7 @@ InitButtonClassDeviceStruct(DeviceIntPtr dev, int numButtons,
     butc->buttonsDown = 0;
     butc->state = 0;
     butc->motionMask = 0;
-    bzero((char *)butc->down, MAP_LENGTH);
+    bzero((char *)butc->down, sizeof(butc->down));
 #ifdef XKB
     butc->xkb_acts=	NULL;
 #endif
diff --git a/include/inputstr.h b/include/inputstr.h
index 7209b2c..de66167 100644
--- a/include/inputstr.h
+++ b/include/inputstr.h
@@ -183,7 +183,7 @@ typedef struct _ButtonClassRec {
     CARD8		buttonsDown;	/* number of buttons currently down */
     unsigned short	state;
     Mask		motionMask;
-    CARD8		down[MAP_LENGTH];
+    CARD8		down[DOWN_LENGTH];
     CARD8		map[MAP_LENGTH];
 #ifdef XKB
     union _XkbAction    *xkb_acts;

-- 
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
URL: <http://lists.x.org/archives/xorg/attachments/20080618/f069320a/attachment.pgp>