Closed Bug 954713 Opened 10 years ago Closed 10 years ago

Sending typing notifications even tho the option for it is disabled

Categories

(Chat Core :: XMPP, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bugzilla, Assigned: benediktp)

Details

(Whiteboard: [1.2-blocking])

Attachments

(1 file, 2 obsolete files)

*** Original post on bio 1281 by Peter SZTAN <sztanpet AT gmail.com> at 2012-02-21 09:15:00 UTC ***

I noticed this issue with gtalk.
In options->privacy "Send typing notifications in converstations" is disabled, yet my partner receives them.

Using 1.2a1pre(20120215041436)
*** Original post on bio 1281 at 2012-02-21 09:55:47 UTC ***

Thanks for noticing and reporting this!

I guess we need to add a test for the preference value around:
http://lxr.instantbird.org/instantbird/source/chat/protocols/xmpp/xmpp.jsm#226
I'm a bit sad that I don't see an easy way to do it once for all JS-protocols in jsProtoHelper :-/.
Severity: minor → normal
Status: UNCONFIRMED → NEW
Component: General → XMPP
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86 → All
Whiteboard: [1.2-blocking]
*** Original post on bio 1281 at 2012-02-21 11:34:51 UTC ***

(In reply to comment #1)
> Thanks for noticing and reporting this!
> 
> I guess we need to add a test for the preference value around:
> http://lxr.instantbird.org/instantbird/source/chat/protocols/xmpp/xmpp.jsm#226
> I'm a bit sad that I don't see an easy way to do it once for all JS-protocols
> in jsProtoHelper :-/.

I'd really prefer if the protocol codes don't need to handle this, could we block the UI from firing if the pref to show typing notifications is disabled?
*** Original post on bio 1281 at 2012-02-21 11:42:29 UTC ***

Not without refactoring the twitter code and the API around typing notifications, as the Twitter plugin currently abuses them to count the characters.
Attached patch Tentative fix, untested (obsolete) — Splinter Review
*** Original post on bio 1281 as attmnt 1287 at 2012-03-30 16:49:00 UTC ***

This patch adds a simple check for the pref in xmpp.jsm which is unfortunate (comment 1) but doesn't involve larger changes (comment 3).

I haven't tested this and I don't know what happens if the pref is toggled while a timer is currently running, therefore "tentative". Any ideas?

Shall I file a follow-up bug to do as suggested in comment 2?
Comment on attachment 8353040 [details] [diff] [review]
Tentative fix, untested

*** Original change on bio 1281 attmnt 1287 at 2012-03-30 16:50:05 UTC ***

Ah, let's request review anyways.
Attachment #8353040 - Flags: review?(clokep)
*** Original post on bio 1281 at 2012-03-30 17:11:27 UTC ***

(In reply to comment #5)
> Comment on attachment 8353040 [details] [diff] [review] (bio-attmnt 1287) [details]
> Tentative fix, untested
> 
> Ah, let's request review anyways.

I greatly dislike this patch. I'm think we should uplift the XMPP code into jsProtoHelper and apply this patch there. Then XMPP doesn't need to ovderride the sendTyping method at all.
*** Original post on bio 1281 at 2012-04-02 13:09:59 UTC ***

(In reply to comment #4)

> I haven't tested this and I don't know what happens if the pref is toggled
> while a timer is currently running, therefore "tentative". Any ideas?

Probably some error. For a patch following this simple approach, I would add:

get sendTypingNotifications()
  this._supportChatStateNotification && Services.prefs.getBoolPref("purple.conversations.im.send_typing")

and replace the 3 tests for _supportChatStateNotification (the one you changed in sendTyping, the one in finishedComposing and the one in sendMsg) with a test for sendTypingNotifications. (sendTypingNotifications may not be the best name, or maybe it should be a method instead of a getter, with a name starting with "should". Not really sure.)
*** Original post on bio 1281 at 2012-04-02 13:45:21 UTC ***

(In reply to comment #6)

> I greatly dislike this patch.

I also dislike this approach (and even the name of the pref!).

> I'm think we should uplift the XMPP code into
> jsProtoHelper and apply this patch there.

I agree on the principle but I'm not sure this would work great in this case.
From what I remember of comments I read in libpurple's code, there are mostly 2 different ways used by IM protocols to handle typing notifications.

- either send a "typing" signal and then if the user hasn't typed for a few seconds send "paused".

- send a "typing" signal every few seconds. If neither a recent "typing" signal nor a message nor a "not typing" signal has been received, the other client assumes the user has paused typing.

In both cases there can exist a "not typing" signal sent if the user empties the textbox, and "not typing" can be assumed after receiving a message (= the user is no longer typing because the message is finished).

I'm not a fan of uplifting the specific code of a protocol if we have reasons to suspect it's not generic enough to be used by other protocols. I think XMPP is currently our only JS-protocol that supports typing notification. I would like us to be sure the code we add to jsProtoHelper is more likely to be helpful than in the way for authors of other protocols.
Maybe someone abstracting this code could also test with the Omegle plugin (if it's easy enough to update it so that it works again).

For what is worth, I would r+ a patch using the simple approach that Mic tried to follow, as fixing this bug is blocking 1.2 (it's a serious regression and information leak) but making the code generic definitely isn't.
*** Original post on bio 1281 at 2012-04-02 14:07:43 UTC ***

(In reply to comment #8)
> (In reply to comment #6)
> > I greatly dislike this patch.
> I also dislike this approach (and even the name of the pref!).
Do we have another approach though? My major issue with it is that I feel that we shouldn't really require the protocol to test for specific preferences, this could easily cause a protocol to cause information leak in this way. :(

> > I'm think we should uplift the XMPP code into
> > jsProtoHelper and apply this patch there.
> 
> I agree on the principle but I'm not sure this would work great in this case.
> From what I remember of comments I read in libpurple's code, there are mostly 2
> different ways used by IM protocols to handle typing notifications.
> 
> - either send a "typing" signal and then if the user hasn't typed for a few
> seconds send "paused".
> 
> - send a "typing" signal every few seconds. If neither a recent "typing" signal
> nor a message nor a "not typing" signal has been received, the other client
> assumes the user has paused typing.
> 
> In both cases there can exist a "not typing" signal sent if the user empties
> the textbox, and "not typing" can be assumed after receiving a message (= the
> user is no longer typing because the message is finished).
> 
> I'm not a fan of uplifting the specific code of a protocol if we have reasons
> to suspect it's not generic enough to be used by other protocols.
I did not have reasons to suspect there were that great of differences in way people did typing notifications. In this case it seems we can't really make the code too much more general to help out multiple protocols though and we should take a patch like this.
*** Original post on bio 1281 as attmnt 1290 at 2012-04-04 16:55:00 UTC ***

(In reply to comment #8)

> For what is worth, I would r+ a patch using the simple approach that Mic tried
> to follow, as fixing this bug is blocking 1.2 (it's a serious regression and
> information leak) but making the code generic definitely isn't.

I followed comment 7 with the patch and will file a bug to solve this in a more general way if it gets accepted.
Attachment #8353043 - Flags: review?(florian)
Comment on attachment 8353040 [details] [diff] [review]
Tentative fix, untested

*** Original change on bio 1281 attmnt 1287 at 2012-04-04 16:55:52 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353040 - Attachment is obsolete: true
Attachment #8353040 - Flags: review?(clokep)
*** Original post on bio 1281 at 2012-04-04 17:05:50 UTC ***

This looks fine to me, but I'll let Florian actually review it. :)
*** Original post on bio 1281 as attmnt 1296 at 2012-04-04 22:49:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353049 - Flags: review?(florian)
Comment on attachment 8353043 [details] [diff] [review]
Fix following comment 7, untested

*** Original change on bio 1281 attmnt 1290 at 2012-04-04 22:49:29 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353043 - Attachment is obsolete: true
Attachment #8353043 - Flags: review?(florian)
Assignee: nobody → benediktp
Status: NEW → ASSIGNED
Comment on attachment 8353049 [details] [diff] [review]
Same as previous patch minus moving the _cancelTypingTimer call

*** Original change on bio 1281 attmnt 1296 at 2012-04-04 22:52:36 UTC ***

Thanks for fixing this! :)
Attachment #8353049 - Flags: review?(florian) → review+
Whiteboard: [1.2-blocking] → [1.2-blocking][checkin-needed]
*** Original post on bio 1281 by Peter SZTAN <sztanpet AT gmail.com> at 2012-04-05 08:27:20 UTC ***

tested on gtalk and msn, just to make sure
works for me,
thanks for the fix!
*** Original post on bio 1281 at 2012-04-05 10:23:42 UTC ***

Committed as http://hg.instantbird.org/instantbird/rev/99e6c4717985  Thanks for fixing this!
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [1.2-blocking][checkin-needed] → [1.2-blocking]
Target Milestone: --- → 1.2
You need to log in before you can comment on or make changes to this bug.