Bug 34869

See bug 34870 for the path_prefix bug. FWIW, I'd like to commit a version of
the spec patch without path_prefix before resolving bug 34870... should be
fine, exiting patch only mentions path_prefix in a non-normative way. Once we
resolve bug 34870 we can change that section to use path_prefix again.

Created attachment 43967 [details] [review]
Updated patch

Here's an updated patch

 - Use trailing () for all methods
 - s/GetAll()/GetManagedObjects()
 - nuke path_prefix in the informative section (for now)

Created attachment 44236 [details] [review]
Proposed patch

This patch should go on top of the patch in bug 34870. Thanks.

For the record, GDBusObjectManager and GDBusProxyManager (which will be part of GLib 2.30) is now using this interface as per commit

 http://cgit.freedesktop.org/~david/gdbus-binding-tool/commit/?id=b2b3375586152985c1d3bf49178419546f55c2af

Here's a screenshot

 http://people.freedesktop.org/~david/ObjectManager-in-dfeet.png

Created attachment 45395 [details] [review]
Updated patch

Here's an updated patch with the following changes

 - use path_namespace instead of path_prefix

 - add "The org.freedesktop.DBus.ObjectManager interface was added
        in version 0.16 of the D-Bus specification." at the end of
   the new section

Created attachment 45484 [details] [review]
Updated patch

Missed the boat for 0.16 of the spec so bumping to 0.17 :-/

(In reply to comment #4)
> For the record, GDBusObjectManager and GDBusProxyManager (which will be part of
> GLib 2.30) is now using this interface

I hope this means "here is a sample implementation for GLib matching the current proposal", rather than "I've already merged this in GLib, it's too late to change the API"...

(In reply to comment #7)
> (In reply to comment #4)
> > For the record, GDBusObjectManager and GDBusProxyManager (which will be part of
> > GLib 2.30) is now using this interface
> 
> I hope this means "here is a sample implementation for GLib matching the
> current proposal", rather than "I've already merged this in GLib, it's too late
> to change the API"...

FYI, it's not in a released GLib version but it will be in the upcoming GLib 2.30. I'll follow up on gtk-devel-list about it in a couple of days.

Review of attachment 45484 [details] [review]:

Conceptually, this looks good. The details need a bit of adjustment.

::: doc/dbus-specification.xml
@@ +3003,3 @@
+      <title><literal>org.freedesktop.DBus.ObjectManager</literal></title>
+      <para>
+        An application can optionally make use of this interface to

I'd be tempted to say "An API can" instead, to emphasize that choosing whether to use ObjectManager is up to the D-Bus API designer, not the implementer (in cases where they might be different people, like Telepathy or MPRIS).

It might also be worth saying something like:

    It is appropriate to use this interface if
    users of the tree of objects are expected to
    be interested in all interfaces of all objects in
    the tree; a more granular API should be used if
    users of the objects are expected to be interested
    in a small subset of the objects, a small subset
    of their interfaces, or both.

@@ +3011,3 @@
+      <para>
+        <programlisting>
+          org.freedesktop.DBus.ObjectManager.GetManagedObjects (out DICT&lt;OBJPATH,DICT&lt;STRING,DICT&lt;STRING,VARIANT&gt;&gt;&gt; objpath_interfaces_and_properties);

So, er, that's a{oa{sa{sv}}}? I thought Telepathy had intimidating data-types :-)

@@ +3017,3 @@
+        The return value of this method is a dict whose keys are
+        object paths. All returned object paths are children of the
+        object path implementing this interface. Each value is a dict

I think this deserves clarification that they're syntactically children in the object-path tree, rather than some loose conceptual relationship:

    ... implementing this interface, i.e. their object
    paths start with the ObjectManager's object path
    plus '/'.

(At least, I assume that's what you meant.)

@@ +3023,3 @@
+        and object path in question that the
+        <literal>org.freedesktop.DBus.Properties.GetAll</literal>
+        method would return. If an interface has no properties, the

This run-on sentence is rather awkward. How about:

    Each value is a dict, whose keys are interface names.
    Each value in this inner dict is the same dict that
    would be returned by the <>...GetAll</> method for that
    combination of object path and interface. If an
    interface has no ...

(I deliberately put object path first here because that's the logical order: as seen in dbus-send, and (hopefully) in the libdbus and GDBus APIs, the hierarchy from largest to smallest is bus name, object path, interface, member.)

Does GetAll actually specifically say that it returns {} for a propertyless interface? If not, it should. (There's a dbus-glib bug about that.)

@@ +3040,3 @@
+        The <literal>InterfacesAdded</literal> signal is emitted when
+        either a new object is added or if an existing object gains
+        one or more interfaces while the

Grammar: choose whether you're saying "when" or "if" (I vote for "when"). I'd say:

    when either a new object is added, or an existing ...

I think this "while" deserves to be a sentence break.

@@ +3045,3 @@
+        interfaces. The second parameter of the
+        <literal>InterfacesAdded</literal> signal contains a dict with
+        the interfaces and properties (if any) that has been added to

"that have been added": it's the interfaces and properties that were added, not the dict

@@ +3048,3 @@
+        the given object path. Similarly, the second parameter of the
+        <literal>InterfacesRemoved</literal> signal contains an array
+        of the interfaces that was removed. Note that changes on

"that were removed": it's the interfaces that were removed, not the array.

Do you have a use-case for interfaces disappearing without the entire object going away? I don't think changes to the list of interfaces are generally part of the de facto D-Bus object model? (dbus-glib and dbus-python certainly can't support it with their current design.)

@@ +3050,3 @@
+        of the interfaces that was removed. Note that changes on
+        properties on existing interfaces are not reported using this
+        interface - an application should use the

s/use/also monitor/ perhaps

@@ +3057,3 @@
+        object (directly or otherwise) implementing this interface but
+        which are not returned in the reply from the
+        <literal>GetAll()</literal> method of this interface on the

it's no longer called GetAll

@@ +3072,3 @@
+      <para>
+        on respectively the message bus and the remote
+        application. Whenever a new remote object is created (or an

I tend to think of method calls as being an operation on objects (or proxies), rather than on destinations: "on the message bus, and the remote application's ObjectManager, respectively".

@@ +3074,3 @@
+        application. Whenever a new remote object is created (or an
+        existing object gains a new interface), the
+        <literal>InterfacesAdded</literal> signal is emitted and since

emitted, and

@@ +3077,3 @@
+        this signal contains all properties for the interfaces, no
+        calls to the <literal>org.freedesktop.Properties</literal>
+        interface on the remote object is needed. Additionally, since

are needed

(In reply to comment #9)
> Review of attachment 45484 [details] [review]:
> 
> Conceptually, this looks good. The details need a bit of adjustment.

Thanks for the review. Much appreciated.

> ::: doc/dbus-specification.xml
> @@ +3003,3 @@
> +      <title><literal>org.freedesktop.DBus.ObjectManager</literal></title>
> +      <para>
> +        An application can optionally make use of this interface to
> 
> I'd be tempted to say "An API can" instead, to emphasize that choosing whether
> to use ObjectManager is up to the D-Bus API designer, not the implementer (in
> cases where they might be different people, like Telepathy or MPRIS).

OK, fixed.

> It might also be worth saying something like:
> 
>     It is appropriate to use this interface if
>     users of the tree of objects are expected to
>     be interested in all interfaces of all objects in
>     the tree; a more granular API should be used if
>     users of the objects are expected to be interested
>     in a small subset of the objects, a small subset
>     of their interfaces, or both.

Sounds good added.


> 
> @@ +3011,3 @@
> +      <para>
> +        <programlisting>
> +          org.freedesktop.DBus.ObjectManager.GetManagedObjects (out
> DICT&lt;OBJPATH,DICT&lt;STRING,DICT&lt;STRING,VARIANT&gt;&gt;&gt;
> objpath_interfaces_and_properties);
> 
> So, er, that's a{oa{sa{sv}}}? I thought Telepathy had intimidating data-types
> :-)

Yah. On the upside, this only needs to be implemented once per binding so...

> 
> @@ +3017,3 @@
> +        The return value of this method is a dict whose keys are
> +        object paths. All returned object paths are children of the
> +        object path implementing this interface. Each value is a dict
> 
> I think this deserves clarification that they're syntactically children in the
> object-path tree, rather than some loose conceptual relationship:
> 
>     ... implementing this interface, i.e. their object
>     paths start with the ObjectManager's object path
>     plus '/'.
> 
> (At least, I assume that's what you meant.)

Yes. Added.

> @@ +3023,3 @@
> +        and object path in question that the
> +        <literal>org.freedesktop.DBus.Properties.GetAll</literal>
> +        method would return. If an interface has no properties, the
> 
> This run-on sentence is rather awkward. How about:
> 
>     Each value is a dict, whose keys are interface names.
>     Each value in this inner dict is the same dict that
>     would be returned by the <>...GetAll</> method for that
>     combination of object path and interface. If an
>     interface has no ...

Sounds good. I've added that.

> 
> (I deliberately put object path first here because that's the logical order: as
> seen in dbus-send, and (hopefully) in the libdbus and GDBus APIs, the hierarchy
> from largest to smallest is bus name, object path, interface, member.)
> 
> Does GetAll actually specifically say that it returns {} for a propertyless
> interface? If not, it should. (There's a dbus-glib bug about that.)

Not sure it does (I think it returns an error in most bindings). For this application, though, I think it is unimportant if it does. Agree it would be nice to specify the behavior. I've filed bug 36190 for this (and also another related issue I've been meaning to report).

> @@ +3040,3 @@
> +        The <literal>InterfacesAdded</literal> signal is emitted when
> +        either a new object is added or if an existing object gains
> +        one or more interfaces while the
> 
> Grammar: choose whether you're saying "when" or "if" (I vote for "when"). I'd
> say:

Fixed.

> 
>     when either a new object is added, or an existing ...
> 
> I think this "while" deserves to be a sentence break.

and fixed.

> 
> @@ +3045,3 @@
> +        interfaces. The second parameter of the
> +        <literal>InterfacesAdded</literal> signal contains a dict with
> +        the interfaces and properties (if any) that has been added to
> 
> "that have been added": it's the interfaces and properties that were added, not
> the dict

Ah, right. Fixed.

> 
> @@ +3048,3 @@
> +        the given object path. Similarly, the second parameter of the
> +        <literal>InterfacesRemoved</literal> signal contains an array
> +        of the interfaces that was removed. Note that changes on
> 
> "that were removed": it's the interfaces that were removed, not the array.

Fixed.

> Do you have a use-case for interfaces disappearing without the entire object
> going away? I don't think changes to the list of interfaces are generally part
> of the de facto D-Bus object model? (dbus-glib and dbus-python certainly can't
> support it with their current design.)

Current bindings seems to be mostly concerned with objects with a single app-specific-and-useful interface (e.g. not counting .Peer, .Properties, .Introspectable). I gave examples back in

 http://lists.freedesktop.org/archives/dbus/2011-February/014161.html

last time we talked about it.

It could be some (existing) bindings won't easily be able to handle interfaces being added/removed on the fly. It could even be these bindings need to invent new types (such as GLib's upcoming GDBusObject/GDBusObjectProxy) to deal with it.

> 
> @@ +3050,3 @@
> +        of the interfaces that was removed. Note that changes on
> +        properties on existing interfaces are not reported using this
> +        interface - an application should use the
> 
> s/use/also monitor/ perhaps

Good idea.

> @@ +3057,3 @@
> +        object (directly or otherwise) implementing this interface but
> +        which are not returned in the reply from the
> +        <literal>GetAll()</literal> method of this interface on the
> 
> it's no longer called GetAll

Fixed.

> 
> @@ +3072,3 @@
> +      <para>
> +        on respectively the message bus and the remote
> +        application. Whenever a new remote object is created (or an
> 
> I tend to think of method calls as being an operation on objects (or proxies),
> rather than on destinations: "on the message bus, and the remote application's
> ObjectManager, respectively".

Good idea.

> 
> @@ +3074,3 @@
> +        application. Whenever a new remote object is created (or an
> +        existing object gains a new interface), the
> +        <literal>InterfacesAdded</literal> signal is emitted and since
> 
> emitted, and

Fixed.

> @@ +3077,3 @@
> +        this signal contains all properties for the interfaces, no
> +        calls to the <literal>org.freedesktop.Properties</literal>
> +        interface on the remote object is needed. Additionally, since
> 
> are needed

Fixed.

Thanks for the review. Will attach new patch.

Created attachment 45558 [details] [review]
Updated patch

Here's an updated patch that takes comment 9 into account.

Created attachment 45559 [details] [review]
Updated patch

This time, attach the right patch...

Review of attachment 45559 [details] [review]:

Looks fine, with trivial grammatical fixes. I'll commit it and add the 2-byte correction.

::: doc/dbus-specification.xml
@@ +3005,3 @@
+        An API can optionally make use of this interface for one or
+        more sub-trees of objects. The root of each sub-tree implement
+        this interface so other applications can get all objects,

should say "... sub-tree implements this ..."

@@ +3094,3 @@
+        <literal>org.freedesktop.Properties</literal> interface on the
+        remote object are needed. Additionally, since the initial
+        <literal>AddMatch()</literal> rule already include signal

"...already includes signal..."

Fixed in git for 1.5.2; I'm not going to apply normative spec changes to 1.4.

Simon: Thanks for the review - I owe you beverage or two next time we'll meet!