Closed Bug 953705 Opened 10 years ago Closed 10 years ago

Pasting in the conversation input box doesn't send typing notifications

Categories

(Instantbird Graveyard :: Conversation, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: florian, Assigned: benediktp)

References

Details

Attachments

(1 file, 2 obsolete files)

*** Original post on bio 260 at 2009-10-27 10:15:00 UTC ***

When pasting the content of the clipboard into a conversation, the information that the user is typing is not sent to the server. It is only sent once a character is actually typed (and not pasted).

Currently the typing notifications are sent from a keypress event handler. Maybe there's another event that is fired when the content of the input box changes?
*** Original post on bio 260 at 2009-10-29 21:17:21 UTC ***

Doesn't work on paste via mouse/context menu.
Doesn't work on paste via drag & drop.

DOES work on paste via keyboard shortcut.
Doesn't work on paste via keyboard invoked context menu (clicking into the text box, pressing context menu key (on Windows)).
*** Original post on bio 260 at 2009-10-29 21:19:01 UTC ***

(In reply to comment #1)
> Doesn't work on paste via mouse/context menu.
> Doesn't work on paste via drag & drop.
> 
> DOES work on paste via keyboard shortcut.
> Doesn't work on paste via keyboard invoked context menu (clicking into the text
> box, pressing context menu key (on Windows)).

That was on Windows XP SP3 by the way.
*** Original post on bio 260 at 2013-02-28 10:18:14 UTC ***

I think we would need to have a separate handler for the "input"-event [1] on the text box which seems to work regardless how the text got into the field and move this code [2] from the keypress handler there.
While we're at it, we should replace [3] and [4] with the string startsWith() methods.
I'm not sure yet how we would need to treat the Enter/Return key.


[1] https://developer.mozilla.org/en-US/docs/DOM/DOM_event_reference/input 
[2] http://lxr.instantbird.org/instantbird/source/instantbird/content/conversation.xml#767
[3] http://lxr.instantbird.org/instantbird/source/instantbird/content/conversation.xml#791
[4] http://lxr.instantbird.org/instantbird/source/instantbird/content/conversation.xml#794
*** Original post on bio 260 at 2013-02-28 11:01:13 UTC ***

(In reply to comment #3)
Seems right.

Would also be nice if someone could take this opportunity to replace the setTimeout at http://lxr.instantbird.org/instantbird/source/instantbird/content/conversation.xml#769 with something better (an equivalent of executeSoon?) and document why that's needed with a comment in the code. I don't remember why we added this setTimeout, but I assume we still want it to avoid calling sendTyping several times if there are several queued keyboard events that are handled at once.
Attached patch WIP (obsolete) — Splinter Review
*** Original post on bio 260 as attmnt 2264 at 2013-03-10 00:17:00 UTC ***

WIP, using the input event suggested in comment 3.

Works on paste via mouse/context menu (didn't work before).
Works on paste via keyboard invoked context menu (clicking into the text
box, pressing context menu key (on Windows)) (didn't work before).
Doesn't work on paste via drag & drop (didn't work before).

Doesn't reset typing state when cutting everything using the context menu (didn't work before) or the keyboard shortcut (regression, used to work) -> Mozilla bio 779738, https://bugzilla.mozilla.org/show_bug.cgi?id=779738

This might fix bug 955326 (bio 1891) automatically, as the typing notifications aren't sent from within the keypress handler any longer.
Attachment #8354029 - Flags: feedback?(florian)
*** Original post on bio 260 at 2013-03-10 10:43:04 UTC ***

I forgot to mention that I removed the setTimeout call for now since I didn't see any changes in behaviour with or without it. I added a reportError()-call at the beginning of the function that is called from setTimeout for that and it the function was called for every keystroke separately as it seemed.

> needed with a comment in the code. I don't remember why we added this
> setTimeout, but I assume we still want it to avoid calling sendTyping several
> times if there are several queued keyboard events that are handled at once.

Florian, could you explain what you mean by "queued keyboard events that are handled at once"?
*** Original post on bio 260 at 2013-03-10 21:59:39 UTC ***

(In reply to comment #6)
> I forgot to mention that I removed the setTimeout call for now since I didn't
> see any changes in behaviour with or without it. I added a reportError()-call
> at the beginning of the function that is called from setTimeout for that and it
> the function was called for every keystroke separately as it seemed.
> 
> > needed with a comment in the code. I don't remember why we added this
> > setTimeout, but I assume we still want it to avoid calling sendTyping several
> > times if there are several queued keyboard events that are handled at once.
> 
> Florian, could you explain what you mean by "queued keyboard events that are
> handled at once"?

The situation I had in mind is when the machine is slow/overloaded and the user can type much faster than the machine can display the characters (and typically you see 5-6 characters appearing at once). Obviously difficult to reproduce ;).
Comment on attachment 8354029 [details] [diff] [review]
WIP

*** Original change on bio 260 attmnt 2264 at 2013-03-12 23:20:07 UTC ***

I didn't see anything obviously wrong here, but didn't to a thorough review. I think aleth can review this. With his work on completion, he probably knows more than anybody else about the way key presses are handled in conversations :).
Attachment #8354029 - Flags: feedback?(florian) → feedback+
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 260 as attmnt 2295 at 2013-03-22 00:32:00 UTC ***

The patch includes the the delay again but replaces the call to setTimeout with one to the thread manager.
Attachment #8354060 - Flags: review?(aleth)
Assignee: nobody → benediktp
Status: NEW → ASSIGNED
Comment on attachment 8354060 [details] [diff] [review]
Patch

*** Original change on bio 260 attmnt 2295 at 2013-03-22 00:35:32 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354060 - Attachment is patch: true
Attachment #8354060 - Attachment mime type: application/octet-stream → text/plain
*** Original post on bio 260 at 2013-03-23 10:18:18 UTC ***

Should we really be sending a typing notification on every keypress? Wouldn't it be better to send at most one per second (or half second, or whatever)?
*** Original post on bio 260 at 2013-03-23 15:38:08 UTC ***

(In reply to comment #10)
> Should we really be sending a typing notification on every keypress? Wouldn't
> it be better to send at most one per second (or half second, or whatever)?

Isn't that something that the prpl (or jsProtoHelper / libpurple's core) should handle?
*** Original post on bio 260 at 2013-03-23 15:50:59 UTC ***

(In reply to comment #11)
> (In reply to comment #10)
> > Should we really be sending a typing notification on every keypress? Wouldn't
> > it be better to send at most one per second (or half second, or whatever)?
> 
> Isn't that something that the prpl (or jsProtoHelper / libpurple's core) should
> handle?

I'm not sure. Libpurple doesn't seem to, and protocols that implement sendTyping will override the jsProtoHelper default which (being blank) is not much use to them, so if this were wanted the easiest place to put it would be here. Especially as it would save some executeSoons. 

To turn it around, is there any conceivable reason a protocol would require *all* the keypresses?
Comment on attachment 8354060 [details] [diff] [review]
Patch

*** Original change on bio 260 attmnt 2295 at 2013-03-24 14:56:14 UTC ***

Please add a boolean to avoid the executeSoon call (and the unnecessary execution of inputInputHandler) if there is already a call to inputInputHandler pending, as discussed on IRC.

Is an input event triggered when the textbox is reset after send? (i.e. do we now send a typing notification after sending the message where we didn't before?) (To find out, it might be useful to call resetInput "by hand").
Attachment #8354060 - Flags: review?(aleth) → review-
Attached patch PatchSplinter Review
*** Original post on bio 260 as attmnt 2307 at 2013-03-30 00:15:00 UTC ***

There's a flag indicating a pending call to delayedInputHandler now.

No input event is sent when the textbox is emptied. I had logging code inserted and saw no call to delayedInputHandler after a message was sent.
Attachment #8354073 - Flags: review?(aleth)
Comment on attachment 8354029 [details] [diff] [review]
WIP

*** Original change on bio 260 attmnt 2264 at 2013-03-30 00:15:21 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354029 - Attachment is obsolete: true
Comment on attachment 8354060 [details] [diff] [review]
Patch

*** Original change on bio 260 attmnt 2295 at 2013-03-30 00:15:21 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354060 - Attachment is obsolete: true
Comment on attachment 8354073 [details] [diff] [review]
Patch

*** Original change on bio 260 attmnt 2307 at 2013-03-30 14:10:56 UTC ***

Thanks for improving this!
Attachment #8354073 - Flags: review?(aleth) → review+
Whiteboard: [checkin-needed]
*** Original post on bio 260 at 2013-04-04 23:04:30 UTC ***

Thanks for fixing this really old bug!

http://hg.instantbird.org/instantbird/rev/d460a4aafe68
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.4
Depends on: 955326
You need to log in before you can comment on or make changes to this bug.