Bug 78376 - remove tp_dbus_properties_mixin_class_init(), use "invisible mixin"
Summary: remove tp_dbus_properties_mixin_class_init(), use "invisible mixin"
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-glib (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL:
Whiteboard:
Keywords: patch
Depends on: 77189
Blocks: 68661
  Show dependency treegraph
 
Reported: 2014-05-07 08:34 UTC by Simon McVittie
Modified: 2014-05-16 14:40 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Description Simon McVittie 2014-05-07 08:34:13 UTC
Xavier writes
> We could use the "invisible mixin" in all CMs to kill
> tp_dbus_properties_mixin_class_init() and all code paths where offset!=0? I'm 
> happier now that it is in -dbus because it should really be considered 
> deprecated and we should try to remove it completely from all CMs on the long 
> term.

to which I replied
> I think I might actually have eradicated offset != 0 while updating the CMs
> (it seemed cleaner to use the invisible mixin than to include
> telepathy-glib-dbus.h in their headers) but this is way off the critical
> path for a clean and future-proof libtelepathy-glib-1.so API.

TpDBusPropertiesMixin is in libtelepathy-glib-1-dbus.so now, so it isn't critical-path, but would be nice.
Comment 1 Guillaume Desmottes 2014-05-15 09:03:22 UTC
What is this "invisible mixin" thing exactly?
Comment 2 Simon McVittie 2014-05-15 10:12:51 UTC
There are two ways to use this mixin.

The original version is that you put a TpDBusPropertiesMixinClass in the class struct, fill it with information about your interfaces, pass a nonzero second parameter to tp_dbus_properties_mixin_class_init(), and the mixin uses qdata to keep track of the offset, find the TpDBusPropertiesMixinClass and read info out of it.

The "invisible mixin" is the code path where you don't put a TpDBusPropertiesMixinClass in the class struct, pass 0 as the second parameter to tp_dbus_properties_mixin_class_init(), and call tp_dbus_properties_mixin_implement_interface() to register your interfaces.

If we convert everything to the "invisible mixin" code path then we can get rid of tp_dbus_properties_mixin_class_init() entirely, and probably TpDBusPropertiesMixinClass and a lot of other API too.

Here is one (already merged) example of switching some code to the "invisible mixin":

http://cgit.freedesktop.org/telepathy/telepathy-mission-control/commit/?h=next&id=0ac96775c905c9b83d99fd5f6aca7bc4a8adf2de
Comment 3 Guillaume Desmottes 2014-05-16 12:44:05 UTC
http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=next-set-77773
http://cgit.collabora.com/git/user/cassidy/telepathy-gabble/log/?h=next-invisible-mixin-78376

The others CMs seem to be fine. Once this is merged we could consider removing tp_dbus_properties_mixin_class_init().
Comment 4 Simon McVittie 2014-05-16 13:02:08 UTC
(In reply to comment #3)
> The others CMs seem to be fine.

I assume you meant <http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=next-invisible-mixin-78376> instead of an unrelated branch. It looks good.

Your Gabble branch also looks good.

What about Mission Control, Folks, TPAW and Empathy? (Use git grep as a starting point.)

Folks will probably need telepathy-glib tests/lib stuff copied into its tests/lib/telepathy/contactlist/ (OK to commit that unreviewed, IMO).

(In reply to comment #3)
> Once this is merged we could consider
> removing tp_dbus_properties_mixin_class_init().

Sounds good. You'll need trivial changes (deleting the call) to all the CMs, MC, Folks, and possibly TPAW and Empathy - I think it'd be OK to merge those unreviewed.
Comment 5 Guillaume Desmottes 2014-05-16 13:34:07 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > The others CMs seem to be fine.
> 
> I assume you meant
> <http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=next-
> invisible-mixin-78376> instead of an unrelated branch. It looks good.

Oops, indeed. :)

> Your Gabble branch also looks good.

Merged tp-glib and Gabble.
 
> What about Mission Control, Folks, TPAW and Empathy? (Use git grep as a
> starting point.)

MC is fine.
Folks just needs tests to be synced (which I did).
TPAW was not impacted.
The change in Empathy was trivial (one test) so I did it right away.
 
> (In reply to comment #3)
> > Once this is merged we could consider
> > removing tp_dbus_properties_mixin_class_init().
> 
> Sounds good. You'll need trivial changes (deleting the call) to all the CMs,
> MC, Folks, and possibly TPAW and Empathy - I think it'd be OK to merge those
> unreviewed.

Still TODO.
Comment 6 Guillaume Desmottes 2014-05-16 14:40:44 UTC
(In reply to comment #5)

> > Sounds good. You'll need trivial changes (deleting the call) to all the CMs,
> > MC, Folks, and possibly TPAW and Empathy - I think it'd be OK to merge those
> > unreviewed.
> 
> Still TODO.

Removed from everywhere \o/


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.