Closed Bug 989271 Opened 7 years ago Closed 7 years ago

Tooltips and context menu should refer to the correct accountbuddy

Categories

(Instantbird :: Conversation, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

Details

Attachments

(1 file, 3 obsolete files)

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).
Attached patch 989271.patch (obsolete) — Splinter Review
Attachment #8398429 - Flags: review?(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).
Attachment #8398429 - Flags: review?(florian) → review-
(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.
Attached patch 989271.patch 2 (obsolete) — Splinter Review
Moved the new method to the contacts service.
Attachment #8398429 - Attachment is obsolete: true
Attachment #8399366 - Flags: review?(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.
Attachment #8399366 - Flags: review?(florian) → review-
Attached patch 989271.patch 3 (obsolete) — Splinter Review
>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!
Attachment #8399366 - Attachment is obsolete: true
Attachment #8399976 - Flags: review?(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.
Attachment #8399976 - Flags: review?(florian) → review+
Attached patch 989271.patch 4Splinter Review
Attachment #8399976 - Attachment is obsolete: true
Attachment #8399990 - Flags: review+
https://hg.mozilla.org/comm-central/rev/58057c999706
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.6
You need to log in before you can comment on or make changes to this bug.