987577 - Improve the way whois idleTime is displayed

Status: RESOLVED FIXED

(Chat Core :: IRC, enhancement)

Description

[aleth [:aleth]]

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.

Comment 1

[aleth [:aleth]]

Attachment: 987577.patch

I tried this and it's a definite improvement.

Comment 2

[Patrick Cloke [: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.

Comment 3

[Florian Quèze [:florian]]

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.

Comment 4

[aleth [:aleth]]

Attachment: 987577.patch 2

>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.

Comment 5

[Patrick Cloke [: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 6

[Patrick Cloke [:clokep]]

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. :(

Comment 7

[aleth [:aleth]]

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