Some quick feedback:

(In reply to comment #0)
>     @dbus.service.property(
>         dbus_signature="s", dbus_interface="org.example.Thing")

In new code, please don't repeat the design mistake of requiring the name of the D-Bus method/signal to match the name of the Python method. (The name of the Python method can be a default for the D-Bus method, if you want.)

In new code, please don't repeat the design mistake of the signature being optional (although it probably isn't, because if it was, I can't see how Introspect() would work). It should be mandatory.

>     @prop2.setter
>     def prop2(self, value):
>         self._prop2 = value

Does that actually work, syntactically?

I think I'd prefer a less decorator-ful approach, maybe with a magic class attribute like GObject properties have:

class MyThing:
    def get_readable_property(self):
        return 'bees'

    def set_rw_property(self, value):
        if not value:
            raise TypeError('ReadableProperty must be non-empty')
        self.__rw = value

    # ... etc.

    __dbus_properties__ = {
        'org.example.Thing': {
            'ReadableProperty': dbus.service.Property('s',
                get_readable_property, None),
            'ReadWriteProperty': dbus.service.Property('s',
                get_rw_property, set_rw_property),
            'WritableProperty': dbus.service.Property('s',
                None, set_w_property),
        },
    }

(but I could be persuaded otherwise).

Setters should be able to be asynchronous with a callback for success/failure, at least optionally (perhaps making it mandatory would be simpler). In telepathy-glib they can't, and with hindsight, that was a mistake.

Should getters be able to be asynchronous? Probably not...

Helper code for emitting PropertiesChanged on property changes from the Python side would be nice.

(In reply to comment #1)
>     __dbus_properties__ = {
>         'org.example.Thing': {
>             'ReadableProperty': dbus.service.Property('s',
>                 get_readable_property, None),
>             'ReadWriteProperty': dbus.service.Property('s',
>                 get_rw_property, set_rw_property),
>             'WritableProperty': dbus.service.Property('s',
>                 None, set_w_property),
>         },
>     }

If you like that syntax, a million penguin points if you can make methods and signals work more like that too, with backwards compatibility for the old decorator-based approach (this would address Bug #25125 and Bug #31777).

On Jun 18, 2013, at 03:25 PM, bugzilla-daemon@freedesktop.org wrote:

>>     @prop2.setter
>>     def prop2(self, value):
>>         self._prop2 = value
>
>Does that actually work, syntactically?

Yes, and in fact, it's the preferred way to add properties that allow setting
and deleting, e.g.

class Foo:
    def __init__(self, a):
        self._a = a
    @property
    def a(self):
        return self._a
    @a.setter
    def a(self, value):
        self._a = value
    @a.deleter
    def a(self):
        self._a = None

foo = Foo(1)
print(foo.a)
foo.a = 7
print(foo.a)
del foo.a
print(foo.a)

>I think I'd prefer a less decorator-ful approach, maybe with a magic class
>attribute like GObject properties have:
>
>class MyThing:
>    def get_readable_property(self):
>        return 'bees'
>
>    def set_rw_property(self, value):
>        if not value:
>            raise TypeError('ReadableProperty must be non-empty')
>        self.__rw = value
>
>    # ... etc.
>
>    __dbus_properties__ = {
>        'org.example.Thing': {
>            'ReadableProperty': dbus.service.Property('s',
>                get_readable_property, None),
>            'ReadWriteProperty': dbus.service.Property('s',
>                get_rw_property, set_rw_property),
>            'WritableProperty': dbus.service.Property('s',
>                None, set_w_property),
>        },
>    }
>
>(but I could be persuaded otherwise).

Seems rather verbose.  I'm not a big fan of magic attributes because it's not
obvious for the reader to know how those interact with the system.  Besides,
decorators rule! :)

(In reply to comment #1)
> Some quick feedback:
> 
> (In reply to comment #0)
> >     @dbus.service.property(
> >         dbus_signature="s", dbus_interface="org.example.Thing")
> 
> In new code, please don't repeat the design mistake of requiring the name of
> the D-Bus method/signal to match the name of the Python method. (The name of
> the Python method can be a default for the D-Bus method, if you want.)
> 
> In new code, please don't repeat the design mistake of the signature being
> optional (although it probably isn't, because if it was, I can't see how
> Introspect() would work). It should be mandatory.
> 
> >     @prop2.setter
> >     def prop2(self, value):
> >         self._prop2 = value
> 
> Does that actually work, syntactically?

Yes, it does. It also matches the (relatively unknown) feature of python
properties that have the same ability.

> I think I'd prefer a less decorator-ful approach, maybe with a magic class
> attribute like GObject properties have:
> 
> class MyThing:
>     def get_readable_property(self):
>         return 'bees'
> 
>     def set_rw_property(self, value):
>         if not value:
>             raise TypeError('ReadableProperty must be non-empty')
>         self.__rw = value
> 
>     # ... etc.
> 
>     __dbus_properties__ = {
>         'org.example.Thing': {
>             'ReadableProperty': dbus.service.Property('s',
>                 get_readable_property, None),
>             'ReadWriteProperty': dbus.service.Property('s',
>                 get_rw_property, set_rw_property),
>             'WritableProperty': dbus.service.Property('s',
>                 None, set_w_property),
>         },
>     }
> 
> (but I could be persuaded otherwise).

I think that's orthogonal. I personally prefer decorators as they both
share similarities to the python world and are easier to implement with subclassing. (But I'm also okay to be persuaded otherwise)

> Setters should be able to be asynchronous with a callback for
> success/failure, at least optionally (perhaps making it mandatory would be
> simpler). In telepathy-glib they can't, and with hindsight, that was a
> mistake.

I haven't done any async dbus coding yet. How does that change the implementation of Set()?
 
> Should getters be able to be asynchronous? Probably not...

I'm fine with making them (this is a library). I just need to give it a try.

> Helper code for emitting PropertiesChanged on property changes from the
> Python side would be nice.

Yeah. I was looking at that (my application didn't really need it but since
I added the signal handler I though it'd be very easy to implement anyway).

For read-write properties it would fire when value is different, for
write only, I guess always?

Thanks
ZK

(In reply to comment #3)
> Yes, and in fact, it's the preferred way to add properties that allow setting
> and deleting, e.g.
> 
> class Foo:
>     def __init__(self, a):
>         self._a = a
>     @property
>     def a(self):
>         return self._a
>     @a.setter
>     def a(self, value):
>         self._a = value
>     @a.deleter
>     def a(self):
>         self._a = None

Ah, I see, it's order-sensitive?

(In reply to comment #4)
> I personally prefer decorators as they both
> share similarities to the python world and are easier to implement with
> subclassing. (But I'm also okay to be persuaded otherwise)

If you can implement them in a way that doesn't fall into the same traps as the signal and method decorators, fine.

> > Setters should be able to be asynchronous with a callback for
> > success/failure, at least optionally (perhaps making it mandatory would be
> > simpler). In telepathy-glib they can't, and with hindsight, that was a
> > mistake.
> 
> I haven't done any async dbus coding yet. How does that change the
> implementation of Set()?

Instead of returning on success and raising an exception on failure, the Set() implementation would have to call a callback on success, or a different callback (with an exception as a parameter) on failure. That means it can do things like "fire off an async request to a network service and go back to the main loop, then when the response comes back, call the appropriate callback".

> > Should getters be able to be asynchronous? Probably not...
> 
> I'm fine with making them (this is a library). I just need to give it a try.

Allowing asynchronous getters would make GetAll() really awkward to implement, so unless you can see a sensible reason, I would say "no" :-)

Another question to ask early in the design is what can fail. In telepathy-glib, we don't allow Get() and GetAll() to fail, but we do allow Set() to fail. In Python, because absolutely anything can raise an exception, you might want to say that if your interface is this pseudocode:

    property Good: 's'
        get
            return 'yes'

    property Bad: 's'
        get
            raise MyError('no!')

then you get:

    Get(iface, 'Good') -> success, 'yes'
    Get(iface, 'Bad') -> error, com.example.Errors.MyError 'no!'
    GetAll(iface) -> success, { 'Good': 'yes' }    # 'Bad' is just omitted

> For read-write properties it would fire when value is different, for
> write only, I guess always?

Well, the real point is to emit PropertiesChanged if something other than a D-Bus method call causes the change. For instance, imagine you're writing NetworkManager, you have a WirelessConnection object, and the bit-rate changes from 54Mbit to 11Mbit. You want an easy way to emit PropertiesChanged('blah.blah.blah.WirelessConnection', 'BitRate', '11000000') or something.

Even when you're receiving a Set() call from D-Bus, it's not necessarily true that the new value is what the user wanted. Imagine you have a property with some sort of concept of normalization - a domain name that is always lower-case, perhaps. If the user said Set('...', 'DomainName', 'ExAmPlE.ORG'), you need a way to emit PropertiesChanged('...', 'DomainName', 'example.org') instead.

(In reply to comment #5)
> You want an easy way to emit
> PropertiesChanged('blah.blah.blah.WirelessConnection', 'BitRate',
> '11000000') or something.

That's not actually how PropertiesChanged works - it's more like

    PropertiesChanged('...', {'BitRate': 11000000}, [])

where the third argument is a list of the names of properties that have changed, but where the service does not want to tell you the new value (perhaps because it's very large or because it's expensive to compute). So you probably also need a way to group multiple property changes into one signal emission, and a way to flag properties as "this one is expensive, don't send it".

If in doubt, I would recommend stealing API from designs that have been thought out well (e.g. TpDBusPropertiesMixin in telepathy-glib is pretty good[1], except for not allowing asynchronous 'set'; GDBusInterfaceSkeleton in GDBus is well-designed but rather GObject-centric, and not particularly obvious to use without wrapping it in generated code).

[1] disclosure: I wrote a lot of it, so of course I'd say that.

On Jun 18, 2013, at 04:14 PM, bugzilla-daemon@freedesktop.org wrote:

>https://bugs.freedesktop.org/show_bug.cgi?id=65900
>
>--- Comment #5 from Simon McVittie <simon.mcvittie@collabora.co.uk> ---
>(In reply to comment #3)
>> Yes, and in fact, it's the preferred way to add properties that allow setting
>> and deleting, e.g.
>> 
>> class Foo:
>>     def __init__(self, a):
>>         self._a = a
>>     @property
>>     def a(self):
>>         return self._a
>>     @a.setter
>>     def a(self, value):
>>         self._a = value
>>     @a.deleter
>>     def a(self):
>>         self._a = None
>
>Ah, I see, it's order-sensitive?

Yes.  There's really nothing magical here; the objects returned by the
property decorators themselves have methods which can be used as decorators.
They can actually be used in any order, and property objects have a .getter
decorator, but that's rarely used since the getter is almost always defined
first.

http://docs.python.org/3/library/functions.html#property

txdbus might also be a good place to steal ideas (or indeed you could use txdbus instead of dbus-python - I'm quite prepared to believe that it's better-designed).

On Jun 18, 2013, at 04:31 PM, bugzilla-daemon@freedesktop.org wrote:

>txdbus might also be a good place to steal ideas (or indeed you could use
>txdbus instead of dbus-python - I'm quite prepared to believe that it's
>better-designed).

I didn't even know about it, thanks for the reference!  I'll have to read up
on it, but at first glance, maybe it would be cool to adapt this to Guido's
tulip work and/or Python 3.3's "yield from" syntax.

Created attachment 81021 [details]
attachment-30358-0.html

I have three patches with a direct work-in-progress port from python3 and
my application code.

I mainly want to get some feedback and see how this can start taking shape.
I will post updates as I remove the duplicated _dbus_class_table-like
feature from my code


On Tue, Jun 18, 2013 at 6:35 PM, <bugzilla-daemon@freedesktop.org> wrote:

>   *Comment # 9 <https://bugs.freedesktop.org/show_bug.cgi?id=65900#c9> on bug
> 65900 <https://bugs.freedesktop.org/show_bug.cgi?id=65900> from Barry
> Warsaw <barry@python.org> *
>
> On Jun 18, 2013, at 04:31 PM, bugzilla-daemon@freedesktop.org wrote:
> >txdbus might also be a good place to steal ideas (or indeed you could use
> >txdbus instead of dbus-python - I'm quite prepared to believe that it's
> >better-designed).
>
>
> I didn't even know about it, thanks for the reference!  I'll have to read up
> on it, but at first glance, maybe it would be cool to adapt this to Guido's
> tulip work and/or Python 3.3's "yield from" syntax.
>
>  ------------------------------
> You are receiving this mail because:
>
>    - You reported the bug.
>
>

Created attachment 81022 [details]
attachment-30358-1.dat

Created attachment 81023 [details]
0001-Add-property-decorator.patch

Created attachment 81024 [details]
0002-Add-_reflect_on_property-to-InterfaceType-and-use-it.patch

Created attachment 81025 [details]
0003-Add-implementation-of-Get-GetAll-Set-to-dbus.service.patch

I've pushed that code (with minor updates) to https://github.com/zyga/dbus-python to the 'properties' branch.

(I don't have a freedesktop account so I cannot use that)

Thanks
ZK

This seems to be a duplicate of bug 26903. I had there a patch that did about the same thing.

Thanks for the effort here. Please if the old implementation I had from year 2011 had some issues, comment and improve on bug 26903.

*** This bug has been marked as a duplicate of bug 26903 ***