Bug 77144

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.