After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 647577 - ObjectManager support and code gdbus-codegen(1)
ObjectManager support and code gdbus-codegen(1)
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gdbus
unspecified
Other Linux
: Normal normal
: 2.30
Assigned To: David Zeuthen (not reading bugmail)
gtkdev
Depends on:
Blocks:
 
 
Reported: 2011-04-12 16:03 UTC by David Zeuthen (not reading bugmail)
Modified: 2011-04-29 17:31 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description David Zeuthen (not reading bugmail) 2011-04-12 16:03:25 UTC
I've been working on porting my gdbus-binding-tool code from

 http://cgit.freedesktop.org/~david/gdbus-binding-tool/

to mainline GLib. I intend to merge this soon - comments/feedback are very welcome.

The code in question is on the gdbus-codegen branch

 http://git.gnome.org/browse/glib/log/?h=gdbus-codegen

and and starts at the "Start merging gdbus-codegen code" commit.

There are three bits to this

 1. GObject generic marshaller support from bug 567087

 2. New types to support the org.freedesktop.DBus.ObjectManager interface
    - See https://bugs.freedesktop.org/show_bug.cgi?id=34869

 3. A new tool, gdbus-codegen(1)

The newly added code includes an extensive test suite as well as thorough documentation. I've uploaded the gio HTML docs from this branch here

 http://people.freedesktop.org/~david/gio-gdbus-codegen-20110412/

In particular, the newly added classes/interfaces are in

 http://people.freedesktop.org/~david/gio-gdbus-codegen-20110412/gdbus-convenience.html

and the new tool is described in

 http://people.freedesktop.org/~david/gio-gdbus-codegen-20110412/gdbus-codegen.html

I've also updated the "Migrating to GDBus" section to take the new code generator into account:

 http://people.freedesktop.org/~david/gio-gdbus-codegen-20110412/ch29.html
Comment 1 Simon McVittie 2011-04-12 16:23:40 UTC
> For example, an exported D-Bus interface may queue up property changes
> and emit the org.freedesktop.DBus.Properties::PropertiesChanged signal
> later (e.g. in an idle handler).

We tried what you're suggesting here in telepathy-mission-control and it was a mess; I would recommend against it.

To avoid misleading signals if a property changes twice, we had a mechanism to flush our PropertiesChanged equivalent when the same property changed again, or after a short hard-coded time (100ms, I think), whichever was sooner.

Because our properties tend to change together (several properties change at the same time when an account is asked to go online, then again when it succeeds), ideally we'd want them to change in a batch of "related" changes. However, the mechanism for not clobbering double property changes often ended up emitting half a batch and half of the next batch, flushing that implicitly to avoid clobbering a double property change, then sending the other half of the second batch (including the double-changed property) as another signal. This was rather confusing and not what we wanted.

We ended up with an API like this instead:

    mcd_account_freeze_notification (a);
    mcd_account_change_property (a, "Connection", x);
    mcd_account_change_property (a, "ConnectionStatus", y);
    mcd_account_change_property (a, "RequestedPresence", z);
    mcd_account_thaw_notification (a);

to put the changes into a batch explicitly.
Comment 2 Simon McVittie 2011-04-12 16:56:07 UTC
> g_dbus_interface_stub_get_properties ()

Is the returned GVariant an a{sv}? If so, say so.

> GDBusInterfaceStub

Maybe rename to GDBusExportable or something? The word "stub" doesn't make it instantly obvious (to me, at least) that it's the exporter (signal-emitting/method-implementing) side, as opposed to the proxy (signal-catching/method-calling) side.

> g_dbus_interface_stub_get_object_path ()
> Gets the object that that interface_ is exported on, if any. 

ITYM "the object path that".

Can an object be at more than one location? It can in dbus-glib, and it seems people actually care about this - I accidentally broke it while fixing a bug.

"That's not in the 95% I care about" is an acceptable answer.

A GDBusObjectAlias subclass of GDBusObjectStub (or something), that re-exports an entire object's API at a different object path for backwards-compat (which is what people use more-than-one-path for in practice), would also be an acceptable answer.

> The "g-authorize-method" signal
> If FALSE is returned then no further handlers are run and the signal
> handler must take ownership of invocation

Is that bindable?

Perhaps worth saying that you could make an async call-out to PolicyKit or whatever in your method, instead, if that's how you like to do things?

> GDBusObjectStub
> This type is intended to be used with GDBusObjectManager

If you're implementing an API where ObjectManager would hurt performance (like the more talkative parts of Telepathy), can you still use GDBusObjectStub / does it make sense to use it for any extra convenience?

Again, I don't think the term "stub" is self-explanatory: GDBusExportedObject?

Would it perhaps make sense for GDBusObjectManager's API requirement for its children to be "must implement Exportable, an interface; here is a sample implementation that 95% of you will use", to avoid wanting multiple inheritance?

> The GDBusObjectManager type is the base type for service- and client-side
> implementations of the standardized org.freedesktop.DBus.ObjectManager
> interface. 

You don't get to say that until we've standardized it :-P

(but see the fd.o bug for that)

> g_dbus_object_manager_get_objects

Why doesn't this return a map { path => object }?

> g_dbus_object_manager_server_export_and_uniquify

Perhaps export_uniquely, to avoid the send_with_reply_and_block_and_then_make_tea API-naming anti-pattern? :-)

GDBusObjectManagerServer could maybe benefit from API to unexport *itself*.

I suggest using example.com, example.net for example namespaces, in an attempt to shift the meme of "D-Bus namespaces are 'org.' plus your project name" (or worse, "land-grab a freedesktop name"). cf. org.awesome, alleged to belong to the Awesome window manager.

I notice gdbus-codegen puts order-dependent things in structs. This means it'll break API/ABI if you re-order your interface XML, which is a regression compared with the codegen we use in telepathy-glib (on the other hand, ours is very introspection-unfriendly).
Comment 3 David Zeuthen (not reading bugmail) 2011-04-12 17:55:33 UTC
(In reply to comment #1)
> > For example, an exported D-Bus interface may queue up property changes
> > and emit the org.freedesktop.DBus.Properties::PropertiesChanged signal
> > later (e.g. in an idle handler).
> 
> We tried what you're suggesting here in telepathy-mission-control and it was a
> mess; I would recommend against it.

Seems to work fine in udisks2 so far - maybe because I use _g_value_equal() (I generate a version that is 'good' enough for the cases gdbus-codegen(1) requires) in the generated code and you didn't? Let me copy-paste the relevant parts in [1].

Specifically, I wanted the code to support this use-case

 static void
 update_bar (SomeInterface *iface)
 {
  some_interface_set_foo (iface, "");
  some_interface_set_bar (iface, "");
  if (some_condition)
    {
      some_interface_set_foo (iface, "something");
      some_interface_set_bar (iface, "something else");
    }
 }

since that is a very common pattern since you don't want to litter you code with code comparing properties...

(and now that I'm copy-pasting the code, I'm seeing we should probably do _g_value_equal() again in _example_animal_emit_changed() too... sounds like a nice optimization.)

> To avoid misleading signals if a property changes twice, we had a mechanism to
> flush our PropertiesChanged equivalent when the same property changed again, or
> after a short hard-coded time (100ms, I think), whichever was sooner.
> 
> Because our properties tend to change together (several properties change at
> the same time when an account is asked to go online, then again when it
> succeeds), ideally we'd want them to change in a batch of "related" changes.
> However, the mechanism for not clobbering double property changes often ended
> up emitting half a batch and half of the next batch, flushing that implicitly
> to avoid clobbering a double property change, then sending the other half of
> the second batch (including the double-changed property) as another signal.
> This was rather confusing and not what we wanted.
> 
> We ended up with an API like this instead:
> 
>     mcd_account_freeze_notification (a);
>     mcd_account_change_property (a, "Connection", x);
>     mcd_account_change_property (a, "ConnectionStatus", y);
>     mcd_account_change_property (a, "RequestedPresence", z);
>     mcd_account_thaw_notification (a);
> 
> to put the changes into a batch explicitly.

For the gdbus-codegen(1) generated code, you can just use g_object_freeze_notify() and g_object_thaw_notify() if you want. Either way, the bottom line is that I don't think we need to sacrifice API usability by requiring the use to enclose property changes between freeze()/thaw() pairs...

     David

[1] :

typedef struct
{
  GDBusPropertyInfo parent_struct;
  const gchar *hyphen_name;
  gboolean use_gvariant;
} _ExtendedGDBusPropertyInfo;

typedef struct
{
  const _ExtendedGDBusPropertyInfo *info;
  GParamSpec *pspec;
  GValue value;
} ChangedProperty;


[...]

static void
example_animal_stub_dbus_interface_flush (GDBusInterfaceStub *_stub)
{
  ExampleAnimalStub *stub = EXAMPLE_ANIMAL_STUB (_stub);
  if (stub->priv->changed_properties_idle_source != NULL)
    {
      g_source_destroy (stub->priv->changed_properties_idle_source);
      stub->priv->changed_properties_idle_source = NULL;
      _example_animal_emit_changed (stub);
    }
}


static gboolean
_example_animal_emit_changed (gpointer user_data)
{
  ExampleAnimalStub *stub = EXAMPLE_ANIMAL_STUB (user_data);
  GList *l;
  GVariantBuilder builder;
  GVariantBuilder invalidated_builder;
  g_variant_builder_init (&builder, G_VARIANT_TYPE ("a{sv}"));
  g_variant_builder_init (&invalidated_builder, G_VARIANT_TYPE ("as"));
  for (l = stub->priv->changed_properties; l != NULL; l = l->next)
    {
      ChangedProperty *cp = l->data;
      GVariant *variant;
      variant = g_dbus_gvalue_to_gvariant (&cp->value, G_VARIANT_TYPE (cp->info->parent_struct.signature));
      g_variant_builder_add (&builder, "{sv}", cp->info->parent_struct.name, variant);
      g_variant_unref (variant);
    }
  g_dbus_connection_emit_signal (g_dbus_interface_stub_get_connection (G_DBUS_INTERFACE_STUB (stub)),
                                 NULL, g_dbus_interface_stub_get_object_path (G_DBUS_INTERFACE_STUB (stub)),
                                 "org.freedesktop.DBus.Properties",
                                 "PropertiesChanged",
                                 g_variant_new ("(sa{sv}as)",
                                                "org.gtk.GDBus.Example.ObjectManager.Animal",
                                                &builder, &invalidated_builder),
                                 NULL);
  g_list_foreach (stub->priv->changed_properties, (GFunc) _changed_property_free, NULL);
  g_list_free (stub->priv->changed_properties);
  stub->priv->changed_properties = NULL;
  stub->priv->changed_properties_idle_source = NULL;
  return FALSE;
}


static void
_example_animal_schedule_emit_changed (ExampleAnimalStub *stub, const _ExtendedGDBusPropertyInfo *info, GParamSpec *pspec, const GValue *value)
{
  ChangedProperty *cp;
  GList *l;
  cp = NULL;
  for (l = stub->priv->changed_properties; l != NULL; l = l->next)
    {
      ChangedProperty *i_cp = l->data;
      if (i_cp->info == info)
        {
          cp = i_cp;
          g_value_unset (&cp->value);
          break;
        }
    }
  if (cp == NULL)
    {
      cp = g_new0 (ChangedProperty, 1);
      cp->pspec = pspec;
      cp->info = info;
      stub->priv->changed_properties = g_list_prepend (stub->priv->changed_properties, cp);
    }
  g_value_init (&cp->value, G_VALUE_TYPE (value));
  g_value_copy (value, &cp->value);
  if (stub->priv->changed_properties_idle_source == NULL)
    {
      stub->priv->changed_properties_idle_source = g_idle_source_new ();
      g_source_set_priority (stub->priv->changed_properties_idle_source, G_PRIORITY_DEFAULT);
      g_source_set_callback (stub->priv->changed_properties_idle_source, _example_animal_emit_changed, g_object_ref (stub), (GDestroyNotify) g_object_unref);
      g_source_attach (stub->priv->changed_properties_idle_source, stub->priv->context);
      g_source_unref (stub->priv->changed_properties_idle_source);
    }
}

static void
example_animal_stub_set_property (GObject      *object,
  guint         prop_id,
  const GValue *value,
  GParamSpec   *pspec)
{
  ExampleAnimalStub *stub = EXAMPLE_ANIMAL_STUB (object);
  g_assert (prop_id - 1 >= 0 && prop_id - 1 < 1);
  if (!_g_value_equal (value, &stub->priv->properties->values[prop_id - 1]))
    {
      g_value_copy (value, &stub->priv->properties->values[prop_id - 1]);
      g_object_notify_by_pspec (object, pspec);
      if (g_dbus_interface_stub_get_connection (G_DBUS_INTERFACE_STUB (stub)) != NULL)
        _example_animal_schedule_emit_changed (stub, _example_animal_property_info_pointers[prop_id - 1], pspec, value);
    }
}
Comment 4 David Zeuthen (not reading bugmail) 2011-04-12 18:19:37 UTC
(In reply to comment #2)
> > g_dbus_interface_stub_get_properties ()
> 
> Is the returned GVariant an a{sv}? If so, say so.

Good idea.

> > GDBusInterfaceStub
> 
> Maybe rename to GDBusExportable or something? The word "stub" doesn't make it
> instantly obvious (to me, at least) that it's the exporter
> (signal-emitting/method-implementing) side, as opposed to the proxy
> (signal-catching/method-calling) side.

Maybe GDBusExportedInterface? (most users will use the abstract GDBusInterface type for such instances the "less typing" argument doesn't do much here...)

> > g_dbus_interface_stub_get_object_path ()
> > Gets the object that that interface_ is exported on, if any. 
> 
> ITYM "the object path that".

Yep. Will fix.

> Can an object be at more than one location? It can in dbus-glib, and it seems
> people actually care about this - I accidentally broke it while fixing a bug.
> 
> "That's not in the 95% I care about" is an acceptable answer.

It can't, no (and g_dbus_interface_stub_export() is one indication it can't).

> A GDBusObjectAlias subclass of GDBusObjectStub (or something), that re-exports
> an entire object's API at a different object path for backwards-compat (which
> is what people use more-than-one-path for in practice), would also be an
> acceptable answer.

With my GDBus maintainer hat on, "Use the existing low-level g_dbus_connection_register_object()" is probably acceptable.... I mean, as mentioned elsewhere, these high-level interfaces are not supposed to end world-hunger the same way core stuff like GDBusConnection/GDBusMessage must be versatile enough that you can do anything with them (e.g. file descriptor passing). IOW, it's fine if we can make the API a bit simpler by making assumptions and not catering to 1% or 5% use-cases. Of course that doesn't mean we shouldn't care about catering to as many people as possible but the bar is different from e.g. GDBusConnection/GDBusMessage.

> > The "g-authorize-method" signal
> > If FALSE is returned then no further handlers are run and the signal
> > handler must take ownership of invocation
> 
> Is that bindable?

Sure. The binding will simply take a reference to invocation if code touches it and give up that reference when done with it.... Just like any other reference-counted parameter passed to a function without transfer.

> Perhaps worth saying that you could make an async call-out to PolicyKit or
> whatever in your method, instead, if that's how you like to do things?

The whole reason for making these run in a separate thread is that you can use blocking IO....

> > GDBusObjectStub
> > This type is intended to be used with GDBusObjectManager
> 
> If you're implementing an API where ObjectManager would hurt performance (like
> the more talkative parts of Telepathy), can you still use GDBusObjectStub /
> does it make sense to use it for any extra convenience?

(Why would ObjectManager hurt performance for such API?)

> Again, I don't think the term "stub" is self-explanatory: GDBusExportedObject?

Could be.

> Would it perhaps make sense for GDBusObjectManager's API requirement for its
> children to be "must implement Exportable, an interface; here is a sample
> implementation that 95% of you will use", to avoid wanting multiple
> inheritance?
> 
> > The GDBusObjectManager type is the base type for service- and client-side
> > implementations of the standardized org.freedesktop.DBus.ObjectManager
> > interface. 
> 
> You don't get to say that until we've standardized it :-P
> 
> (but see the fd.o bug for that)

Sure. If it doesn't get into the D-Bus spec (which would be a surprise to me), we can always do s/org.freedesktop.DBus.ObjectManager/org.gtk.GDBus.ObjectManager/ just before releasing 2.30.0. That would be a shame though.... I mean, I really want other bindings to support this too.

> > g_dbus_object_manager_get_objects
> 
> Why doesn't this return a map { path => object }?

Because you must never use GHashTable in public API that is supposed to be bindable - and you can already get the object path from the returned objects.

> > g_dbus_object_manager_server_export_and_uniquify
> 
> Perhaps export_uniquely, to avoid the
> send_with_reply_and_block_and_then_make_tea API-naming anti-pattern? :-)

I agree, that's a nicer name!

> GDBusObjectManagerServer could maybe benefit from API to unexport *itself*.

I did think about adding start()/stop() methods... but didn't do that so far. Couldn't think of any reasonable use-case. Anyone?

> I suggest using example.com, example.net for example namespaces, in an attempt
> to shift the meme of "D-Bus namespaces are 'org.' plus your project name" (or
> worse, "land-grab a freedesktop name"). cf. org.awesome, alleged to belong to
> the Awesome window manager.

Good idea.

> I notice gdbus-codegen puts order-dependent things in structs. This means it'll
> break API/ABI if you re-order your interface XML, which is a regression
> compared with the codegen we use in telepathy-glib (on the other hand, ours is
> very introspection-unfriendly).

All function pointers are in GTypeInterface-derived struct and reordering there isn't an ABI/API break for C at least. Is it a problem for other languages?

(If it's a problem, we could sort things in the struct by the tupple (since_tag, name)... I want to add support for a @since annotation anyway since it's useful for the generated docs.)
Comment 5 David Zeuthen (not reading bugmail) 2011-04-12 18:22:18 UTC
(In reply to comment #4)
> > I notice gdbus-codegen puts order-dependent things in structs. This means it'll
> > break API/ABI if you re-order your interface XML, which is a regression
> > compared with the codegen we use in telepathy-glib (on the other hand, ours is
> > very introspection-unfriendly).
> 
> All function pointers are in GTypeInterface-derived struct and reordering there
> isn't an ABI/API break for C at least. Is it a problem for other languages?
> 
> (If it's a problem, we could sort things in the struct by the tupple
> (since_tag, name)... I want to add support for a @since annotation anyway since
> it's useful for the generated docs.)

Actually, I'm smoking crack. Of course reordering is a problem there, I was thinking of extending the interface. I'll add support for @since and sorting right away....
Comment 6 David Zeuthen (not reading bugmail) 2011-04-12 20:19:33 UTC
(In reply to comment #5)
> Actually, I'm smoking crack. Of course reordering is a problem there, I was
> thinking of extending the interface. I'll add support for @since and sorting
> right away....

Fixed here

 http://git.gnome.org/browse/glib/commit/?h=gdbus-codegen&id=34a28f2f062281d9fb75fcd02c4df238def6396e

including tests and docs.
Comment 7 David Zeuthen (not reading bugmail) 2011-04-12 20:27:31 UTC
(In reply to comment #4)
> > Perhaps worth saying that you could make an async call-out to PolicyKit or
> > whatever in your method, instead, if that's how you like to do things?
> 
> The whole reason for making these run in a separate thread is that you can use
> blocking IO....

I obviously misread your comment, sorry. To answer, I think it should be obvious that you don't _have_ to use the ::g-authorize-method machinery at all and you can do any check, authorization or otherwise, you want from the method handler.
Comment 8 David Zeuthen (not reading bugmail) 2011-04-12 23:45:43 UTC
OK, so I thought about the suggested

 GDBusInterfaceStub -> GDBusExportedInterface (or GDbusExportable)
 GDBusObjectStub    -> GDBusExportedObject

Note that gdbus-codegen(1) generates three types

 MyFrobber      (A GInterface)
 MyFrobberProxy extends GDBusProxy, implements MyFrobber
 MyFrobberStub  extends GDBusInterfaceStub, implements MyFrobber

The intent is that API users either use MyFrobberStub directly or subclass it.

So what would that look like with the suggested rename? Obviously, we'd still have MyFrobberProxy (and this type is only useful for it constructors - everything else you want to use is on MyFrobber itself) but... MyExportedFrobber?

 MyFrobber
 MyFrobberProxy
 MyExportedFrobber

It does look a bit weird. Let's try with real names

 UDisksFilesystem
 UDisksFilesystemProxy
 UDisksExportedFilesystem

I'm not sure that works... Maybe Implementation (or short Impl) would be better?

For the record, I originally used "Stub" because I thought I remembered that's what CORBA and RMI uses...

 http://en.wikipedia.org/wiki/Stub_(distributed_computing)
 http://en.wikipedia.org/wiki/Java_remote_method_invocation
 http://en.wikipedia.org/wiki/Corba

But I actually got that wrong - CORBA/RMI is using Skeleton. So maybe

 MyFrobber          (A GInterface)
 MyFrobberProxy     extends GDBusProxy, implements MyFrobber
 MyFrobberSkeleton  extends GDBusInterfaceSkeleton, implements MyFrobber

 UDisksFilesystem
 UDisksFilesystemProxy
 UDisksFilesystemSkeleton

which actually sounds a lot better (since skeleton actually implies that you just need to add the meat and organs in the right places.. and then some blood to get it running.. so to speak.. sorry for the gross analogy)...

So maybe Skeleton is what we want? Answers on a post-card....
Comment 9 David Zeuthen (not reading bugmail) 2011-04-13 00:18:28 UTC
(In reply to comment #4)
> (In reply to comment #2)
> > > g_dbus_interface_stub_get_properties ()
> > 
> > Is the returned GVariant an a{sv}? If so, say so.
> 
> Good idea.

Done. Also see bug 647614.

> > > g_dbus_interface_stub_get_object_path ()
> > > Gets the object that that interface_ is exported on, if any. 
> > 
> > ITYM "the object path that".
> 
> Yep. Will fix.

Fixed.

> > > g_dbus_object_manager_server_export_and_uniquify
> > 
> > Perhaps export_uniquely, to avoid the
> > send_with_reply_and_block_and_then_make_tea API-naming anti-pattern? :-)
> 
> I agree, that's a nicer name!

Fixed.

Commit with these fixes: http://git.gnome.org/browse/glib/commit/?h=gdbus-codegen&id=6a4ab7c5d3d98318c38816a64b3c444a2c99ea9b
Comment 10 David Zeuthen (not reading bugmail) 2011-04-13 01:42:11 UTC
Further research shows that others also uses stub and skeleton interchangeably, from http://en.wikipedia.org/wiki/Class_skeleton: note that the stub is called "proxy" and the skeleton is called "stub". Oh well.
Comment 11 Simon McVittie 2011-04-13 10:18:55 UTC
(In reply to comment #4)
> > > The "g-authorize-method" signal
> > > If FALSE is returned then no further handlers are run and the signal
> > > handler must take ownership of invocation
> > 
> > Is that bindable?
> 
> Sure. The binding will simply take a reference to invocation if code touches it
> and give up that reference when done with it.... Just like any other
> reference-counted parameter passed to a function without transfer.

Ah, good. This could probably do with clearer docs then ("must take a reference to @invocation" or something; I'd read this as being a "ref-stealing" API, like g_value_take_object/g_hash_table_steal.

> > If you're implementing an API where ObjectManager would hurt performance (like
> > the more talkative parts of Telepathy), can you still use GDBusObjectStub /
> > does it make sense to use it for any extra convenience?
> 
> (Why would ObjectManager hurt performance for such API?)

It's more about power consumption/wasted work than performance, I suppose. For instance, a Telepathy Connection has Channel children, so it'd be tempting to make the Connection be an ObjectManager<Channel>. However, a Telepathy Connection emits signals quite frequently for various protocol events, and so do Channels. If we used ObjectManager, everything that interacted with the Connection in any way would be woken up whenever anything at all happened in the protocol: someone's alias/capabilities changed, someone sent us an IM, whatever.

We avoid that by dividing signals among interfaces and using dbus-glib's default "bind to the whole interface" behaviour, to make sure that only the "interested" subset of UIs get woken up by each event.

> > > g_dbus_object_manager_get_objects
> > 
> > Why doesn't this return a map { path => object }?
> 
> Because you must never use GHashTable in public API that is supposed to be
> bindable - and you can already get the object path from the returned objects.

Does (element-type utf8 GObject) not work? :-( If it doesn't, we should put a great big warning on the "writing bindable APIs" wiki page (and we'll need to re-think a lot of C API for Telepathy 1.0). Or fix it, of course.

But, yeah, the GList makes sense.

> > GDBusObjectManagerServer could maybe benefit from API to unexport *itself*.
> 
> I did think about adding start()/stop() methods... but didn't do that so far.
> Couldn't think of any reasonable use-case. Anyone?

When an object has a natural life-cycle, it's good to take it off the bus explicitly, rather than relying on weak refs or whatever (which, in practice, doesn't work, because someone will be holding an unexpected ref). For instance, when a Telepathy Connection disconnects (we probably won't actually be using ObjectManager for those, but it's the D-Bus API I know best).

> (If it's a problem, we could sort things in the struct by the tupple
> (since_tag, name)... I want to add support for a @since annotation anyway since
> it's useful for the generated docs.)

Would you consider a tuple (since_tag, sequence number, name) or something (with the sequence number optional and defaulting to 0, or whatever), so people who care about ABI in development versions/are control freaks can use the sequence number? :-)

I'm a bit worried that if libraries use GLib-like @since tags (2.28 rather than 2.27.5), every development version that added signals would break ABI: yes I know you consider this to be acceptable in GLib, but there's a difference between "might break ABI" and "breaks ABI for no reason".

(Admittedly, this wouldn't affect Telepathy, since we only add ABI even in development releases, and we set @since to the development version.)

(In reply to comment #8)
> But I actually got that wrong - CORBA/RMI is using Skeleton. So maybe
> 
>  MyFrobber          (A GInterface)
>  MyFrobberProxy     extends GDBusProxy, implements MyFrobber
>  MyFrobberSkeleton  extends GDBusInterfaceSkeleton, implements MyFrobber

I think this is much better. Or, if a D-Bus API uses the "interfaces are verbable" naming convention, we'd get

MyBadgerable           (A GInterface)
MyBadgerableProxy      extends GDBusProxy, implements MyBadgerable
MyBadgerableSkeleton   extends GDBusInterfaceSkeleton, implements MyBadgerable

which also seems fine (the interface is still verbable and the objects are nouns - success!)

FWIW, in Telepathy we typically use MyFrobber for the client-side proxy (although it sounds as though with GDBus' emphasis on interfaces, giving the client-side proxy a longer name isn't a problem, because you hardly ever mention it), and MyBaseFrobber for the service-side base class.

(In reply to comment #10)
> Further research shows that others also uses stub and skeleton interchangeably,
> from http://en.wikipedia.org/wiki/Class_skeleton: note that the stub is called
> "proxy" and the skeleton is called "stub". Oh well.

I'd say that's less "interchangeably" and more "skeleton is always the service, proxy is always the client, and nobody knows what stub means" :-P
Comment 12 Matthias Clasen 2011-04-13 17:16:57 UTC
(In reply to comment #4)

> 
> > GDBusObjectManagerServer could maybe benefit from API to unexport *itself*.
> 
> I did think about adding start()/stop() methods... but didn't do that so far.
> Couldn't think of any reasonable use-case. Anyone?

I vaguely recall unexporting coming up in the context of having interfaces exported from a plugin, and then unloading the plugin. g-s-d, maybe ?
Comment 13 David Zeuthen (not reading bugmail) 2011-04-13 20:59:08 UTC
Hey,

(In reply to comment #11)
> (In reply to comment #4)
> > > > The "g-authorize-method" signal
> > > > If FALSE is returned then no further handlers are run and the signal
> > > > handler must take ownership of invocation
> > > 
> > > Is that bindable?
> > 
> > Sure. The binding will simply take a reference to invocation if code touches it
> > and give up that reference when done with it.... Just like any other
> > reference-counted parameter passed to a function without transfer.
> 
> Ah, good. This could probably do with clearer docs then ("must take a reference
> to @invocation" or something; I'd read this as being a "ref-stealing" API, like
> g_value_take_object/g_hash_table_steal.

Sure. Clarified in http://git.gnome.org/browse/glib/commit/?h=gdbus-codegen&id=8826ad046d3dfa1a0fbaca1cab1086d12f31d0a5

(Actually the g_dbus_method_invocation_return_*() family of methods on GDBusMethodInvocation are ref-stealing for C convenience... not sure how bindings currently deal with this or whether it was a good idea to do that in the first place. But it's done and also orthogonal to this problem.)

> > (Why would ObjectManager hurt performance for such API?)
> 
> It's more about power consumption/wasted work than performance, I suppose. For
> instance, a Telepathy Connection has Channel children, so it'd be tempting to
> make the Connection be an ObjectManager<Channel>. However, a Telepathy
> Connection emits signals quite frequently for various protocol events, and so
> do Channels. If we used ObjectManager, everything that interacted with the
> Connection in any way would be woken up whenever anything at all happened in
> the protocol: someone's alias/capabilities changed, someone sent us an IM,
> whatever.
> 
> We avoid that by dividing signals among interfaces and using dbus-glib's
> default "bind to the whole interface" behaviour, to make sure that only the
> "interested" subset of UIs get woken up by each event.

OK. In somes case having a forest of ObjectManager trees is also useful - then clients can subscribe to only the events they are interested in. I don't know, there are always many right ways to design an API (and also many wrong ways but that's not the point). I don't think ObjectManager is the be-all-end-all solution by any means...

So the original question was something like "can GDBusObjectSkeleton be useful outside GDBusObjectManagerServer?" I think. I think the answer to that question is something like: yeah, it can be useful as you want given you have as much access to internals as GDBusObjectManagerServer... one way to think of it, is that it's just a collection of service-side interface implementations with enough information to export them on a GDBusConnection. Is that an acceptable answer?

> > > > g_dbus_object_manager_get_objects
> > > 
> > > Why doesn't this return a map { path => object }?
> > 
> > Because you must never use GHashTable in public API that is supposed to be
> > bindable - and you can already get the object path from the returned objects.
> 
> Does (element-type utf8 GObject) not work? :-( If it doesn't, we should put a
> great big warning on the "writing bindable APIs" wiki page (and we'll need to
> re-think a lot of C API for Telepathy 1.0). Or fix it, of course.
> 
> But, yeah, the GList makes sense.

I'm 99% sure that isn't enough. Last time I ran into this was for the PolicyKit 1.0 work where I ended up doing a PolkitDetails class instead of using a GHashTable that always mapped from utf8 -> utf8.... Adding Colin as Cc as he might be able to shed some more light on this. Colin?

> > > GDBusObjectManagerServer could maybe benefit from API to unexport *itself*.
> > 
> > I did think about adding start()/stop() methods... but didn't do that so far.
> > Couldn't think of any reasonable use-case. Anyone?
> 
> When an object has a natural life-cycle, it's good to take it off the bus
> explicitly, rather than relying on weak refs or whatever (which, in practice,
> doesn't work, because someone will be holding an unexpected ref). For instance,
> when a Telepathy Connection disconnects (we probably won't actually be using
> ObjectManager for those, but it's the D-Bus API I know best).

Makes sense, I will make this possible. Probably by being able to set the GDBusConnection that the objects are exported on...

> > (If it's a problem, we could sort things in the struct by the tupple
> > (since_tag, name)... I want to add support for a @since annotation anyway since
> > it's useful for the generated docs.)
> 
> Would you consider a tuple (since_tag, sequence number, name) or something
> (with the sequence number optional and defaulting to 0, or whatever), so people
> who care about ABI in development versions/are control freaks can use the
> sequence number? :-)
> 
> I'm a bit worried that if libraries use GLib-like @since tags (2.28 rather than
> 2.27.5), every development version that added signals would break ABI: yes I
> know you consider this to be acceptable in GLib, but there's a difference
> between "might break ABI" and "breaks ABI for no reason".
> 
> (Admittedly, this wouldn't affect Telepathy, since we only add ABI even in
> development releases, and we set @since to the development version.)

Not sure how a sequence_number would help - what if a signal is removed during the development-cycle? (e.g. then we'd need to insert dummy padding)... so I think the answer is no here... I mean, if people need this they can always just add public C functions to set the function pointer like this

 void my_application_set_handle_foo_method_ptr (MyApplicationIface *iface, void (*function_pointer) (MyApplication *object, GDBusMethodInvocation *invocation, const gchar *greeting));

> (In reply to comment #8)
> > But I actually got that wrong - CORBA/RMI is using Skeleton. So maybe
> > 
> >  MyFrobber          (A GInterface)
> >  MyFrobberProxy     extends GDBusProxy, implements MyFrobber
> >  MyFrobberSkeleton  extends GDBusInterfaceSkeleton, implements MyFrobber
> 
> I think this is much better. Or, if a D-Bus API uses the "interfaces are
> verbable" naming convention, we'd get
> 
> MyBadgerable           (A GInterface)
> MyBadgerableProxy      extends GDBusProxy, implements MyBadgerable
> MyBadgerableSkeleton   extends GDBusInterfaceSkeleton, implements MyBadgerable
> 
> which also seems fine (the interface is still verbable and the objects are
> nouns - success!)
> 
> FWIW, in Telepathy we typically use MyFrobber for the client-side proxy
> (although it sounds as though with GDBus' emphasis on interfaces, giving the
> client-side proxy a longer name isn't a problem, because you hardly ever
> mention it), and MyBaseFrobber for the service-side base class.
> 
> (In reply to comment #10)
> > Further research shows that others also uses stub and skeleton interchangeably,
> > from http://en.wikipedia.org/wiki/Class_skeleton: note that the stub is called
> > "proxy" and the skeleton is called "stub". Oh well.
> 
> I'd say that's less "interchangeably" and more "skeleton is always the service,
> proxy is always the client, and nobody knows what stub means" :-P

Excellent, glad you also like Skeleton. I did that change in

 http://git.gnome.org/browse/glib/commit/?h=gdbus-codegen&id=6ccca55752c41001f3af3430d3d93f587fd42383
Comment 14 David Zeuthen (not reading bugmail) 2011-04-15 19:59:59 UTC
(In reply to comment #3)
> (and now that I'm copy-pasting the code, I'm seeing we should probably do
> _g_value_equal() again in _example_animal_emit_changed() too... sounds like a
> nice optimization.)

OK, I just added this optimization - the trickiest part here was figuring out how to write a test case that ensures _no signal_ is sent:

 http://git.gnome.org/browse/glib/commit/?h=gdbus-codegen&id=6bccc46d152079d69cf8aebef08433b1ec6055c7
Comment 15 Colin Walters 2011-04-18 15:05:03 UTC
(In reply to comment #11)
>
> > Because you must never use GHashTable in public API that is supposed to be
> > bindable - and you can already get the object path from the returned objects.
> 
> Does (element-type utf8 GObject) not work? 

It "should" work but it's discouraged; one problem is that in particular we don't have a way to express it in signal handlers with GType.  gjs is still using GType for signal data for example.  We will eventually switch over to introspection data which will make this work.

Another rationale is that it's not a great API for C programmers either.
Comment 16 Colin Walters 2011-04-18 15:13:24 UTC
(In reply to comment #13)
>
> (Actually the g_dbus_method_invocation_return_*() family of methods on
> GDBusMethodInvocation are ref-stealing for C convenience... not sure how
> bindings currently deal with this or whether it was a good idea to do that in
> the first place. But it's done and also orthogonal to this problem.)

In theory we supported non-none transfer on input arguments at some point, but i removed it since it was too much of a mess for no gain.  We could look at readding it just for GObjects maybe.
Comment 17 David Zeuthen (not reading bugmail) 2011-04-27 16:37:42 UTC
Btw, here's a real-word example using gdbus-codegen:

 http://people.freedesktop.org/~david/goa-20110427/

that isn't udisks :-) ... this also shows the new GDBusObject{,Proxy,Skeleton} specialization that I recently added. In this case, it's

 http://people.freedesktop.org/~david/goa-20110427/GoaObject.html

which makes things a bit more type-safe compared to using GDBusObject (and its implementations) directly.. but more importantly, allows non-generated API like GoaClient

 http://people.freedesktop.org/~david/goa-20110427/GoaClient.html

to use GoaObject instead of GDBusObject. It just makes the API feel a lot less generated this way.
Comment 18 David Zeuthen (not reading bugmail) 2011-04-29 17:25:10 UTC
Merged this to master after getting the OK from Matthias. Please open separate bugs for remaining issues (I don't think there are any). Thanks.
Comment 19 David Zeuthen (not reading bugmail) 2011-04-29 17:31:24 UTC
Filed bug 648959 for the GDBusObjectManagerServer start()/stop() thing that Simon raised in comment 11 and we discussed after that.