Mir

Code review comment for lp:~andreas-pokorny/mir/libinput-input-device-provider

Revision history for this message
Chris Halse Rogers (raof) wrote :

On an overall level I'm not sure that InputDeviceInfo/InputDeviceIdentifier is a good abstraction; it seems like a thin wrapper around what you can get from evdev. This doesn't seem friendly to platforms that aren't directly tied to the hardware, like nested or (hopefully!) X11.

Hm. Having said that, I guess this MP is actually the start of the implementation for the hardware/evdev platform, and won't be loaded in nested situations. In that case I'd probably use Udev::Device as the device identifier to pass to the InputDeviceProvider/InputDeviceFactory; that's what you'll be getting out of the monitor.

The fun thing here is that we've really got two levels of ‘platform’ - there's the connect-to-the-rest-of-Mir input platform layer, but on the hardware/evdev platform there's *also* a platformish interface. We'll have AndroidInput/libinput as two consumers of the hardware/evdev interface, and we may well want a dynamic-loading interface so third parties can ship things like Wacom drivers or whatever.

Code-related comments below:

Lots of std::runtime_error that should be std::system_error.

655 + enum DeviceClasses
656 + {
657 + cursor = 0x00000001,
658 + keyboard = 0x00000002,
659 + touchpad = 0x00000004,
660 + touchscreen = 0x00000008,
661 + gamepad = 0x00000010,
662 + joystick = 0x00000020,
663 + };

Hm. I usually find the idiom
cursor = 1<<0,
keyboard = 1<<1,
touchpad = 1<<2,

as more obvious for bitfields. Do we have any other bitfields in our code ATM? What's our current style?

726 + virtual Priority get_support(InputDeviceInfo const& info) const = 0;

get_support seems like it could be better named. Maybe probe() is better?

review: Needs Fixing

« Back to merge proposal