Closed Bug 955570 Opened 6 years ago Closed 6 years ago

Sender name not set in Yahoo conversations

Categories

(Chat Core :: Yahoo! Messenger, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: qheaden)

References

Details

Attachments

(2 files, 1 obsolete file)

*** Original post on bio 2132 at 2013-08-27 13:17:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***
Attached image Screenshot
*** Original post on bio 2132 as attmnt 2792 at 2013-08-27 13:17:00 UTC ***

STR
Receive a message from a Yahoo contact
Summary: Sender name not set correctly in Yahoo conversations → Sender name not set in Yahoo conversations
*** Original post on bio 2132 at 2013-08-27 13:28:48 UTC ***

To clarify: It's the user's own name that is not set. The name of the contact one is talking to is set correctly. The STR are to indicate that an incoming message opened the conversation (if that makes a difference -- it shouldn't).
Attached patch Patch 1 (obsolete) — Splinter Review
*** Original post on bio 2132 as attmnt 2801 at 2013-08-29 02:33:00 UTC ***

Good catch aleth! 

This patch fixes the issue, and I placed the sender name in a separate variable to make the code cleaner instead of putting it inline with the function call.
Attachment #8354571 - Flags: review?(clokep)
Assignee: nobody → qheaden
Status: NEW → ASSIGNED
Comment on attachment 8354571 [details] [diff] [review]
Patch 1

*** Original change on bio 2132 attmnt 2801 at 2013-08-29 10:39:18 UTC ***

>+    let senderName = this._account.imAccount.alias || this._account.cleanUsername;
Should this be cleanUsername or the real username? Also, I don't think you should be touching imAccount here.
Attachment #8354571 - Flags: review?(clokep) → review-
*** Original post on bio 2132 at 2013-08-29 14:33:58 UTC ***

(In reply to comment #3)
> Comment on attachment 8354571 [details] [diff] [review] (bio-attmnt 2801) [details]
> Patch 1
> 
> >+    let senderName = this._account.imAccount.alias || this._account.cleanUsername;
> Should this be cleanUsername or the real username? Also, I don't think you
> should be touching imAccount here.

I think it should be cleanUsername. Remember that JS-Yahoo allows people to put their @yahoo.* address in their username. I don't think that should show next to each message sent. cleanUsername strips that.
*** Original post on bio 2132 at 2013-08-29 15:00:12 UTC ***

(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 8354571 [details] [diff] [review] (bio-attmnt 2801) [details]
> > Patch 1
> > 
> > >+    let senderName = this._account.imAccount.alias || this._account.cleanUsername;
> > Should this be cleanUsername or the real username? Also, I don't think you
> > should be touching imAccount here.
> 
> I think it should be cleanUsername. Remember that JS-Yahoo allows people to put
> their @yahoo.* address in their username. I don't think that should show next
> to each message sent. cleanUsername strips that.

Yes, I remember that is what it's doing, my thought process was that it should match what the user typed in. I'm OK using this though.

See [1] for using the alias.

[1] http://log.bezut.info/instantbird/today/#m177
Attached patch Patch 2Splinter Review
*** Original post on bio 2132 as attmnt 2803 at 2013-08-29 20:39:00 UTC ***

This patch uses the new preferred way of using the user's alias as a sender name.
Attachment #8354573 - Flags: review?(aleth)
Comment on attachment 8354571 [details] [diff] [review]
Patch 1

*** Original change on bio 2132 attmnt 2801 at 2013-08-29 20:39:03 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354571 - Attachment is obsolete: true
*** Original post on bio 2132 at 2013-08-30 09:56:42 UTC ***

Do you know how setting the _alias flag ends up in msg.alias? (I see XMPP sets it this way too, but it puzzles me.)
*** Original post on bio 2132 at 2013-08-30 10:19:52 UTC ***

(In reply to comment #7)
> Do you know how setting the _alias flag ends up in msg.alias? (I see XMPP sets
> it this way too, but it puzzles me.)

http://lxr.instantbird.org/instantbird/source/chat/modules/jsProtoHelper.jsm#382
382   _alias: "",
383   get alias() this._alias || this.who,
Comment on attachment 8354573 [details] [diff] [review]
Patch 2

*** Original change on bio 2132 attmnt 2803 at 2013-08-30 10:24:14 UTC ***

Thanks!
Attachment #8354573 - Flags: review?(aleth) → review+
Whiteboard: [checkin-needed]
Comment on attachment 8354573 [details] [diff] [review]
Patch 2

*** Original change on bio 2132 attmnt 2803 at 2013-08-30 10:28:56 UTC ***

See how XMPP does it:

http://lxr.instantbird.org/instantbird/source/chat/protocols/xmpp/xmpp.jsm#259
259     let alias = this.account.alias || this.account.statusInfo.displayName;
Attachment #8354573 - Flags: review+ → review-
*** Original post on bio 2132 at 2013-08-30 10:37:27 UTC ***

(In reply to comment #10)
> 259     let alias = this.account.alias || this.account.statusInfo.displayName;

If we decide to do this generally (rather than just for XMPP) we should discuss it, abstract it and do it in jsProtoHelper. That would be a separate bug imho.
An argument against would be that the displayName is not visible to the receiving end of the conversation in Yahoo, and there is then nothing showing the username (which the receiving end uses for the same purpose) on the IB end.
Comment on attachment 8354573 [details] [diff] [review]
Patch 2

*** Original change on bio 2132 attmnt 2803 at 2013-08-30 10:44:22 UTC ***

Removing my r-, I looked at this a bit too quickly, sorry.

Showing the display name in the conversation makes sense only if we sent the display name to the server and the remote contact is likely to see the display name in the conversation too.

It appears you aren't sending the display name to the Yahoo server. Is this a bug, or something the protocol doesn't support? Anyway, if it's a bug, you can add the display name as alias on the messages at the same time as you send it to the server in a follow up bug.
Attachment #8354573 - Flags: review-
Comment on attachment 8354573 [details] [diff] [review]
Patch 2

*** Original change on bio 2132 attmnt 2803 at 2013-08-31 12:36:30 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354573 - Flags: review+
Blocks: 955574
*** Original post on bio 2132 at 2013-09-06 00:18:03 UTC ***

http://hg.instantbird.org/instantbird/rev/9db515149f68

Please answer the following:
(In reply to comment #12)
> It appears you aren't sending the display name to the Yahoo server. Is this a
> bug, or something the protocol doesn't support? Anyway, if it's a bug, you can
> add the display name as alias on the messages at the same time as you send it
> to the server in a follow up bug.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.5
*** Original post on bio 2132 at 2013-09-06 05:43:26 UTC ***

> Please answer the following:
> (In reply to comment #12)
> > It appears you aren't sending the display name to the Yahoo server. Is this a
> > bug, or something the protocol doesn't support? Anyway, if it's a bug, you can
> > add the display name as alias on the messages at the same time as you send it
> > to the server in a follow up bug.

I think its a bit complicated. I'm not sure if you can send any random display name. Even so, I don't think it would be a good thing, as people could take on the usernames of other Yahoo accounts, which would be confusing. Yahoo profiles do support aliases, but to work with them, I think you need to start messing around with one or more of Yahoo's APIs, which is another complicated issue.
You need to log in before you can comment on or make changes to this bug.