GNOME Bugzilla – Bug 645585
Use TpBaseClient instead of TpSimpleObserver
Last modified: 2011-04-27 08:47:10 UTC
The first step before implementing more client type (bug #643594) is to port to TpBaseClient. I have a branch doing exactly that while staying feature equivalent with the current code.
Created attachment 184147 [details] [review] http://git.collabora.co.uk/?p=user/cassidy/gnome-shell;a=shortlog;h=refs/heads/base-client-645585 js/ui/telepathyClient.js | 10 +-- src/Makefile.am | 2 + src/shell-tp-client.c | 192 ++++++++++++++++++++++++++++++++++++++++++++++ src/shell-tp-client.h | 60 ++++++++++++++ 4 files changed, 256 insertions(+), 8 deletions(-)
Comment on attachment 184147 [details] [review] http://git.collabora.co.uk/?p=user/cassidy/gnome-shell;a=shortlog;h=refs/heads/base-client-645585 quick skim... If we're going to have shell-tp-client.c, can you move the telepathy-related wrapper methods from shell-global there? Yes, they don't belong with ShellTpClient, but they belong even less with ShellGlobal... The function-pointer-as-object-property thing is very weird... you should have ShellTpClient emit a signal, which the JS code would connect to. >+ * shell_tp_client_new: >+ * @dbus: a #TpDBusDaemon object, may not be %NULL >+ * @observe_impl: (scope call): the function called when ObserveChannels >+ * is called So this is irrelevant if you rewrite to use a signal, but "(scope call)" means "the callback can be freed once shell_tp_client_new() returns". (You want "(scope notified)", but you don't actually need to specify it anyway because it figures that out automatically from the function signature.)
(In reply to comment #2) > (From update of attachment 184147 [details] [review]) > If we're going to have shell-tp-client.c, can you move the telepathy-related > wrapper methods from shell-global there? Yes, they don't belong with > ShellTpClient, but they belong even less with ShellGlobal... done > The function-pointer-as-object-property thing is very weird... you should have > ShellTpClient emit a signal, which the JS code would connect to. I did it the same way as it's implemented in TpSimpleObserver. A signal doesn't really fit here because we have to make sure that the callback is called once and only once. > >+ * shell_tp_client_new: > >+ * @dbus: a #TpDBusDaemon object, may not be %NULL > >+ * @observe_impl: (scope call): the function called when ObserveChannels > >+ * is called > > So this is irrelevant if you rewrite to use a signal, but "(scope call)" means > "the callback can be freed once shell_tp_client_new() returns". (You want > "(scope notified)", but you don't actually need to specify it anyway because it > figures that out automatically from the function signature.) I removed the annotation (squased).
Created attachment 184417 [details] [review] http://git.collabora.co.uk/?p=user/cassidy/gnome-shell;a=shortlog;h=refs/heads/base-client-645585 updated js/ui/telepathyClient.js | 10 +- src/Makefile.am | 2 + src/shell-global.c | 127 ------------------ src/shell-global.h | 27 ---- src/shell-tp-client.c | 321 ++++++++++++++++++++++++++++++++++++++++++++++ src/shell-tp-client.h | 87 +++++++++++++ 6 files changed, 412 insertions(+), 162 deletions(-)
(In reply to comment #3) > > The function-pointer-as-object-property thing is very weird... you should have > > ShellTpClient emit a signal, which the JS code would connect to. > > I did it the same way as it's implemented in TpSimpleObserver. A signal > doesn't really fit here because we have to make sure that the callback is > called once and only once. I tried this and failed because of bug #645978. I also tried passing the different callbacks to the constructor and failed because of bug #645979 so I ended up using setter to pass the callbacks. Attaching a updated patch.
Created attachment 184483 [details] [review] http://git.collabora.co.uk/?p=user/cassidy/gnome-shell;a=shortlog;h=refs/heads/base-client-645585 js/ui/telepathyClient.js | 11 +-- src/Makefile.am | 2 + src/shell-global.c | 127 ----------------------- src/shell-global.h | 27 ----- src/shell-tp-client.c | 254 ++++++++++++++++++++++++++++++++++++++++++++++ src/shell-tp-client.h | 89 ++++++++++++++++ 6 files changed, 348 insertions(+), 162 deletions(-)
Created attachment 185114 [details] [review] add shell-tp-client
Created attachment 185115 [details] [review] telepathyClient.js: use ShellTpClient instead of TpSimpleObserver
Created attachment 185116 [details] [review] Move Telepathy utility functions from shell-global to shell-tp-client
Review of attachment 185114 [details] [review]: Looks fine if you're sure that this will be a drop-in replacement. I assume you're going to add all the Approver/Handler stuff? ::: src/shell-tp-client.c @@ +61,3 @@ + ShellTpClient *self = (ShellTpClient *) client; + + self->priv->observe_impl (self, account, connection, channels, Might want to add a guard for observe_impl, to make sure it's not NULL. ::: src/shell-tp-client.h @@ +34,3 @@ + (G_TYPE_CHECK_CLASS_CAST ((klass), SHELL_TYPE_TP_CLIENT, \ + ShellTpClientClass)) +#define TP_IS_TP_CLIENT(obj) \ was this supposed to be SHELL_IS_TP_CLIENT?
(In reply to comment #10) > Review of attachment 185114 [details] [review]: > > Looks fine if you're sure that this will be a drop-in replacement. I tested it and didn't notice any regression. The internal code path is pretty similar. > I assume you're going to add all the Approver/Handler stuff? Yep, that's the plan. > ::: src/shell-tp-client.c > @@ +61,3 @@ > + ShellTpClient *self = (ShellTpClient *) client; > + > + self->priv->observe_impl (self, account, connection, channels, > > Might want to add a guard for observe_impl, to make sure it's not NULL. Good idea; added. > ::: src/shell-tp-client.h > @@ +34,3 @@ > + (G_TYPE_CHECK_CLASS_CAST ((klass), SHELL_TYPE_TP_CLIENT, \ > + ShellTpClientClass)) > +#define TP_IS_TP_CLIENT(obj) \ > > was this supposed to be SHELL_IS_TP_CLIENT? Nice catch; fixed.
Created attachment 185178 [details] [review] add shell-tp-client
Anything blocking this branch?
gnome-shell is in self-imposed code freeze until 3.0.1 goes out. We should be able to land it shortly after that.
Comment on attachment 185178 [details] [review] add shell-tp-client just nitpicks. ok to commit after fixing >+static void >+observe_channels ( >+ TpBaseClient *client, >+ TpAccount *account, fix this to be indented/aligned like other shell .c files. (no newline after "(", and with variable names aligned on each line). Likewise for shell_tp_client_set_observe_channels_func, and also in the .h file.
Comment on attachment 185116 [details] [review] Move Telepathy utility functions from shell-global to shell-tp-client (didn't actually look very careful. I'm assuming you can cut+paste :)
Cool, I fixed the identation/alignement and pushed the branch. Thanks for the review! commit 227da257765a94a8dd0734b0d632005e25b99840 Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk> Date: Mon Mar 28 09:08:13 2011 +0200 Move Telepathy utility functions from shell-global to shell-tp-client https://bugzilla.gnome.org/show_bug.cgi?id=645585 commit 2028f33e3856e05c166919c286469d4ffd40cce8 Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk> Date: Wed Mar 23 11:22:42 2011 +0100 telepathyClient.js: use ShellTpClient instead of TpSimpleObserver https://bugzilla.gnome.org/show_bug.cgi?id=645585 commit 145bf19636a5ba73f8e8382783e6905a6fdc811b Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk> Date: Mon Mar 14 12:38:06 2011 +0100 add shell-tp-client https://bugzilla.gnome.org/show_bug.cgi?id=645585