[PATCH 2/2] Handle non continuous valuator data in getValuatorEvents

[Jamey Sharp]

Hi Chase! I thoroughly sympathize with removing code duplication,
especially once it's gotten complicated like this, but please, no
macro magic...

After trying a bunch of alternatives with GCC 4.4.5 on x86-64 to see
whether it would generate sane code, it looks to me like the right
answer is a getValuatorValue static function implementing the
right-hand side of the assignment, and leaving the assignment itself
written out explicitly in the switch.

A less important complaint: It wasn't obvious to me on initial review
that it was safe to remove the assignment "dev = NULL". It turns out
that it is safe because dixLookupDevice ensures it's NULL on failure,
and the loop only runs if num_valuators > 0, in which case
dixLookupDevice has certainly been called first. But perhaps some
short comment is in order?

(It also took me a while to notice that there were no break statements
in the switch, which led to a few minutes of wtfs...)

Jamey

On Mon, Mar 28, 2011 at 10:53 AM, Chase Douglas
<chase.douglas at canonical.com> wrote:
> This allows for masked valuators to be handled properly in XI 1.x
> events. Any unset valuators in the device event are set to the last
> known value when transmitted on the wire through XI 1.x valuator events.
>
> Fixes https://bugs.launchpad.net/ubuntu/+source/xorg-server/+bug/736500
>
> Signed-off-by: Chase Douglas <chase.douglas at canonical.com>
> ---
>  dix/eventconvert.c |   24 +++++++++++++-----------
>  1 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/dix/eventconvert.c b/dix/eventconvert.c
> index 9fce447..edecf6c 100644
> --- a/dix/eventconvert.c
> +++ b/dix/eventconvert.c
> @@ -358,27 +358,30 @@ countValuators(DeviceEvent *ev, int *first)
>     return num_valuators;
>  }
>
> -#define set_valuator_value(xv, ev, num) \
> -    xv->valuator##num = (ev)->valuators.data[(xv)->first_valuator + num];
> +#define set_valuator_value(xv, ev, dev, num) \
> +    if (BitIsOn((ev)->valuators.mask, (xv)->first_valuator + num)) \
> +        xv->valuator##num = (ev)->valuators.data[(xv)->first_valuator + num]; \
> +    else \
> +        xv->valuator##num = \
> +            (dev)->valuator->axisVal[(xv)->first_valuator + num];
>  static int
>  getValuatorEvents(DeviceEvent *ev, deviceValuator *xv)
>  {
>     int i;
>     int state = 0;
>     int first_valuator, num_valuators;
> +    DeviceIntPtr dev;
>
>
>     num_valuators = countValuators(ev, &first_valuator);
>     if (num_valuators > 0)
>     {
> -        DeviceIntPtr dev = NULL;
>         dixLookupDevice(&dev, ev->deviceid, serverClient, DixUseAccess);
>         /* State needs to be assembled BEFORE the device is updated. */
>         state = (dev && dev->key) ? XkbStateFieldFromRec(&dev->key->xkbInfo->state) : 0;
>         state |= (dev && dev->button) ? (dev->button->state) : 0;
>     }
>
> -    /* FIXME: non-continuous valuator data in internal events*/
>     for (i = 0; i < num_valuators; i += 6, xv++) {
>         xv->type = DeviceValuator;
>         xv->first_valuator = first_valuator + i;
> @@ -388,17 +391,17 @@ getValuatorEvents(DeviceEvent *ev, deviceValuator *xv)
>
>         switch (xv->num_valuators) {
>         case 6:
> -            set_valuator_value(xv, ev, 5);
> +            set_valuator_value(xv, ev, dev, 5);
>         case 5:
> -            set_valuator_value(xv, ev, 4);
> +            set_valuator_value(xv, ev, dev, 4);
>         case 4:
> -            set_valuator_value(xv, ev, 3);
> +            set_valuator_value(xv, ev, dev, 3);
>         case 3:
> -            set_valuator_value(xv, ev, 2);
> +            set_valuator_value(xv, ev, dev, 2);
>         case 2:
> -            set_valuator_value(xv, ev, 1);
> +            set_valuator_value(xv, ev, dev, 1);
>         case 1:
> -            set_valuator_value(xv, ev, 0);
> +            set_valuator_value(xv, ev, dev, 0);
>         }
>
>         if (i + 6 < num_valuators)
> @@ -407,7 +410,6 @@ getValuatorEvents(DeviceEvent *ev, deviceValuator *xv)
>
>     return (num_valuators + 5) / 6;
>  }
> -#undef set_valuator_value
>
>
>  static int
> --
> 1.7.4.1
>
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
>