394516 - Figure out a remaining-time rounding scheme for minutes -> hours/days

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect, P2)

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

Build ID: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a8pre) Gecko/2007083104 Minefield/3.0a8pre

Summary: Figure out a remaining-time rounding scheme for minutes -> hours/days

1. Download an ISO from ftp://fedora.mirror.iweb.ca/core/6/i386/iso (or any ISO)
2. Look at its progress, which says "210 minutes left..." (larger files / slower speeds will have much a much larger time remaining status)

Actual Results:

We always display the minutes raw, until we get down to 59 seconds, at which case we report it as seconds.

Expected Results:

We should figure out a time-rounding scheme such that raw minutes are rounded into hours (or even days, in the case of ISOs on dialup, gag).  Don't know what the cut off should be, ~90 minutes, in which case, 91 minutes would be 1 hour 31 minutes.

Comment 1

[Ed Lee :Mardak]

I patched/suggested 90 seconds at one point when redoing the time display, but that was a no go, so I doubt it'll work for deciding to show minutes/hours at 90.

I wonder if it'll be acceptable to show "5.1 minutes" "12.7 hours" "1.1 days"... basically for time units bigger than seconds, show into 1/10s for minutes, hours, days. Basically this would avoid doing "x minutes and x seconds" or "x hours and x minutes". There's only so much precision that one cares about when the units are so big.

Comment 2

[Ed Lee :Mardak]

Attachment: v1

Convert seconds into the appropriate unit resulting in a time with 1 decimal point if it's not seconds.

Since we're cleaning things up for downloads for big times, fix up the hysteresis to better handle big times. Also, instead of always trusting a downwards swing, do exponential smoothing.

Comment 3

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #1)

<snip>

> I wonder if it'll be acceptable to show "5.1 minutes" "12.7 hours" "1.1
> days"... basically for time units bigger than seconds, show into 1/10s for
> minutes, hours, days. Basically this would avoid doing "x minutes and x
> seconds" or "x hours and x minutes". There's only so much precision that one
> cares about when the units are so big.

I second that.  It'd be nice to see this in action (yes, yes, I should get a build env going).  Can you buildbot it? 

Comment 4

[Ed Lee :Mardak]

Patch depends on bug 394263 for the code refactoring.

(In reply to comment #3)
> It'd be nice to see this in action. Can you buildbot it? 
Hrmm.. never tried buildbot, but this will require several stacked patches to work. It requires an LDAP account? If not I suppose I could send my own os x 10.4 ppc mac build..

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 279499 [details] [diff] [review]
v1

>+/**
>+  * Converts a number of seconds to the appropriate unit that doesn't result in
>+  * a value less than 1. Any non-seconds value will have 1 digit after the dot.
>+  *
>+  * @return a pair: [new value, its unit]
>+  */
nit:
/**
 * ....

Comment 6

[Ed Lee :Mardak]

Attachment: v1.1

Comment 7

[Stephen Donner [:stephend] Not actively reading bugmail]

As long as this passed UI/UX review I'm fine with it...I think anything is a step forward from what we currently display for longer downloads.

Comment 8

[Ed Lee :Mardak]

(In reply to comment #3)
> It'd be nice to see this in action
If you've got a mac..
https://build.mozilla.org/tryserver-builds/[e-mail]
or linux..
https://build.mozilla.org/tryserver-builds/[e-mail]2
not sure what happened to windows :(

It also has patches for bug 385734, bug 394548, bug 394263, bug 394615, bug 394516, bug 394798, bug 223895, bug 393821

Comment 9

[Asaf Romano (gone)]

Comment on attachment 279529 [details] [diff] [review]
v1.1

you already have r= from the component owner, so just ui-r.

Comment 10

[Ed Lee :Mardak]

Attachment: v1.2

Comment 11

[Ed Lee :Mardak]

Attachment: v1.3

sdwilsh: Ugh, interdiff is killing me. The only difference between what you r+d and this is..

< +          if (lastSec != Infinity) {
---
> +          // Apply smoothing if the new time isn't a huge change (3x faster)
> +          // The 3x check is useful for downloads that start/resume slowly
> +          if (seconds * 3 > lastSec) {
64c68
< +              seconds = lastSec + (diff < 0 ? .3 : .1) * diff;;
---
> +              seconds = lastSec + (diff < 0 ? .3 : .1) * diff;


beltzner: There's 2 main things going on here: 1) time display for big times and 2) time smoothing for big times. Each has some details..

1.1) Seconds will be converted to minutes, hours, or days if it can show a time greater than "1.0" in the next unit.
1.1.1) This means we won't show "1.0 minutes" either, it'll be "60 seconds".
1.2) There is always 1 digit after the decimal except for seconds
1.3) As usual, we show "A few seconds left" for less than 4... 3.5 seconds.

2.1) Apply hysteresis for *both* upward and downward movement, so instead of completely trusting a shorter download time like we did before, we'll only trust 30% of it -- and 10% for upward. (Before we would eagerly take a downward motion, but that resulted in the download time wanting to move back up again.)
2.2) We reuse the old time if it's only somewhat longer. (This is what we're doing right now.) But that doesn't help large times, so let's compute a percent difference as well.
2.3) The 3x seconds is the newest addition that solves an issue I noticed when dogfooding. Sometimes downloads start verryy slowly like in the bytes/sec range, so this results in a HUGE time. But with the hysteresis, it'll only take 30% of the new value, so it would start off at 10 days then drop to 7 days and so on when it actually only needs 30 seconds. Basically 3x says, if the change was huge going faster, let's go with it; otherwise, do the smoothing.

So.. big question is where did these numbers come from?! ;) Well, I tried various combinations and even simulated what would go on for various sinusoidal download speeds. But I guess the only real good way is to have people try it out and see if it bothers them less than it did before.

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 280846 [details] [diff] [review]
v1.3

r=sdwilsh

Switching to faaborg for ui-r since I'm pretty sure he's doing the DM stuff now.

Comment 13

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 280846 [details] [diff] [review]
v1.3

Switching ui-r to Madhava, who is taking over download manager design work.

Comment 14

[Ed Lee :Mardak]

madhava: See comment #11 for the explanation of what the patch does.

Comment 15

[Ed Lee :Mardak]

Attachment: v1.3 unbitrot

Comment 16

[Shawn Wilsher :sdwilsh]

Edward - it's probably easier to get ui-r from a screenshot ;)

Comment 17

[Ed Lee :Mardak]

Attachment: screenshot of v1.3

Here's a screenshot of various times as specified below.. the download name is the number of seconds that's being displayed. Ignore the fact that they aren't actually downloading..

  let secs = [3.49, 3.5, // few sec <-> seconds
              59.49, 59.5, // seconds <-> minutes
              1.04*60, 1.05*60, // max seconds <-> min minutes
              1000, 3000, // random minutes
              59.49*60, 59.5*60, // minutes <-> hours
              1.04*60*60, 1.05*60*60, // max minutes <-> min hours
              5000, 50000, // random hours
              23.49*60*60, 23.5*60*60, // hours <-> days
              1.04*24*60*60, 1.05*24*60*60, // max hours <-> min days
              100000, 200000]; // random days

Comment 18

[Ed Lee :Mardak]

Attachment: v1.4

Oops. A couple issues with convertTimeUnits..

1) Couldn't show 59.9 minutes because it was checking 60-.5 which worked for seconds but not minutes (or 23.9 hours).
2) Wasn't bounds checking before accessing.

For 1, refactor the toFixed(unitIndex != 0 ? 1 : 0) into a helper [lambda] function.
For 2, reorder the while check.

Comment 19

[Madhava Enros [:madhava]]

I like the rounding scheme, and especially the effort to not have the time remaining drift up and down as much.

I think, though, that we're not being as helpful to users as we could be if we present them with decimal fractions of units of time.  It's _far_ easier for firefox than for them to do the calculations to go from (to pull some figures at random from the screenshot):

16.7 minutes --> 16 minutes 42 seconds
1.4 hours --> 1 hour 24 minutes
1.2 days --> 1 day 4 hours 48 minutes
and so on.

People think about time in 60ths and 24ths rather than in 10ths, so that's how we should present it.

We can further simplify, once we've done this, by leaving off minutes and seconds when we're talking in terms of days, and leaving off seconds when talking in terms of hours, and rounding whichever way makes sense for the non-wavering algorithm.

Comment 20

[Ed Lee :Mardak]

Attachment: screenshot of v2

Here's the same test seconds with the new display code. It'll use the 2 largest units of times, so it can skip intermediate ones.. e.g., "1 day 3 seconds"

Comment 21

[Ed Lee :Mardak]

Attachment: v2

I'll request review when this gets ui-r+'d.

Comment 22

[Reed Loden [:reed]]

Could we add a comma between the time units to separate them (e.g., "12 hours, 10 minutes")? I'd really appreciate it, and it would help keep us grammatically correct. :)

Comment 23

[Ed Lee :Mardak]

Attachment: v2.1

So now that we're potentially showing down to the seconds instead of showing just a decimal point, I updated the seconds display to include the time between updates. (Before it would jump from 5.3 hours to 5.2 hours -- a difference in 360 seconds that it didn't need to bother updating.)

Comment 24

[AndrewM]

In attachment 287278 [details], it shows "1 hour 0 seconds left" and "1 day 0 seconds left". Would it be possible just to show "1 hour left" and "1 day left" in those cases? It looks a little strange to me showing the "0 seconds" bit.

Comment 25

[Ed Lee :Mardak]

I explicitly kept the "0 seconds" because in the normal use case, it'll be something like..

2 hours 3 seconds left
2 hours 2 seconds left
2 hours 1 second left
2 hours 0 seconds left
1 hour 59 minutes left

If we dropped off the 0 seconds left, the text would become really short for 1 second and become long again on the next second.

Comment 26

[Madhava Enros [:madhava]]

Days / hours rounding seems to be working really well.  I have just a few more comments:

- we should be using "remaining" rather than "left" -- as in "1 day, 3 hours remaining"
- we should put commas between the units if we're listing multiple (m days, n hours;  m hours, n minutes)
- we probably should not display the number of seconds remaining unless the total time is under a minute, rounding to the nearest minute otherwise -- minutes, when there are still multiple of them remaining, seems like fine enough granularity and buys us back some horizontal space

Comment 27

[Ed Lee :Mardak]

Attachment: screenshot of v2.2

<primary unit>, <secondary unit> remaining for "day/hour", "hour/minute", "minute/second"
<primary unit> remaining for "seconds"

The seconds shown with minutes should be pretty stable.

Comment 28

[Ed Lee :Mardak]

Attachment: v2.2

Tweaked around the fudging and things seem to be good for me.. probably should have other people try it out to see if it's accurate and non-jittery.

Comment 29

[Madhava Enros [:madhava]]

I like everything about it with one small additional request -- I think it'd be simpler without losing any user-value if, when in minutes range (more than 59 seconds, less than an hour) we didn't show the seconds.  I've been watching of a number of downloads go by, and this behavior seems the most straightforward.

Comment 30

[Ed Lee :Mardak]

Attachment: v2.3

Only show 1 unit for things under 1 hour (so only minutes for 60-3600 seconds and only seconds for 0-60 seconds).

Comment 31

[Ed Lee :Mardak]

Attachment: screenshot of v2.3

Comment 32

[Shawn Wilsher :sdwilsh]

Comment on attachment 289847 [details] [diff] [review]
v2.3

>+# LOCALIZATION NOTE (timePair): #1 time number; #2 time unit
>+# LOCALIZATION NOTE (timeLeftSingle): #1 time left
>+# LOCALIZATION NOTE (timeLeftDouble): #1 time left; #2 time left sub units
>+# examples: 11 minutes left; 1 minute 2 seconds left
I think we can just use one line that says LOCALIZATION NOTE here, and then have the notes under it (or did we decided differently on another bug?

>+timePair=#1 #2
>+timeLeftSingle=#1 remaining
>+timeLeftDouble=#1, #2 remaining
>+timeFewSeconds=A few seconds remaining
>+timeUnknown=Unknown time remaining
>+second=second
>+seconds=seconds
>+minute=minute
>+minutes=minutes
>+hour=hour
>+hours=hours
>+day=day
>+days=days
I also want Pike to give the OK on this.  Want to make sure that this is actually localizable still.

>+/**
>+ * Converts a number of seconds to the largest unit. Time values are whole
>+ * numbers, and units have the correct plural/singular form. Whatever isn't
>+ * covered by the largest unit is considered "extra".
>+ *
>+ * @return a triple: [new value, its unit, extra seconds left]
>+ */
nit: needs @param aSecs, even if it seems obvious
nit: return line should read "@returns a triple (new value, value's unit, extra seconds left)".  I've never seen a tuple wrapped in anything other than parans :p

>+function convertTimeUnits(aSecs)
>+{
>+  // These maximum values corresponds to gStr.time units without the last item
>+  let timeSize = [60, 60, 24];
I worry a bit about this being down here, despite your comment.  What's the argument for not placing this in the array as well?

Also, the whole not being in the same units seems a bit confusing...

r=sdwilsh with these fixed.

Comment 33

[Axel Hecht [:Pike]]

I assume that this is pretty busted from an l10n point of view.

The assumption of one singular and one plural form only works for a few languages, sadly, we don't really have a good way to deal with it right now. Polish is one of the languages with multiple grammar forms, let's see what they think.

CCing some of the Polish localizers, could you have a look at this patch and see if you can work around this?

Comment 34

[Shawn Wilsher :sdwilsh]

(In reply to comment #33)
> CCing some of the Polish localizers, could you have a look at this patch and
> see if you can work around this?
Or at least what we can do to make this work for you.

Comment 35

[Zibi Braniecki [:zbraniecki][:gandalf]]

> CCing some of the Polish localizers, could you have a look at this patch and
> see if you can work around this?

No. and I doubt any other language that has more than one plural form will.

The issue here is that in most cases, we decline the word (hour, minute, second) depending on the given form.
gettext solved it by the way ;)

(http://en.wikipedia.org/wiki/Plural)

> Or at least what we can do to make this work for you.

We could workaround it by using short JS expressions.

Example:

+timeFormat=(n==1)?0:1
+timePair=#1 #2
+timeLeftSingle=#1 remaining
+timeLeftDouble=#1, #2 remaining
+timeFewSeconds=A few seconds remaining
+timeUnknown=Unknown time remaining
+second=second|seconds
+minute=minute|minutes
+hour=hour|hours
+day=day|days

Same in polish:

+timeFormat=(n==1 ? 0 : n%10>=2 && n%10<=4 && (n%100<10 || n%100>=20) ? 1 : 2);
+timePair=#1 #2
+timeLeftSingle=Pozostało: #1
+timeLeftDouble=Pozostało: #1, #2
+timeFewSeconds=Zostało kilka sekund
+timeUnknown=Pozostała nieznana ilość czasu
+second=sekunda|sekundy|sekund
+minute=minuta|minuty|minut
+hour=godzina|godziny|godzin
+day=dzień|dni|dni

It's first guess, but it should work... That emulates a bit gettext, but gives us the same flexibility.

Comment 36

[Zibi Braniecki [:zbraniecki][:gandalf]]

And

var form = eval(timeFormat);
var minute = bundle.getString('minute').split('|')[form];

gives you what you want no matter of the number of plural forms

Comment 37

[Shawn Wilsher :sdwilsh]

Comment on attachment 289847 [details] [diff] [review]
v2.3

Revoking per comments.  This should be fun to implement! ;)

Comment 38

[Axel Hecht [:Pike]]

Of course that assumes that | doesn't appear anywhere for real, and that localizers understand that they're supposed to code JavaScript here.

How important is it to fix this bug really? This would be the first time we do this, and I'm not sure how error prone would be.

After doing a little chat with gandalf on irc, I think that floats are safer. Think of "1 mile" vs "1.0 miles". AFAICT, that works for Polish grammar cases, too.

So as much as I'd regret to see floats in the UI, they're probably the safest hack out of the plural mess.

Comment 39

[Ed Lee :Mardak]

I had an implementation with floats before that always used plural forms, but it was specifically requested to switch to integers.

I'm assuming it's safe to assume that 1 is going to be singular everywhere and 0 isn't an issue because we don't display 0 <units> (it's sometimes singular.. e.g. french).

We only display times up to 60s (minutes, seconds) or 24 for hours, so the only issue is that days can go up forever.. unless we make it say "More than a month.."

So knowing that, we can have..

polish:
#LOCALIZATION NOTE (plurals): Numbers that require a special plural form separated by | for each special form. Don't include the "catch-all plural"
plurals=12,13,14
#LOCALIZATION NOTE (seconds): Units for each special plural form followed by the catch-all plural then the singular. Separate by |s.
seconds=sekundy|sekund|sekunda

english:
plurals=
seconds=seconds|second


So if a language had special plural forms for <10 and 10-19 and a general plural for 20+..

plurals=2,3,4,5,6,7,8,9|10,11,12,13,14,15,16,17,18,19
seconds=singles|tens|plurals|singular

Comment 40

[Ed Lee :Mardak]

And for chinese that doesn't have a difference between plurals and singular..

timePair=#1 #2
timeLeftSingle=剩下 #1
timeLeftDouble=剩下 #1 #2
timeFewSeconds=幾秒剩下
timeUnknown=剩下時間未知
second=秒|秒
minute=分|分
hour=時|時
day=日|日

For "5 hours, 3 minutes remaining":
剩下 5 時 3 分

Comment 41

[Mike Beltzner [:beltzner, not reading bugmail]]

Is there any way to get floats for languages where it's required to work around
plural complexities? Or better, using a standard time format like hh:mm:ss
(truncating as available, so 34m would be "34:02 remaining")?

I don't really like the idea of hurting locales which don't require floats
solely in order to service those that do, as that seems like a lowest common
denominator sort of approach.

(not implying that Polish is any better or worse a language, just that its
requirements shouldn't necessarily impact other languages)

That said, Mardak's approach seems palatable ..

Comment 42

[Cédric Corazza]

From a localizer point of view, if this concerns a dozens of extra strings (or whatever) which will be the same in a given locale but is needed in other locales, that's not a big problem or extra work : either the localization note would have to be clear about that extra strings, or the entity names would have to be clear. And as localizers are concerned with the particularities of their own language, they do understand what's needed for other langauges. So, imho, let's do what is needed to be done to fulfill the peculiarities of each language, this will be not the most painful task in localization, for coding, I don't know.

Comment 43

[Zibi Braniecki [:zbraniecki][:gandalf]]

Mike: we speak about majority of locales here.

Axel: I'm not sure which form you'd like to see, (1.5 minute / 1.5 hour?). In such case, I find it a little unintuitive.

"1,7 hour left"? 

Mardak: Not all languages have just one additional form. There may happen to be 2 additional...
To be honest, I prefer my solution, since the only risk is bad JS in timeFormat string, which can be easily checked by check-locales script (js shell...).

examples:

hungarian, japanese, turkish: no plurals (timeFromat = 0)

eng., german: timeFromat = (n==1)?0:1 // or n!=1

french: timeFromat = n>1?1:0

russian: timeFromat = n%10==1 ? 0 : n%10>=2 && n%10<=4 && (n%100<10 || n%100>=20) ? 1 : 2

polish: timeFromat = n==1 ? 0 : n%10>=2 && n%10<=4 && (n%100<10 || n%100>=20) ? 1 : 2

slovenian: timeFromat = n==1 ? 0 : n%10==2 ? 1 : n%10==3 || n%10==4 ? 2 : 3

Babel Tower Children we are ;)

Comment 44

[Ed Lee :Mardak]

(In reply to comment #43)
> Mardak: Not all languages have just one additional form.
Right. I addressed that with my example of [2,10) vs [10,20) vs 20+ vs 1.

But the russian example is interesting in that multiple values can be singular. So tweaking what I suggested, instead of defaulting singular to 1, it would be treated as just another special case and the default is the catch-all plural (which is the common case).

By explicitly listing the values, localizers don't need to even think about coding a bunch of nested ternary expressions with modulos and boolean logic.

# LOCALIZATION NOTE (timeVals): A pipe-separated list of comma-separated time numbers that do *not* use the default plural value. Each pipe separation indicates a new special form. Only consider values less than 60.
# E.g., 1|2,3 indicates special forms for 1 and 2,3 and every thing else uses the default
# LOCALIZATION NOTE (seconds): A pipe-separated list of forms of "seconds" that match with timeVals. The last entry is the default plural value.
# E.g., sec|secz|secs (1 sec, 2 secz, 3 secz, 4 secs, 5 secs)

english: timeVals=1; seconds=second|seconds

french: timeVals=0,1 // actually just 1 because we don't display 0s

japanese: timeVals= // blank to always use the "catch-all plural"

russian: timeVals=1,21,31,41,51|2,3,4,22,23,24,32,33,34,42,43,44,52,53,54
^^ (no special case for 11 even though it ends in 1?, so n%10==1 doesn't work??)

polish: timeVals=1|2,3,4,22,23,24,32,33,34,42,43,44,52,53,54

slovenian: timeVals=1|2,12,22,32,42,52|3,4,13,14,23,24,33,34,43,44,53,54

Comment 45

[Zibi Braniecki [:zbraniecki][:gandalf]]

I still find your solution not scalable and thus "dirty".

I never said I'd like localizers to write their JS code. Fortunately, we can stand on the shoulders of others and simply use gettext expressions that are publicly available.

Comment 46

[Ed Lee :Mardak]

If anything, I would prefer regular expressions over evaling javascript code. But javascript doesn't support negative lookaheads.. (Also, automatic $ anchor for each)

english: timeVals=^1

french: timeVals=^[01]

japanese: timeVals=

russian: timeVals=(?<!1)1;(?<!1)[234]
1) anything ending in 1 not preceded by 1
2) anything ending in 2,3,4 not preceded by 1

polish: timeVals=^1;(?<1)[234]

slovenian: timeVals=^1;2;[34]
1) 1
2) anything ending in 2
3) anything ending in 3 or 4

Comment 47

[Ed Lee :Mardak]

gavin: this isn't too hacky is it.. ? ;)

// prepend 0 in front of the number to match (simplifies matching single digit)
let num = '0' + realNum;
// fix up "start" matches for above, and anchor expressions to the end
let reg = new RegExp(realRegExp.replace(/^\^/, '^0') + '$');

if (num.search(reg) >= 0)) ...

polish: ^1 ; [^1][234]
^^ exactly 1 ; 2,3,4s not preceded by 1

slovenian: ^1 ; 2 ; [34]
^^ exactly 1 ; ending in 2 ; ending in 3,4

Comment 48

[Ed Lee :Mardak]

Attachment: v3

A quick implementation that also caches value -> index results. I'll add/clean up comments later. Oh, and also it treats singular as 1 and any number ending with 2,3,4,5 gets a special plural form for now...

Comment 49

[Axel Hecht [:Pike]]

I rule out regexps right away. There are probably 2 people on the planet tha can debug regexps, and I'm not one of them. I seriously doubt that any serious number of our localization community is among them. I can hack regexps allright, but debug them? Nah.

The js approach requires some serious thinking about the security implications. Is Components.util.Sandbox good enough these days? Localized content is non-trusted code in general, so a straight eval won't work.

Note, I'm not sure that the stacking of formatted strings on top of each other reliably works in general. The common candidates for problems would be Japanese to some extent, and a bit more, Finnish.

Dynamis, Ville, Jussi, could you read through the bug and look at the alternatives and see whether that introduces non-localizable output for you? In particular for finnish, I'm wondering if the vocal harmony would require a different form for "minutes", depending on whether it's preceded by hours, followed by seconds, or neither.

Comment 50

[Ed Lee :Mardak]

Localizers don't need to exercise the full power of JS regexps, and the syntax shouldn't be too bad for what needs to be used.

There's some basic patterns that are necessary and at least with the languages discussed so far, they're just..

"^1" to match only the number 1
"2" to match any numbers ending in 2
"[34]" to match any numbers ending in 3 or 4
"[^5]" any number not ending in 5
"[^6]7" any number ending in 7 except 67


About stacked formatted strings, the ordering of text and spacing is controllable by the localization.

timePair=#1 #2 // 1 minute
timeLeftDouble=#1, #2 remaining // 1 hour, 2 minutes remaining

So another language could have..
timePair=#2#1 // 分1 "minute 1"
timeLeftDouble=剩下#2#1 // 剩下分2時1 "remaining minute 2 hour 1"
(just for example.. you would probably actually have "#1#2" for timePair)

Comment 51

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to comment #50)
> Localizers don't need to exercise the full power of JS regexps, and the syntax
> shouldn't be too bad for what needs to be used.
> 
> There's some basic patterns that are necessary and at least with the languages
> discussed so far, they're just..

you're cruel. Axel meant that the IDEA of regular expressions is something pretty far beyond average non-geek. Like idea of quantum theory. Or RDF.
You can't say "Oh, we don't expect them anything complex, just a simple equations for quantum theory" or "let them build a very simple RDF tree"...

The advantage of using gettext solution is that it already works in tens of thousands of projects over the world. It's tested.
And we can reuse their JS, instead of asking localizers to "write a simple regexp".

I did a little research and we have at least (!!!) 11 different forms. 

Please, read at: http://www.gnu.org/software/gettext/manual/html_node/gettext_150.html#Plural-forms

Comment 52

[Axel Hecht [:Pike]]

How about this as a compromise between floats and readability:

12:15 hours remaining
45 minutes reamining
5:23 minutes remaining
87 seconds remaining

and to use mm:ss and hh:mm as a human readable version of floats?

We could actually use plain printfs for this

hoursRemaining = %1$S:%2$S hours remaining
minutesRemaining = %S minutes remaining
minutesSecondsRemaining = %1$S:%2$S minutes remaining
secondsRemaining = %S seconds remaining

I think this should get around all of the plural problems, as I'd consider the mixed form to be the same as a float in terms of grammar, but not causing the mess of actually having to parse floats. And I think that 1:30 hours is a well established way to express periods of time, too.

Comment 53

[Zibi Braniecki [:zbraniecki][:gandalf]]

5:05 minutes remaining looks odd to me. And I'm afraid that we're not escaping the plural forms loop since 4:05 uses different form than 12:01 or 24:30.

Marcoos, Staszyna, can you comment on this?

Comment 54

[Reed Loden [:reed]]

(In reply to comment #52)
> How about this as a compromise between floats and readability:
> 
> 12:15 hours remaining
> 5:23 minutes remaining

I personally have no idea what either of those two strings mean, and I doubt most people would be able to decipher them. I don't think that's an acceptable compromise here.

Comment 55

[Ed Lee :Mardak]

(In reply to comment #51)
> you're cruel. http://www.gnu.org/software/gettext/manual/html_node/gettext_150.html#Plural-forms

Thanks for the link. I've gone ahead and converted those:

Plural-Forms: nplurals=1; plural=0;
timeVals=

Plural-Forms: nplurals=2; plural=n != 1;
timeVals=^1

Plural-Forms: nplurals=2; plural=n>1;
timeVals=^[01]

Plural-Forms: nplurals=3; plural=n%10==1 && n%100!=11 ? 1 : n != 0 ? 2 : 0; // was ? 0 : ? 1 : 2
timeVals=^0;[^1]1

Plural-Forms: nplurals=3; plural=n==1 ? 0 : n==2 ? 1 : 2;
timeVals=^1;^2

Plural-Forms: nplurals=3; plural=n==1 ? 0 : (n==0 || (n%100 > 0 && n%100 < 20)) ? 1 : 2;
timeVals=^1;^0|0[^0]|1.

Plural-Forms: nplurals=3; plural=n%10==1 && n%100!=11 ? 0 : n%10>=2 && (n%100<10 || n%100>=20) ? 2 : 1; // was ? 0 : ? 1: 2
timeVals=[^1]1;0|1.

Plural-Forms: nplurals=3; plural=n%10==1 && n%100!=11 ? 0 : n%10>=2 && n%10<=4 && (n%100<10 || n%100>=20) ? 1 : 2;
timeVals=[^1]1;[^1][234]

Plural-Forms: nplurals=3; plural=(n==1) ? 0 : (n>=2 && n<=4) ? 1 : 2;
timeVals=^1;^[234]

nplurals=3; plural=n==1 ? 0 : n%10>=2 && n%10<=4 && (n%100<10 || n%100>=20) ? 1 : 2;
timeVals=^1;[^1][234]

nplurals=4; plural=n%100==1 ? 0 : n%100==2 ? 1 : n%100==3 || n%100==4 ? 2 : 3;
timeVals=01;02;0[34]


And if anyone wants to double check or test them.. you can use this script... might be useful for a unit test eventually.. ;)

let reg, expr;
function test(r, e)
{
  [reg, expr] = [r,e];
  for (let n = 0, f = 100; n < 1000; n++) {
    let [x, y] = [a(n), b(n)];
    if (x != y && --f > 0)
      print([n, x, y]);
  }
}
function a(aTime)
{
  let forms = reg.split(/;/);
  for (var i = 0; i < forms.length; i++) {
    let re = new RegExp("(" + forms[i].replace(/^\^/, "^0") + ")$");
    let num = "0" + aTime;

    if (num.search(re) >= 0)
      break;
  } 
      
  return i;
}
function b(n)
{
  return eval(expr);
}

test("", "0");
test("^1", "n != 1");
test("^[01]", "n > 1");
test("^0;[^1]1", "n%10==1 && n%100!=11 ? 1 : n != 0 ? 2 : 0");
test("^1;^2", "n==1 ? 0 : n==2 ? 1 : 2");
test("^1;^0|0[^0]|1.", "n==1 ? 0 : (n==0 || (n%100 > 0 && n%100 < 20)) ? 1 : 2");
test("[^1]1;0|1.", "n%10==1 && n%100!=11 ? 0 : n%10>=2 && (n%100<10 || n%100>=20) ? 2 : 1");
test("[^1]1;[^1][234]", "n%10==1 && n%100!=11 ? 0 : n%10>=2 && n%10<=4 && (n%100<10 || n%100>=20) ? 1 : 2");
test("^1;^[234]", "(n==1) ? 0 : (n>=2 && n<=4) ? 1 : 2");
test("^1;[^1][234]", "n==1 ? 0 : n%10>=2 && n%10<=4 && (n%100<10 || n%100>=20) ? 1 : 2");
test("01;02;0[34]", "n%100==1 ? 0 : n%100==2 ? 1 : n%100==3 || n%100==4 ? 2 : 3");

Comment 56

[Ed Lee :Mardak]

Attachment: v3 v1

This is a patch on top of v3 that allows the localizer to specify a number for the desired plural form instead of coming up with a pattern. But there's a fallback to allow for custom patterns if necessary.

For example, the localizer would just need to specify "1" for english and "second;seconds".. or "0" for chinese and "分" for seconds.

The basic comments and notes I've added are very rough but those can be fixed up. The functionality is there though.

Comment 57

[Ed Lee :Mardak]

Oh and dolske just reminded me of something I thought about while I was on the plane..

What /do/ programs written in those native languages do anyway?

There are english programs that don't even bother with removing the 's' for singular, and that's already pretty simple to code. I know it annoys some people like me, but a lot of programs do that and it's somewhat accepted.

For attachment 290169 [details] [diff] [review], it allows for a custom rule, but that could be removed and force the localizer to "deal with it" instead of having him/her figure out the pattern specification to come up with the perfect pattern.

For example, if "timeForm 1" wasn't there, english would probably just use "timeForm 0" which makes everything plural.

Not perfect, but "good enough" or at least "acceptable for most".

Comment 58

[Zibi Braniecki [:zbraniecki][:gandalf]]

Open Source apps use gettext. (php use gettext, wordpress, drupal, etc.), same with QT/Gtk apps.

Comment 59

[Ed Lee :Mardak]

How many special forms are there outside of the listed 11? And should we allow customizable gettext/regex patterns? If not, then the interface for localizers is pretty simple -- just pick the correct number or best-fit.

Then at that point, the only thing exposed is the table-lookup and the appropriate number of plural forms of seconds, etc.

To the localizer, it doesn't matter if we use gettext or regex. It's not hard to do either, and I already have implemented the regex one, and gettext would be something along the lines of..

let ctuForms = [
  function(n) 0,
  function(n) n == 1 ? 0 : 1,
  function(n) n <= 1 ? 0 : 1,
  function(n) n%10==1 && n%100!=11 ? 0 : n != 0 ? 1 : 2,
  ...
];

ctuForms[i](n);

Comment 60

[Ed Lee :Mardak]

No objections to providing 11 plural forms and all the localizer needs to do is pick the appropriate one and fill in the plurals of seconds, etc?

I'll probably just implement it in the back-end with gettext-like expressions.

Any suggestions on how to make this information available to localizers? Put a big LOCALIZATION NOTE table of languages and number of plural forms and which values get what forms?

Comment 61

[Cédric Corazza]

Edward,
Maybe the localization note should only be a link towards a wiki page explaining what this is about, as I guess this could be used somewhere else in the code?

Comment 62

[Ed Lee :Mardak]

Attachment: category values script

For the wiki, I could list out what values go in which plural form position for each category. So the localizer just needs to look at which category has the matching numbers for his/her localization instead of trying to figure out nested ternary expressions or regular expressions.

What else should go on the wiki? I suppose I could list off common languages that use that form from the gnu gettext page, but that seems to be wrong.. at least for brazilian portuguese and its plural/non-pluralness of 0.

---

Localizing plural forms of strings:

Pick an appropriate category number (see below). The category has some number of plural forms, and you'll need to provide a semi-colon separated list of that many plural forms for a given word. Make sure the order matches up with the numbers listed for the category.

E.g., you picked category 1 which has 2 plural forms, so localizing "seconds" would be "seconds=second;seconds".

Category numbers:

For each category, each line starts with a "plural form" number followed by the corresponding values that fit in the category. Values go from 0 to 200, but only the first several are listed. Pick the category that has the best matching "plural forms". Note that the last "plural form" is generally the common plural case.

E.g., your language treats 1 as singular and everything else plural, so pick category #1.

Plural category #0 (1 form):
0: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16

Plural category #1 (2 forms):
0: 1
1: 0, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17

Plural category #2 (2 forms):
0: 0, 1
1: 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18

Plural category #3 (3 forms):
0: 0
1: 1, 21, 31, 41, 51, 61, 71, 81, 91, 101, 121, 131, 141, 151, 161, 171, 181
2: 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18

Plural category #4 (3 forms):
0: 1
1: 2
2: 0, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18

Plural category #5 (3 forms):
0: 1
1: 0, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17
2: 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36

Plural category #6 (3 forms):
0: 1, 21, 31, 41, 51, 61, 71, 81, 91, 101, 121, 131, 141, 151, 161, 171, 181
1: 0, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 30, 40, 50, 60, 70
2: 2, 3, 4, 5, 6, 7, 8, 9, 22, 23, 24, 25, 26, 27, 28, 29, 32

Plural category #7 (3 forms):
0: 1, 21, 31, 41, 51, 61, 71, 81, 91, 101, 121, 131, 141, 151, 161, 171, 181
1: 2, 3, 4, 22, 23, 24, 32, 33, 34, 42, 43, 44, 52, 53, 54, 62, 63
2: 0, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20

Plural category #8 (3 forms):
0: 1
1: 2, 3, 4
2: 0, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20

Plural category #9 (3 forms):
0: 1
1: 2, 3, 4, 22, 23, 24, 32, 33, 34, 42, 43, 44, 52, 53, 54, 62, 63
2: 0, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20

Plural category #10 (4 forms):
0: 1, 101
1: 2, 102
2: 3, 4, 103, 104
3: 0, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20

Comment 63

[Caio Tiago Oliveira (asrail)]

Brazilian portuguese uses plural for 0, 0.1, 0.99, 1.1, -1 and whatever is not exactly 1.

The table looks clear, but some people will prefer relating it to the meaning of each line and listing some languages that use that form.

Comment 64

[Benoit]

I'm afraid I'll have to ask if there are languages other than French where the word "remaining" itself should have a specific plural form.

For example, we would say:
2 minutes restantes.
but:
1 minute restante.

If that's really an edge case, we could keep on whith "restante(s)" as in Firefox 2, nobody ever complained about that.

And fortunately, "hour", "minute" and "second" all have the same gender, it could have been worse :)

Comment 65

[Gabriel Chadwick]

(In reply to comment #64)
> I'm afraid I'll have to ask if there are languages other than French where the
> word "remaining" itself should have a specific plural form.
> 
> For example, we would say:
> 2 minutes restantes.
> but:
> 1 minute restante.
> 
> If that's really an edge case, we could keep on whith "restante(s)" as in
> Firefox 2, nobody ever complained about that.
> 
> And fortunately, "hour", "minute" and "second" all have the same gender, it
> could have been worse :)
> 

Why not include remaining in the string that is changed by the script that is used to determine the plural form to be used. That way it would change at the same time as the minutes.

Comment 66

[Ed Lee :Mardak]

I believe russian has a similar issue with english's simplicity, but there are other strings that have this issue and are worked with like..

Restantes: 1 minute
Restantes: 2 minutes

Does that work for French?

Also, is it 2 heures, 1 minute restante or with a s?

(Is it only the smaller unit that affects the "remaining" plural/singular?)

And isn't jour male? :(

Comment 67

[Benoit]

> Restantes: 1 minute
> Restantes: 2 minutes
>
> Does that work for French?

This could work with "Temps restant :". Thanks for the suggestion.

> Also, is it 2 heures, 1 minute restante or with a s?
>
> (Is it only the smaller unit that affects the "remaining" plural/singular?)

Rather the larger one. I'd read that "2 hours and one minute", making that a plural.

> And isn't jour male? :(

You're right, I didn't think of this kind of duration. We'll probably follow your advice with "Temps restant :".

Comment 68

[Axel Hecht [:Pike]]

Back from India, sorry for the silence for a while here. Interesting to see that neither China nor India are mentioned in the list of countries, btw.

I don't see how this will work for localizers. I don't understand the comments, at least not if I remove my prior knowledge from the equation. Assuming that all of them know what gettext does is flawed, too, and that you moved from gettext's '|' to ';' as separator won't help those that do.

It'd help if we could actually test that the localization isn't broken, no idea how to do that though. Nor how to hook up the test to our test infrastructure.

I know it'd be nice if I could come up with something constructive, it's just that I don't. Sorry.

Comment 69

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to comment #68)
> Back from India, sorry for the silence for a while here. Interesting to see
> that neither China nor India are mentioned in the list of countries, btw.

It's good that you pointed that out. When I was listing country models of plurals, I wrote that each time that we still *don't know* how it works in other countries, and that the list of possible models is far beyond our current one.
So in result, assuming that we will hardcode 11 models and let localizer choose one is faulty by design imho.

In worst case (which is that we keep the current approach), we should give localizers a way to write their own model in case none of those works for them.
 

Comment 70

[Shawn Wilsher :sdwilsh]

umm, Chinese is mentioned in comment 40...

Comment 71

[Ed Lee :Mardak]

Attachment: v4

Comment 72

[Ed Lee :Mardak]

Attachment: v4 l10n v1

Comment 73

[Ed Lee :Mardak]

Patch v4 is essentially the original patch with better framework for adding support for plural forms. Can we land the first part so that it can be tested?

I'm still not sure what's so wrong with the 11 plural forms. Localizers probably have had to deal with plural issues before and the patch provides a lot more flexibility to them. Instead of working around code that assumes 1 singular, else plural, they can actually choose a better match.

If it's not a perfect match, a close match will probably work pretty well.

I can only give english examples, but if there was no flexibility and the code was originally written for asian languages with no distinction between plurals, there are multiple ways to deal with it.

Such as.. seconds, second(s), sec, s

10 seconds remaining
1 seconds remaining

10 second(s) remaining
1 second(s) remaining

10 sec remaining
1 sec remaining

10s remaining
1s remaining

I would assume most users would figure out the information being conveyed to them no matter which string is used.

Comment 74

[Ed Lee :Mardak]

Can we land the first part of the patch so that the code can be used for later patches as well as get testing?

Comment 75

[Zibi Braniecki [:zbraniecki][:gandalf]]

can you also explain why not to allow localizers to build custom expressions in case none of those works?

Comment 76

[Ed Lee :Mardak]

I did have code that supported custom expressions, but it's unclear if it's needed and if the cost of documenting and training the localizer to use it is worth it.

This doesn't just impact the localizers that need to use a custom expression either. There were already questions in #l10n about what to do with the custom expression string for languages that didn't need it.

It just seems to add unnecessary confusion in the common case without much added benefit.

It's not a coding issue, but as Pike already pointed out, the tools localizers use aren't the greatest and adding this extra customization complexity isn't ideal.

For localizations of beta2, there already are the strings for "seconds" and "minutes" (e.g., "5 minutes left") and localizers already have had to deal with "everything is the same type of plural". There was no choice and the strings had to follow the same plural form as English.

The l10n patch here adds a ton more flexibility than what's currently available for localizers.

Comment 77

[Axel Hecht [:Pike]]

Hrm. So we could add plural forms without doing string changes, so that should be good enough for a stable branch, too. It is somewhat sad that language packs won't be able to do that, but I think that's OK.

I'd like to see the wiki page with the documentation before landing this, really, and I guess that both entries should point to it.

A more complex question is, should the actual code be already factored, either in a xpcom service or a js module? mconnor?

And I'd like to see a test for a localization, too, to check if the plurals actually match the plural form. The plural function should be more fault tolerant, too, I guess, and the initial function should probably throw something reasonable to make sure that startup errors or such are well handled.

Comment 78

[Ed Lee :Mardak]

(In reply to comment #77)
> So we could add plural forms without doing string changes
Right, it would just default to the 'always plural' case.

> I'd like to see the wiki page with the documentation before landing this,
Anything in particular you want to flesh out on the wiki page? I suggested some instructions and the list of plural rules including the families/languages, and description of the categories and the numbers in them. Should I just create a new page under wiki.mozilla.org/L10n/

> really, and I guess that both entries should point to it.
"Both" referring to? The localizable file and the code?

> A more complex question is, should the actual code be already factored, either
> in a xpcom service or a js module? mconnor?
It would be nice if we could get it factored so that it's reusable, but we're at beta 2 coming on 3. So unless you know of some other bugs that have been waiting to land that needs this same feature, then probably not until later.... but I suppose that's not really my call. ;)

> And I'd like to see a test for a localization, too, to check if the plurals
> actually match the plural form.
sdwilsh: Any suggestions on how to test this if it's all in js? I suppose it doesn't necessarily need all the rest of the download manager..?

> The plural function should be more fault tolerant, too, I guess,
You mean instead of just giving an empty string if there aren't enough semicolons?

> and the initial function should probably throw
> something reasonable to make sure that startup errors or such are well handled.
Instead of defaulting to 'always plural'?

Comment 79

[Axel Hecht [:Pike]]

Actually, I think the documentation is technical documentation and should probably be on MDC, how about http://developer.mozilla.org/en/docs/Localization_and_Plurals ? The content sounds good, though.

Both entries referred to both pluralRule and to seconds, the latter should probably be changed to say

# LOCALIZATION NOTE (seconds, minutes, hours, days): ...

Regarding the error checks, I'm mostly concerned about 

aStr.split(/;/)[pluralFunc(aNum)]

and that the amount of provided and required plural forms don't match. Some useful error message in the error console would be great, and I'm tempted to say "make it not localizable" so that I can read it if a localization busts it. I have a hard enough time with xml error messages in indic languages as it is :-)

function pluralForm(aNum, aStr) doesn't do anything right now, if Startup() failed, and it should throw something, or otherwise make itself known in the error console. Maybe just a Components.errors.NS_ERROR_NOT_INITIALIZED.

Comment 80

[Shawn Wilsher :sdwilsh]

Edward - do you still need anything from me, or did Axel answer your question(s)?

Comment 81

[Axel Hecht [:Pike]]

Shawn, the testing question is still open, AFAICT.

I guess that factoring the plural code would make testing easier, too, btw.

Comment 82

[Shawn Wilsher :sdwilsh]

Are we saying how to test (I suppose xpcshell test cases might work - but I don't know if we can load property files with that), or what to test?  It's not immediately clear to me what testing question you are referring too.

Comment 83

[Ed Lee :Mardak]

Yeah, a testcase that makes sure the plural rule is selected and the correct plural form of a word is returned. Basically testing makePluralFormFunction

Comment 84

[Ed Lee :Mardak]

sdwilsh: Is there a way to test the pluralForm function in downloads.js from a toolkit/mozapps/downloads/tests/browser/ test?

Comment 85

[Shawn Wilsher :sdwilsh]

You could get the download manager window, and then call the function and make sure it's what you expect.  Not sure if it's totally worth it.

One additional comment though about this patch:  I'd like it if all the functions related to time were put into their own JS file that is imported with Cu.import.  This will make it easier for us to use these time related functions elsewhere (like addon's manager and AUS), as well as being available to add-on developers who want to redo the download manager UI.

Comment 86

[Ed Lee :Mardak]

(In reply to comment #85)
> You could get the download manager window, and then call the function and make
> sure it's what you expect.  Not sure if it's totally worth it.
So what would we be testing exactly?

Pike: Do we want this to land for beta3 l10n freeze and try to get everyone to understand what to do with the plural form stuff for beta3 release?

> One additional comment though about this patch:  I'd like it if all the
> functions related to time were put into their own JS file that is imported with
> Cu.import.  This will make it easier for us to use these time related functions
> elsewhere (like addon's manager and AUS), as well as being available to add-on
> developers who want to redo the download manager UI.
How exactly does that work? Would the files that import the separate js function be able to use functions defined in that file? Would we need to define them in an object? (e.g., XPCOMUtils.*) We would have to move out some helper functions and variables such as gStr, replaceInsert, pluralForm, ctu*. How about the strings? Would we leave them in downloads.properties? Duplicate them to a new properties file? Move them?

Comment 87

[Shawn Wilsher :sdwilsh]

(In reply to comment #86)
> (In reply to comment #85)
> > You could get the download manager window, and then call the function and make
> > sure it's what you expect.  Not sure if it's totally worth it.
> So what would we be testing exactly?
Mostly that we never ever break it again.  You should make the logic of the function more clear than how it's implemented I suppose too.

> How exactly does that work? Would the files that import the separate js
> function be able to use functions defined in that file? Would we need to define
> them in an object? (e.g., XPCOMUtils.*) We would have to move out some helper
> functions and variables such as gStr, replaceInsert, pluralForm, ctu*. How
> about the strings? Would we leave them in downloads.properties? Duplicate them
> to a new properties file? Move them?
See http://developer.mozilla.org/en/docs/Using_JavaScript_code_modules
The strings are fine in there.  It's all still part of the download manager, we are just making the code more accessible for other people to use.

Comment 88

[Marek Stępień [:marcoos, inactive]]

Fixing the plurals *just* in the download manager is a bit weird, I think.

There's a separate bug 177097 for plurals support for the whole toolkit.

Comment 89

[Axel Hecht [:Pike]]

I'd suggest to put the makePluralForm into a jsm, too.

And yes, we should try hard to get this in by string freeze.

Comment 90

[Shawn Wilsher :sdwilsh]

(In reply to comment #88)
> Fixing the plurals *just* in the download manager is a bit weird, I think.
We have to start somewhere - and it's too late in the game to fix everything at once (unless Pike thinks otherwise).

Comment 91

[Axel Hecht [:Pike]]

Well, having a jsm for makePluralForm will be a good step forward, even as much that I'd mark bug 177097 as WORKSFORME or FIXED. It won't solve it for c++, but that's as far as I'd go right now.

Comment 92

[Ed Lee :Mardak]

Attachment: v5 pt1/3 (DownloadUtils)

Here's moving things out of downloads.js into DownloadUtils.jsm. Most if it is just code moving and adding a lot of comments, but some places I've refactored out some more to provide some additional function to potential consumers of DownloadUtils. This means I needed to provide some more out-params like newLast. (This patch is against after bug 410289.)

(I ran into a separate "let" issue where you can't redefine a let variable, so I added blocks around the case statements.)

Should I also move the "done status" code to DownloadUtils as well? (case nsIDM.DOWNLOAD_FINISHED, etc code)

Comment 93

[Ed Lee :Mardak]

Attachment: v5 part 1 of 2 -- DownloadUtils

Mostly minor changes and fixes to reduce the number of changes for the next patch. Beefed up the logging function to show to error console and command line. Added some more comments.

Comment 94

[Ed Lee :Mardak]

Attachment: v5 part 2 of 2 -- better time, l10n getPluralForm

Combined the two v4 patches. Used an idea from bug 177097 of putting the pluralRule value in chrome://global/locale/intl.properties

Made some forward references to the dev wiki site, but that page isn't there yet.

No need for a separate makePluralFormFunction because this module can init itself and grab the plural form from the intl stringbundle without waiting on the download manager.

Comment 95

[Ed Lee :Mardak]

Comment on attachment 298402 [details] [diff] [review]
v5 part 2 of 2 -- better time, l10n getPluralForm

Pike: r? mainly for the error checking aspect

1) invalid rule numbers are logged but default to 0
2) missing strings are logged with extra info and returns undefined.. so it'll say something like "4 minutes, 3 undefined remaining" assuming the plural form of 3 didn't work

and making sure it's okay to use chrome/global/intl.properties

Comment 96

[Shawn Wilsher :sdwilsh]

Comment on attachment 298400 [details] [diff] [review]
v5 part 1 of 2 -- DownloadUtils

>+EXPORTED_SYMBOLS = [ "DownloadUtils" ];
>+
>+/**
>+ * This module provides the DownloadUtils object which contains useful methods
>+ * for downloads such as displaying file sizes, transfer times, and download
>+ * locations.
>+ */
It'd be cool to have a list of functions that are exported up here.

>+let DownloadUtils = {
>+  /**
>+   * Generate a full status string for a download given its current progress,
>+   * total size, speed, last time remaining
>+   *
>+   * @param aCurrBytes
>+   *        Number of bytes transferred so far
>+   * @param aMaxBytes
>+   *        Total number of bytes or -1 for unknown
>+   * @param aSpeed
>+   *        Current transfer rate in bytes/sec or -1 for unknown
>+   * @param aLastSec
>+   *        Last time remaining in seconds or Infinity for unknown
>+   * @return A pair: [download status text, new value of "last seconds"]
>+   */
Let's add the [optional] flag on the appropriate ones:
>+   * @param aMaxBytes
>+   *        [optional] Total number of bytes or -1 for unknown

>+  getDownloadStatus: function(aCurrBytes, aMaxBytes, aSpeed, aLastSec)
>+  {
and then if that paticular argument is null, we should set it to the proper unknown value so people don't have to call it with all the arguments :)

>+  /**
>+   * Generate the transfer progress string to show the current and total byte
>+   * size. Byte units will be as large as possible and the same units for
>+   * current and max will be supressed for the former.
>+   *
>+   * @param aCurrBytes
>+   *        Number of bytes transferred so far
>+   * @param aMaxBytes
>+   *        Total number of bytes or -1 for unknown
>+   * @return The transfer progress text
>+   */
>+  getTransferTotal: function(aCurrBytes, aMaxBytes)
>+  {
ditto

>+  /**
>+   * Generate a "time left" string given an estimate on the time left and the
>+   * last time. The extra time is used to give a better estimate on the time to
>+   * show.
>+   *
>+   * @param aSeconds
>+   *        Current estimate on number of seconds left for the download
>+   * @param aLastSec
>+   *        Last time remaining in seconds or Infinity for unknown
>+   * @return A pair: [time left text, new value of "last seconds"]
>+   */
>+  getTimeLeft: function(aSeconds, aLastSec)
>+  {
ditto
>+/**
>+ * Helper function to replace a placeholder string with a real string
nit: Private Helper...

>+EXTRA_JS_MODULES = DownloadUtils.jsm
>+
nit: I like to do each thing on its own line - easier to add stuff later:
EXTRA_JS_MODULES = \
  DownloadUtils.jsm \
  $(NULL)

r=sdwilsh
Our current tests will ensure that this patch won't break things in the UI.

Comment 97

[Shawn Wilsher :sdwilsh]

Comment on attachment 298402 [details] [diff] [review]
v5 part 2 of 2 -- better time, l10n getPluralForm

I think that getPluralForm should be in a different jsm file.  This one can import it.  This would also show people how to actually use this file elsewhere.

You'll also need a toolkit peer to review this (aka, not me).

Comment 98

[Ed Lee :Mardak]

Attachment: v5.1 part 1 of 2 -- DownloadUtils

Should be a quicky-r.. just double checking it's the right comment format.. and added isNil

Comment 99

[Ed Lee :Mardak]

Attachment: v5.2 part 1 of 2 -- DownloadUtils

Comment 100

[Ed Lee :Mardak]

Attachment: v5.1 part 2 of 2 -- better time, l10n getPluralForm

Split PluralForm into its own module.. for now it's sitting at mozapps/downloads/src/ but any place can import it from the same location even if it does move.. Cu.import("resource://gre/modules/PluralForm.jsm")

I'll r? from a toolkit reviewer to double check location.

shawn: Most of the code you've already r+'d before in previous version of this patch.. it's just in a different file now

pike: Still double checking on the error logging, etc.

Comment 101

[Ed Lee :Mardak]

I've edited the devmo wiki docs "Localization and Plurals"
http://developer.mozilla.org/en/docs/Localization_and_Plurals

I've put up usage description, the list of plural rules, as well as some examples (french, chinese, polish)

Feedback would be welcome from the localizers already CC'd on this bug or anyone else that could take a look.

Comment 102

[Axel Hecht [:Pike]]

Comment on attachment 298418 [details] [diff] [review]
v5.1 part 2 of 2 -- better time, l10n getPluralForm

r=me with comments:

+# LOCALIZATION NOTE (timePair): #1 time number; #2 time unit
+# example: 1 minute; 11 hours
+# LOCALIZATION NOTE (timeLeftSingle): #1 time left
+# example: 1 minute remaining; 11 hours remaining
+# LOCALIZATION NOTE (timeLeftDouble): #1 time left; #2 time left sub units
+# example: 11 hours, 2 minutes remaining; 1 day, 22 hours remaining
+timePair=#1 #2
+timeLeftSingle=#1 remaining
+timeLeftDouble=#1, #2 remaining
+timeFewSeconds=A few seconds remaining
+timeUnknown=Unknown time remaining
 
Can you order the comments to the appropriate entries? That's doubled markup, but I don't know of any tool that would actually glue those together.

As PluralForm.jsm, I guess it's OK to use intl.properties, though I'm not convinced that it's generally right. Hardcoding the chrome path makes testing harder, too, I think, as it'll require a really localized build, at least for an xpcshell test. As that's not loading language packs for example. If we could just make chrome://global/locale/intl.properties the default, that'd help, though I don't have a constructive comment on how.

I don't expect generalizing that to break the strings, though, so I'm giving you an r+ on this, and we figure out a testing scheme in a follow up?

Comment 103

[Shawn Wilsher :sdwilsh]

Comment on attachment 298418 [details] [diff] [review]
v5.1 part 2 of 2 -- better time, l10n getPluralForm

>     // Calculate the time remaining if we have valid values
>     let seconds = (aSpeed > 0) && (aMaxBytes > 0) ?
>-      Math.ceil((aMaxBytes - aCurrBytes) / aSpeed) : -1;
>+      (aMaxBytes - aCurrBytes) / aSpeed : -1;
I'm not sure why I understand the dropping of Math.ceil.  Care to explain?

>+    // Apply smoothing only if the new time isn't a huge change -- e.g., if the
>+    // new time is more than half the previous time; this is useful for
>+    // downloads that start/resume slowly
>+    if (aSeconds > aLastSec / 2) {
>+      // Apply hysteresis to favor downward over upward swings
>+      // 30% of down and 10% of up (exponential smoothing)
>+      let (diff = aSeconds - aLastSec)
>+        aSeconds = aLastSec + (diff < 0 ? .3 : .1) * diff;
nit: I'd actually like to see braces here just to enforce that this is block scope please

>+      // If the new time is similar, reuse something close to the last seconds,
>+      // but subtract a little to provide forward progress
>+      let diff = aSeconds - aLastSec;
>+      let diffPct = diff / aLastSec * 100;
>+      if (Math.abs(diff) < 5 || Math.abs(diffPct) < 5)
>+        aSeconds = aLastSec - (diff < 0 ? .4 : .2);
Oh wait, this is why you don't want seconds as an integer type - can you have a comment further up specifying what it is that they are supposed to be please to better explain the code?

>+      // Convert the seconds into its two largest units to display
>+      let [time1, unit1, time2, unit2] = DownloadUtils.
>+        convertTimeUnits(aSeconds);
nit: everything after = on a new line please

>+  /**
>+   * Helper for convertTimeUnits that gets the display value of a time
>+   *
>+   * @param aTime
>+   *        Time value for display
>+   * @return An integer value for the time rounded down
>+   */
>+  convertTimeUnitsValue: function(aTime)
>+  {
>+    return Math.floor(aTime);
>+  },
This is private, right?  Comment should say so, and it should not be a part of the object so it is *not* exported.

>+  /**
>+   * Helper for convertTimeUnits that gets the display units of a time
>+   *
>+   * @param aTime
>+   *        Time value for display
>+   * @param aIndex
>+   *        Index into gStr.timeUnits for the appropriate unit
>+   * @return The appropriate plural form of the unit for the time
>+   */
>+  convertTimeUnitsUnits: function(aTime, aIndex)
>+  {
>+    return PluralForm.get(aTime, gStr.timeUnits[aIndex]);
>+  },
ditto

>diff --git a/toolkit/mozapps/downloads/src/PluralForm.jsm b/toolkit/mozapps/downloads/src/PluralForm.jsm
This can't land here - talk to bsmedberg or gavin as to where it should go.  Also, there should be some kind of tests to make sure that this works as expected.

Comment 104

[Ed Lee :Mardak]

Attachment: v5.2 part 2 of 2 -- better time, l10n getPluralForm

(In reply to comment #102)
> Can you order the comments to the appropriate entries?
Shuffled around so it's # LOC; # example; thing=

> and we figure out a testing scheme in a follow up?
I've added an english plural form testcase that makes sure PluralForm is working correctly. A simple separate extension can make sure pluralRule is valid, and show the localizer which numbers are getting which plural form. Checking to make sure all "pluralForm" strings have the right number of entries might be difficult if this extension is separate from the codebase as new strings would need to get added to the extension. But for now I could default it with the download ones.

(In reply to comment #103)
> >+      (aMaxBytes - aCurrBytes) / aSpeed : -1;
> I'm not sure why I understand the dropping of Math.ceil.
It's because the old consumer of seconds was expecting an int which it would display to the user. The new consumer will make sure the format is correct for display as well as use the extra precision for estimation.

> >+      let (diff = aSeconds - aLastSec)
> >+        aSeconds = aLastSec + (diff < 0 ? .3 : .1) * diff;
> nit: I'd actually like to see braces here just to enforce that this is block
> scope please
Blocked (instead of expression)

> Oh wait, this is why you don't want seconds as an integer type
I've added comments to the method description block above the method.

> >+      let [time1, unit1, time2, unit2] = DownloadUtils.
> >+        convertTimeUnits(aSeconds);
> nit: everything after = on a new line please
Much nicer. ;)

> >+   * Helper for convertTimeUnits that gets the display value of a time
..
> >+  convertTimeUnitsValue: function(aTime)
> This is private, right?  Comment should say so, and it should not be a part of
> the object so it is *not* exported.
Moved outside and fixed comments for both functions.

> >diff --git a/toolkit/mozapps/downloads/src/PluralForm.jsm b/toolkit/mozapps/downloads/src/PluralForm.jsm
> This can't land here - talk to bsmedberg or gavin as to where it should go. 
Placed in intl/locale per smontagu and pike

Comment 105

[Axel Hecht [:Pike]]

Comment on attachment 298612 [details] [diff] [review]
v5.2 part 2 of 2 -- better time, l10n getPluralForm

r=me on the l10n parts here.

Nits, I'm not sure if the test needs to iterate to 1000, and I guess there's a way to test the other plural forms with artificial strings, too. I'd use a finite set of somewhat golden numbers (0,1,2,5,10,11.. or so) there and a multidimensional array. 

I'd add a link to http://developer.mozilla.org/en/docs/Localization_and_Plurals to PluralForm.jsm, too.

Comment 106

[Ed Lee :Mardak]

Part 1 of 2:
Checking in toolkit/mozapps/downloads/content/downloads.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js,v  <--  downloads.js
new revision: 1.132; previous revision: 1.131
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/downloads/src/DownloadUtils.jsm,v
done
Checking in toolkit/mozapps/downloads/src/DownloadUtils.jsm;
/cvsroot/mozilla/toolkit/mozapps/downloads/src/DownloadUtils.jsm,v  <--  DownloadUtils.jsm
initial revision: 1.1
done
Checking in toolkit/mozapps/downloads/src/Makefile.in;
/cvsroot/mozilla/toolkit/mozapps/downloads/src/Makefile.in,v  <--  Makefile.in
new revision: 1.9; previous revision: 1.8
done

Part 2 of 2:
Checking in intl/locale/src/Makefile.in;
/cvsroot/mozilla/intl/locale/src/Makefile.in,v  <--  Makefile.in
new revision: 1.37; previous revision: 1.36
done
RCS file: /cvsroot/mozilla/intl/locale/src/PluralForm.jsm,v
done
Checking in intl/locale/src/PluralForm.jsm;
/cvsroot/mozilla/intl/locale/src/PluralForm.jsm,v  <--  PluralForm.jsm
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/intl/locale/tests/unit/test_pluralForm_english.js,v
done
Checking in intl/locale/tests/unit/test_pluralForm_english.js;
/cvsroot/mozilla/intl/locale/tests/unit/test_pluralForm_english.js,v  <--  test_pluralForm_english.js
initial revision: 1.1
done
Checking in toolkit/locales/en-US/chrome/global/intl.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/global/intl.properties,v  <--  intl.properties
new revision: 1.7; previous revision: 1.6
done
Checking in toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties,v  <--  downloads.properties
new revision: 1.21; previous revision: 1.20
done
Checking in toolkit/mozapps/downloads/src/DownloadUtils.jsm;
/cvsroot/mozilla/toolkit/mozapps/downloads/src/DownloadUtils.jsm,v  <--  DownloadUtils.jsm
new revision: 1.2; previous revision: 1.1
done

Comment 107

[Ed Lee :Mardak]

Whoops.. didn't notice the last comment from pike...

I added the comment/link.. it's NPOTB yeah? :)

Checking in intl/locale/src/PluralForm.jsm;
/cvsroot/mozilla/intl/locale/src/PluralForm.jsm,v  <--  PluralForm.jsm
new revision: 1.2; previous revision: 1.1
done

Comment 108

[Ed Lee :Mardak]

Attachment: screenshot of test extension

Here's a quick extension that I whipped up to check the pluralRule value, let the localizer figure out if the strings have enough plural forms, and if the chosen plural rule is correct by listing off example uses.

Comment 109

[Ed Lee :Mardak]

Attachment: pluralForm-checker.xul from extension

Comment 110

[Ed Lee :Mardak]

Attachment: pluralForm Checker Extension

Comment 111

[Marek Stępień [:marcoos, inactive]]

+# LOCALIZATION NOTE (timePair): #1 time number; #2 time unit
+# example: 1 minute; 11 hours
+# LOCALIZATION NOTE (timeLeftSingle): #1 time left
+# example: 1 minute remaining; 11 hours remaining
+# LOCALIZATION NOTE (timeLeftDouble): #1 time left; #2 time left sub units
+# example: 11 hours, 2 minutes remaining; 1 day, 22 hours remaining
+timePair=#1 #2
+timeLeftSingle=#1 remaining
+timeLeftDouble=#1, #2 remaining
+timeFewSeconds=A few seconds remaining
+timeUnknown=Unknown time remaining

Does this mean that "#1 remaining" will always be the same, regardless of the number in #1?

If so, then this is still a bug, as the plural form may affect the noun in some languages. Also the unit may have different gramatical genders, like this:

"Remain" is "pozostać" in Polish, but "#1 remaining" is:

for #1 being minutes:

Pozostała 1 minuta ("minuta" is feminine, so the singular verb ends in "-a")
Pozostały 2 (3, 4) minuty
Pozostało 5 (7, ..., 11, 21) minut

etc.

for #1 being days:
Pozostał 1 dzień ("dzień" is masculine, so the singular verb does not end in "-a")
Pozostały 2 (3, 4) dni
Pozostało 5 (7, ..., 11, 21) dni

So, the whole sentences should vary depending on the number and we should have separate properties for each of the unit, as you can't really say that "day" and "second" can have the same gramatical gender.

Comment 112

[Marek Stępień [:marcoos, inactive]]

(In reply to comment #111)
> If so, then this is still a bug, as the plural form may affect the noun in some
> languages.

s/noun/verb/

Comment 113

[Ed Lee :Mardak]

There's a similar problem for french:

(In reply to comment #67)
> We'll probably follow your advice with "Temps restant :".
(e.g., "Time remaining: 1 hour, 3 minutes")

I know it's not ideal, but it's not an entirely new localization problem, and we're already moving in the right direction with plural noun forms.

Comment 114

[Michael Wolf]

(In reply to comment #111)
> Does this mean that "#1 remaining" will always be the same, regardless of the
> number in #1?
> 
> If so, then this is still a bug, as the plural form may affect the noun in some
> languages. Also the unit may have different gramatical genders, like this:
> 
> "Remain" is "pozostać" in Polish, but "#1 remaining" is:
> 
> for #1 being minutes:
> 
> Pozostała 1 minuta ("minuta" is feminine, so the singular verb ends in "-a")
> Pozostały 2 (3, 4) minuty
> Pozostało 5 (7, ..., 11, 21) minut
> 
> etc.
> 
> for #1 being days:
> Pozostał 1 dzień ("dzień" is masculine, so the singular verb does not end in
> "-a")
> Pozostały 2 (3, 4) dni
> Pozostało 5 (7, ..., 11, 21) dni
> 
> So, the whole sentences should vary depending on the number and we should have
> separate properties for each of the unit, as you can't really say that "day"
> and "second" can have the same gramatical gender.
> 

Shouldn't be used the present tense here? pozosta; pozostają; pozosta? What you mean is past tense and it's for e.g. "elapsed". But the problem is the same. On Sorbian this is a little bit simplier because Sorbian has a synthetic past tense that a lot of other Slavic language lost (but not the Bulgarian for instance). In Sorbian it would be for remaining:

Present tense (remaining):

Zwostanje 1 sekunda (mjeńšina, hodźina, dźeń)
Zwostanjetej 2 sekundźe (mjeńšinje, hodźinje, dnjej)
Zwostanu 3 or 4 sekundy (mjeńšiny, hodźiny, dny)
Zwostanje 5-100, 105-200... sekundow (mjeńšin, hodźin, dnjow)

Past tense (elapsed):

Zańdźe 1 sekunda (...)
Zandźeštej 2 sekundźe (...)
Zańdźechu 3 or 4 sekundy (...)
Zańdźe 5-100, 105-200... sekundow (...)

But Sorbian has Perfect (and Past perfect too) which corresponds to the Polish Past tense but Sorbian additonally uses the auxiliary być (=to be) to build the Perfect tense:

Perfect tense (elapsed)

Je 1 sekunda zašła (mjeńšina zašła, hodźina zašła, dźeń zašły)
Stej 2 sekundźe zašłoj (mjeńšinje zašłoj, hodźinje zašłoj, dnjej zašłoj)
Su 3 or 4 sekundy zašłe (mjeńšiny zašłe, hodźiny zašłe, dny zašłe)
Je 5-100, 105-200... sekundow zašło (mjeńšin zašło, hodźin zašło, dnjow zašło)

Only with number "1" are different gender forms (there's a third form zašło for neuter gender. This form is the same as it used for the numbers 5-100, 105-200 and so on).

Comment 115

[Michael Wolf]

(In reply to comment #114)
> Je 1 sekunda zašła (mjeńšina zašła, hodźina zašła, dźeń zašły)

Sorry, zašly is wrong. Must be zašoł.

Comment 116

[Vitaly Fedrushkov]

(In reply to comment #111)
> If so, then this is still a bug, as the plural form may affect the noun in some
> languages. Also the unit may have different gramatical genders, like this:

> "Remain" is "pozostać" in Polish, but "#1 remaining" is:

See also bug 412161 -- the same problem in Bugzilla.

As seen from a patch (attachment 298020 [details] [diff] [review]), it is not easy to figure out long-distance syntax dependencies.  The whole phrase may change, and one can't easily disassemble it to translate 'just' separate words.

See also http://perldoc.perl.org/Locale/Maketext/TPJ13.html

Comment 117

[Ed Lee :Mardak]

dev-doc-needed: We should probably put information about PluralForm and DownloadUtils modules so extension developers know how to use it. I could edit the pages myself or provide content.. I'm just not sure where I'm supposed to organize the pages or what rules there are.

Comment 118

[Marek Stępień [:marcoos, inactive]]

(In reply to comment #114)
> Shouldn't be used the present tense here? pozosta; pozostają; pozosta? 

As weird as it may seem - no. "pozostaje;pozostają;pozostaje" would be what you want and it would still be correct, though a bit unusual, as people use the (gramatically) past form here. Take a look at Gnome and KDE, for example: http://en.pl.open-tran.eu/suggest/remaining

Comment 119

[Michael Wolf]

(In reply to comment #118)
> As weird as it may seem - no. "pozostaje;pozostają;pozostaje" would be what
> you want and it would still be correct, though a bit unusual, as people use the
> (gramatically) past form here. Take a look at Gnome and KDE, for example:
> http://en.pl.open-tran.eu/suggest/remaining

I see. I looked into my  dictionary once more and saw that the form "pozostały" is quasi an adjective from here that probably belongs - as it seems - to imperfective "pozostawiać" and not to imperfective "pozostawać" what I thought. Am I right?

Comment 120

[Eric Shepherd [:sheppy]]

Is it just me or is this fixed by the document currently at http://developer.mozilla.org/en/docs/Localization_and_Plurals ?

Comment 121

[Eric Shepherd [:sheppy]]

OK, marking this as complete since there's been no response to the previous comment.

Comment 122

[Ed Lee :Mardak]

That wiki page focuses on localizers using it. I'll need to update it more for devs/extension writers when bug 416446 lands.

Comment 123

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED from a functionality standpoint using:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b4pre) Gecko/2008022804 Minefield/3.0b4pre

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022806 Minefield/3.0b4pre

-and-

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4pre) Gecko/2008022904 Minefield/3.0b4pre

I let Linux ISOs download in various VMs and checked in on them at various points to verify.