Closed Bug 955698 Opened 6 years ago Closed 6 years ago

Participant tooltips should set presence info

Categories

(Instantbird :: Conversation, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

Details

Attachments

(1 file, 3 obsolete files)

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

At least for participants that are contacts.
Assignee: nobody → aleth
Depends on: 955553
Attached patch Patch (obsolete) — Splinter Review
*** 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.
Attachment #8354828 - Flags: review?(clokep)
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 ***
Attachment #8354828 - Flags: review?(florian)
*** 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.)
*** 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?
*** 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 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.
Attachment #8354828 - Flags: review?(florian) → review-
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 ***
Attachment #8354828 - Flags: review?(clokep)
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8354828 - Attachment is obsolete: true
Attachment #8362249 - Flags: review?(clokep)
Attachment #8362249 - Flags: review?(florian)
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!
Attachment #8362249 - Flags: review?(clokep) → review-
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.
Attachment #8362249 - Flags: review?(florian) → review-
Attached patch Patch 3 (obsolete) — Splinter Review
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.
Attachment #8394688 - Flags: review?(florian)
I think I would prefer it the parseInt call was done by the caller.
Attached patch Patch 4Splinter Review
Done.
Attachment #8394722 - Flags: review?(florian)
Attachment #8394688 - Attachment is obsolete: true
Attachment #8394688 - Flags: review?(florian)
Comment on attachment 8394722 [details] [diff] [review]
Patch 4

Thanks.
Attachment #8394722 - Flags: review?(florian) → review+
Attachment #8362249 - Attachment is obsolete: true
Attachment #8394722 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/c288b8a5f6dc
Status: NEW → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6
You need to log in before you can comment on or make changes to this bug.