Improve the way whois idleTime is displayed

RESOLVED FIXED in 1.6

Status

enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aleth, Assigned: aleth)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

5 years ago
Two good ideas from flo:
* Improve the en-US string from "Idle for" to the clearer "Last activity"
* Use the idle status after a nick has been idle for a long time.
Assignee

Comment 1

5 years ago
Posted patch 987577.patch (obsolete) — Splinter Review
I tried this and it's a definite improvement.
Attachment #8396267 - Flags: review?(clokep)
Comment on attachment 8396267 [details] [diff] [review]
987577.patch

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

::: chat/locales/en-US/irc.properties
@@ +185,5 @@
>  # The away message of the user
>  tooltip.away=Away
>  tooltip.ircOp=IRC Operator
>  tooltip.bot=Bot
> +tooltip.idleTime=Last activity

You need to bounce the string constant to something else.

@@ +186,5 @@
>  tooltip.away=Away
>  tooltip.ircOp=IRC Operator
>  tooltip.bot=Bot
> +tooltip.idleTime=Last activity
> +# %S is the timespan elapsed since the last activity.

Florian should look over this description and see if it's detailed enough.

::: chat/protocols/irc/irc.js
@@ +1036,5 @@
>  
> +    // Convert timespan in seconds into a human-readable form.
> +    let normalizeTime = function(aTime) {
> +      let valuesAndUnits = DownloadUtils.convertTimeUnits(aTime);
> +      if (!valuesAndUnits[2])

I just had to look up what this is doing again, so while we're updating this, let's add a comment: "If the time is exact to the first set of units, trim off the empty ones." or something that makes sense.

Actually, is this line even necessary or will they just be joined as ""?

@@ +1068,5 @@
>          tooltipInfo.push(new TooltipInfo(_("tooltip." + field), value));
>        }
>      }
>  
> +    const kSetIdleStatusAfterSeconds = 3600;

I wonder if this sohuld be at the top of the file or not.

@@ +1079,5 @@
>      else if ("offline" in whoisInformation)
>        statusType = Ci.imIStatusInfo.STATUS_OFFLINE;
> +    else if ("idleTime" in whoisInformation &&
> +             whoisInformation["idleTime"] > kSetIdleStatusAfterSeconds)
> +      statusType = Ci.imIStatusInfo.STATUS_IDLE;

How will this interact with bug 955698 that assumes STATUS_AWAY is the only other status?

::: chat/protocols/irc/ircBase.jsm
@@ +737,2 @@
>        return this.setWhois(aMessage.params[1],
> +                           {idleTime: parseInt(aMessage.params[2])});

This is totally how we should be storing this, by the way, as the raw data and modifying it on display. Good change.
Attachment #8396267 - Flags: review?(clokep) → review-
Comment on attachment 8396267 [details] [diff] [review]
987577.patch

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

::: chat/protocols/irc/irc.js
@@ +1068,5 @@
>          tooltipInfo.push(new TooltipInfo(_("tooltip." + field), value));
>        }
>      }
>  
> +    const kSetIdleStatusAfterSeconds = 3600;

I wonder if it's OK to have this value hard coded or if it should re-use the preference we have in the Privacy tab for the time before the user is shown as idle.

These two values are related, but are also arguably different because when we send the idleness it's about the user not touching keyboard/mouse, and the idleness on IRC (or at least moznet) is the time since the last message sent.
Assignee

Comment 4

5 years ago
>Actually, is this line even necessary or will they just be joined as ""?

See the improved comment ;)

> +    const kSetIdleStatusAfterSeconds = 3600;
> I wonder if this sohuld be at the top of the file or not.

It's easier to find here, next to where it is used.

> +    else if ("idleTime" in whoisInformation &&
> +             whoisInformation["idleTime"] > kSetIdleStatusAfterSeconds)
> +      statusType = Ci.imIStatusInfo.STATUS_IDLE;
>
>How will this interact with bug 955698 that assumes STATUS_AWAY is the only >other status?

AWAY is the only status for which we have a status message in IRC. So it's got nothing to do with this, which only affects AVAILABLE.

>I wonder if it's OK to have this value hard coded or if it should re-use the
>preference we have in the Privacy tab for the time before the user is shown as
>idle.

The main reason not to reuse that pref is that the Privacy default of 5 minutes would be really annoying, which reflects the difference you point out.
Attachment #8396267 - Attachment is obsolete: true
Attachment #8396306 - Flags: review?(clokep)
(In reply to aleth from comment #4)
> >I wonder if it's OK to have this value hard coded or if it should re-use the
> >preference we have in the Privacy tab for the time before the user is shown as
> >idle.
> 
> The main reason not to reuse that pref is that the Privacy default of 5
> minutes would be really annoying, which reflects the difference you point
> out.

I think this is unrelated, some users might turn this up as a "privacy" thing, but want to know when other users are "idle" ASAP.
Comment on attachment 8396306 [details] [diff] [review]
987577.patch 2

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

::: chat/protocols/irc/ircBase.jsm
@@ +737,2 @@
>        return this.setWhois(aMessage.params[1],
> +                           {lastActivity: parseInt(aMessage.params[2])});

I dislike changing this from idleTime, but I guess it needs to match what's in irc.properties. :(
Attachment #8396306 - Flags: review?(clokep) → review+
Assignee

Comment 7

5 years ago
https://hg.mozilla.org/comm-central/rev/c360b8faf0d4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.6
You need to log in before you can comment on or make changes to this bug.