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

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[bastien.rene]

Attachment: 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.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

Comment 2

[bastien.rene]

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
          }

Comment 3

[Mike Conley (:mconley) (:⚙️)]

(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 4

[Florian Quèze [:florian]]

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.

Comment 5

[bastien.rene]

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 ;)

Comment 6

[Mike Conley (:mconley) (:⚙️)]

(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. :)

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2

Thanks for the review Florian - and I agree, let's DRY this sucker up. :)

Comment 8

[Florian Quèze [:florian]]

Comment on attachment 644347 [details] [diff] [review]
Patch v2

Thanks!

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 644347 [details] [diff] [review]
Patch v2

Yeah, I really think we want to have this on aurora/beta.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

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

Comment 11

[Mike Conley (:mconley) (:⚙️)]

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