Last Comment Bug 772773 - "is typing..." message in chat box doesn't disappear
: "is typing..." message in chat box doesn't disappear
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Instant Messaging (show other bugs)
: 16 Branch
: x86 All
: -- minor (vote)
: Thunderbird 17.0
Assigned To: Mike Conley (:mconley) - (Away until June 29th)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-11 01:45 PDT by bastien.rene
Modified: 2012-07-23 13:02 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed


Attachments
comparison between error and normal message (7.64 KB, image/png)
2012-07-11 01:45 PDT, bastien.rene
no flags Details
Patch v1 (722 bytes, patch)
2012-07-19 12:39 PDT, Mike Conley (:mconley) - (Away until June 29th)
florian: review-
Details | Diff | Review
Patch v2 (1.41 KB, patch)
2012-07-20 08:14 PDT, Mike Conley (:mconley) - (Away until June 29th)
florian: review+
bwinton: approval‑comm‑aurora+
bwinton: approval‑comm‑beta+
Details | Diff | Review

Description bastien.rene 2012-07-11 01:45:01 PDT
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.
Comment 1 Mike Conley (:mconley) - (Away until June 29th) 2012-07-19 12:39:55 PDT
Created attachment 643976 [details] [diff] [review]
Patch v1
Comment 2 bastien.rene 2012-07-20 03:56:07 PDT
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) - (Away until June 29th) 2012-07-20 06:31:32 PDT
(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] [:flo] 2012-07-20 06:59:22 PDT
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 2012-07-20 07:46:21 PDT
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) - (Away until June 29th) 2012-07-20 08:13:13 PDT
(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) - (Away until June 29th) 2012-07-20 08:14:37 PDT
Created attachment 644347 [details] [diff] [review]
Patch v2

Thanks for the review Florian - and I agree, let's DRY this sucker up. :)
Comment 8 Florian Quèze [:florian] [:flo] 2012-07-20 08:20:03 PDT
Comment on attachment 644347 [details] [diff] [review]
Patch v2

Thanks!
Comment 9 Blake Winton (:bwinton) (:☕️) 2012-07-23 11:49:59 PDT
Comment on attachment 644347 [details] [diff] [review]
Patch v2

Yeah, I really think we want to have this on aurora/beta.
Comment 11 Mike Conley (:mconley) - (Away until June 29th) 2012-07-23 13:02:32 PDT
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

Note You need to log in before you can comment on or make changes to this bug.