GNOME Bugzilla – Bug 643594
The Telepathy client should be an Approver and Handler
Last modified: 2011-06-09 12:02:17 UTC
Atm the TP client is only an Observer, this lead to some issues; the biggest one being the channel staying in the pending approval state so it continues to blink in Empathy (status icon and contacts list). This client should be an Approver and Handler to interact properly with the channel.
This is blocking acking of messages: bug #644297
First step is to port to TpBaseClient: bug #645585
I started working on this. It's going fine but I'd have one questions to Shell expert: I need to know when the Notification is removed from the tray (when user right clicks on it and ask to remove it). How can I do that?
if a source is removed for any reason, Source.destroy() will be called. TelepathyClient.Source doesn't currently override that, so just add a method to it: destroy: function() { // your code here... // chain up MessageTray.Source.prototype.destroy.call(this); }
*** Bug 649694 has been marked as a duplicate of this bug. ***
Guillaume started work on this here: http://cgit.collabora.co.uk/git/user/cassidy/gnome-shell/commit/?h=handler-643594 It looks basically fine. Here are some comments so far. The delegate_channels_async stuff is changing but seeing that you pass null as a callback it needs no change. You probably should handle the error case there though? throw new Error('Couldn\'t register SimpleObserver. Error: \n' + e); This existing code needs updating. + _handlingCHannels: function(account, conn, channels) { CH-CH-CH-CHannels + // We are handling the shell, try to pass it to Empathy s/shell/channel/ + * @account: a #TpAccount having %TP_ACCOUNT_FEATURE_CORE prepared if possible + * @connection: a #TpConnection having %TP_CONNECTION_FEATURE_CORE prepared + * if possible What does "if possible" mean?! Sure it either is or isn't prepared? + if (self->priv->destroy_apr != NULL) + { + self->priv->destroy_apr (self->priv->user_data_apr); + self->priv->destroy_apr = NULL; + } You should also unset ->user_data_apr -- same for the other two.
I fixed your comments http://cgit.collabora.com/git/user/cassidy/gnome-shell/commit/?h=handler-643594
Created attachment 188044 [details] [review] Implement a Telepathy Approver and Handler (#643594)
I've been using this for about a week or so and it seems to work nicely. You should note that this depends on MC ≥ 5.9.0 (for DelegateChannels and PresentChannel), tp-glib 0.15.0 (for the convenience API) and Empathy 3.1.2 or whatever the next version will be (for setting the preferred handler for opening channels as they always pop up in the shell which is pretty annoying). I also can't help thinking you should just add message acking to this patch too. It's not so complicated, after all? But yeah, I'm no shell reviewer.
Review of attachment 188044 [details] [review]: ::: js/ui/telepathyClient.js @@ +171,3 @@ + let source = this._sources[channel.get_object_path()]; + if (source) + source.notify(); Why pop up a notification? The current TpClient.Source will already do this if there's any pending messages.
For the same reason as Empathy will present the chat when calling PresentChannel() in the Shell: "Called by the channel dispatcher when this client should handle these channels, or when this client should present channels that it is already handling to the user (e.g. bring them into the foreground)." http://telepathy.freedesktop.org/spec/Client_Handler.html#Method:HandleChannels
Review of attachment 188044 [details] [review]: Commit message should be a lot more descriptive: What effect does this give the shell? Why is this commit message necessary or better? What does the user see? Why is the telepathy-glib version bump required? ::: js/ui/telepathyClient.js @@ +79,3 @@ // for any existing channel as well. let dbus = Tp.DBusDaemon.dup(); + this._tp_client = Shell.TpClient.new(dbus); Does "new Shell.TpClient(dbus)" not work for some reason? @@ +180,3 @@ + // Approve the channels right away as we are going to handle it + dispatchOp.claim_with_async(this._tp_client, + Lang.bind (this, function(f, res) { Indentation is a bit screwy here. "f" and "res" should probably be a bit more descriptive. @@ +272,3 @@ + if (this._client.is_handling_channel(this._channel)) { + // We are handling the channel, try to pass it to Empathy + this._client.delegate_channels_async([this._channel], Clutter.get_current_event_time(), "", null); Do we want Clutter.get_current_event_time() or global.get_current_time() here? What purpose does the timestamp serve? @@ +277,3 @@ + // We are not the handler, just ask to present the channel + let dbus = Tp.DBusDaemon.dup(); + let cd = Tp.ChannelDispatcher.new(dbus); "new Tp.ChannelDispatcher(dbus)", etc. @@ +279,3 @@ + let cd = Tp.ChannelDispatcher.new(dbus); + + cd.present_channel_async(this._channel, Clutter.get_current_event_time(), null); Timestamp? ::: src/shell-tp-client.c @@ +17,3 @@ + GDestroyNotify destroy_apr; + + ShellTpClientHandleChannelsImpl handle_impl; I'm not sure I like the word "Handle" for this. "handle_channels_impl" and "user_data_handle_channels" may be a bit more verbose, but it won't interfere with "handle" being used as "reference", I hope. @@ +236,3 @@ +void +shell_tp_client_set_approve_channels_func (ShellTpClient *self, You should probably add some annotations here, although they're not required IIRC.
Created attachment 188992 [details] [review] Implement a Telepathy Approver and Handler (#643594) As the Shell does more than observing channels (users can interact with them), it has to be an Handler as well. We have to make sure than all the new incoming text channels are handled by the Shell by default, so we make it an Approver as well. From an user point of view, the only difference is that Empathy's tray icon will stop blicking when receiving new channels. We rely on ChannelDispatcher.DelegateChannels() and PresentChannel() to interact with Empathy. Those methods have been implemented in telepathy-mission-control 5.9.0 and telepathy-glib 0.15.0.
(In reply to comment #14) > Review of attachment 188044 [details] [review]: > > Commit message should be a lot more descriptive: What effect does this give the > shell? Why is this commit message necessary or better? What does the user see? > Why is the telepathy-glib version bump required? I ammended the commit message. > ::: js/ui/telepathyClient.js > @@ +79,3 @@ > // for any existing channel as well. > let dbus = Tp.DBusDaemon.dup(); > + this._tp_client = Shell.TpClient.new(dbus); > > Does "new Shell.TpClient(dbus)" not work for some reason? Yeah it doesn't pass the property and so we hit this error: tp-glib:ERROR:base-client.c:1095:tp_base_client_constructed: assertion failed: (self->priv->dbus != NULL || self->priv->account_mgr != NULL) > @@ +180,3 @@ > + // Approve the channels right away as we are going to handle it > + dispatchOp.claim_with_async(this._tp_client, > + Lang.bind (this, function(f, res) { > > Indentation is a bit screwy here. > > "f" and "res" should probably be a bit more descriptive. renamed. > @@ +272,3 @@ > + if (this._client.is_handling_channel(this._channel)) { > + // We are handling the channel, try to pass it to Empathy > + this._client.delegate_channels_async([this._channel], > Clutter.get_current_event_time(), "", null); > > Do we want Clutter.get_current_event_time() or global.get_current_time() here? > What purpose does the timestamp serve? We want the equivalent of gtk_get_current_event_time() which is, I think, Clutter.get_current_time(). See http://telepathy.freedesktop.org/doc/telepathy-glib/telepathy-glib-account-channel-request.html#TpAccountChannelRequest--user-action-time > @@ +277,3 @@ > + // We are not the handler, just ask to present the channel > + let dbus = Tp.DBusDaemon.dup(); > + let cd = Tp.ChannelDispatcher.new(dbus); > > "new Tp.ChannelDispatcher(dbus)", etc. Same problem as above. > @@ +279,3 @@ > + let cd = Tp.ChannelDispatcher.new(dbus); > + > + cd.present_channel_async(this._channel, > Clutter.get_current_event_time(), null); > > Timestamp? Same rational as with DelegateChannelAsync, we pass it to the other client (Empathy) so it can present the window using the right timestamp. > ::: src/shell-tp-client.c > @@ +17,3 @@ > + GDestroyNotify destroy_apr; > + > + ShellTpClientHandleChannelsImpl handle_impl; > > I'm not sure I like the word "Handle" for this. "handle_channels_impl" and > "user_data_handle_channels" may be a bit more verbose, but it won't interfere > with "handle" being used as "reference", I hope. Yeah that's the proper Telepathy word. > @@ +236,3 @@ > > +void > +shell_tp_client_set_approve_channels_func (ShellTpClient *self, > > You should probably add some annotations here, although they're not required > IIRC. Indeed it works fine without. I don't really see the point adding annotations tbh, the function header is pretty straightforward.
(In reply to comment #16) > > Does "new Shell.TpClient(dbus)" not work for some reason? > > Yeah it doesn't pass the property gjs ignores the actual constructors for GObject types and just makes them all work g_object_new()-style: this._tp_client = new Shell.TpClient({ dbus_daemon: dbus, name: "GnomeShell", uniquify_name: true }); (and then you can just drop shell_tp_client_new().) > > + this._client.delegate_channels_async([this._channel], > > Clutter.get_current_event_time(), "", null); > > > > Do we want Clutter.get_current_event_time() or global.get_current_time() here? > > We want the equivalent of gtk_get_current_event_time() which is, I think, > Clutter.get_current_time(). Clutter.get_current_event_time() will give the wrong answer if your code has been reached via a (non-clutter) metacity event. global.get_current_time() deals with figuring out whether clutter's or metacity's idea of the current time is correct. > > You should probably add some annotations here, although they're not required > > IIRC. > > Indeed it works fine without. I don't really see the point adding annotations > tbh, the function header is pretty straightforward. Yeah, if you're using the standard "callback, user_data, destroy_notify" idiom, you don't need annotations.
Created attachment 189314 [details] [review] Implement a Telepathy Approver and Handler (#643594) As the Shell does more than observing channels (users can interact with them), it has to be an Handler as well. We have to make sure than all the new incoming text channels are handled by the Shell by default, so we make it an Approver as well. From an user point of view, the only difference is that Empathy's tray icon will stop blicking when receiving new channels. We rely on ChannelDispatcher.DelegateChannels() and PresentChannel() to interact with Empathy. Those methods have been implemented in telepathy-mission-control 5.9.0 and telepathy-glib 0.15.0.
(In reply to comment #17) > (In reply to comment #16) > > > Does "new Shell.TpClient(dbus)" not work for some reason? > > > > Yeah it doesn't pass the property > > gjs ignores the actual constructors for GObject types and just makes them all > work g_object_new()-style: > > this._tp_client = new Shell.TpClient({ dbus_daemon: dbus, > name: "GnomeShell", > uniquify_name: true }); I changed this one but now the ChannelDispatcher one as tp_channel_dispatcher_new() is a bit more than a simple wrapper: TpChannelDispatcher * tp_channel_dispatcher_new (TpDBusDaemon *bus_daemon) { TpChannelDispatcher *self; g_return_val_if_fail (bus_daemon != NULL, NULL); self = TP_CHANNEL_DISPATCHER (g_object_new (TP_TYPE_CHANNEL_DISPATCHER, "dbus-daemon", bus_daemon, "dbus-connection", ((TpProxy *) bus_daemon)->dbus_connection, "bus-name", TP_CHANNEL_DISPATCHER_BUS_NAME, "object-path", TP_CHANNEL_DISPATCHER_OBJECT_PATH, NULL)); return self; } Doing that in g-s would make code more complicated for no good reason. > (and then you can just drop shell_tp_client_new().) removed. > > > + this._client.delegate_channels_async([this._channel], > > > Clutter.get_current_event_time(), "", null); > > > > > > Do we want Clutter.get_current_event_time() or global.get_current_time() here? > > > > We want the equivalent of gtk_get_current_event_time() which is, I think, > > Clutter.get_current_time(). > > Clutter.get_current_event_time() will give the wrong answer if your code has > been reached via a (non-clutter) metacity event. global.get_current_time() > deals with figuring out whether clutter's or metacity's idea of the current > time is correct. changed.
Review of attachment 189314 [details] [review]: Overall, the behavior seems correct, however I haven't tested the tens of edge cases that seem to be popping up: chats appearing before the other contact's, after the other contact's, interleaved but incorrect, etc. Your indentation seems to be wacky in a few places. ::: js/ui/telepathyClient.js @@ +1,3 @@ /* -*- mode: js2; js2-basic-offset: 4; indent-tabs-mode: nil -*- */ +const Clutter = imports.gi.Clutter; Leftover import. @@ +81,3 @@ + this._tp_client = new Shell.TpClient({ 'dbus_daemon': dbus, + 'name': 'GnomeShell', + 'uniquify-name': true }) Can you fix the indentation here? @@ +150,3 @@ + if (this._tp_client.is_handling_channel(channel)) { + // Close the channel as we can't handle it any + // more "Telepathy is handling the channel, not us." @@ +158,3 @@ + }, + + _handlingChannels: function(account, conn, channels) { Is "_handleChannels" not appropriate? We usually don't have progressive verb tenses in function names. @@ +164,3 @@ + + // We can only handle text channel, so close any other channel + if (!(channel instanceof Tp.TextChannel)) { Is checking the type the only way to do this? @@ +173,3 @@ + let source = this._sources[channel.get_object_path()]; + if (source) + source.notify(); Four spaces. @@ +182,3 @@ + // Approve the channels right away as we are going to handle it + dispatchOp.claim_with_async(this._tp_client, + Lang.bind (this, function(dispatchOp, result) { Watch your indentation. Additionally, this function seems to be a bit long to have inline. @@ +197,3 @@ + this._handlingChannels(account, conn, channels); + context.accept(); + }, No comma at the end of an object literal. @@ +276,3 @@ + this._client.delegate_channels_async([this._channel], global.get_current_time(), "", null); + } + else { } else { ::: src/shell-tp-client.c @@ +19,3 @@ + ShellTpClientHandleChannelsImpl handle_impl; + gpointer user_data_handle; + GDestroyNotify destroy_handle; I'm still not a fan of the word "handle" for this, so I'd really like "approve_channels" and "handle_channels"
(In reply to comment #20) > ::: js/ui/telepathyClient.js > @@ +1,3 @@ > /* -*- mode: js2; js2-basic-offset: 4; indent-tabs-mode: nil -*- */ > > +const Clutter = imports.gi.Clutter; > > Leftover import. removed. > @@ +81,3 @@ > + this._tp_client = new Shell.TpClient({ 'dbus_daemon': dbus, > + 'name': 'GnomeShell', > + 'uniquify-name': true }) > > Can you fix the indentation here? fixed. > @@ +150,3 @@ > + if (this._tp_client.is_handling_channel(channel)) { > + // Close the channel as we can't handle it any > + // more > > "Telepathy is handling the channel, not us." What do you mean? The Shell is the one handling the channel at this point, so we have to close it. > @@ +158,3 @@ > + }, > + > + _handlingChannels: function(account, conn, channels) { > > Is "_handleChannels" not appropriate? We usually don't have progressive verb > tenses in function names. We already have _handleChannels which is the HandleChannels() implementation. This one is a convenient function used by HandleChannels() and AddDispatchOperation() (the Approver). > @@ +164,3 @@ > + > + // We can only handle text channel, so close any other channel > + if (!(channel instanceof Tp.TextChannel)) { > > Is checking the type the only way to do this? Yeah, that's how it's done in Empathy as well. > @@ +173,3 @@ > + let source = this._sources[channel.get_object_path()]; > + if (source) > + source.notify(); > > Four spaces. fixed. > @@ +182,3 @@ > + // Approve the channels right away as we are going to handle it > + dispatchOp.claim_with_async(this._tp_client, > + Lang.bind (this, > function(dispatchOp, result) { > > Watch your indentation. fixed. > Additionally, this function seems to be a bit long to have inline. How can I still get access to account, conn and channels if I don't inline it? (Sorry I kinda suck at js). > @@ +197,3 @@ > + this._handlingChannels(account, conn, channels); > + context.accept(); > + }, > > No comma at the end of an object literal. Rahh I hate this syntax... Removed. > ::: src/shell-tp-client.c > @@ +19,3 @@ > + ShellTpClientHandleChannelsImpl handle_impl; > + gpointer user_data_handle; > + GDestroyNotify destroy_handle; > > I'm still not a fan of the word "handle" for this, so I'd really like > "approve_channels" and "handle_channels" renamed.
Created attachment 189450 [details] [review] Implement a Telepathy Approver and Handler (#643594) As the Shell does more than observing channels (users can interact with them), it has to be an Handler as well. We have to make sure than all the new incoming text channels are handled by the Shell by default, so we make it an Approver as well. From an user point of view, the only difference is that Empathy's tray icon will stop blicking when receiving new channels. We rely on ChannelDispatcher.DelegateChannels() and PresentChannel() to interact with Empathy. Those methods have been implemented in telepathy-mission-control 5.9.0 and telepathy-glib 0.15.0.
(In reply to comment #21) > > ::: js/ui/telepathyClient.js > > @@ +150,3 @@ > > + if (this._tp_client.is_handling_channel(channel)) { > > + // Close the channel as we can't handle it any > > + // more > > > > "Telepathy is handling the channel, not us." > > What do you mean? The Shell is the one handling the channel at this point, so > we have to close it. I meant that the comment you had was a bit confusing. Why is it that "we can't it any more?" "Telepathy is handling the channel now, not us." I'd argue that we can already see that you do a close_async on the next line, so explaining what you do (you close it) isn't necessary. Explaining *why* you close it is. > > @@ +158,3 @@ > > + }, > > + > > + _handlingChannels: function(account, conn, channels) { > > > > Is "_handleChannels" not appropriate? We usually don't have progressive verb > > tenses in function names. > > We already have _handleChannels which is the HandleChannels() implementation. > This one is a convenient function used by HandleChannels() and > AddDispatchOperation() (the Approver). Alright. I'll try to think of better names, but for now, it's fine. > > @@ +164,3 @@ > > + > > + // We can only handle text channel, so close any other channel > > + if (!(channel instanceof Tp.TextChannel)) { > > > > Is checking the type the only way to do this? > > Yeah, that's how it's done in Empathy as well. :( > > Additionally, this function seems to be a bit long to have inline. > > How can I still get access to account, conn and channels if I don't inline it? > (Sorry I kinda suck at js). Hm, you could do some trickery with closures or bindings, but just format it differently and it will look a bit better to me: put the function() declaration on the next line. > > @@ +197,3 @@ > > + this._handlingChannels(account, conn, channels); > > + context.accept(); > > + }, > > > > No comma at the end of an object literal. > > Rahh I hate this syntax... > Removed. Yeah, Python's nicer about that :D > > ::: src/shell-tp-client.c > > @@ +19,3 @@ > > + ShellTpClientHandleChannelsImpl handle_impl; > > + gpointer user_data_handle; > > + GDestroyNotify destroy_handle; > > > > I'm still not a fan of the word "handle" for this, so I'd really like > > "approve_channels" and "handle_channels" > > renamed. You didn't rename "user_data_apr" and "destroy_apr". Not that it matters *too* much.
(In reply to comment #23) > (In reply to comment #21) > > > ::: js/ui/telepathyClient.js > > > @@ +150,3 @@ > > > + if (this._tp_client.is_handling_channel(channel)) { > > > + // Close the channel as we can't handle it any > > > + // more > > > > > > "Telepathy is handling the channel, not us." > > > > What do you mean? The Shell is the one handling the channel at this point, so > > we have to close it. > > I meant that the comment you had was a bit confusing. Why is it that "we can't > it any more?" > > "Telepathy is handling the channel now, not us." > > I'd argue that we can already see that you do a close_async on the next line, > so explaining what you do (you close it) isn't necessary. Explaining *why* you > close it is. I changed it to "The chat box has been destroyed so it can't handle the channel any more.". > > > Additionally, this function seems to be a bit long to have inline. > > > > How can I still get access to account, conn and channels if I don't inline it? > > (Sorry I kinda suck at js). > > Hm, you could do some trickery with closures or bindings, but just format it > differently and it will look a bit better to me: put the function() declaration > on the next line. done. > > > ::: src/shell-tp-client.c > > > @@ +19,3 @@ > > > + ShellTpClientHandleChannelsImpl handle_impl; > > > + gpointer user_data_handle; > > > + GDestroyNotify destroy_handle; > > > > > > I'm still not a fan of the word "handle" for this, so I'd really like > > > "approve_channels" and "handle_channels" > > > > renamed. > > You didn't rename "user_data_apr" and "destroy_apr". Not that it matters *too* > much. renamed.
Created attachment 189462 [details] [review] Implement a Telepathy Approver and Handler (#643594) As the Shell does more than observing channels (users can interact with them), it has to be an Handler as well. We have to make sure than all the new incoming text channels are handled by the Shell by default, so we make it an Approver as well. From an user point of view, the only difference is that Empathy's tray icon will stop blicking when receiving new channels. We rely on ChannelDispatcher.DelegateChannels() and PresentChannel() to interact with Empathy. Those methods have been implemented in telepathy-mission-control 5.9.0 and telepathy-glib 0.15.0.
(In reply to comment #25) > We have to make sure than all the new incoming text channels are handled by > the Shell Minor nitpick: We have to make sure *that* ...
(In reply to comment #21) > > Additionally, this function seems to be a bit long to have inline. > > How can I still get access to account, conn and channels if I don't inline it? > (Sorry I kinda suck at js). for future reference, you can pass extra args to Lang.bind: Lang.bind(this, this._myCallback, account, conn, channels) which will then get appended to the normal args when myCallback is called. Alternatively, if myCallback has stupid arguments, or you want to reuse a single method for several different callbacks with different args, you can do Lang.bind(this, function (usefulArg, uselessArg) { this._myCallback(account, usefulArg, conn, channels); });
(In reply to comment #26) > (In reply to comment #25) > > We have to make sure than all the new incoming text channels are handled by > > the Shell > > Minor nitpick: We have to make sure *that* ... fixed. Dan: thanks for the info; but in this case I think it's clearer as an inline function.
Merged to master; thanks guys! commit 60c88612f7c4f8d302c05bd3ee6101904f6d8b34 Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk> Date: Tue Apr 5 13:28:11 2011 +0200 Implement a Telepathy Approver and Handler (#643594) As the Shell does more than observing channels (users can interact with them), it has to be an Handler as well. We have to make sure that all the new incoming text channels are handled by the Shell by default, so we make it an Approver as well. From an user point of view, the only difference is that Empathy's tray icon will stop blicking when receiving new channels. We rely on ChannelDispatcher.DelegateChannels() and PresentChannel() to interact with Empathy. Those methods have been implemented in telepathy-mission-control 5.9.0 and telepathy-glib 0.15.0. This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.