388506 - Use emdash for download status [time — progress (rate)]

Status: VERIFIED FIXED

(Toolkit :: Downloads API, enhancement)

Description

[Ed Lee :Mardak]

Attachment: v1

"show time remaining, file size remaining and download data rate, use emdashes to separate these pieces of information instead of commas"

This doesn't change the separate date formatting requirement "use nicer date formatting" which I'm assuming to mean "always display |XX minute(s) left| or |Less than a minute|"

Comment 1

[Ed Lee :Mardak]

Attachment: screenshot

Comment 2

[Ed Lee :Mardak]

Attachment: v2

Turns out the "date" thing is for the properties. This patch fixes the time left as well as adding emdash.

<Mardak> does that mean always only show either "XX minutes left" or "Less than a minute" (don't drop into seconds and take the ceiling minute, so 3 -> 2 -> less than minute)
<beltzner> yeah

Comment 3

[Ed Lee :Mardak]

Attachment: v2 screenshot

Comment 4

[Shawn Wilsher :sdwilsh]

Comment on attachment 272708 [details] [diff] [review]
v2

>+# — is the "em dash" (long dash)
>+# #1 transfer progress; #2 rate number; #3 rate unit; #4 time left; e.g., 1 minute — 2 of 3 GB (4 MB/sec)
nit: line wrapping


>+# #1 progress number; #2 progress unit; #3 total number; #4 total unit; e.g., 12.3 of 456 MB
nit: same

>-  this._transferSameUnits = aStringBundle.getString("transferSameUnits");
>-  this._transferDiffUnits = aStringBundle.getString("transferDiffUnits");
>-  this._transferNoTotal = aStringBundle.getString("transferNoTotal");
>+  this._transferSameUnits = aStringBundle.getString("transferSameUnits");
>+  this._transferDiffUnits = aStringBundle.getString("transferDiffUnits");
>+  this._transferNoTotal = aStringBundle.getString("transferNoTotal");
nit: no need to move these - please leave them where they were.

>+  this._timeLeft= aStringBundle.getString("timeLeft");
>+  this._timeLessMinute= aStringBundle.getString("timeLessMinute");
>+  this._timeUnknown= aStringBundle.getString("timeUnknown");
nit: spaces after variables and before equals

Comment 5

[Ed Lee :Mardak]

Attachment: v3

Fixed nits and added better/more examples in downloads.properties

Comment 6

[Mike Connor [:mconnor]]

Comment on attachment 272709 [details]
v2 screenshot

ui-r=mconnor

I'm curious how the "power users" will like this, but I think its probably the right level of detail in general.

Comment 7

[Shawn Wilsher :sdwilsh]

Checking in toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties;
new revision: 1.5; previous revision: 1.4
Checking in toolkit/mozapps/downloads/content/DownloadProgressListener.js;
new revision: 1.15; previous revision: 1.14

Comment 8

[Steffen Wilberg]

"If you change the semantics of a localized string, change the key. This will more likely be a good key name, and it will help tools to pick up that the change you do is different from just a spell fix."
http://developer.mozilla.org/en/docs/Writing_localizable_code

This affects "statusFormat".

Comment 9

[Axel Hecht [:Pike]]

http://developer.mozilla.org/en/docs/Localization_notes can help to make the good comment better still. Thanks to Steffen for catching this, this isn't strictly a "semantic" change, but we should change statusFormat to something silly like statusFormat2 nevertheless.

Comment 10

[Shawn Wilsher :sdwilsh]

Edward - please address these concerns.

Comment 11

[Ed Lee :Mardak]

Attachment: l10n notes v1

Shawn: Renamed statusFormat to statusFormat2 but didn't change the variable used in the code to avoid unnecessary bitrotting.

Alex: Do newlines need to be prefixed with LOCALIZATION NOTE for them to get noticed? And does the key have to match up exactly? And has there been any discussion to make it something shorter... "# L10N key: ..."

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 273133 [details] [diff] [review]
l10n notes v1

Although, I'd really like you to not have to put that note on every line (I still want line wrapping)

Comment 13

[Axel Hecht [:Pike]]

Comment on attachment 273133 [details] [diff] [review]
l10n notes v1

One "LOCALIZATION NOTE" per comment is good enough, that doesn't need to be repeated for every line.

I'm not too fond of the wildcard in transfer*, can you make that a list? 
# LOCALIZATION NOTE (transferSameUnits, transferDiffUnits, transferDiffUnits):
# #1...

or so. Shell globbing is likely the least portable wildcarding, and it'd match "transferred" in that file, too, which would be awkward.

With those nits, r=me.

Comment 14

[Ed Lee :Mardak]

Attachment: l10n notes v1.1

(In reply to comment #13)
> One "LOCALIZATION NOTE" per comment is good enough
Done.

> I'm not too fond of the wildcard in transfer*, can you make that a list? 
> # LOCALIZATION NOTE (transferSameUnits, transferDiffUnits, transferDiffUnits):
Done.

Comment 15

[Nickolay_Ponomarev]

mozilla/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties  1.6
mozilla/toolkit/mozapps/downloads/content/DownloadProgressListener.js        1.16

Comment 16

[Ed Lee :Mardak]

Bug was reopened for localization changes which have been checked in.

Comment 17

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

Verified FIXED using:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a7pre) Gecko/2007080304 Minefield/3.0a7pre, Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a8pre) Gecko/2007080309 Minefield/3.0a8pre

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

I now see the emdash, instead of the semicolon.  I've also reviewed the other UI elements, and they match the screenshot UI at https://bugzilla.mozilla.org/attachment.cgi?id=272709, which was approved by mconnor as ui-review+

Comment 18

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

Sorry; build ID for Linux (Ubuntu) should have been: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a8pre) Gecko/2007080308 Minefield/3.0a8pre