955698 - Participant tooltips should set presence info

Status: RESOLVED FIXED

(Instantbird Graveyard :: Conversation, enhancement)

Description

[aleth [:aleth]]

*** Original post on bio 2250 at 2013-11-12 11:53:00 UTC ***

At least for participants that are contacts.

Comment 1

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 2250 as attmnt 3047 at 2013-11-13 10:53:00 UTC ***

This will fetch the status from the contact whenever possible, and uses the "status" entry of the tooltip as a source for status information if not. It adds such an entry for IRC. The status message in the tooltip is made consistent with what we have in the conversation top.

Comment 2

[aleth [:aleth]]

Comment on attachment 8354828 [details] [diff] [review]
Patch

*** Original change on bio 2250 attmnt 3047 at 2013-11-13 12:04:36 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 3

[Patrick Cloke [:clokep]]

*** Original post on bio 2250 at 2013-11-13 15:22:09 UTC ***

Comment on attachment 8354828 [details] [diff] [review] (bio-attmnt 3047)
Patch

>diff --git a/instantbird/content/buddytooltip.xml b/instantbird/content/buddytooltip.xml
>@@ -197,39 +195,35 @@
>+         this.setAttribute("status", Status.toAttribute(aBuddy.statusType));
>+         let statusMessage = Status.toLabel(aBuddy.statusType);
>+         let statusText = aBuddy.statusText;
>+         if (statusText)
>+           statusMessage += " - " + statusText;
This doesn't look very localizable, should it be? (I also wonder if we do this in another place and if this should be part of the Status util thing).

>@@ -254,36 +248,45 @@
>          while (aTooltipInfo.hasMoreElements()) {
>            var elt =
>              aTooltipInfo.getNext().QueryInterface(Ci.prplITooltipInfo);
>            switch (elt.type) {
>              case Ci.prplITooltipInfo.pair:
>              case Ci.prplITooltipInfo.sectionHeader:
>+               if (elt.label.toLowerCase() == "status") {
>+                 // We don't want to add a row for a status entry
>+                 // as we already display that separately.
>+                 if (this.getAttribute("status") == "unknown") {
>+                   // Use the data if we don't already know the status
>+                   // (i.e. if this is not for a buddy).
>+                   this.setAttribute("status", elt.value);
>+                   this.setMessage(Status.toLabel(elt.value));
>+                 }
>+                 break;
Shouldn't this be continue, not break?

>diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js
Do we cache any of this data? (I don't think we do.)

Comment 4

[aleth [:aleth]]

*** Original post on bio 2250 at 2013-11-13 15:57:44 UTC ***

(In reply to comment #2)
> >diff --git a/instantbird/content/buddytooltip.xml b/instantbird/content/buddytooltip.xml
> >@@ -197,39 +195,35 @@
> >+         this.setAttribute("status", Status.toAttribute(aBuddy.statusType));
> >+         let statusMessage = Status.toLabel(aBuddy.statusType);
> >+         let statusText = aBuddy.statusText;
> >+         if (statusText)
> >+           statusMessage += " - " + statusText;
> This doesn't look very localizable, should it be? (I also wonder if we do this
> in another place and if this should be part of the Status util thing).

It is in fact essentially duplicated code that is already duplicated in at least two places. I agree it should be an API call, but on buddies rather than in Status, as Status doesn't know about status messages. Adding that API should be a separate bug though.

> >+                 break;
> Shouldn't this be continue, not break?

Good catch. Wasn't noticeable as 'status' is added last ;)

> >diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js
> Do we cache any of this data? (I don't think we do.)

Why do you ask?

Comment 5

[aleth [:aleth]]

*** Original post on bio 2250 at 2013-11-13 16:01:54 UTC ***

(In reply to comment #3)
> > >+                 break;
> > Shouldn't this be continue, not break?
> 
> Good catch. Wasn't noticeable as 'status' is added last ;)

Gah, you confused me ;) The break is from a switch statement, not from the loop.

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8354828 [details] [diff] [review]
Patch

*** Original change on bio 2250 attmnt 3047 at 2013-11-13 19:42:25 UTC ***

>diff --git a/instantbird/content/buddytooltip.xml b/instantbird/content/buddytooltip.xml

>            switch (elt.type) {
>              case Ci.prplITooltipInfo.pair:
>              case Ci.prplITooltipInfo.sectionHeader:
>+               if (elt.label.toLowerCase() == "status") {
>+                 // We don't want to add a row for a status entry
>+                 // as we already display that separately.
>+                 if (this.getAttribute("status") == "unknown") {
>+                   // Use the data if we don't already know the status
>+                   // (i.e. if this is not for a buddy).
>+                   this.setAttribute("status", elt.value);
>+                   this.setMessage(Status.toLabel(elt.value));
>+                 }
>+                 break;

I dislike this hack in general, and even more because if we don't land something in mail/ at the same time as we merge chat/, these "status" items that aren't localized will be visible in the Tb UI.

Suggestion: add one (statusType) or two (+statusText) new possible values for elt.type.

Comment 7

[aleth [:aleth]]

Comment on attachment 8354828 [details] [diff] [review]
Patch

*** Original change on bio 2250 attmnt 3047 at 2013-11-14 23:10:55 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 8

[aleth [:aleth]]

Attachment: Patch

This adds a status type to prplITooltipInfo as suggested above. 

The status with status message display in the tooltip is now localized. Ideally conversation.xml would use the same to avoid duplication, but the way the dash is added in conversation.xml is a bit subtle so I avoided touching that code in this patch, which is long enough as it is.

The improved presence info won't work in private messages of course.

Comment 9

[Patrick Cloke [:clokep]]

Comment on attachment 8362249 [details] [diff] [review]
Patch

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

I didn't review buddytooltip.xml. Overall this change looks OK, but I'd really like Florian to look over it.

Shouldn't this also contain changes to xmpp.jsm? It seems like this would be the protocol that would most benefit from this change? Or maybe I'm misunderstanding what this change is trying to do.

::: chat/modules/imStatusUtils.jsm
@@ +23,3 @@
>  
> +  get bundle() this._bundle || (this._bundle =
> +    Services.strings.createBundle("chrome://chat/locale/status.properties")),

Maybe use l10nHelper here now?

@@ +26,4 @@
>    _labels: {},
> +  toLabel: function(aStatusType, aStatusText) {
> +    if (!(typeof aStatusType == "string") ||
> +        parseInt(aStatusType) == aStatusType)

I'd like a comment above this!

Comment 10

[Florian Quèze [:florian]]

Comment on attachment 8362249 [details] [diff] [review]
Patch

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

Looking forward to the next version :)

::: chat/modules/imStatusUtils.jsm
@@ +26,4 @@
>    _labels: {},
> +  toLabel: function(aStatusType, aStatusText) {
> +    if (!(typeof aStatusType == "string") ||
> +        parseInt(aStatusType) == aStatusType)

This is indeed quite obscure, I can't understand what it's trying to do.

::: im/content/buddytooltip.xml
@@ -231,5 @@
>           this.addRow(this.bundle.GetStringFromName("buddy.account"), account.name);
>  
> -         var loggedIn = aBuddy.loggedIn;
> -         if (loggedIn)
> -           this.addRow(this.bundle.GetStringFromName("buddy.loggedIn"), loggedIn);

Are you removing this because it's dead code?
I'm not against removing it (although I don't see how this change relates to the current patch) but if we remove it we should also remove the buddy.loggedIn localized string from buddytooltip.properties.

Comment 11

[aleth [:aleth]]

Attachment: Patch 3

The dead code removal should strictly speaking be another bug, but so should the tidying up of imStatusUtils, really. Neither are extensive enough for it to be worth the effort though I think.

Comment 12

[Florian Quèze [:florian]]

I think I would prefer it the parseInt call was done by the caller.

Comment 13

[aleth [:aleth]]

Attachment: Patch 4

Done.

Comment 14

[Florian Quèze [:florian]]

Comment on attachment 8394722 [details] [diff] [review]
Patch 4

Thanks.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/c288b8a5f6dc