Bug 77882 - TpBaseClient: stop implementing TpSvcAnything
Summary: TpBaseClient: stop implementing TpSvcAnything
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: http://cgit.collabora.com/git/user/ca...
Whiteboard:
Keywords: patch
Depends on: 77189
Blocks: 68660
  Show dependency treegraph
 
Reported: 2014-04-24 09:51 UTC by Simon McVittie
Modified: 2014-09-17 13:17 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
base-client: be a GDBusObjectSkeleton (3.16 KB, patch)
2014-05-12 12:35 UTC, Guillaume Desmottes
Details | Splinter Review
base-client: use gdbus-codegen's skeleton to implement Client (6.38 KB, patch)
2014-05-12 12:36 UTC, Guillaume Desmottes
Details | Splinter Review
base-client: use gdbus-codegen's skeleton to implement Observer (14.09 KB, patch)
2014-05-12 12:36 UTC, Guillaume Desmottes
Details | Splinter Review
base-client: use gdbus-codegen's skeleton to implement Approver (12.47 KB, patch)
2014-05-12 12:36 UTC, Guillaume Desmottes
Details | Splinter Review
base-client: handle clients being NULL (1.08 KB, patch)
2014-05-12 12:36 UTC, Guillaume Desmottes
Details | Splinter Review
test-base-client: check that Handler.HandledChannels is updated (2.83 KB, patch)
2014-05-12 12:37 UTC, Guillaume Desmottes
Details | Splinter Review
base-client: use gdbus-codegen's skeleton to implement Handler (13.71 KB, patch)
2014-05-12 12:37 UTC, Guillaume Desmottes
Details | Splinter Review
base-client: use gdbus-codegen's skeleton to implement Requests (7.33 KB, patch)
2014-05-12 12:37 UTC, Guillaume Desmottes
Details | Splinter Review
_tp_client_factory_ensure_channel_request: take a GVariant (5.80 KB, patch)
2014-05-12 12:38 UTC, Guillaume Desmottes
Details | Splinter Review
test-base-client: unregister the client before destroying it (904 bytes, patch)
2014-05-12 12:38 UTC, Guillaume Desmottes
Details | Splinter Review
base-client: pass a GVariant to ensure_account_connection_channel() (4.72 KB, patch)
2014-05-12 12:38 UTC, Guillaume Desmottes
Details | Splinter Review
base-client: don't create skeletons more than once (4.35 KB, patch)
2014-05-12 13:19 UTC, Guillaume Desmottes
Details | Splinter Review
base-client: warn if user try to add/remove iface when registered (1.95 KB, patch)
2014-05-14 09:10 UTC, Guillaume Desmottes
Details | Splinter Review

Description Simon McVittie 2014-04-24 09:51:17 UTC
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.
Comment 1 Guillaume Desmottes 2014-05-12 12:35:37 UTC
Created attachment 98903 [details] [review]
base-client: be a GDBusObjectSkeleton
Comment 2 Guillaume Desmottes 2014-05-12 12:36:09 UTC
Created attachment 98904 [details] [review]
base-client: use gdbus-codegen's skeleton to implement Client
Comment 3 Guillaume Desmottes 2014-05-12 12:36:25 UTC
Created attachment 98905 [details] [review]
base-client: use gdbus-codegen's skeleton to implement Observer
Comment 4 Guillaume Desmottes 2014-05-12 12:36:40 UTC
Created attachment 98906 [details] [review]
base-client: use gdbus-codegen's skeleton to implement Approver
Comment 5 Guillaume Desmottes 2014-05-12 12:36:54 UTC
Created attachment 98907 [details] [review]
base-client: handle clients being NULL
Comment 6 Guillaume Desmottes 2014-05-12 12:37:10 UTC
Created attachment 98908 [details] [review]
test-base-client: check that Handler.HandledChannels is updated
Comment 7 Guillaume Desmottes 2014-05-12 12:37:24 UTC
Created attachment 98909 [details] [review]
base-client: use gdbus-codegen's skeleton to implement Handler
Comment 8 Guillaume Desmottes 2014-05-12 12:37:53 UTC
Created attachment 98910 [details] [review]
base-client: use gdbus-codegen's skeleton to implement Requests
Comment 9 Guillaume Desmottes 2014-05-12 12:38:07 UTC
Created attachment 98911 [details] [review]
_tp_client_factory_ensure_channel_request: take a GVariant
Comment 10 Guillaume Desmottes 2014-05-12 12:38:21 UTC
Created attachment 98912 [details] [review]
test-base-client: unregister the client before destroying it
Comment 11 Guillaume Desmottes 2014-05-12 12:38:35 UTC
Created attachment 98913 [details] [review]
base-client: pass a GVariant to ensure_account_connection_channel()
Comment 12 Guillaume Desmottes 2014-05-12 13:19:24 UTC
Created attachment 98919 [details] [review]
base-client: don't create skeletons more than once
Comment 13 Xavier Claessens 2014-05-12 14:22:26 UTC
"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.
Comment 14 Xavier Claessens 2014-05-12 14:27:37 UTC
(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().
Comment 15 Guillaume Desmottes 2014-05-12 16:20:55 UTC
(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?
Comment 16 Simon McVittie 2014-05-14 07:50:20 UTC
(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 :-)
Comment 17 Guillaume Desmottes 2014-05-14 09:10:04 UTC
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
Comment 18 Simon McVittie 2014-09-17 13:17:28 UTC
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.