989271 - Tooltips and context menu should refer to the correct accountbuddy

Status: RESOLVED FIXED

(Instantbird Graveyard :: Conversation, defect)

Description

[aleth [:aleth]]

The current code uses the preferredAccountbuddy, rather than the account buddy associated with the account of the conversation. This leads to various bugs (e.g. tooltip not showing because a buddy for the same protocol but on a different account exists, also can't add an accountbuddy for that account from the context menu).

Comment 1

[aleth [:aleth]]

Attachment: 989271.patch

Comment 2

[Florian Quèze [:florian]]

Comment on attachment 8398429 [details] [diff] [review]
989271.patch

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

::: im/content/buddytooltip.xml
@@ +341,5 @@
>           // To try to ensure that we aren't misidentifying a nick with a
>           // contact, we require at least that the normalizedChatBuddyName of
>           // the nick is normalized like a normalizedName for contacts.
>           if (normalizedNick == account.normalize(normalizedNick)) {
> +           let accountbuddy = account.getAccountBuddy(normalizedNick);

I'm not fully sure we need a new API (Could we just loop over the accountbuddies here instead of using the preferredAccountBuddy property? or could we add a method to the conversation binding instead?).

But if you do think we need a new API, I don't think it belongs to imIAccount. I would expect it on imIBuddy (eg getAccountBuddyForAccount(imIAccount)) or on the contacts service (eg. getAccountBuddyFromNameAndAccount).

Comment 3

[aleth [:aleth]]

(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> I'm not fully sure we need a new API (Could we just loop over the
> accountbuddies here instead of using the preferredAccountBuddy property? or
> could we add a method to the conversation binding instead?).

I added an API to avoid duplication. The conversation binding seems a strange place to put a shared method especially for the tooltip code.
 
> But if you do think we need a new API, I don't think it belongs to
> imIAccount. I would expect it on imIBuddy (eg
> getAccountBuddyForAccount(imIAccount)) or on the contacts service (eg.
> getAccountBuddyFromNameAndAccount).

I considered putting the API on the contacts service but decided against it as given the "andAccount" part, one can save oneself the extra parameter by putting it on the account. Usually that's a good indication of where the method "wants" to live ;) Especially as the implementation I am adding gets away with using the existing contact service API, it doesn't provide anything fundamentally new. 

I didn't put the API on the buddy because then you would not have deduplicated the getBuddyByNameAndProtocol call.

Comment 4

[aleth [:aleth]]

Attachment: 989271.patch 2

Moved the new method to the contacts service.

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 8399366 [details] [diff] [review]
989271.patch 2

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

::: chat/components/public/imIContactsService.idl
@@ +26,5 @@
>    imIBuddy getBuddyById(in long aId);
>    imIBuddy getBuddyByNameAndProtocol(in AUTF8String aNormalizedName,
>                                       in prplIProtocol aPrpl);
> +  imIAccountBuddy getAccountBuddyByNameAndAccount(in AUTF8String aNormalizedName,
> +                                                   in imIAccount aAccount);

Misalignment here.

::: chat/components/src/imContacts.js
@@ +1341,5 @@
>    },
> +  getAccountBuddyByNameAndAccount: function(aNormalizedName, aAccount) {
> +    let buddy = this.getBuddyByNameAndProtocol(aNormalizedName,
> +                                               aAccount.protocol);
> +    if (buddy) {

let accountId = aAccount.id;
(to avoid pointlessly calling the id getters several times)

@@ +1342,5 @@
> +  getAccountBuddyByNameAndAccount: function(aNormalizedName, aAccount) {
> +    let buddy = this.getBuddyByNameAndProtocol(aNormalizedName,
> +                                               aAccount.protocol);
> +    if (buddy) {
> +      for (let accountbuddy of buddy.getAccountBuddies()) {

accountbuddy -> accountBuddy ?

::: im/content/buddytooltip.xml
@@ +341,5 @@
>           // To try to ensure that we aren't misidentifying a nick with a
>           // contact, we require at least that the normalizedChatBuddyName of
>           // the nick is normalized like a normalizedName for contacts.
>           if (normalizedNick == account.normalize(normalizedNick)) {
> +           let accountbuddy =

accountBuddy

::: im/content/nsContextMenu.js
@@ +175,5 @@
>        // normalizedChatBuddyName of the nick is normalized like a normalizedName
>        // for contacts.
>        let normalizedNick = this.conv.target.getNormalizedChatBuddyName(nick);
>        if (normalizedNick == account.normalize(normalizedNick)) {
> +        let accountbuddy =

accountBuddy

@@ +179,5 @@
> +        let accountbuddy =
> +          Services.contacts.getAccountBuddyByNameAndAccount(normalizedNick,
> +                                                            account);
> +        if (accountbuddy)
> +          this.buddy = accountbuddy.buddy;

Do you know what the |this.buddy| value is used for? I looked very briefly and couldn't find any use of it.

Comment 6

[aleth [:aleth]]

Attachment: 989271.patch 3

>Do you know what the |this.buddy| value is used for? I looked very briefly and >couldn't find any use of it.

It does not seem to be used any more, so I removed it. Thanks!

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 8399976 [details] [diff] [review]
989271.patch 3

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

Looks good, thanks! Just one nit:

::: im/content/buddytooltip.xml
@@ +341,5 @@
>           // To try to ensure that we aren't misidentifying a nick with a
>           // contact, we require at least that the normalizedChatBuddyName of
>           // the nick is normalized like a normalizedName for contacts.
>           if (normalizedNick == account.normalize(normalizedNick)) {
> +           let accountbuddy =

The comment I had here hasn't been addressed.

Comment 8

[aleth [:aleth]]

Attachment: 989271.patch 4

Comment 9

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/58057c999706