Bug 77144 - [next] implement properties per-interface, not monolithically
Summary: [next] implement properties per-interface, not monolithically
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-glib (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Simon McVittie
QA Contact: Telepathy bugs list
URL:
Whiteboard: review?
Keywords: patch
Depends on:
Blocks: 76369 77145 77187
  Show dependency treegraph
 
Reported: 2014-04-07 17:03 UTC by Simon McVittie
Modified: 2014-04-08 19:34 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Description Simon McVittie 2014-04-07 17:03:28 UTC
> 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.
Comment 1 Simon McVittie 2014-04-07 17:03:51 UTC
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.
Comment 2 Simon McVittie 2014-04-07 17:04:03 UTC

> +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.)
Comment 3 Simon McVittie 2014-04-07 19:30:00 UTC
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.
Comment 5 Xavier Claessens 2014-04-08 18:08:05 UTC
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.
Comment 6 Simon McVittie 2014-04-08 18:29:31 UTC
(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.
Comment 8 Simon McVittie 2014-04-08 18:36:20 UTC
(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
Comment 9 Simon McVittie 2014-04-08 19:34:07 UTC
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.