GNOME Bugzilla – Bug 781907
xrandr: Move GsdDeviceMapper handling to wacom plugin
Last modified: 2017-07-18 20:47:26 UTC
I'll be moving the xrandr plugin functionality to mutter soon. Before, here's a bit that can't be moved and should in fact be in wacom.
Created attachment 350667 [details] [review] power: Remove gsd-device-mapper.h include It's unused.
Created attachment 350668 [details] [review] xrandr: Move GsdDeviceMapper handling to wacom plugin GsdDeviceMapper's only remaining role is to map wacom tablets/touchscreens to outputs which logically belongs in the wacom plugin and is already being done there for some device types so let's move the other device type that was being handled here to that plugin too.
I think Carlos needs to look at this.
Review of attachment 350667 [details] [review]: Sure.
Review of attachment 350668 [details] [review]: ::: plugins/wacom/gsd-wacom-manager.c @@ +287,3 @@ + if (((device_type & GSD_DEVICE_TYPE_TABLET) != 0 && + (device_type & GSD_DEVICE_TYPE_TOUCHPAD) == 0) + || Great cleanup, and but this is really ugly. Splitting the condition in a separate function, or repeating the content of the conditional might be better. if ((device_type & GSD_DEVICE_TYPE_TABLET) != 0 && (device_type & GSD_DEVICE_TYPE_TOUCHPAD) == 0) { _add_input(); } else if (device_type & GSD_DEVICE_TYPE_TOUCHSCREEN) != 0) { _add_input(); } would be more readable.
Created attachment 350978 [details] [review] xrandr: Move GsdDeviceMapper handling to wacom plugin -- reworked the conditional as suggested
Review of attachment 350978 [details] [review]: That looks fine. Carlos?
I'm fairly sure this is correct. If it isn't we can easily revert. Attachment 350667 [details] pushed as 107c36c - power: Remove gsd-device-mapper.h include Attachment 350978 [details] pushed as 41f1564 - xrandr: Move GsdDeviceMapper handling to wacom plugin
Uhm, sorry, missed this bug... Kinda late, but looks alright to me too, makes sense to make all mapping happen in a single place.
(In reply to Carlos Garnacho from comment #9) > Uhm, sorry, missed this bug... Kinda late, but looks alright to me too, > makes sense to make all mapping happen in a single place. Thanks. In fact, since this code was now running in separate processes I think it wasn't even doing what it was supposed to do.
Oh, as far as I could tell it still did. IIRC GsdDeviceMapper discriminates so it doesn't try to eg. assign the same output to two touchscreen devices, but tablet and touchscreen devices should be isolated sets anyway. Still seems better to have this in a single place, touchscreen handling in xrandr module was just as alien :).