955085 - Typing status not cleared when contact goes offline

Status: RESOLVED FIXED

(Chat Core :: XMPP, defect)

Description

[Patrick Cloke [:clokep]]

*** Original post on bio 1656 at 2012-08-21 16:19:00 UTC ***

This was seen with Google Talk, so it might be a bug with jsProtoHelper / the XMPP implementation of it.

A contact was typing to me when they signed off, the conversation notified me that "<foo is now offline>", but the conversation binding (i.e. the tab title) still had the typing icon and was green. This seems like a bug (and flo agreed that he's seen it before). We aren't sure if it's a UI bug, a protocol bug or a bug in jsProtoHelper, however. Filing it here for now.

Comment 1

[Harshit [:harshit]]

Hello,

I am a novice to Instantbird/Mozilla code-base. I would like to start with this bug. Would it be a right for me? 

Thank you.

PS: I have noticed this bug personally.

Comment 2

[aleth [:aleth]]

(In reply to Harshit [:harshit] from comment #1)
> I am a novice to Instantbird/Mozilla code-base. I would like to start with
> this bug. Would it be a right for me? 

Hi Harshit, 

you can certainly start with this bug! 

First step is to set up your own Instantbird build, following the instructions here: https://developer.mozilla.org/en-US/docs/Simple_Instantbird_build 

Second step is to try to find reliable STR (steps to reproduce), so you have a good way to test if any changes you make to the code actually fix the bug.

When you've done that, I can suggest some places in the code to start looking.

If you run into any problems with this, you can ask on #instantbird.

Comment 3

[Harshit [:harshit]]

Attachment: patch

(This is my first patch here. I am very sorry if I am missing on any standard procedure.)
In this patch, I have just added a check for the type of presenceStanza received. If it is 'unavailable', then typingState is changed to 'NOT_TYPING' before calling onPresenceStanza method from relevant buddy account. Though this works, I am not very confident for the position of check.

I have tested this patch with Google talk.

Comment 4

[aleth [:aleth]]

Comment on attachment 8566143 [details] [diff] [review]
patch

Review of attachment 8566143 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this! 

First, there are some details to take care of so that you generate patches suitable for being checked in:
Please use hg export to generate the patch, as this will automatically add required metadata like the commit message, the author of the patch, etc. Also, make sure your hg is configured correctly, following the instructions here https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F . Usually running "mach mercurial-setup" is all you have to do! If you're using mercurial queues (https://developer.mozilla.org/en-US/docs/Mercurial_Queues) to handle your patches, which I would recommend, you can use hg qref -e to add a suitable commit message. Mozilla commit messages are of the form "Bug XXX - Description. r=reviewer".

::: chat/protocols/xmpp/xmpp.jsm
@@ +982,5 @@
>        // receive a roster push containing more or less the same information
>        return;
>      }
> +    else if (this._buddies.has(jid))	{
> +      if(type == "unavailable")	{

Please don't add this extra code here, add it to the onPresenceStanza method of the buddy (in the same file) which is called on the next line. That's what it's there for!

@@ +983,5 @@
>        return;
>      }
> +    else if (this._buddies.has(jid))	{
> +      if(type == "unavailable")	{
> +      	this._conv.get(this.normalize(from)).updateTyping(Ci.prplIConvIM.NOT_TYPING);

This assumes a conversation with the buddy exists. That doesn't have to be the case.

Comment 5

[Harshit [:harshit]]

I will prepare a new patch ASAP and incorporate all your suggestions.

Comment 6

[Harshit [:harshit]]

Attachment: Moved the availability check as suggested to buddy prototype and also placed a check if conversation with buddy exists

Comment 7

[aleth [:aleth]]

Comment on attachment 8567490 [details] [diff] [review]
Moved the availability check as suggested to buddy prototype and also placed a check if conversation with buddy exists

Review of attachment 8567490 [details] [diff] [review]:
-----------------------------------------------------------------

The code is looking good, but there are some coding style nits to take care of.

::: chat/protocols/xmpp/xmpp.jsm
@@ +508,5 @@
>      // Facebook chat's XMPP server doesn't send resources, let's
>      // replace undefined resources with empty resources.
>      let resource =
>        this._account._parseJID(aStanza.attributes["from"]).resource || "";
> +    let norm = this.normalizedName;

Please put a blank line above this line because you're adding a new section that deals with conversations the buddy might be in.

@@ +509,5 @@
>      // replace undefined resources with empty resources.
>      let resource =
>        this._account._parseJID(aStanza.attributes["from"]).resource || "";
> +    let norm = this.normalizedName;
> +    let conv = this._account._conv.get(norm);

You can just do 
let conv = this._account._conv.get(this.normalizedName);
and save yourself the norm variable which you never use again.

@@ +510,5 @@
>      let resource =
>        this._account._parseJID(aStanza.attributes["from"]).resource || "";
> +    let norm = this.normalizedName;
> +    let conv = this._account._conv.get(norm);
> +    // check to remove typing status when buddy becomes unavailable

Move the comment above the first line of the section.

Comment style: Start with a capital letter and end in a period (.). So here I'd suggest
// Reset typing status if the buddy is in a conversation and becomes unavailable.

@@ +511,5 @@
>        this._account._parseJID(aStanza.attributes["from"]).resource || "";
> +    let norm = this.normalizedName;
> +    let conv = this._account._conv.get(norm);
> +    // check to remove typing status when buddy becomes unavailable
> +	if(type == "unavailable" && conv)	{		

This whole block should not be indented compared to the comment.

Indentation is 2 spaces and not tabs. Please set up your editor accordingly.

Also, tell your editor to remove trailing whitespace automatically (such as you added on this line).

Put a space between if and (type...

Comment 8

[Harshit [:harshit]]

Attachment: Created a new patch following the standard coding style

Comment 9

[aleth [:aleth]]

Comment on attachment 8567516 [details] [diff] [review]
Created a new patch following the standard coding style

Review of attachment 8567516 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/protocols/xmpp/xmpp.jsm
@@ +515,5 @@
> +    // Reset typing status if the buddy is in a conversation and becomes unavailable.
> +    let conv = this._account._conv.get(this.normalizedName);
> +    if (type == "unavailable" && conv) {
> +      conv.updateTyping(Ci.prplIConvIM.NOT_TYPING);
> +    }

Final nit: When a {...} only contains a single line, we drop the brackets:

if (type == "unavailable" && conv)
  conv.updateTyping(Ci.prplIConvIM.NOT_TYPING);

Comment 10

[Harshit [:harshit]]

Attachment: Created a new patch following the standard coding style

I don't know how I can act so dumb. I had observed styling about if..else long back but still forgot about it when needed.

Comment 11

[aleth [:aleth]]

Comment on attachment 8567599 [details] [diff] [review]
Created a new patch following the standard coding style

Review of attachment 8567599 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this bug!

Comment 12

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/686d538c7eeb