|
Description
Simon McVittie
2014-04-24 09:51:17 UTC
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.