Interface comments

[whot]

I had a read through the docs in regards to the API and I'll summarise the various comments here, easier than filing a million separate issues :)

The main issue is that, right now, the Python API is a C API in the Python language. That's not the same as a real Python API and there's lot for improvement. Implementation-wise what I did in libevdev-python was have an internal class that matches the C API (like you already have) but have the public API be a hand-written one that wraps the C wrapper.

Anyway, here's a list of comments regarding the current API:

• [x] The libinput context should be one of udev or path, see #2
• [x] BaseEvent shouldn't be visible if it's not to be used, but ....
• [x] Event should be the equiv to what BaseEvent is right now
• [x] Event.get_device() should be a read-only device property. This applies to almost all get_... functions in the events, they should be properties.
• [x] Event.get_pointer_event() shouldn't exist. The C API does that because we don't have classes and have to work around it this way, but for Python, LibInput.get_event() should just return the right type of object, e.g. PointerEvent.
• [x] Logical pairs like dx/dy maybe better returned as tuple?
• [x] get_absolute_x_transformed could be a x, y = event.transform_absolute(width, height) or something
• [x] get_time vs get_time_usec() - just drop get_time() [edit: make get_time return us]. the only reason it exists in the libinput API is because we realised too late ms isn't enough and we have to be backwards-compatible.
• [x] DeviceConfig.config_* is redundant, drop the config_ prefix
• [x] config_status_to_str() should just be a __repr__
• [x] The config may be worth subclassing, e.g. DeviceConfig.Tap.finger_count or so
• [x] libinput.constant.Event -> EventType ?
• [x] for the event codes, it may be better relying on evdev directly, but I understand that the python evdev situation isn't great right now

[OzymandiasTheGreat]

First, thank you! This is probably the best feedback I've ever gotten. :smiley: All good points, some I have considered myself and decided to be lazy (like returning device events directly). DeviceConfig is a base class for Device, so it's methods are available through Device instance, that's why I left the config_ prefix, to clearly differentiate between configuration methods and the rest. Could you elaborate on subclassing config? I don't think I quite understand what you mean and what purpose it would serve. As for the rest, will do :+1:

[whot]

tbh, I don't think I really know what the best option is for config. First, having it exposed as device.config would be useful, that can be a class that handles the actual config requests.

The goal would be to have a pythonic API instead of a C wrapper, so we'd get e.g. AttributeError when calling a non-existent configuration item. The libinput config items are (almost) all a triplet of get/set/get_default. Obviously get/set would be property candidates, not sure how to handle get_default yet.

Leaving that aside, the config property can have sub-properties for each class, so that you can do

  device.config.tap.enabled = True

And that can either work, or throw an error where tapping isn't supported. I honestly don't know 100% how this should look like in detail, but I think the above would be better than having tap_set_enabled.

[edit: best approach would likely be to write a test program using libinput and writing it against the API you'd expect libinput to have (even if it doesn't right now). That gives you the best insighed on how it should look like]

[OzymandiasTheGreat]

This does look much nicer, I'll think 'bout how to best implement it. As for get_default, I guess I'll just make it a read-only property.

[OzymandiasTheGreat]

@whot I could use some advice. I'm halfway done rewriting the pythonic api, but I'm not sure if raising an exception when accessing a property on the wrong type of event is the best way to go. I do think it's better than just returning None, less error prone. Thoughts?

[whot]

I'm assuming you're talking about cases where a developer ties to e.g. get dx/dy from a keyboard event?

None should indicate 'no value', whereas an exception is more "you've done something wrong" or "something is out of the ordinary". So I do think that accessing properties that don't exist should be an exception. None can be mistaken for 0 or False in a condition.

[OzymandiasTheGreat]

I made a new release, I think I implemented all of your suggestions. That actually made the code neater. As for the event codes, I opted to remove them completely, because I haven't considered not all event codes may be defined in input.h.

[whot]

Ok, this looks a lot better now and a lot more Pythonic, thanks heaps for the work. I had a look through the API documentation on RTD again (not the code, just the docs):

• [x] context_type=constant.ContextType.PATH seems clunky, why not just libinput.ContextType.PATH. Is hiding in a constant module a python thing? Same for device capabilities, event types, etc. Shouldn't these all be part of the main libinput namespace?
• [x] grab argument: what is this for? grabbing a device should be hardly ever needed these days?
• [x] "Resuming udev context before assigning seat causes segfault." I fixed that in libinput :) fwiw, any crash is a bug, so rather than hiding this in docs it would be easier to report it upstream
• [ ] get_event() I'd explicitly document what happens at timeout 0
• [x] get_event() IMO the timeout handling should move to the caller and instead use a .events property that is the generator. That's personal preference though
• [x] next_event_type remove type EventType.NONE and instead return Python's None. The NONE type only exists because C doesn't have a generic None.
• [x] "For pointer events that are not of type POINTER_MOTION, this property raises AssertionError." - AttributeError seems more appropriate?
• [ ] absolute_coords I think absolute would be fine here too, but I'm not 100% sure. Alternatively, maybe use something like position? [now I see you're using coords for touch, would be good to have this consistent]
• [x] coords_have_changed: I wonder if a better approach would be to have coords return a tuple of the value and a boolean: ((x, y), has_changed). And all the other axes as well, e.g. (pressure, has_changed). I can't think of many cases where you want to check has_changed and then not get the coordinates anyway.
• [ ] led_update(leds)- probably better as getter/setter?
• [ ] config.*.is_available() - should be a property

Special case:

• [x] has_capability(capability) - this is more complicated. A good python api would return a list so a caller can do if cap in device.capabilities. The C api doesn't have that because C's array management is nonexistent and you cannot wrap this without adding extra code in python-libinput (i.e. that you internally loop through has_capability. Might still be worth it though.
• [x] pointer_has_button: I wonder if that was something better done as device.pointer.has_button or so? You'd get an AttributeError if the device isn't a pointer. And the implementation can be a hidden class.

[OzymandiasTheGreat]

Sorry, life got a bit hectic lately. Got less time to work on this but I'm still going.

I'm not sure if it's a python thing, but keeping constants/enumerations in separate module is definitely my thing. Helps me keep track of what's where. I could expose them through main libinput namespace, I'm just not sure if keeping code in a separate module and importing names into the main namespace is pythonic.

grab is a leftover. When I was trying to figure out libinput I looked closely at libinput-debug-events source. While I can't think of a use case off the top of my head, I don't see a reason to remove it either. Edit: looking at the current implementation, it's a bit broken. Might as well remove it.

About the segfault, in my rush to get this out I have not even considered reporting it. That would make a lot more sense and I'll keep it mind. Also thanks for fixing it!

If I understand correctly, you're suggesting removing timeout handling from current implementation and instead returning plain generator as the events property. That may be a good idea.

OK, EventType.NONE will go. While the idea of pythonic is rather elusive to me, this is definitely it. Pythonic. :smiley:

Agreed. AttributeError is more appropriate.

I'm not looking at the code right now, but iirc using plain absolute would be confusing in some places and I want to keep this consistent. As for the coords vs position I don't really see a reason to use one over the other.

Agreed. Returning a tuple of values and a boolean is very good idea!

I don't see how led_update(leds) would work as a getter since libinput_device_led_update(leds) returns void.

I thought about that and decided that all is_*/has_* functions should be methods for consistence. I could make the is_available methods available properties if you think it's worth it.

device.capability property, will do. This is a great idea.

I got lazy here again. While it definitely makes sense for tablet pad devices, pointer and keyboard has only one function each. I do agree it would look much nicer. Will do.

[whot]

I don't see a reason to remove it either.

Take that as a lesson I learned the hard way: Unless you have a specific use-case in mind (and that use-case is one you want to enable and support), don't expose anything in your library you don't immediately need. You will have to spend time maintaining a bit of code that may never be used, and, worse, you may actually implement it wrong (or incompatible) with a real use case that comes later, making your life doubly hard.

As for the coords vs position I don't really see a reason to use one over the other.

Pick one and use it for both touch and absolute pointer coordinates, just for consistency. I'd go with position because it's less ambiguous.

[OzymandiasTheGreat]

So I implemented most of that. As for coords vs position, the documentation uses the word coordinates, so I think coords is less confusing. And I still don't see how led_update can be a getter/setter.

[whot]

yeah, forget about the led_update comment, this is a write-only thing right now so can't be done with getters/setters.