Bug 26903

Created attachment 35713 [details]
Implementation of D-Bus properties

The attached code is what I wrote to use D-Bus properties in my project.  It contains a DBusObjectWithProperties class which I then use in place of dbus.service.Object.  There is also a "dbus_service_property" decorator, to be used like the normal dbus.service.method and dbus.service.signal decorators.

(In reply to comment #1)

> The attached code is what I wrote to use D-Bus properties in my project.  It
> contains a DBusObjectWithProperties class which I then use in place of
> dbus.service.Object.  There is also a "dbus_service_property" decorator, to be
> used like the normal dbus.service.method and dbus.service.signal decorators.

That code has a bug created by a cut-and-paste mistake - the "try:" on the third line of the "Introspect" method should not be there.  Remove it and unindent the code and it should all work.

Created attachment 48084 [details] [review]
A patch to add properties support

Attached patch (made with git format-patch) adds some support for properties and introspection.

Use is along the lines of

class AnObject(dbus.service.Object, dbus.service.PropertiesInterface):

    @dbus.service.property(MY_IFACE, signature="i")
    def i(self):
        return self._i

    @i.setter
    def i(self, value):
        self._i = value

    @dbus.service.property(MY_IFACE, signature="s")
    def s(self):
        return "foo"

    x = dbus.service.property(MY_IFACE, "s", "x", fget=lambda i: 0)

Please comment.

I was unsure if we could add the PropertiesInterface automatically, so I required it explicitly.

Created attachment 48183 [details] [review]
A patch to add properties support, second version

I changed the patch to add the PropertiesInterface to bases automatically if necessary. So no need to explicitly inherit from it.

Also PropertiesChanged was missing from the interface.

The above patch isn't added to the git tree of dbus-python. Anyone know why it isn't added?

I'd like to add my support for this feature. I have been trying to solve the problem of properties and was contemplating adding them in a very similar way, so this is marvellous.

What I'd like to see is the PropertiesInterface hooking exposing the properties via __getattr__ etc, so that they look like properties from the client side.

I am happy to contribute if there is some traction on this.

If I apply the patch to dbus-python-0.83 i can use properties but the patch should not be applied to the dbus-python-1.1.1 .
Working how suggested in http://stackoverflow.com/questions/3740903/python-dbus-how-to-export-interface-property does not work.

I need to use properties to implement mpris2 specification http://specifications.freedesktop.org/mpris-spec/latest/index.html in a player.

Will be any support for properties in the last dbus python version ?
Will be the patch ported to dbus-python-1.1.1 and added to the git tree ?

I'm in the same situation, I started implementing mpris2, and it was marvelous at first, the decorators for methods and signals make the implementation so easy !

Then it comes to properties, and I've been stuck on that for two days :(

I looked at the other python binding available, PyGObject, but it doesn't allow to implement a dbus server yet [1].

The best solution I've found so far was in quodlibet. Their implementation is very clean, and easy to integrate in other projects, it's a great piece of code. But it's not trivial, it involves overriding the introspection method. So basically, re-implementing stuff that are already provided by the binding. Very frustrating...

And furthermore, quodlibet is GPLv2, and I'm on a GPLv3 software at the moment, so I can't copy/paste code like that, but on the other hand there's not so many solutions around for that problem, so... I'm still stuck... Anyway, that was a little bit of topic, right ?

I do hope that someone can integrate the patch attached here one day.

Cheers

[1] http://www.piware.de/2011/01/na-zdravi-pygi/

Created attachment 98993 [details] [review]
A patch to add properties support, third version

I updated the patch to apply and work on dbus-python 1.2.0.

*** Bug 65900 has been marked as a duplicate of this bug. ***

Ping, any upstream feedback here?

It was pointed out to me that the candidate patch from comment #9 apparently changes/breaks api slightly, making blueman non-functional, see also:
https://github.com/blueman-project/blueman/issues/161

(In reply to leon from comment #5)
> The above patch isn't added to the git tree of dbus-python. Anyone know why
> it isn't added?

Because I have a limited amount of time I can spend on D-Bus, and when I use up that time, making libdbus stable and secure has been a higher priority for me than dbus-python enhancements. Sorry.

Introspect() is meant to be something for debug tools to use, and hence not essential functionality. The fact that dbus-python uses it for method calls is a piece of historical mis-design (violating the Python design principles "explicit is better than implicit" and "in the face of ambiguity, resist the temptation to guess") to which we are constrained by backwards compatibility.

As far as I'm concerned, dbus-python is not actually a particularly good D-Bus implementation: other implementations that have learned from its mistakes, like GDBus via pygi, are likely to be rather better. I haven't personally used txdbus, but I suspect that it's also rather more Pythonic than dbus-python.

Comment on attachment 98993 [details] [review]
A patch to add properties support, third version

Review of attachment 98993 [details] [review]:
-----------------------------------------------------------------

This is a quick skim-read.

::: dbus/decorators.py
@@ +356,5 @@
> +        `dbus.service.Object`.
> +
> +        :Parameters:
> +            `dbus_interface` : str
> +                The D-Bus interface owning the property

The None default doesn't make sense; every property is on a D-Bus interface, and as far as I can see, dbus_interface == None would crash in validate_interface_name() anyway.

@@ +360,5 @@
> +                The D-Bus interface owning the property
> +
> +            `signature` : str
> +                The signature of the property in the usual D-Bus notation. The
> +                signature must be suitable to be carried in a variant.

Similarly, this should be mandatory, and I think None would already crash anyway. Properties with an unspecified signature don't make sense, and backwards compatibility is not relevant here.

The D-Bus jargon for the required format for the signature is that it "must be a single complete type" ("i.e. something that can be placed in a variant" would be fine as an additional hint though).

@@ +370,5 @@
> +            `emits_changed_signal` : True, False, "invalidates", or None
> +                Tells for introspection if the object emits PropertiesChanged
> +                signal.
> +
> +            `fget` : func

> python -c "import this"
...
Readability counts.
...

I'd prefer "getter". However, since Fedora have apparently already forked dbus-python's API by applying this, perhaps compatibility with that matters more...

@@ +394,5 @@
> +                property_name = fget.__name__
> +            elif fset is not None:
> +                property_name = fset.__name__
> +        if property_name:
> +            validate_member_name(property_name)

D-Bus property names being valid member names is strongly recommended, but not actually required. You might want to accept any UTF-8 string (in Python 2) or any unicode (in Python 3).

::: dbus/service.py
@@ +35,5 @@
>  import _dbus_bindings
>  from dbus import (
> +    INTROSPECTABLE_IFACE, ObjectPath, PROPERTIES_IFACE, SessionBus, Signature,
> +    Struct, validate_bus_name, validate_object_path)
> +_builtin_property = property

I think this is an indication that "property" is not the ideal name for this decorator. dbus_property? Or use "import dbus.decorators" and consistently use dbus.decorators.property?

On the other hand, Fedora.

@@ +41,1 @@
>  from dbus.decorators import method, signal

Duplicate import?

@@ +309,5 @@
> +                else:
> +                    bases += (PropertiesInterface,)
> +                break
> +
> +        interface_table = dct.setdefault('_dbus_interface_table', {})

Is this an instance attribute now? I could see that becoming a problem if you have lots of small objects.

@@ +379,5 @@
>  
> +    def _reflect_on_property(cls, descriptor):
> +        signature = descriptor._dbus_signature
> +        if signature is None:
> +            signature = 'v'

signature should never be None. Explicit is better than implicit.

@@ +392,5 @@
> +        else:
> +            return ""
> +        reflection_data = '    <property access="%s" type="%s" name="%s"' % (access, signature, descriptor.__name__)
> +        if descriptor._emits_changed_signal is not None:
> +            value = {True: "true", False: "false", "invalidates": "invalidates"}[descriptor._emits_changed_signal]

So, er, if _emits_changed_signal is not one of the known values, Introspect() crashes?

I would expect the @property constructor to validate this, at least.

@@ +412,5 @@
> +        interfaces = self._dbus_interface_table
> +        if interface_name:
> +            interface = interfaces.get(interface_name)
> +            if interface is None:
> +                raise DBusException("No interface %s on object" % interface_name)

This should be something with "org.freedesktop.DBus.Error.UnknownInterface" as its machine-readable D-Bus error name. (I can't actually remember how that works on the service side.)

@@ +415,5 @@
> +            if interface is None:
> +                raise DBusException("No interface %s on object" % interface_name)
> +            prop = interface.get(property_name)
> +            if prop is None:
> +                raise DBusException("No property %s on object interface %s" % (property_name, interface_name))

Similarly, this is UnknownProperty...

@@ +417,5 @@
> +            prop = interface.get(property_name)
> +            if prop is None:
> +                raise DBusException("No property %s on object interface %s" % (property_name, interface_name))
> +            if not isinstance(prop, property):
> +                raise DBusException("Name %s on object interface %s is not a property" % (property_name, interface_name))

... and can this even happen? But if it does, it's UnknownProperty again.

@@ +423,5 @@
> +        else:
> +            for interface in interfaces.itervalues():
> +                prop = interface.get(property_name)
> +                if prop and isinstance(prop, property):
> +                    return prop

I don't think any other implementation allows Get("", "Foo") to return the property com.example.Foo (or whatever)? The D-Bus Specification does allow it, but it calls it out as undefined behaviour, and also allows raising an error.

If there is not a pressing need to do this because of compatibility with a lot of existing broken code, then "explicit is better than implicit" IMO.

@@ +424,5 @@
> +            for interface in interfaces.itervalues():
> +                prop = interface.get(property_name)
> +                if prop and isinstance(prop, property):
> +                    return prop
> +            raise DBusException("No property %s found" % (property_name,))

o.fd.DBus.Error.UnknownProperty

@@ +434,5 @@
> +        behaviour is undefined.
> +        """
> +        prop = self._get_decorator(interface_name, property_name)
> +        if not prop.fget:
> +            raise DBusException("Property %s not readable" % property_name)

I'm not actually sure what machine-readable error name is best for this, but please check other implementations.

@@ +445,5 @@
> +        behaviour is undefined.
> +        """
> +        prop = self._get_decorator(interface_name, property_name)
> +        if not prop.fset:
> +            raise DBusException("Property %s not writable" % property_name)

o.fd.DBus.Error.PropertyReadOnly

@@ +457,5 @@
> +        interfaces = self._dbus_interface_table
> +        if interface_name:
> +            iface = interfaces.get(interface_name)
> +            if iface is None:
> +                raise DBusException("No interface %s on object" % interface_name)

UnknownInterface

@@ +460,5 @@
> +            if iface is None:
> +                raise DBusException("No interface %s on object" % interface_name)
> +            ifaces = [iface]
> +        else:
> +            ifaces = interfaces.values()

Again, I don't think anything else actually supports GetAll("") so I would go for "explicit is better than implicit" and raise an error here.

::: examples/example-client.py
@@ +46,4 @@
>              dbus_interface = "com.example.SampleInterface")
>      except dbus.DBusException:
>          print_exc()
> +        print(usage)

This stuff is an unrelated change (Bug #85720).

(In reply to Simon McVittie from comment #12)
> As far as I'm concerned, dbus-python is not actually a particularly good
> D-Bus implementation

Expanding on that a little:

The fact that types of method call arguments are guessed based on remote Introspect() results is a huge API design bug, slows dbus-python down measurably, and means you crash unpredictably if the remote service's API changes.

The API is full of circular dependencies between modules, and I would not be at all surprised if it's also full of circular dependencies between objects.

The fact that "import dbus-glib" has side-effects is pretty atrocious. Behind the scenes, it also depends on a (rightfully!) deprecated library, dbus-glib. 

However, those can't be fixed without breaking dbus-python's biggest feature, which is "you don't have to port code that used older dbus-python versions". If compatibility breaks, and you have to port code to keep it working, then you might as well port it to something better-designed instead; so keeping backwards compat is key.

As a result, yes I'm aware I'm a bad maintainer and not giving dbus-python enough time; but I'm also reluctant to hand over the maintainer hat to anyone whose attention to backwards compat I am not confident about, because that would remove its one advantage.

If someone wants to design a good Pythonic D-Bus API, learning from dbus-python's mistakes, and using either libdbus, GDBus or a pure-Python D-Bus implementation behind the scenes, that would be a great thing to do; and if that requires forking parts of dbus-python under a different name, go for it, it's MIT-licensed. Possibly txdbus is already that good Pythonic D-Bus API, I don't know.

Comment on attachment 98993 [details] [review]
A patch to add properties support, third version

Review of attachment 98993 [details] [review]:
-----------------------------------------------------------------

I can completely understand lack of time.

Since writing the patch, I have lost use for it myself. I however now put some time into fixing according to some of the comments you provided. Thank you for those.

When I first made it, I was hoping for some comments on the patch before spending too much effort on it.

I know, it bundles at least three different changes in one patch.
a) the issue of replacing _dbus_class_table with _dbus_interface_table, which I think is a significant simplification and clarification,
b) the property decorator that I like writing using the _dbus_interface_table
c) python 3 update for the example applications since I did not write proper unit tests.

I do not mind if someone else took over with this patch or patches.

::: dbus/decorators.py
@@ +370,5 @@
> +            `emits_changed_signal` : True, False, "invalidates", or None
> +                Tells for introspection if the object emits PropertiesChanged
> +                signal.
> +
> +            `fget` : func

The built-in property uses fget and fset. Getter and setter methods are not the same function. I think it is less surprising this way.

@@ +394,5 @@
> +                property_name = fget.__name__
> +            elif fset is not None:
> +                property_name = fset.__name__
> +        if property_name:
> +            validate_member_name(property_name)

I don't know why you prefer utf-8. It would cause problems in Introspect if some names are unicode and others utf-8 str. I'll go for str (ASCII) or unicode.

::: dbus/service.py
@@ +35,5 @@
>  import _dbus_bindings
>  from dbus import (
> +    INTROSPECTABLE_IFACE, ObjectPath, PROPERTIES_IFACE, SessionBus, Signature,
> +    Struct, validate_bus_name, validate_object_path)
> +_builtin_property = property

Yes, it could be indication of that, but I think use of dbus.decorators.property consistently is the clearer solution. It is a matter of opinion, and I dislike dbus.decorators.dbus_property. Also having decorators "method" and "dbus_property" do not follow same naming.

@@ +309,5 @@
> +                else:
> +                    bases += (PropertiesInterface,)
> +                break
> +
> +        interface_table = dct.setdefault('_dbus_interface_table', {})

This is now __new__ method and dct contains class attributes for the new interface class being created. Or did you refer to something else?

@@ +423,5 @@
> +        else:
> +            for interface in interfaces.itervalues():
> +                prop = interface.get(property_name)
> +                if prop and isinstance(prop, property):
> +                    return prop

I also prefer explicit, but I do not find the specification that says this is undefined. I only find http://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-properties that says if an object has multiple properties by same name, then it is undefined.

Created attachment 112098 [details] [review]
A patch to add properties support, fourth version

Some updates according to review comments.

*** Bug 85374 has been marked as a duplicate of this bug. ***

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus-python/issues/11.