1173366 - Use aData parameter of update-typing notifications to pass a short name to the UI

Status: RESOLVED FIXED

(Instantbird Graveyard :: Conversation, defect)

Description

[Abdelrahman Ahmed [:abdelrahman]]

Use aData parameter of observe as a parameter for updateTyping to make conversation.xml independent of calculating the name and let protocols do that.

Comment 1

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 1 - Use aData parameter for updateTyping

Comment 2

[aleth [:aleth]]

Comment on attachment 8617952 [details] [diff] [review]
rev 1 - Use aData parameter for updateTyping

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

Please include in this patch the corresponding changes to where the notification is being sent by JS-XMPP and JS-IRC (and maybe JS-Yahoo).

::: im/content/conversation.xml
@@ +1650,5 @@
>          ]]>
>          </body>
>        </method>
>  
> +      <field name="_aName">""</field>

Do you need to store this value?

@@ +1663,5 @@
>            this.tab.removeAttribute("typing");
>            this.tab.removeAttribute("typed");
>  
>            let typingText = "";
>            var name = this._conv.title.replace(/^([a-zA-Z0-9.]+)[@\s].*/, "$1");

No need to run the regex if it's not needed.

@@ +1877,1 @@
>              this.updateTyping();

Would this.updateTyping(aData) make more sense?

Comment 3

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #2)
> Please include in this patch the corresponding changes to where the
> notification is being sent by JS-XMPP and JS-IRC (and maybe JS-Yahoo).
By adding a comment for that?

> Do you need to store this value?
Yes, because this method is used more than one time in conversation.xml to update typing state and these callings can not be able to get the name.

> No need to run the regex if it's not needed.
I left it because current protocols does not support sending name in aData, so for them now they can use old method to work correctly.

> Would this.updateTyping(aData) make more sense?
currently there is a parameter called "aForceUpdate" and that will be passed to it so it needs to be something like that this.updateTyping(false, aData).
Do you mean after converting aName to a parameter or let aName as is and also use a parameter and assign its value to it?

Comment 4

[aleth [:aleth]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #3)
> (In reply to aleth [:aleth] from comment #2)
> > Please include in this patch the corresponding changes to where the
> > notification is being sent by JS-XMPP and JS-IRC (and maybe JS-Yahoo).
> By adding a comment for that?

By sending the appropriate aData with the notification. Otherwise this patch is dead code when it lands.
 
> > Do you need to store this value?
> Yes, because this method is used more than one time in conversation.xml to
> update typing state and these callings can not be able to get the name.

OK, in that case give the field a more descriptive name (e.g. _currentTypingName).

> > No need to run the regex if it's not needed.
> I left it because current protocols does not support sending name in aData,
> so for them now they can use old method to work correctly.

But you only have to use it if _currentTypingName is not set.

> > Would this.updateTyping(aData) make more sense?
> currently there is a parameter called "aForceUpdate" and that will be passed
> to it so it needs to be something like that this.updateTyping(false, aData).

You're right, as aForceUpdate is an optional parameter it's better to do it the way you did it.

Comment 5

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 2 - Use aData parameter for updateTyping

(In reply to aleth [:aleth] from comment #4)
> By sending the appropriate aData with the notification. Otherwise this patch
> is dead code when it lands.
Ah, I thought that would be in a follow up bugs.
Seems that IRC does not use this method.
(https://dxr.mozilla.org/comm-central/search?q=updateTyping&case=false)

Comment 6

[aleth [:aleth]]

Comment on attachment 8620383 [details] [diff] [review]
rev 2 - Use aData parameter for updateTyping

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

Please add the corresponding change for Thunderbird too (mail/components/im/content/imconversation.xml).

::: chat/protocols/xmpp/xmpp.jsm
@@ +1322,5 @@
>        else if (state == "paused")
>          typingState = Ci.prplIConvIM.TYPED;
>      }
>      let conv = this._conv.get(norm);
> +    conv.updateTyping(typingState, this._parseJID(from).node);

This will not always match this._conv.contactDisplayName. The two updateTyping calls should be consistent, using a shared helper function if necessary.

::: chat/protocols/yahoo/yahoo.js
@@ +406,5 @@
>      let message = this.decodeMessage(aMessage)
>                        .replace(/(<font[^>]+)size=\"(\d+)\"/g, this._fixFontSize);
>  
>      conv.writeMessage(aName, message, {incoming: true});
> +    conv.updateTyping(Ci.prplIConvIM.NOT_TYPING, conv._buddyUserName);

Just use conv.name (it's the same, but not a private variable).

Comment 7

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #6)
> ::: chat/protocols/yahoo/yahoo.js

Also, have you checked to see what yahoo names look like? Are you sure the current regex doesn't affect the name?

Comment 8

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #7)
> Also, have you checked to see what yahoo names look like? Are you sure the
> current regex doesn't affect the name?
No, I have not tested that yet and I do not know yahoo names look like!

Comment 9

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 3 - Use aData parameter for updateTyping

Comment 10

[aleth [:aleth]]

Comment on attachment 8620695 [details] [diff] [review]
rev 3 - Use aData parameter for updateTyping

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

This patch changes the current behaviour

1) for full names (currently we show "Patrick" for "Patrick Cloke"). Changing this is probably a good thing as it makes all kinds of assumptions that may be broken by how the user names their contacts and/or l10n.

2) for contactDisplaynames that are JIDs (currenty "name" for "[e-mail]"). It would be nice to keep just showing the name part here. Bonus points if this also works in the future for MUC participants.

::: im/content/conversation.xml
@@ +1663,5 @@
>            this.tab.removeAttribute("typing");
>            this.tab.removeAttribute("typed");
>  
>            let typingText = "";
> +          var name = this._currentTypingName;

Nit: As we're touching this code anyway, let's use "let" instead of "var".

::: mail/components/im/content/imconversation.xml
@@ +1279,5 @@
>            cti.removeAttribute("typed");
>  
> +          let name = this._currentTypingName;
> +          if (!this._currentTypingName)
> +            name = this._conv.title;//.replace(/^([a-zA-Z0-9.]+)[@\s].*/, "$1");

I wish whoever commented out the replace had added a comment explaining why :-(

It may have been 1) above I suppose...

Comment 11

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #10)
> This patch changes the current behaviour

I think we just need to do changes for conversation.xml, imconversation.xml, jsProtoHelper.jsm and test_yahooAccount.js.

We do these changes to support sending name as a parameter if needed and it's not passed we just use the ordinary way of conversation.xml to get the name e.g(replace...) and that was compatible with all protocols, but we add this option for cases like private muc.
The jid is like(room@domain/nick) and detecting user nick using replace would be not correct, so in cases like that the protocol sends the name with notifier.

So for line like that we will not change it to send the name.
> conv.updateTyping(Ci.prplIConvIM.NOT_TYPING);

Comment 12

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #10)
I wasn't saying changing the current behaviour was wrong, just making it clear that's what this patch is doing.

> 2) for contactDisplaynames that are JIDs (currenty "name" for
> "[e-mail]"). It would be nice to keep just showing the name part
> here. Bonus points if this also works in the future for MUC participants.

How about using contactDisplayName for XMPP AccountBuddies, and for XMPP conversations add a new getter, conv.shortName, which gives
- buddy.contactDisplayName if conv.buddy (for consistency)
- the nick for MUC participant conversations
- the username part of the JID otherwise

Comment 13

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 4 - Use aData parameter for updateTyping

Comment 14

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #10)
> I wish whoever commented out the replace had added a comment explaining why
> :-(
> 
> It may have been 1) above I suppose...

This caused the issue of getting the first name in conversation.xml. probably the commenting out was made for that reason.

(In reply to aleth [:aleth] from comment #12)
> How about using contactDisplayName for XMPP AccountBuddies, and for XMPP
> conversations add a new getter, conv.shortName, which gives
> - buddy.contactDisplayName if conv.buddy (for consistency)
> - the nick for MUC participant conversations
> - the username part of the JID otherwise

yes, that's good and will help us for muc private bugs.

Comment 15

[aleth [:aleth]]

Comment on attachment 8622050 [details] [diff] [review]
rev 4 - Use aData parameter for updateTyping

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +375,5 @@
>    get buddy() this._account._buddies.get(this.name),
>    get title() this.contactDisplayName,
>    get contactDisplayName() this.buddy ? this.buddy.contactDisplayName : this.name,
>    get userName() this.buddy ? this.buddy.userName : this.name,
> +  get shortName() {

Add a comment, something like "used to avoid showing full jids in typing notifications"

@@ +384,5 @@
> +    if (!jid)
> +      return this.name;
> +    if (this._account._mucs.has(this._account.normalize(this.name)) &&
> +        jid.resource)
> +      return jid.resource;

Move this if clause to the MUC private messages patch, as I assume this patch will land before the MUC private messages patch is finished.

::: im/content/conversation.xml
@@ +1665,5 @@
>  
>            let typingText = "";
> +          let name = this._currentTypingName;
> +          if (!this._currentTypingName)
> +            name = this._conv.title;

Please keep the replace() if this._currentTypingName is not set. Add a comment saying this regex is intended to get a shortened name for prpls that don't provide a currentTypingName.

Comment 16

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 5 - Use aData parameter for updateTyping

Comment 17

[aleth [:aleth]]

Comment on attachment 8622071 [details] [diff] [review]
rev 5 - Use aData parameter for updateTyping

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

Great, thanks!

Comment 18

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/535a02525340