GNOME Bugzilla – Bug 650196
Message tray should send typing notifications
Last modified: 2011-08-04 01:49:12 UTC
I'm not sure how to show typing notifications, if at all, but it should at least send them.
Created attachment 187830 [details] [review] telepathyClient: send typing notifications Here's a patch. Author: Jonny Lamb <jonnylamb@gnome.org> Date: Sun May 15 01:07:44 2011 +0100 telepathyClient: send typing notifications If we're typing we want to send composing. If we empty the entry we want to send active. If we're typing but don't type any more for COMPOSING_STOP_TIMEOUT seconds, we want to send paused. Simple. This behaviour was stolen from Empathy where it has won many awards. Signed-off-by: Jonny Lamb <jonnylamb@gnome.org>
Review of attachment 187830 [details] [review]: Nice. We should probably be displaying any state changes from our contact, but this helps us play nice with others first. ::: js/ui/telepathyClient.js @@ +396,3 @@ this._history = []; this._timestampTimeoutId = 0; + No blank line @@ +566,3 @@ + let text = this._responseEntry.get_text(); + + // If we're typing we want to send composing. If we empty the This comment was extremely confusing for me... probably because it's late. It might read better: // If we're typing, we want to send COMPOSING. // If we empty the entry, we want to send ACTIVE. // If we've stopped typing for COMPOSING_STOP_TIMEOUT // seconds, we want to send PAUSED. // Simple. @@ +577,3 @@ + this._composingTimeoutId = 0; + } else { + this.source.setChatState(Tp.ChannelChatState.COMPOSING); Why are we only COMPOSING if we don't have a timeout?
Created attachment 188016 [details] [review] telepathyClient: send typing notifications (In reply to comment #3) > We should probably be displaying any state changes from our contact, but this > helps us play nice with others first. Yeah, I didn't know how this should be displayed, so I just left it for now. > No blank line Done. > This comment was extremely confusing for me... probably because it's late. It > might read better: > > // If we're typing, we want to send COMPOSING. > // If we empty the entry, we want to send ACTIVE. > // If we've stopped typing for COMPOSING_STOP_TIMEOUT > // seconds, we want to send PAUSED. > // Simple. OK, done. > Why are we only COMPOSING if we don't have a timeout? If we have a timeout it means we have already sent COMPOSING. We don't want to send COMPOSING every time you type a letter into the entry.
Review of attachment 188016 [details] [review]: ::: js/ui/telepathyClient.js @@ +577,3 @@ + } else { + this.source.setChatState(Tp.ChannelChatState.COMPOSING); + } > If we have a timeout it means we have already sent COMPOSING. We don't want to > send COMPOSING every time you type a letter into the entry. OK, that makes perfect sense, but I'm still finding the code confusing... I didn't catch that we only have an active timeout if we're already in COMPOSING... additionally, piggybacking onto the "if" statement that clears the timeout didn't help. So, how about we make it explicit: "this._chatState" that gets set in setChatState, and you only make the call out to Tp if it's changed. (Is there a condition where Tp changes it behind our back?) You remove the timeout unconditionally, and do: // Any time the entry has changed, we need to re-evaluate our timeout. if (this._composingTimeoutId > 0) { ... } // If we're typing, we want to ... if (text != '') { this.source.setChatState(Tp.ChannelChatState.COMPOSING); this._composingTimeoutId = Mainloop.timeout_add_seconds( ... ); } else { this.source.setChatState(Tp.ChannelChatState.ACTIVE); }
Created attachment 191447 [details] [review] telepathyClient: send typing notifications (In reply to comment #5) > So, how about we make it explicit: "this._chatState" that gets set in > setChatState, and you only make the call out to Tp if it's changed. (Is there a > condition where Tp changes it behind our back?) Empathy could changes it behind gnome-shell's back if the user types both in the gnome-shell's message tray and in the Empathy conversation window. But I don't think it worth fixing typing notifications in this weird use case. The attached patch refreshes Jonny's patch and updates it with your review in Comment #5.
Review of attachment 191447 [details] [review]: ::: js/ui/telepathyClient.js @@ +413,3 @@ + setChatState: function(state) { + if (state != this._chat_state) { + this._chat_state = state; this._chatState All JS code gets "camelCase" Additionally, you never seem to initally set it in _init. You should do that.
Created attachment 191468 [details] [review] telepathyClient: send typing notifications (In reply to comment #7) > Review of attachment 191447 [details] [review]: > > ::: js/ui/telepathyClient.js > @@ +413,3 @@ > + setChatState: function(state) { > + if (state != this._chat_state) { > + this._chat_state = state; > > this._chatState > > All JS code gets "camelCase" Fixed. > Additionally, you never seem to initially set it in _init. You should do that. It is initialized actually: + this._composingTimeoutId = 0; + this._chatState = Tp.ChannelChatState.ACTIVE; I also added a comment in setChatState() about the possibility of Empathy changing the chat state behind our back, as you suggested on IRC.
Created attachment 191472 [details] [review] telepathyClient: send typing notifications Update patch: initialize _chatState in ChatSource._init
Review of attachment 191472 [details] [review]: LGTM.
Looks like this was already pushed. Oh well.