[2/2 v2] winex11: Use XIAllDevices to select for XI_RawMotion

[Henri Verbeet]

On 18 November 2016 at 12:43, Carlos Garnacho <carlosg at gnome.org> wrote:
> Change the strategy used to get raw motion from slave devices. Instead
> of selecting for XI2 events for every slave device individually, do it
> for XIAllDevices, and store the current device's relative X/Y valuators
> so they can be quickly looked up in the XI_RawMotion events received.
>
I think this makes sense, and in my (limited) testing this correctly
handles the case of attaching a new input device. I do have a few
minor comments though:

> +static void update_relative_valuators(XIAnyClassInfo **valuators,
> +                                      int              n_valuators)
This should probably just be "static void update_relative_valuators(
XIAnyClassInfo **valuators, int n_valuators )"

> +    free( thread_data->x_rel_valuator );
> +    free( thread_data->y_rel_valuator );
...
> +            thread_data->x_rel_valuator = malloc( sizeof(XIValuatorClassInfo) );
> +            memcpy ( thread_data->x_rel_valuator, class, sizeof(XIValuatorClassInfo) );
We generally avoid malloc()/free() in Wine code and use
HeapAlloc()/HeapFree() instead. The main consideration has to do with
the way address space is allocated for Win32 applications, effectively
making malloc() space a bit more scarce than HeapAlloc() space. We'd
probably prefer the sizeof as "sizeof(*thread_data->x_rel_valuator)",
or perhaps "sizeof(*class)".

I'm not sure how much we should worry about this in practice, but when
multiple slaves are attached to the master, and they're not completely
idle, update_relative_valuators() can end up getting called quite a
bit. It doesn't seem hard to at least do the allocation only once, and
only update the data in update_relative_valuators(). Perhaps it's also
fine the way it is though.

> -    XISetMask( mask_bits, XI_ButtonPress );
This was added intentionally (see also
https://bugs.winehq.org/show_bug.cgi?id=27522#c18). Perhaps it's no
longer needed, but that would need to be a separate patch.

> +    pointer_info = pXIQueryDevice( data->display, data->xi2_core_pointer, &count );
> +    update_relative_valuators( pointer_info->classes, pointer_info->num_classes );
> +    pXIFreeDeviceInfo( pointer_info );
...
> +    pXISelectEvents( data->display, DefaultRootWindow( data->display ), &mask, 1 );
I'm not too concerned about it, but can this race? I.e. switching
slaves between when update_relative_valuators() is called above and
when XISelectEvents() takes effect. Applies to querying the devices
list as well.

> +    if (thread_data->xi2_current_slave == 0)
We'd typically write that "if (!thread_data->xi2_current_slave)",
although I don't think anyone would be overly concerned about it.

> +    for (i = 0; i <= max ( x_rel->number, y_rel->number ); i++)
> +    {
> +»······if (!XIMaskIsSet( event->valuators.mask, i )) continue;
> +»······val = *values++;
> +»······if (i == x_rel->number)
> +»······{
> +            input.u.mi.dx = dx = val;
> +»······    if (x_rel->min < x_rel->max)
> +                input.u.mi.dx = val * (virtual_rect.right - virtual_rect.left)
> +                                    / (x_rel->max - x_rel->min);
> +»······}
> +»······if (i == y_rel->number)
> +»······{
> +»······    input.u.mi.dy = dy = val;
> +»······    if (y_rel->min < y_rel->max)
You're using tabs for indentation here.