pywlroots icon indicating copy to clipboard operation
pywlroots copied to clipboard

Lots of similar properties in Ptr classes

Open m-col opened this issue 4 years ago • 2 comments

Lots of the Ptr classes have properties that get and sometimes set attributes that belong to their _ptr attribute. Others do the same thing but wrapped with another Ptr class, where the getter returns e.g. Output(self._ptr.output), and would set with self._ptr.<attr> = value._ptr.

We could simplify the layout of the code and reduce the need for us to manually write up lots of similar properties (which is error-prone).

Here is an example implementation:

diff --git a/wlroots/__init__.py b/wlroots/__init__.py
index 0fc0d1e..649d77b 100644
--- a/wlroots/__init__.py
+++ b/wlroots/__init__.py
@@ -28,3 +28,43 @@ class Ptr:
     def __hash__(self) -> int:
         """Use the hash from `object`, which is unique per object"""
         return super().__hash__()
+
+    @staticmethod
+    def _ptr_property(attr, immutable=False, wrapper=None, doc=None):
+        """
+        Create a property that proxies an attribute on the pointer.
+
+        :param attr:
+            The name of the attribute on the pointer.
+        :param immutable:
+            If True, no setter is added.
+        :param wrapper:
+            Another Ptr subclass which wraps the attribute on the pointer. The getter
+            will wrap the returned pointer with this, and the setter will set the
+            pointer's attribute with the pointer from the wrapper.
+        :param doc:
+            A docstring to add to the property. If not provided, the attribute name is
+            used.
+        """
+        if wrapper:
+            # getter and setter proxied by the wrapper
+            def getter(self):
+                return wrapper(getattr(self._ptr, attr))
+
+            if not immutable:
+                def setter(self):
+                    setattr(self._ptr, attr, value._ptr)
+
+        else:
+            # getter and setter accessing attribute directly
+            def getter(self):
+                return getattr(self._ptr, attr)
+
+            if not immutable:
+                def setter(self, value):
+                    setattr(self._ptr, attr, value)
+
+        if immutable:
+            setter = None
+
+        return property(fget=getter, fset=setter, doc=doc or attr)

A simple example use-case:

class OutputMode(Ptr):
    def __init__(self, ptr) -> None:
        self._ptr = ptr

    width = Ptr._ptr_property("width")
    height = Ptr._ptr_property("height")
    refresh = Ptr._ptr_property("refresh")
    preferred = Ptr._ptr_property("preferred")

A more complex example use-case:

class KeyboardKeyEvent(Ptr):
    def __init__(self, ptr) -> None:
        """Event that a key has been pressed or release

        This event is emitted before the xkb state of the keyboard has been
        updated (including modifiers).
        """
        self._ptr = ffi.cast("struct wlr_event_keyboard_key *", ptr)

    time_msec = Ptr._ptr_property("time_msec", immutable=True, doc="Time of the key event")
    keycode = Ptr._ptr_property("keycode", immutable=True, doc="Keycode triggering the event")
    update_state = Ptr._ptr_property(
        "update_state", immutable=True, doc="If backend doesn't update modifiers on its own"
    )
    state = Ptr._ptr_property(
        "state", immutable=True, wrapper=WlKeyboard.key_state,
        doc="The state of the keycode triggering the event"
    )

I post these two examples because this idea started off from the simple example, which I thought looked nice, but it was pretty limited so I then added immutable, then wrapper, then doc when I came across existing properties that needed these. Then when we get to the more complex example the readability takes a hit, so I'm not 100% certain of it's even desired.

I'm throwing this out there as an idea in case it's worth discussing whether we should implement something like this, and how. Also working on this it became more clear how formulaic a lot of the code is, which is making me a bit more interested in an auto-generated approach, which'd be awesome.

m-col avatar May 09 '21 19:05 m-col

A downside of this (as implemented above) is it removes type checking from the values.

m-col avatar May 09 '21 19:05 m-col

Looking at this reminds me of David Beazley's talk on metaprogramming: https://www.youtube.com/watch?v=sPiWg5jSoZI

I feel like there is definitely something that we could do here to simplify some of the basic properties around built-in types. It may be possible to type annotate them using some form of generics where the method to build the property would also define the types.

Maybe, it could be even simpler if we did something like:

class OutputMode(Ptr):
    def __init__(self, ptr) -> None:
        self._ptr = ptr

    width: int = Ptr._ptr_property("width")
    height: int = Ptr._ptr_property("height")
    refresh: int = Ptr._ptr_property("refresh")
    preferred: int = Ptr._ptr_property("preferred")

I'm not sure which is able to work better with mypy and if that is sufficient for this.

flacjacket avatar May 09 '21 21:05 flacjacket