Using techniques similar to Bug #77189, we can stop TpBaseClient exposing any dbus-glib or TpSvc* API. IMO this is higher priority than any other base class, including TpBaseChannel, because it is used by client code (which could conceivably be relying on TpSvc* functionality). We can't really ask GNOME to switch over to Telepathy1 until the client side of telepathy-glib has a frozen or near-frozen ABI, even if the CM side of the same library continues to change. We should make sure TpBaseClient has enough ABI padding while we're there.
Created attachment 98903 [details] [review] base-client: be a GDBusObjectSkeleton
Created attachment 98904 [details] [review] base-client: use gdbus-codegen's skeleton to implement Client
Created attachment 98905 [details] [review] base-client: use gdbus-codegen's skeleton to implement Observer
Created attachment 98906 [details] [review] base-client: use gdbus-codegen's skeleton to implement Approver
Created attachment 98907 [details] [review] base-client: handle clients being NULL
Created attachment 98908 [details] [review] test-base-client: check that Handler.HandledChannels is updated
Created attachment 98909 [details] [review] base-client: use gdbus-codegen's skeleton to implement Handler
Created attachment 98910 [details] [review] base-client: use gdbus-codegen's skeleton to implement Requests
Created attachment 98911 [details] [review] _tp_client_factory_ensure_channel_request: take a GVariant
Created attachment 98912 [details] [review] test-base-client: unregister the client before destroying it
Created attachment 98913 [details] [review] base-client: pass a GVariant to ensure_account_connection_channel()
Created attachment 98919 [details] [review] base-client: don't create skeletons more than once
"base-client: be a GDBusObjectSkeleton": +1 "base-client: use gdbus-codegen's skeleton to implement Observer": - update_interfaces() does more than updating interfaces now since it calls observer_skeleton_init(). Also it can be called each time when unregister and re-register and callding observer_skeleton_init() multiple times will leak self->priv->observer_skeleton. What about inlining that function into _register() and clear all skeletons in _unregister() ? - create_channel_request_array() is leaking key, "o" will dup key, you can use "&o" instead to get internal "const gchar *". - in _tp_base_client_observe_channel() the requests_hash argument is not an "hash" anymore, maybe rename to requests_variant ? I didn't read the rest of the branch yet.
(In reply to comment #13) > "base-client: be a GDBusObjectSkeleton": +1 Actually as discussed on IRC, you could also connect "interface-added/removed" on self and warn when they are emitted after register to make sure a buggy client won't do g_dbus_object_skeleton_add_interface().
(In reply to comment #13) > "base-client: be a GDBusObjectSkeleton": +1 > > "base-client: use gdbus-codegen's skeleton to implement Observer": > > - update_interfaces() does more than updating interfaces now since it calls > observer_skeleton_init(). Also it can be called each time when unregister > and re-register and callding observer_skeleton_init() multiple times will > leak self->priv->observer_skeleton. What about inlining that function into > _register() and clear all skeletons in _unregister() ? Can't we just re-use the existing skeletons? (see my latest patch in this branch). We don't have API to stop being an observer/approver/handler so I think that should be fine. > - create_channel_request_array() is leaking key, "o" will dup key, you can > use "&o" instead to get internal "const gchar *". Good point; fixed. > - in _tp_base_client_observe_channel() the requests_hash argument is not an > "hash" anymore, maybe rename to requests_variant ? Done. The new patches are in http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=next-base-client-77882 (In reply to comment #14) > (In reply to comment #13) > > "base-client: be a GDBusObjectSkeleton": +1 > > Actually as discussed on IRC, you could also connect > "interface-added/removed" on self and warn when they are emitted after > register to make sure a buggy client won't do > g_dbus_object_skeleton_add_interface(). I'm not convinced by this actually. Why should we prevent a custom TpBaseClient subclass to add not-Telepathy-related ifaces at any time it wants to?
(In reply to comment #15) > I'm not convinced by this actually. Why should we prevent a custom > TpBaseClient subclass to add not-Telepathy-related ifaces at any time it > wants to? The API of a Client is that the Interfaces property is immutable and safe to cache, but if you add or remove interfaces after the Client is registered, the Interfaces property changes - that doesn't seem great. If you want non-Telepathy-related interfaces, don't put them on the Client :-)
Created attachment 99023 [details] [review] base-client: warn if user try to add/remove iface when registered I updated http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=next-base-client-77882 as well
Fixed in git for 0.99.12, thanks
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.