1199668 - Contact status and icon not shown for search result conversations

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[Thomasy]

Attachment: Search Result have no user status or user icon

Search instant message and open the result.
The user status and user icon at right hand side is missing.

Comment 1

[Thomasy]

Attachment: Chat window have user status and user icon

Comment 2

[aleth [:aleth]]

This is because user icons are not cached, but there's no reason not to show the icon when it's available.

Comment 3

[Thomasy]

(In reply to aleth [:aleth] from comment #2)
> This is because user icons are not cached, but there's no reason not to show
> the icon when it's available.

Since the log save only account.normalizedName and account.protocol.normalizedName, we need a function for look up contact from account.normalizedName
and protocol. We only have functions getAccountBuddyByNameAndAccount (need aAccount) and getBuddyByNameAndProtocol. Maybe we should build aAccount from  account.normalizedName
and protocol or we should save account id in log.


https://dxr.mozilla.org/comm-central/source/chat/components/src/logger.js#184
>    if (this.format == "json") {
>      header = {
>        date: new Date(this._startTime),
>        name: this._conv.name,
>        title: this._conv.title,
>        account: account.normalizedName,
>        protocol: account.protocol.normalizedName,
>        isChat: this._conv.isChat,
>        normalizedName: this._conv.normalizedName
>      };




https://dxr.mozilla.org/comm-central/source/chat/components/src/imContacts.js#1345
>  getAccountBuddyByNameAndAccount: function(aNormalizedName, aAccount) {
>    let buddy = this.getBuddyByNameAndProtocol(aNormalizedName,
>                                               aAccount.protocol);

https://dxr.mozilla.org/comm-central/source/chat/components/src/imContacts.js#1329
>  getBuddyByNameAndProtocol: function(aNormalizedName, aPrpl) {
>    let statement =
>      DBConn.createStatement("SELECT b.id FROM buddies b " +
>                             "JOIN account_buddy ab ON buddy_id = b.id " +
>                             "JOIN accounts a ON account_id = a.id " +
>                             "WHERE b.key = :buddyName and a.prpl = :prplId");
>    statement.params.buddyName = aNormalizedName;

Comment 4

[aleth [:aleth]]

(In reply to Thomasy from comment #3)
> (In reply to aleth [:aleth] from comment #2)
> > This is because user icons are not cached, but there's no reason not to show
> > the icon when it's available.
> 
> Since the log save only account.normalizedName and
> account.protocol.normalizedName, we need a function for look up contact from
> account.normalizedName
> and protocol. We only have functions getAccountBuddyByNameAndAccount (need
> aAccount) and getBuddyByNameAndProtocol. Maybe we should build aAccount from
> account.normalizedName
> and protocol or we should save account id in log.

The normalized names are good identifiers, there's just no single API function that does the reverse lookup you want. E.g. use getAccounts() to get the list of accounts, among those pick the account with the right normalized name, check the protocol matches, and then use getAccountBuddyByNameAndAccount.

Comment 5

[Thomasy]

Attachment: WIP

Comment 6

[Thomasy]

Attachment: WIP screenshot

WIP: The user status and user icon are there but will not update even if I add to observedContact.

Comment 7

[Patrick Cloke [:clokep]]

Feel free to request feedback from someone (sounds like aleth would be the right one) if you want to know if the approach is OK!

Comment 8

[aleth [:aleth]]

Comment on attachment 8656156 [details] [diff] [review]
WIP

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

Looks like a good start!

::: chat/components/src/imContacts.js
@@ +1348,5 @@
>      if (buddy) {
>        let id = aAccount.id;
>        for (let accountBuddy of buddy.getAccountBuddies()) {
>          if (accountBuddy.account.id == id)
> +          return buddy;

Don't change this, this function is *supposed* to return a prplIAccountBuddy and not an imIBuddy (see imIContactsService.idl).

::: mail/components/im/content/chat-messenger-overlay.js
@@ +406,5 @@
>      Services.obs.addObserver(this, "conversation-loaded", false);
>      // Conversation title may not be set yet if this is a search result.
>      let cti = document.getElementById("conv-top-info");
>      cti.setAttribute("displayName", aConversation.title);
> +    let accounts = imServices.accounts.getAccounts();

You should return early if aConversation.isChat is true (chatrooms don't have user icons etc), or if this.observedContact is already set (you don't want to add flicker just because someone changed the log they are looking at).

@@ +408,5 @@
>      let cti = document.getElementById("conv-top-info");
>      cti.setAttribute("displayName", aConversation.title);
> +    let accounts = imServices.accounts.getAccounts();
> +    while (accounts.hasMoreElements()) {
> +      let aAccount = accounts.getNext();

Nit: the aName convention is used for function arguments. So don't use it here. let account = ... is OK.

@@ +412,5 @@
> +      let aAccount = accounts.getNext();
> +      if (aAccount.normalizedName == aConversation.account.normalizedName &&
> +          aAccount.protocol.normalizedName == aConversation.account.protocol.name) {
> +        let aContact =
> +          imServices.contacts.getAccountBuddyByNameAndAccount(aConversation.normalizedName, aAccount);

This doesn't return a contact, it returns an accountbuddy.

You should check it's actually found one and return early if not.

Then to get the contact, use
let contact = accountBuddy.buddy.contact;

(Look at the idl file for details. In Thunderbird the distinction between contacts, buddies, and accountbuddies is a bit hidden as the UI doesn't really expose it.)

@@ +561,5 @@
>      let cti = document.getElementById("conv-top-info");
>      cti.setAttribute("userIcon", aContact.buddyIconFilename);
>      cti.setAttribute("displayName", aContact.displayName);
> +    let proto =
> +      aContact.preferredBuddy ? aContact.preferredBuddy.protocol : aContact.protocol;

Not needed if you fix the code above.

@@ -605,5 @@
> -      cti.removeAttribute("statusMessageWithDash");
> -      cti.removeAttribute("statusMessage");
> -      cti.removeAttribute("status");
> -      cti.removeAttribute("statusTypeTooltiptext");
> -      cti.removeAttribute("statusTooltiptext");

Why did you remove these?

@@ +626,5 @@
>            this._showLogList(aSimilarLogs, aLog);
>          });
>        });
> +      item.convView.updateConvStatus();
> +      item.update();

What are these for?

Comment 9

[Thomasy]

Attachment: bug1199668_WIP_v2.patch

Comment 10

[Thomasy]

For WIP v2, I also found other bug and enhancement. I will file other bugs.

>Enhancement
>** Back to current conversation button for search result
>Bugs
>** Remove highlight bar upon switching conversations
>** The protocol icon will not change upon switching conversations to search result from a conversation.
>** logDisplay box will flicker if upon switching to a search result that already opened (that it search
> keyword => click result => open the result => switch to previous search result tab => click the result 
> again when it switch back, the logDisplay box flickers.

Comment 11

[Thomasy]

(In reply to aleth [:aleth] from comment #8)
> Comment on attachment 8656156 [details] [diff] [review]
> WIP
> 
> Review of attachment 8656156 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks like a good start!
> 
> ::: chat/components/src/imContacts.js
> @@ +1348,5 @@
> >      if (buddy) {
> >        let id = aAccount.id;
> >        for (let accountBuddy of buddy.getAccountBuddies()) {
> >          if (accountBuddy.account.id == id)
> > +          return buddy;
> 
> Don't change this, this function is *supposed* to return a prplIAccountBuddy
> and not an imIBuddy (see imIContactsService.idl).
> 
> ::: mail/components/im/content/chat-messenger-overlay.js
> @@ +406,5 @@
> >      Services.obs.addObserver(this, "conversation-loaded", false);
> >      // Conversation title may not be set yet if this is a search result.
> >      let cti = document.getElementById("conv-top-info");
> >      cti.setAttribute("displayName", aConversation.title);
> > +    let accounts = imServices.accounts.getAccounts();
> 
> You should return early if aConversation.isChat is true (chatrooms don't
> have user icons etc), or if this.observedContact is already set (you don't
> want to add flicker just because someone changed the log they are looking
> at).
> 

I should return just before the newly added lines, since even if it is chat room the behavior will remain unchanged (the behavior without the patch).
Second part may similar to mail/components/im/content/chat-messenger-overlay.js#663

> @@ +408,5 @@
> >      let cti = document.getElementById("conv-top-info");
> >      cti.setAttribute("displayName", aConversation.title);
> > +    let accounts = imServices.accounts.getAccounts();
> > +    while (accounts.hasMoreElements()) {
> > +      let aAccount = accounts.getNext();
> 
> Nit: the aName convention is used for function arguments. So don't use it
> here. let account = ... is OK.
> 
> @@ +412,5 @@
> > +      let aAccount = accounts.getNext();
> > +      if (aAccount.normalizedName == aConversation.account.normalizedName &&
> > +          aAccount.protocol.normalizedName == aConversation.account.protocol.name) {
> > +        let aContact =
> > +          imServices.contacts.getAccountBuddyByNameAndAccount(aConversation.normalizedName, aAccount);
> 
> This doesn't return a contact, it returns an accountbuddy.
> 
> You should check it's actually found one and return early if not.
> 
> Then to get the contact, use
> let contact = accountBuddy.buddy.contact;
> 
> (Look at the idl file for details. In Thunderbird the distinction between
> contacts, buddies, and accountbuddies is a bit hidden as the UI doesn't
> really expose it.)
> 
> @@ +561,5 @@
> >      let cti = document.getElementById("conv-top-info");
> >      cti.setAttribute("userIcon", aContact.buddyIconFilename);
> >      cti.setAttribute("displayName", aContact.displayName);
> > +    let proto =
> > +      aContact.preferredBuddy ? aContact.preferredBuddy.protocol : aContact.protocol;
> 
> Not needed if you fix the code above.
> 

The code looks much better after fixing the code above.

> @@ -605,5 @@
> > -      cti.removeAttribute("statusMessageWithDash");
> > -      cti.removeAttribute("statusMessage");
> > -      cti.removeAttribute("status");
> > -      cti.removeAttribute("statusTypeTooltiptext");
> > -      cti.removeAttribute("statusTooltiptext");
> 
> Why did you remove these?
> 
> @@ +626,5 @@
> >            this._showLogList(aSimilarLogs, aLog);
> >          });
> >        });
> > +      item.convView.updateConvStatus();
> > +      item.update();
> 
> What are these for?

The code will remain unchanged. I should not modify them.

Comment 12

[aleth [:aleth]]

(In reply to Thomasy from comment #10)
> For WIP v2, I also found other bug and enhancement. I will file other bugs.

Please do!
There is also a known backlog of bugs that have been fixed for Instantbird but not ported to Thunderbird yet, if you are interested in helping to fix those let us know.
 
> >Bugs
> >** Remove highlight bar upon switching conversations

I'm not sure what you mean by this.

> >** The protocol icon will not change upon switching conversations to search result from a conversation.

Let's fix this in this bug, it's part of displaying the correct information at the top of the sidebar.

The little MUC icon next to the status message (where the status icon is for normal conversations) is also missing for search results from chatrooms.

Comment 13

[aleth [:aleth]]

Comment on attachment 8657021 [details] [diff] [review]
bug1199668_WIP_v2.patch

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

::: mail/components/im/content/chat-messenger-overlay.js
@@ +408,5 @@
>      let cti = document.getElementById("conv-top-info");
>      cti.setAttribute("displayName", aConversation.title);
> +    if (aConversation.isChat)
> +      return;
> +    let accounts = imServices.accounts.getAccounts();

Please add a comment above this block, e.g.
// Find and display the contact for this log.

@@ +421,5 @@
> +        if (!accountBuddy)
> +          return;
> +        let contact = accountBuddy.buddy.contact;
> +        if (this.observedContact && contact &&
> +            this.observedContact.id == contact.id)

This isn't quite right. If !contact you want to return as well.

@@ +424,5 @@
> +        if (this.observedContact && contact &&
> +            this.observedContact.id == contact.id)
> +          return; // onselect has just been fired again because a status
> +                  // change caused the imcontact to move.
> +                  // Return early to avoid flickering and changing the selected log.

Please remove the copied comment, it doesn't really make sense here.

Comment 14

[Thomasy]

Attachment: bug1199668_WIP_v3.patch

Comment 15

[aleth [:aleth]]

Comment on attachment 8658328 [details] [diff] [review]
bug1199668_WIP_v3.patch

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

The prplIcon attribute is currently not removed in onListItemSelected, leading to the occasional bug when the icon from some previous conversation is displayed.

::: mail/components/im/content/chat-messenger-overlay.js
@@ +407,5 @@
>      // Conversation title may not be set yet if this is a search result.
>      let cti = document.getElementById("conv-top-info");
>      cti.setAttribute("displayName", aConversation.title);
> +
> +    // Find the account for this log

Nit: For new comments, we generally use full sentences, so add a "." at the end.

@@ +413,5 @@
> +    while (accounts.hasMoreElements()) {
> +      let account = accounts.getNext();
> +      if (account.normalizedName == aConversation.account.normalizedName &&
> +          account.protocol.normalizedName == aConversation.account.protocol.name) {
> +        let accountBuddy =

Add a comment above this, like
// Find and display the contact for this log.

@@ +418,5 @@
> +          imServices.contacts
> +                    .getAccountBuddyByNameAndAccount(aConversation.normalizedName,
> +                                                     account);
> +        // Display information for the chatroom
> +        if (aConversation.isChat) {

Move this above the getAccountBuddy... call, which will fail for chatrooms.

Comment 16

[aleth [:aleth]]

With this bug fixed, there will be an UX issue: The sidebar of search result tabs will look a lot like sidebar for normal conversations, and you can only tell you are looking at search results if you look at the pane name or the tab title.

Maybe the "Previous Conversations" string above the log list should change to "Conversations containing '<search string>'" for search result conversations, to make clear the log list is a subset of the full log list?

Comment 17

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #16)
> Maybe the "Previous Conversations" string above the log list should change
> to "Conversations containing '<search string>'" for search result
> conversations, to make clear the log list is a subset of the full log list?

Actually this isn't true - the log list is always complete, it's just that the log with the search result hit is selected. So that suggestion doesn't work.

So my concern in comment 16 is a non-problem, all that's needed is something like a "open conversation with this contact" button as suggested in comment 10.

Comment 18

[Thomasy]

(In reply to aleth [:aleth] from comment #12)
> (In reply to Thomasy from comment #10)
> > For WIP v2, I also found other bug and enhancement. I will file other bugs.
> 
> Please do!
> There is also a known backlog of bugs that have been fixed for Instantbird
> but not ported to Thunderbird yet, if you are interested in helping to fix
> those let us know.
Please advise. I will take the bug if I can help.
>  
> > >Bugs
> > >** Remove highlight bar upon switching conversations
> 
> I'm not sure what you mean by this.
> 
I mean the findbar at the bottom of the logDisplayBrowserBox as bug 1202942 attachment 8658480 [details]
> > >** The protocol icon will not change upon switching conversations to search result from a conversation.
> 
> Let's fix this in this bug, it's part of displaying the correct information
> at the top of the sidebar.
> 
> The little MUC icon next to the status message (where the status icon is for
> normal conversations) is also missing for search results from chatrooms.

Comment 19

[Thomasy]

Attachment: bug1199668_WIP_v4.patch

Comment 20

[Thomasy]

(In reply to aleth [:aleth] from comment #15)
> Comment on attachment 8658328 [details] [diff] [review]
> bug1199668_WIP_v3.patch
> 
> Review of attachment 8658328 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The prplIcon attribute is currently not removed in onListItemSelected,
> leading to the occasional bug when the icon from some previous conversation
> is displayed.

I test a little bit. For the time being, add remove prplIcon do not make difference in my simple test case (just switch between offline contact, online contact, converstations). I think add it or not is okay.

@@ -597,16 +629,17 @@ var chatHandler = {

       cti.removeAttribute("userIcon");
+      cti.removeAttribute("prplIcon");
       cti.removeAttribute("statusMessageWithDash");

> 
> ::: mail/components/im/content/chat-messenger-overlay.js
> @@ +407,5 @@
> >      // Conversation title may not be set yet if this is a search result.
> >      let cti = document.getElementById("conv-top-info");
> >      cti.setAttribute("displayName", aConversation.title);
> > +
> > +    // Find the account for this log
> 
> Nit: For new comments, we generally use full sentences, so add a "." at the
> end.
> 
> @@ +413,5 @@
> > +    while (accounts.hasMoreElements()) {
> > +      let account = accounts.getNext();
> > +      if (account.normalizedName == aConversation.account.normalizedName &&
> > +          account.protocol.normalizedName == aConversation.account.protocol.name) {
> > +        let accountBuddy =
> 
> Add a comment above this, like
> // Find and display the contact for this log.
> 
> @@ +418,5 @@
> > +          imServices.contacts
> > +                    .getAccountBuddyByNameAndAccount(aConversation.normalizedName,
> > +                                                     account);
> > +        // Display information for the chatroom
> > +        if (aConversation.isChat) {
> 
> Move this above the getAccountBuddy... call, which will fail for chatrooms.

Comment 21

[aleth [:aleth]]

Comment on attachment 8658490 [details] [diff] [review]
bug1199668_WIP_v4.patch

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

(In reply to Thomasy from comment #20)
> 
>        cti.removeAttribute("userIcon");
> +      cti.removeAttribute("prplIcon");
>        cti.removeAttribute("statusMessageWithDash");

It's needed for the (unlikely) edge case where we can't find the account, maybe because it has been deleted.

Comment 22

[aleth [:aleth]]

(In reply to Thomasy from comment #18)
> > There is also a known backlog of bugs that have been fixed for Instantbird
> > but not ported to Thunderbird yet, if you are interested in helping to fix
> > those let us know.
> Please advise. I will take the bug if I can help.

Basically, some UI files are forked and the upstream fixes that have accumulated need to be ported with appropriate changes.

A good place to start would be to compare im/content/conversation.xml with mail/.../imconversation.xml. hg blame should identify the context.

The main thing to watch out for in porting is that the participant list is not contained in the conversation binding in TB.

(In reply to aleth [:aleth] from comment #21)
> It's needed for the (unlikely) edge case where we can't find the account,

(or the contact)

Comment 23

[Thomasy]

Attachment: bug1199668_WIP_v5.patch

Comment 24

[aleth [:aleth]]

Comment on attachment 8658815 [details] [diff] [review]
bug1199668_WIP_v5.patch

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

Thanks for fixing this!

Comment 25

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/ecfa2567b7d89b8c3c8b8c21128288efb6f29fe7
Bug 1199668 - Make contact status and icon show for search result conversations. r=aleth