[PATCH 09/21] Xi: split updating button count and state into helper functions

[Chase Douglas]

On 12/12/2011 03:38 AM, Peter Hutterer wrote:
> On 12/12/11 15:27 , Chase Douglas wrote:
>> On 12/08/2011 07:36 PM, Peter Hutterer wrote:
>>> Functional change: for a button mapped to 0, the motionHintWindow is not
>>> updated to the NullWindow anymore. Before it got updated unconditionally to
>>> the button mapping. I have no idea what the practical effect of this is, but
>>> I guess it's closer to the correct behaviour: pressing a button that's
>>> logically disabled now does not disrupt the motion hint delivery.
>>>
>>> Signed-off-by: Peter Hutterer<peter.hutterer at who-t.net>
>>> ---
>>>   Xi/exevents.c |   41 +++++++++++++++++++++++++++++------------
>>>   1 files changed, 29 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/Xi/exevents.c b/Xi/exevents.c
>>> index 1140554..dab467c 100644
>>> --- a/Xi/exevents.c
>>> +++ b/Xi/exevents.c
>>> @@ -726,6 +726,32 @@ UpdateDeviceMotionMask(DeviceIntPtr device, unsigned short state,
>>>       SetMaskForEvent(device->id, mask, MotionNotify);
>>>   }
>>>
>>> +static void
>>> +IncreaseButtonCount(DeviceIntPtr dev, int key, CARD8 *buttons_down,
>>> +                    Mask *motion_mask, unsigned short *state)
>>> +{
>>> +    if (dev->valuator)
>>> +        dev->valuator->motionHintWindow = NullWindow;
>>> +
>>> +    (*buttons_down)++;
>>> +    *motion_mask = DeviceButtonMotionMask;
>>> +    if (dev->button->map[key]<= 5)
>>> +        *state |= (Button1Mask>>  1)<<  dev->button->map[key];
>>> +}
>>> +
>>> +static void
>>> +DecreaseButtonCount(DeviceIntPtr dev, int key, CARD8 *buttons_down,
>>> +                    Mask *motion_mask, unsigned short *state)
>>> +{
>>> +    if (dev->valuator)
>>> +        dev->valuator->motionHintWindow = NullWindow;
>>> +
>>> +    if (*buttons_down>= 1&&  !--(*buttons_down))
>>> +        *motion_mask = 0;
>>> +    if (dev->button->map[key]<= 5)
>>> +        *state&= ~((Button1Mask>>  1)<<  dev->button->map[key]);
>>> +}
>>> +
>>>   /**
>>>    * Update the device state according to the data in the event.
>>>    *
>>> @@ -831,15 +857,11 @@ UpdateDeviceState(DeviceIntPtr device, DeviceEvent* event)
>>>               return DONT_PROCESS;
>>>
>>>           set_button_down(device, key, BUTTON_PROCESSED);
>>> -	if (device->valuator)
>>> -	    device->valuator->motionHintWindow = NullWindow;
>>> +
>>>           if (!b->map[key])
>>>               return DONT_PROCESS;
>>> -        b->buttonsDown++;
>>> -	b->motionMask = DeviceButtonMotionMask;
>>> -        if (b->map[key]<= 5)
>>> -	    b->state |= (Button1Mask>>  1)<<  b->map[key];
>>>
>>> +        IncreaseButtonCount(device, key,&b->buttonsDown,&b->motionMask,&b->state);
>>>           UpdateDeviceMotionMask(device, b->state, b->motionMask);
>>>       } else if (event->type == ET_ButtonRelease) {
>>>           if (!b)
>>> @@ -867,15 +889,10 @@ UpdateDeviceState(DeviceIntPtr device, DeviceEvent* event)
>>>               }
>>>           }
>>>           set_button_up(device, key, BUTTON_PROCESSED);
>>> -	if (device->valuator)
>>> -	    device->valuator->motionHintWindow = NullWindow;
>>>           if (!b->map[key])
>>>               return DONT_PROCESS;
>>> -        if (b->buttonsDown>= 1&&  !--b->buttonsDown)
>>> -	    b->motionMask = 0;
>>> -	if (b->map[key]<= 5)
>>> -	    b->state&= ~((Button1Mask>>  1)<<  b->map[key]);
>>>
>>> +        DecreaseButtonCount(device, key,&b->buttonsDown,&b->motionMask,&b->state);
>>>           UpdateDeviceMotionMask(device,  b->state, b->motionMask);
>>>       } else if (event->type == ET_ProximityIn)
>>>   	device->proximity->in_proximity = TRUE;
>>
>> Why not just pass a pointer to the button class instead of pointers to
>> three fields of the button class?
> 
> ah, yeah, I can see how that wouldn't make sense without seeing the 
> follow-up. the TouchClassRec likewise has buttonsDown, state, motionMask 
> so that function is called for both ButtonPress/Release as well as 
> TouchBegin/End with different parameters. Passing the button class in 
> would require the touch class to be partially binary compatible. This 
> way is easier, imo

I thought that might be the case :). With that clarification:

Reviewed-by: Chase Douglas <chase.douglas at canonical.com>