> Next, I'll probably stop implementing TpSvcDBusProperties (which is a weird > special case) in any of our high-level API, and make the TpSvcInterface code > call into TpDBusPropertiesMixin instead. MC can still override that with its > own TpSvcDBusProperties implementation if it really needs to, but it seems bad > for it to be an API guarantee that TpBaseConnection implements D-Bus > Properties monolithically, rather than letting GDBus do it per-interface. Xavier implemented this for telepathy-glib: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=gdbus-properties I need to get MC, the CMs, and Folks' regression tests caught up, and fix anything that I don't like about Xavier's implementation.
Mission Control has its own implementation of TpSvcDBusProperties for some reason or other (Bug #32416) and, while I acknowledge that that's a design flaw, I really don't want to put it on the critical path for Telepathy 1. However, now that you've deleted tp_dbus_properties_mixin_iface_init(), I think we can safely remove the TpSvcDBusProperties special case and just have MC keep implementing it. That's how I imagined it would initially work.
> +tp_dbus_object_add_skeleton I'm really not sure I like that API. I would like to be able to have this: * parent class P implements interface I with one skeleton * subclass S reimplements interface I with a different skeleton Ideally, the parent class's skeleton for that interface would not even get constructed if that happens. Concretely, this is necessary if we want to be able to move new functionality from subclasses up to a parent class when it stabilizes. Maybe something like this: interface Exportable { vfunc fill_skeletons (Exportable self, GHashTable<GQuark,GDBusInterfaceSkeleton> skeletons); } with a typical implementation looking like this pseudocode: fill_skeletons (self, skeletons) { if (skeletons.lookup (TP_IFACE_QUARK_BADGERS) == NULL) skeletons.insert (TP_IFACE_QUARK_BADGERS, badger_skeleton_new (self)); if (skeletons.lookup (TP_IFACE_QUARK_MUSHROOMS) == NULL) skeletons.insert (TP_IFACE_QUARK_MUSHROOMS, mushroom_skeleton_new (self)); parent_class.fill_skeletons (self); /* we want to override one method, or something */ snake_skeleton = skeletons.lookup (TP_IFACE_QUARK_SNAKES); } The fallback implementation of fill_skeletons in the GInterface would be what tp_dbus_connection_try_register() does now: fill_skeletons (self, skeletons) { foreach TpSvc interface, svc { if (skeletons.lookup (svc.interface_name) == NULL) skeletons.insert (svc.interface_name, _tp_svc_interface_new (self)); } } (Implementation detail: tp_dbus_connection_try_register() would make the GHashTable, iterate it, and free it before returning.)
WiP: http://cgit.freedesktop.org/~smcv/telepathy-glib/log/?h=gdbus-properties (mostly works, but one test fails under load) Matching Gabble branch: http://cgit.freedesktop.org/~smcv/telepathy-gabble/log/?h=gdbus-properties MC, Salut, Idle, Haze, Rakia tests pass, with changes remarkably similar to Gabble's "Stop using tp_dbus_properties_mixin_iface_init which has been removed". Folks, TPAW, Empathy not yet tested, but I expect Folks will need a resync from tests/lib and the others should be unaffected.
Ready for review, I think. I tried various ways to pass info from the GDBus filter to the main thread and they all had timing issues, so I brought back TpSvcDBusProperties in that one test instead. http://cgit.freedesktop.org/~smcv/telepathy-glib/log/?h=gdbus-properties2 http://cgit.freedesktop.org/~smcv/telepathy-mission-control/log/?h=gdbus-properties http://cgit.freedesktop.org/~smcv/telepathy-gabble/log/?h=gdbus-properties http://cgit.freedesktop.org/~smcv/telepathy-rakia/log/?h=gdbus-properties http://cgit.freedesktop.org/~smcv/telepathy-salut/log/?h=gdbus-properties http://cgit.freedesktop.org/~smcv/telepathy-idle/log/?h=gdbus-properties http://cgit.freedesktop.org/~smcv/telepathy-haze/log/?h=gdbus-properties http://cgit.freedesktop.org/~smcv/folks/log/?h=gdbus-properties (TPAW: no changes needed) http://cgit.freedesktop.org/~smcv/empathy/log/?h=next-gdbus-properties
Small details on tp-glib's gdbus-properties2, can be merged directly: - "TpCapabilities: operate in terms of GVariant": Is the DEBUG in tp_capabilities_set_property() still desired? Doesn't hurt I guess. - tp_connection_manager_param_dup_default_variant() could be renamed to _get_default_variant() if it uses a variant internally as well. Shouldn't be a lot to change afaik. - tp_message_set_variant() could be renamed to tp_message_set() - "tp_account_get_avatar_async: rename to _dup_, return GBytes + MIME type": Good, in the future that could become a g_variant_get_data_as_bytes() and will be zero-copy.
(In reply to comment #5) > - "TpCapabilities: operate in terms of GVariant": Is the DEBUG in > tp_capabilities_set_property() still desired? Doesn't hurt I guess. *shrug* I'll leave it in for now. > - tp_connection_manager_param_dup_default_variant() could be renamed to > _get_default_variant() if it uses a variant internally as well. Shouldn't be > a lot to change afaik. The policy I'm following at the moment (yes, I know I haven't been 100% consistent about this) is "if it doesn't need renaming, don't rename it". The time for gratuitous renaming has passed, IMO. OTOH, if a function is breaking API anyway, *then* I rename. > - tp_message_set_variant() could be renamed to tp_message_set() Likewise.
(In reply to comment #4) > http://cgit.freedesktop.org/~smcv/telepathy-haze/log/?h=gdbus-properties Sorry, make that http://cgit.freedesktop.org/~smcv/telepathy-haze/log/?h=next-handle-type (no new changes were needed for this bug). > http://cgit.freedesktop.org/~smcv/folks/log/?h=gdbus-properties Sorry, didn't push. Use http://cgit.freedesktop.org/~smcv/folks/log/?h=gdbus-properties2
(In reply to comment #4) > (TPAW: no changes needed) If you're not reviewing earlier branches separately, you will need to review http://cgit.freedesktop.org/~smcv/telepathy-account-widgets/log/?h=next-gvariant1
Fixed in git for 0.99.11. Some commits to core CMs (Bug #77200), MC (Bug #77199), Folks <https://bugzilla.gnome.org/show_bug.cgi?id=708871>, Empathy <https://bugzilla.gnome.org/show_bug.cgi?id=727043> were pushed unreviewed.
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.