"is typing..." message in chat box doesn't disappear

RESOLVED FIXED in Thunderbird 17.0

Status

Thunderbird
Instant Messaging
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bastien.rene, Assigned: mconley)

Tracking

16 Branch
Thunderbird 17.0
x86
All

Thunderbird Tracking Flags

(thunderbird15 fixed, thunderbird16 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 640978 [details]
comparison between error and normal message

The message "is typing" in the "contact box" in instant messaging tab stay there even if my friend has sent his message. 
I found that the message is replaced by the status (available for example) if i change chat window and come back on the first one, I think the problem can be solved by refreshing the status in the same way that it is done when changing chat window.
(Assignee)

Comment 1

5 years ago
Created attachment 643976 [details] [diff] [review]
Patch v1
Assignee: nobody → mconley
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #643976 - Flags: review?(florian)
(Reporter)

Comment 2

5 years ago
By looking at your patch and going deeply in the code, i think my idea was not good and will not solve the problem because the typing status is not changed in the updateConvStatus function. 

I think the problem come from the updateTyping function, there is some typingState not managed. Actually, now it manage only TYPING and TYPED typing status, but maybe there is other status like 'not typing' or some others. So this function should contain default traitment to update the text in that particular case where typingStatus is not managed.

I don't have access to developpment tools and Mercurial on that computer, but here are the lines and my modification :
lines 1026-1041, replace (and complete missing lines) with :
switch(typingState) {
			case Ci.prplIConvIM.TYPING :
					cti.setAttribute("typing", "true");
					cti.setAttribute("statusTypeTooltiptext",
									 this.bundle.formatStringFromName("chat.contactIsTyping",
																	  [name], 1));
					cti.setAttribute("statusMessage",
									 this.bundle.GetStringFromName("chat.isTyping"));
				break;
			case Ci.prplIConvIM.TYPED :
					cti.setAttribute("typed", "true");
					cti.setAttribute("statusTypeTooltiptext",
									 this.bundle.formatStringFromName("chat.contactHasStoppedTyping",
																	  [name], 1));
					cti.setAttribute("statusMessage",
									 this.bundle.GetStringFromName("chat.hasStoppedTyping"));
				break;
			default : 
					//Default traitment to put 'Available' typingMessage
          }
(Assignee)

Comment 3

5 years ago
(In reply to bastien.rene from comment #2)
> By looking at your patch and going deeply in the code, i think my idea was
> not good and will not solve the problem because the typing status is not
> changed in the updateConvStatus function. 

The updateConvStatus() function calls the updateTyping() function, and essentially "resets" the display to the neutral state. updateTyping() takes care of the cases when the typing state is not neutral, and overwrites the work that updateConvStatus() did.
Comment on attachment 643976 [details] [diff] [review]
Patch v1

This seems good, I've just a comment to reduce code duplication:

>diff --git a/mail/components/im/content/imconversation.xml b/mail/components/im/content/imconversation.xml

>          case "update-typing":
>            if (this.tab && this.tab.selected)
>-             this.updateTyping();
>+             this.updateConvStatus();
>            break;

Please move the |case "update-typing":| line next to the update-buddy-status line and remove the duplicated lines :).


With this patch, updateConvStatus becomes the only caller of updateTyping so I wondered if we should inline that method, but maybe not. It's possible having a separate method with a clear name adds some clarity here.
Attachment #643976 - Flags: review?(florian) → review-
(Reporter)

Comment 5

5 years ago
Ok, i understand it better know. But maybe there is some parts of the updateConvStatus that are not necessary to update the isTyping, it might do unnecessary calculations and setting on the object.

it should be splited in a better way to put the necessary parts for updateTyping in that function and the function updateTyping should be called by the case "update-typing" in that way.

I'm not sure about what i'm saying because i don't know all the code behind, i hope it's not a waste of time for you ;)
(Assignee)

Comment 6

5 years ago
(In reply to bastien.rene from comment #5)
> it should be splited in a better way to put the necessary parts for
> updateTyping in that function and the function updateTyping should be called
> by the case "update-typing" in that way.

I agree - but at this point, since we're fixing these things for beta, I think the less architectural restructuring we do, the better.

> 
> I'm not sure about what i'm saying because i don't know all the code behind,
> i hope it's not a waste of time for you ;)

Always appreciated - It's always good to have another pair of eyes. :)
(Assignee)

Comment 7

5 years ago
Created attachment 644347 [details] [diff] [review]
Patch v2

Thanks for the review Florian - and I agree, let's DRY this sucker up. :)
Attachment #643976 - Attachment is obsolete: true
Attachment #644347 - Flags: review?(florian)
Comment on attachment 644347 [details] [diff] [review]
Patch v2

Thanks!
Attachment #644347 - Flags: review?(florian)
Attachment #644347 - Flags: review+
Attachment #644347 - Flags: approval-comm-beta?
Attachment #644347 - Flags: approval-comm-aurora?
Comment on attachment 644347 [details] [diff] [review]
Patch v2

Yeah, I really think we want to have this on aurora/beta.
Attachment #644347 - Flags: approval-comm-beta?
Attachment #644347 - Flags: approval-comm-beta+
Attachment #644347 - Flags: approval-comm-aurora?
Attachment #644347 - Flags: approval-comm-aurora+
(Assignee)

Comment 10

5 years ago
comm-central: https://hg.mozilla.org/comm-central/rev/04beb37df7a5
comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/da63d81338ee
comm-beta: https://hg.mozilla.org/releases/comm-beta/rev/15a684fd4a09
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-thunderbird15: --- → fixed
status-thunderbird16: --- → fixed
OS: Windows XP → All
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 17.0
(Assignee)

Comment 11

5 years ago
Backed out of comm-beta since we landed on a SeaMonkey relbranch (oops).

Re-landed on comm-beta as: https://hg.mozilla.org/releases/comm-beta/rev/443add20071a
You need to log in before you can comment on or make changes to this bug.