Bug 59024

Extra potential g-i problems:

6) tp_foo_mixin_init_dbus_properties(GObjectClass*): is it possible to call in class_init in binding languages? what are the alternatives?

7) tp_foo_mixin_iface_init(): Can we give functions to implement the dbus ifaces in a subclasses not in C?

8) tp_foo_mixin_do_something(): Will g-i understand that this is a function on TpBaseConnection object? Will I be able to do base_connection.do_something()?


Maybe crazy, but I'm wondering if we should make those mixins really part of TpBaseConnection, but optionally initialized. how it would looks like:

TpBaseConnectionPrivate{TpFooMixinPrivate *foo_mixin; ...}

TpBaseConnection::finalize{_tp_foo_mixin_free (self->priv->foo_mixin); ...}

tp_base_connection_init_foo(TpBaseConnection *self){self->priv->foo_mixin = _tp_foo_mixin_new();}

tp_base_connection_foo_changed(TpBaseConnection *self)
{g_return_val_if_fail(self->priv->foo_mixin != NULL); ...}

But still that won't fix 6) and 7).

(In reply to comment #1)
> 6) tp_foo_mixin_init_dbus_properties(GObjectClass*): is it possible to call
> in class_init in binding languages? what are the alternatives?

If necessary, we can teach the TpDBusPropertiesMixin to search a per-instance list before the per-class one.

I have a work-in-progress branch which reduces it to:

* implement TpExportable with no methods (introspectable, I think)

* call Tp.DBusPropertiesMixin.implement_interface(...) from class_init
  (not introspectable)

* call Tp.DBusPropertiesMixin.init(self) from __init__ or equivalent
  (introspectable, I think)

> 7) tp_foo_mixin_iface_init(): Can we give functions to implement the dbus
> ifaces in a subclasses not in C?

No, but we can use TpExportable.

> 8) tp_foo_mixin_do_something(): Will g-i understand that this is a function
> on TpBaseConnection object? Will I be able to do
> base_connection.do_something()?

No, you'll have to do Tp.FooMixin.do_something(self). I don't think that's *so* bad.

We can use GInterfaces for syntactic sugar if necessary? If you implement Tp.FooMixable with a foo_do_something() method, you can call self.foo_do_something() (really tp_foo_mixable_foo_do_something() in C). We could use the same GInterface to hold the necessary vfuncs for the actual implementation.

> Maybe crazy, but I'm wondering if we should make those mixins really part of
> TpBaseConnection, but optionally initialized.

I would like to keep this as a last resort: if all our objects are this tightly coupled, replacing one becomes really annoying.

Created attachment 87989 [details] [review]
[bugfix] inspect-cm: fix the "one CM" case

---

I happened to run into this while testing. Please review.

Created attachment 87990 [details] [review]
[cleanup] codegen: factor out copy_into_gvalue

---

I needed this for TpExportable, but it's already a small code reduction, so we could apply it.

Created attachment 87991 [details] [review]
[WiP] Add TpExportable, an introspectable way for mixins to  implement methods

The main problems with our current mixins, from an introspection point
of view, are:

* needing to be able to call functions during class initialization;
* needing to be able to implement a parent object's D-Bus methods,
  currently done with G_IMPLEMENT_INTERFACE, which is not introspectable

The first can be avoided by calling similar functions during instance
initialization, if necessary.

The second is somewhat harder, but we can use a signal as our hook.

---

We could add get-property and set-property signals which work similarly, if desired.

Open questions include:

* Should the signal be call-method::inter.face.method or just call-method::inter.face? (Currently call-method::inter.face.method.)

* If a parent class and its subclass both connect to call-method for the same method, I think the parent class' implementation will get the first chance to respond to the method call, which is clearly wrong. Can we arrange for the most recently-connected signal connection to be called first, instead?

This needs more regression tests, probably.

Created attachment 87992 [details] [review]
[WiP] TpExportable: add introspectable ways to return values

dbus-glib types are awkward in introspected languages. Let's try
GVariant instead.

This requires an as-yet unmerged dbus-glib branch (Bug #48821).

Created attachment 87993 [details] [review]
[WiP] TpDBusPropertiesMixin: allow linking to an object via  TpExportable

This should let introspected code use the mixin, I think.

---

... or not, because it can't call implement_interface() yet. Still, it's a start...

We'd also have to export at least a trivial amount of the TpSvc* stuff (just the fact that the struct exists) to introspected code, for it to be able to do a trivial no-vfuncs implementation of an interface.

The Contacts mixin is the obvious candidate for the next place to use this technology.

Created attachment 87994 [details] [review]
[WiP] TpBaseConnection: implement Exportable

Created attachment 87995 [details] [review]
[WiP] TpBaseConnection: implement properties via  TpExportable::call-method

Comment on attachment 87989 [details] [review]
[bugfix] inspect-cm: fix the "one CM" case

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

++

Comment on attachment 87990 [details] [review]
[cleanup] codegen: factor out copy_into_gvalue

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

++

Now that we ported tp-glib to GDBus, it became possible[1] to implement dbus interfaces using gdbus-codegen skeletons. That means that TpPresenceMixin can be just like TpBaseContactList[2]: a standalone GObject.

TpPresenceMixin could be turned into an abstract GObject that takes a TpBaseConnection as construct-only property. It will then create the gdbus-codegen' skeleton, implement all its stuff, and simply call tp_dbus_object_add_skeleton(base_connection, presence_skeleton);

We only have 3 mixins, How Hard Can It Be?™

[1] With few extra patches, still unmerged at time of writing:
http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=gdbus-custom-skeletons

[2] With few extra patches, still unmerged at time of writing: 
http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=gdbus-contactlist

(In reply to comment #12)
> TpPresenceMixin could be turned into an abstract GObject that takes a
> TpBaseConnection as construct-only property. It will then create the
> gdbus-codegen' skeleton, implement all its stuff, and simply call
> tp_dbus_object_add_skeleton(base_connection, presence_skeleton);

Yes, I think this is a good concept - but I think tp_dbus_object_add_skeleton() can in fact just be g_dbus_object_skeleton_add_interface() in constructed(). I have a rather sketchy branch which implements this for TpBaseProtocol (because it's the simplest).

I'll try it for TpBaseConnection, rebasing your implementation of a custom GDBusInterfaceSkeleton onto it, when I get a chance.

> We only have 3 mixins, How Hard Can It Be?™

Indeed.

I changed my mind actually. If we make a TpBasePresence it means we'll have to subclass it in every CM (and test CM) and I'm far too lazy to do that in C.

So I turned it into a GInterface instead and TpBaseConnection implements the dbus interface using gdbus-codegen's skeleton if (TP_IS_PRESENCE_MIXIN (self)). Simple, introspectable, and minimal changes needed in CMs.

My branches are starting to stack... http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=gdbus-presence

(In reply to comment #14)
> TpBaseConnection implements the
> dbus interface using gdbus-codegen's skeleton if (TP_IS_PRESENCE_MIXIN
> (self)).

I didn't like this design in the past, and I still don't. I would like to keep non-core interfaces out of TpBaseConnection.

With the GDBusObjectSkeleton branch I'm trying to finish, I think we can do this, though:

* TpPresenceMixin is a GInterface
* TpPresenceSkeleton is a GDBusInterfaceSkeleton subclass
* HazeConnection has-a TpPresenceSkeleton, and implements TpPresenceMixin
* TpPresenceSkeleton calls methods on TpPresenceMixin
* haze_connection_constructed() adds its TpPresenceSkeleton to its
  GDBusObjectSkeleton list of interfaces

which isn't that much boilerplate and keeps the loose coupling.

(In reply to comment #14)
> My branches are starting to stack...

I would like to finish my GDBusObjectSkeleton branch so it can replace your tp_dbus_object_add_interface(). Then we can pull in your TpBaseConnection and TpBaseContactList changes.

I am strongly opposed to letting telepathy-glib get a significant distance ahead of the CMs, which is why I'm taking so long to do anything on this - I don't consider it to be done until the CMs pass tests. We decoupled them early on in 'next' development, and it was months/years before we had versions of all the bits that compiled together, let alone worked.

(In reply to comment #15)
> * TpPresenceMixin is a GInterface
> * TpPresenceSkeleton is a GDBusInterfaceSkeleton subclass
> * HazeConnection has-a TpPresenceSkeleton, and implements TpPresenceMixin
> * TpPresenceSkeleton calls methods on TpPresenceMixin
> * haze_connection_constructed() adds its TpPresenceSkeleton to its
>   GDBusObjectSkeleton list of interfaces

Hmmm. But TpPresenceSkeleton would be subclass of _TpGDBusConnectionInterfacePresence1 and I don't want that codegen to be public. I woudld the rather have:

 * TpPresenceMixinInterface is-a GInterface for TpBaseConnection subclasses.
 * TpPresenceMixin is-a GObject.
 * TpPresenceMixin has-a _TpGDBusConnectionInterfacePresence1.
 * TpPresenceMixin has-a TpPresenceMixinInterface implementation (HazeConnection).
 * TpPresenceMixin::constructed calls g_dbus_object_skeleton_add_interface(self->priv->base_conn, self->priv->skeleton);

I think presence is small and fundamental enough to be part of TpBaseConnection instead of having yet another object with all its boilerplate. And that doesn't prevent doing standalone objects for future sidecars interfaces. What I like in my branch is that CMs only need to implement an interface, nothing else. Even fill_contact_attributes() is handled for them.

-- 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/telepathy/telepathy-glib/issues/106.