1213908 - Add user icons to prplIChatBuddies and their tooltips

Status: RESOLVED FIXED

(Chat Core :: General, defect)

Comment 1

[aleth [:aleth]]

The icon itself should probably be made available through a property of the prplIConvChatBuddy.

Comment 2

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: Add support for setting buddyIcon and getting participant with nick

Comment 3

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: XPCOM for libpurple

Comment 4

[aleth [:aleth]]

Comment on attachment 8673710 [details] [diff] [review]
XPCOM for libpurple

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

::: purplexpcom/src/purpleConvChat.cpp
@@ +85,5 @@
> +
> +  purpleConvChatBuddy *participant = new purpleConvChatBuddy();
> +  participant->Init(purple_conv_chat_cb_find(chat,
> +                                             PromiseFlatCString(aNick).get()));
> +  NS_ADDREF(*aResult = static_cast<prplIConvChatBuddy *>(participant));

Did you check this all works as expected if no chat buddy is found for the nick?

::: purplexpcom/src/purpleConvChatBuddy.cpp
@@ +60,5 @@
> +/* readonly attribute AUTF8String buddyIconFilename; */
> +NS_IMETHODIMP
> +purpleConvChatBuddy::GetBuddyIconFilename(nsACString& aBuddyIconFilename)
> +{
> +  aBuddyIconFilename = nullptr;

Please add a comment here like "not supported by libpurple".

Shouldn't this return an empty string, or is there a good reason why this works?

Comment 5

[aleth [:aleth]]

Comment on attachment 8673708 [details] [diff] [review]
Add support for setting buddyIcon and getting participant with nick

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

This looks good! Just a few nits.

I'll hold off reviewing this until you add another patch with the implementation for XMPP or Twitter, so that it can be tested.

::: chat/components/public/prplIConversation.idl
@@ +128,3 @@
>  interface prplIConvChat: prplIConversation {
>  
> +  /* Get an object of prplIConvChatBuddy if a participant with aNick exists */

/* Get the prplIConvChatBuddy if a participant with name aName exists */

@@ +128,4 @@
>  interface prplIConvChat: prplIConversation {
>  
> +  /* Get an object of prplIConvChatBuddy if a participant with aNick exists */
> +  prplIConvChatBuddy getParticipant(in AUTF8String aNick);

aName (for consistency with the property name in prplIConvChatBuddy)

::: chat/components/public/prplITooltipInfo.idl
@@ +20,5 @@
>    readonly attribute short type;
>  
>    /*
>     * When type == status, the label holds the statusType (a short
>     * converted to a string), while the value holds the statusText.

Expand this comment to explain where and what the data is if type == icon.

::: chat/content/imtooltip.xml
@@ +352,5 @@
>         <![CDATA[
>           if (!aConv.target)
>             return false;     // We're viewing a log.
>  
> +         let participant = aConv.target.getParticipant(aNick);

How about giving this method an optional third parameter (aParticipant) that callers can use if they already have the participant, so you don't have to look it up when it's not needed?

@@ +354,5 @@
>             return false;     // We're viewing a log.
>  
> +         let participant = aConv.target.getParticipant(aNick);
> +         if (!participant)
> +           return false;     // This is not participant in MUC.

That's not the desired behaviour: if a nick is no longer in the room, we still show the tooltip. E.g. on IRC this provides WHOWAS information, and shows the status.

::: chat/modules/jsProtoHelper.jsm
@@ +700,4 @@
>    __proto__: ClassInfo("prplIConvChatBuddy", "generic ConvChatBuddy object"),
>  
>    _name: "",
>    get name() this._name,

Please add { ... } here while you are at it (it must have been missed when expression closures were removed)

@@ +700,5 @@
>    __proto__: ClassInfo("prplIConvChatBuddy", "generic ConvChatBuddy object"),
>  
>    _name: "",
>    get name() this._name,
>    set name(aName) this._name = aName,

Here too

Comment 6

[aleth [:aleth]]

Attachment: Twitter WIP

Here's a WIP of the required changes for twitter.

Comment 7

[aleth [:aleth]]

Attachment: Twitter WIP v2

Comment 8

[aleth [:aleth]]

Comment on attachment 8673708 [details] [diff] [review]
Add support for setting buddyIcon and getting participant with nick

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

::: chat/modules/jsProtoHelper.jsm
@@ +708,5 @@
> +  _buddyIconFileName: "",
> +  get buddyIconFilename() { return this._buddyIconFileName; },
> +  set buddyIconFilename(aNewFileName) {
> +    this._buddyIconFileName = aNewFileName;
> +  },

Looks like you don't need the getter/setter. A normal property (buddyIconFilename: "") should be enough.

Comment 9

[aleth [:aleth]]

Comment on attachment 8673708 [details] [diff] [review]
Add support for setting buddyIcon and getting participant with nick

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

::: chat/modules/jsProtoHelper.jsm
@@ +728,5 @@
>      this.type = Ci.prplITooltipInfo.status;
>      this.label = aLabel.toString();
>      this.value = aValue || "";
>    }
> +  else if (aLabel == Ci.prplITooltipInfo.icon) {

It's not great to use aLabel to set the type. The way the parameters are interpreted is already too overloaded.

Unfortunately whatever way you clean up the initialization parameters will mean having to check existing callers and modifying them appropriately.

It looks like most callers use two parameters (pair). So to avoid having to change too many callers, how about something like
function TooltipInfo(aLabel, aValue, aType = Ci.prplpITooltipInfo.pair)

It's unfortunate as this will lead to some callers having to pass undefined parameters, so if you have a better idea...

Comment 10

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 2 - Add support for buddyIcon

Comment 11

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 2 - XPCOM for libpurple

(In reply to aleth [:aleth] from comment #4)
> Did you check this all works as expected if no chat buddy is found for the
> nick?
No, this caused a problem and I handled that in this patch.

> Shouldn't this return an empty string, or is there a good reason why this
> works?
Yes, It should return an empty string, the previous way was the same, but I updated it to be clear in this patch.

Comment 12

[aleth [:aleth]]

Comment on attachment 8678260 [details] [diff] [review]
rev 2 - Add support for buddyIcon

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

::: chat/components/public/prplIConversation.idl
@@ +112,5 @@
> +
> +  /* Indicates if this chat buddy corresponds to a buddy in our buddy list */
> +  readonly attribute boolean buddy;
> +
> +  readonly attribute AUTF8String buddyIconURI;

Let's call this buddyIconFilename for consistency (even though buddyIconURI is more correct, it's better to avoid confusion). If you like, you can mention URIs are allowed in a comment, or file a separate bug that renames buddyIconFilename everywhere.

::: chat/content/imtooltip.xml
@@ +438,5 @@
>             let contactlistbox = document.getElementById("contactlistbox");
>             let conv = contactlistbox.selectedItem.conv;
> +           let participant = conv.target.getParticipant(elt.chatBuddy.name);
> +           return updateTooltipFromParticipant(elt.chatBuddy.name, conv,
> +                                               participant);

You don't need 'participant', just use elt.chatBuddy

@@ +460,5 @@
>           if (localName == "listitem") {
>             let conv = document.getBindingParent(elt).conv;
> +           let participant = conv.target.getParticipant(elt.chatBuddy.name);
> +           return updateTooltipFromParticipant(elt.chatBuddy.name, conv,
> +                                               participant);

same here

@@ +468,5 @@
>           let classList = elt.classList;
>           if (classList.contains("ib-nick") ||
>               classList.contains("ib-sender")) {
>             let conv = getBrowser()._conv;
> +           let participant = conv.target.getParticipant(elt.textContent);

Move this line to updateTooltipFromParticipant and use it if the aParticipant parameter is undefined.

Putting here you'd also need to check conv.target exists etc.

::: chat/modules/jsProtoHelper.jsm
@@ +721,1 @@
>  {

Nit: Move the { to the end of the previous line, to be consistent with the rest of the file.

@@ +731,1 @@
>    else if (aLabel === undefined)

|| aType == Ci.prplITooltipInfo.sectionBreak

::: chat/protocols/xmpp/xmpp.jsm
@@ +1313,5 @@
>            tooltipInfo.push(new TooltipInfo(_("tooltip." + field), vCardInfo[field]));
>        }
> +      if (vCardInfo.photo) {
> +        tooltipInfo.push(new TooltipInfo(null, this.getPhotoURI(vCardInfo.photo),
> +                                         Ci.prplITooltipInfo.icon));

Store the photo URI in participant.buddyIconFilename. This will reduce flicker if the user checks the tooltip for the same participant a second time.

Since I added the possibility of updating tooltips, it would also make sense to cache the whole vcard once we have it (maybe in parsed form). When requestBuddyInfo is called, it could notifyObservers with the cached data, and then again after the fresh vcard has arrived in the callback.

Comment 13

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 3 - XPCOM for libpurple

Changing |GetBuddyIconURI| to |GetBuddyIconFilename|

Comment 14

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 3 - Add support for buddyIcon

(In reply to aleth [:aleth] from comment #12)
> Let's call this buddyIconFilename for consistency (even though buddyIconURI
> is more correct, it's better to avoid confusion). If you like, you can
> mention URIs are allowed in a comment, or file a separate bug that renames
> buddyIconFilename everywhere.

Let it be in a separate bug.

> Since I added the possibility of updating tooltips, it would also make sense
> to cache the whole vcard once we have it (maybe in parsed form). When
> requestBuddyInfo is called, it could notifyObservers with the cached data,
> and then again after the fresh vcard has arrived in the callback.

Yes, we would use this advantage for improving XMPP requests with tooltips in general.
I will handle this in a separate bug to keep this bug focused on icons stuff.

Comment 15

[aleth [:aleth]]

Comment on attachment 8678510 [details] [diff] [review]
rev 3 - Add support for buddyIcon

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

> Yes, we would use this advantage for improving XMPP requests with tooltips
> in general.
> I will handle this in a separate bug to keep this bug focused on icons stuff.

OK, that should also tidy up the code further.

::: chat/content/imtooltip.xml
@@ +353,5 @@
>         <![CDATA[
>           if (!aConv.target)
>             return false;     // We're viewing a log.
> +         if (!aParticipant)
> +           aParticipant = aConv.target.getParticipant(aNick);

In this case it's OK as you only do it if it's undefined, but generally we avoid changing the value of arguments as objects in JS are passed by reference and not as a copy.

::: chat/modules/jsProtoHelper.jsm
@@ +718,5 @@
>  };
>  
> +function TooltipInfo(aLabel, aValue, aType = Ci.prplITooltipInfo.pair) {
> +  if (aType == Ci.prplITooltipInfo.status) {
> +    this.type = aType;

Let's simplify this function a bit by removing the this.type duplication:

this.type = aType;
if (aType == Ci.prplITooltipInfo.status) ...
else if ...
else ...

::: chat/protocols/xmpp/xmpp.jsm
@@ +1321,5 @@
> +        let dataURI = this.getPhotoURI(vCardInfo.photo);
> +
> +        // Store the photo URI for this participant.
> +        if (muc && muc._participants.has(jid.resource)) {
> +          let participant = muc._participants.get(jid.resource);

Isn't 
if (participant) participant.buddyIconFilename = dataURI;
enough?
(You may have to move the variable declaration ("let participant;") outside the if clause so it's in scope).

Comment 16

[aleth [:aleth]]

Also note your patch does not apply cleanly as it's been bitrotted by some c-c changes.

Comment 17

[aleth [:aleth]]

Attachment: Participant icons for twitter

Comment 18

[Patrick Cloke [:clokep]]

Comment on attachment 8678512 [details] [diff] [review]
Participant icons for twitter

Looks pretty reasonable to me. Lets see what Abdelrhman thinks.

Comment 19

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 4 - Add support for buddyIcon

Comment 20

[aleth [:aleth]]

Comment on attachment 8678594 [details] [diff] [review]
rev 4 - Add support for buddyIcon

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +1325,5 @@
> +      if (vCardInfo.photo) {
> +        let dataURI = this.getPhotoURI(vCardInfo.photo);
> +
> +        // Store the photo URI for this participant.
> +        if (participant) participant.buddyIconFilename = dataURI;

Nit: put the participant.buddyIconFilename =... on a separate line.

@@ +1336,5 @@
>    },
>  
> +  // Parses the photo node of a received vCard if exists and returns string of
> +  // data URI, otherwise returns null.
> +  getPhotoURI: function(aPhotoNode) {

Nit: it might make sense to call this _getPhotoURI if it's just a helper function.

Comment 21

[aleth [:aleth]]

Comment on attachment 8678594 [details] [diff] [review]
rev 4 - Add support for buddyIcon

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

Not sure if interface changes and purple changes still require r+florian, but here goes!

Comment 22

[Abdelrahman Ahmed [:abdelrahman]]

Comment on attachment 8678512 [details] [diff] [review]
Participant icons for twitter

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

::: chat/protocols/twitter/twitter.js
@@ +29,5 @@
> +  __proto__: GenericConvChatBuddyPrototype,
> +  get buddyIconFilename() {
> +    let userInfo = this._account._userInfo.get(this.name);
> +    if (userInfo)
> +      this._buddyIconFilename = userInfo.profile_image_url;

I'm not sure of using |_buddyIconFilename| as I use |buddyIconFilename| property.
(may be we need to use _buddyIconFilename as a property with setter and getter in jsProtoHelper)

Comment 23

[aleth [:aleth]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #22)
> I'm not sure of using |_buddyIconFilename| as I use |buddyIconFilename|
> property.
> (may be we need to use _buddyIconFilename as a property with setter and
> getter in jsProtoHelper)

No, you're right! This needs changing to match the changes in your code.

Comment 24

[aleth [:aleth]]

Attachment: Participant icons for twitter, v2

Comment 25

[Florian Quèze [:florian]]

Comment on attachment 8678594 [details] [diff] [review]
rev 4 - Add support for buddyIcon

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

Looks reasonable (I looked mostly at the API changes, not at the JS code), thanks!

::: chat/components/public/prplIConversation.idl
@@ +112,5 @@
> +
> +  /* Indicates if this chat buddy corresponds to a buddy in our buddy list */
> +  readonly attribute boolean buddy;
> +
> +  readonly attribute AUTF8String buddyIconFilename;

This could do with a comment documenting expectations here. From looking at the code, this is actually expected to be an URI, not really a 'filename'.

@@ +128,3 @@
>  interface prplIConvChat: prplIConversation {
>  
> +  /* Get the prplIConvChatBuddy if a participant with name aName exists */

This comment could be improved.
- what's aName? Is it expected to be already normalized? Can it be whatever?
- what if a participant with name aName doesn't exist? Will this throw? Return null?

Comment 26

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 5 - Add support for buddyIcon

Comment 27

[Patrick Cloke [:clokep]]

Comment on attachment 8679134 [details] [diff] [review]
rev 5 - Add support for buddyIcon

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

::: chat/components/public/prplIConversation.idl
@@ +101,5 @@
>  };
>  
> +/* This represents a participant in a chat room */
> +[scriptable, uuid(b0e9177b-40f6-420b-9918-04bbbb9ce44f)]
> +interface prplIConvChatBuddy: nsISupports {

Why was this moved? It makes the diff confusing.

@@ +113,5 @@
> +  /* Indicates if this chat buddy corresponds to a buddy in our buddy list */
> +  readonly attribute boolean buddy;
> +
> +  /* Data URI of icon for the buddy */
> +  readonly attribute AUTF8String buddyIconFilename;

I think Florian wanted this changed to Uri (as do I).

::: chat/components/public/prplITooltipInfo.idl
@@ +21,5 @@
>  
>    /*
>     * When type == status, the label holds the statusType (a short
>     * converted to a string), while the value holds the statusText.
> +   * When type == icon, the value holds the data URI.

I don't think this is necessarily a *data* URI.

Comment 28

[Patrick Cloke [:clokep]]

Comment on attachment 8678600 [details] [diff] [review]
Participant icons for twitter, v2

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

r+ Assuming we keep calling it buddyIconFilename.

::: chat/protocols/twitter/twitter.js
@@ +1070,5 @@
>            value = kFields[field](value);
>          tooltipInfo.push(new TooltipInfo(_("tooltip." + field), value));
>        }
>      }
> +    tooltipInfo.push(new TooltipInfo(null, userInfo.profile_image_url,

Can userInfo.profile_image_url be undefined/null/empty?

Comment 29

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to Patrick Cloke [:clokep] from comment #27)
> Why was this moved? It makes the diff confusing.
To allow us to use |prplIConvChatBuddy| in |prplIConvChat| interface as it needs to be defined before using it.

> I think Florian wanted this changed to Uri (as do I).
(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #14)
> (In reply to aleth [:aleth] from comment #12)
> > Let's call this buddyIconFilename for consistency (even though buddyIconURI
> > is more correct, it's better to avoid confusion). If you like, you can
> > mention URIs are allowed in a comment, or file a separate bug that renames
> > buddyIconFilename everywhere.
> 
> Let it be in a separate bug.

Comment 30

[Florian Quèze [:florian]]

(In reply to Patrick Cloke [:clokep] from comment #27)

> @@ +113,5 @@
> > +  /* Indicates if this chat buddy corresponds to a buddy in our buddy list */
> > +  readonly attribute boolean buddy;
> > +
> > +  /* Data URI of icon for the buddy */
> > +  readonly attribute AUTF8String buddyIconFilename;
> 
> I think Florian wanted this changed to Uri (as do I).

No, I prefer consistency, and to avoid unnessary API changes. I was just suggesting we add a comment to avoid confusion.

Comment 31

[aleth [:aleth]]

(In reply to Patrick Cloke [:clokep] from comment #28)
> Comment on attachment 8678600 [details] [diff] [review]
> Participant icons for twitter, v2
> 
> Review of attachment 8678600 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ Assuming we keep calling it buddyIconFilename.
> 
> ::: chat/protocols/twitter/twitter.js
> @@ +1070,5 @@
> >            value = kFields[field](value);
> >          tooltipInfo.push(new TooltipInfo(_("tooltip." + field), value));
> >        }
> >      }
> > +    tooltipInfo.push(new TooltipInfo(null, userInfo.profile_image_url,
> 
> Can userInfo.profile_image_url be undefined/null/empty?

No, it's always set, but if there were some twitter error and it was undefined, it won't break the code anyway.

Comment 32

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/3231b63d0267c8e5e1a4720157c7dbfcf17e4537
Bug 1213908 - Add support for setting buddyIcon and getting participant with nick in MUCs. r=aleth,florian

https://hg.mozilla.org/comm-central/rev/29fdfd7d6ee1363a40760fa3f6edfa18fa345401
Bug 1213908 - Add user icons to twitter participants. r=clokep

Comment 33

[aleth [:aleth]]

https://hg.mozilla.org/users/florian_queze.net/purple/rev/d18b0bddc4f8

Comment 34

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/290ae28c7e983753e2c0e9cceb124ae0b3207996
Bug 1213908 - Add user icons to prplIChatBuddies and their tooltips: followup to fix comment. rs,a=aleth CLOSED TREE

Comment 35

[aleth [:aleth]]

"Data URI" references fixed on checkin (I missed one on the first push, unfortunately.)

Comment 36

[aleth [:aleth]]

Attachment: Add user icon support, checked in version