On 18 November 2016 at 12:43, Carlos Garnacho carlosg@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.