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)
Instantbird Graveyard
Conversation
Tracking
(Not tracked)
RESOLVED
FIXED
1.4
People
(Reporter: florian, Assigned: benediktp)
References
Details
Attachments
(1 file, 2 obsolete files)
5.48 KB,
patch
|
aleth
:
review+
|
Details | Diff | Splinter Review |
*** 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?
Assignee | ||
Comment 1•10 years ago
|
||
*** 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)).
Assignee | ||
Comment 2•10 years ago
|
||
*** 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.
Assignee | ||
Comment 3•10 years ago
|
||
*** 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
Reporter | ||
Comment 4•10 years ago
|
||
*** 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.
Assignee | ||
Comment 5•10 years ago
|
||
*** 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)
Assignee | ||
Comment 6•10 years ago
|
||
*** 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"?
Reporter | ||
Comment 7•10 years ago
|
||
*** 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 ;).
Reporter | ||
Comment 8•10 years ago
|
||
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+
Assignee | ||
Comment 9•10 years ago
|
||
*** 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 | ||
Updated•10 years ago
|
Assignee: nobody → benediktp
Status: NEW → ASSIGNED
Reporter | ||
Comment 10•10 years ago
|
||
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
Comment 11•10 years ago
|
||
*** 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)?
Reporter | ||
Comment 12•10 years ago
|
||
*** 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?
Comment 13•10 years ago
|
||
*** 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 14•10 years ago
|
||
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-
Assignee | ||
Comment 15•10 years ago
|
||
*** 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)
Assignee | ||
Comment 16•10 years ago
|
||
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
Assignee | ||
Comment 17•10 years ago
|
||
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 18•10 years ago
|
||
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+
Updated•10 years ago
|
Whiteboard: [checkin-needed]
Comment 19•10 years ago
|
||
*** 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
You need to log in
before you can comment on or make changes to this bug.
Description
•