Closed Bug 388506 Opened 14 years ago Closed 14 years ago

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

Categories

(Toolkit :: Downloads API, enhancement)

enhancement
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9alpha7

People

(Reporter: Mardak, Assigned: Mardak)

References

()

Details

Attachments

(3 files, 4 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
"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|"
Attachment #272704 - Flags: review?(sdwilsh)
Attached image screenshot (obsolete) —
Attached patch v2 (obsolete) — Splinter Review
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
Attachment #272704 - Attachment is obsolete: true
Attachment #272708 - Flags: review?(sdwilsh)
Attachment #272704 - Flags: review?(sdwilsh)
Attached image v2 screenshot
Attachment #272705 - Attachment is obsolete: true
Attachment #272709 - Flags: ui-review?
Attachment #272709 - Flags: ui-review? → ui-review?(beltzner)
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
Attachment #272708 - Flags: review?(sdwilsh) → review+
Attached patch v3Splinter Review
Fixed nits and added better/more examples in downloads.properties
Attachment #272708 - Attachment is obsolete: true
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.
Attachment #272709 - Flags: ui-review?(beltzner) → ui-review+
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite-
Flags: in-litmus?
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 M7
"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".
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.
Edward - please address these concerns.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch l10n notes v1 (obsolete) — Splinter Review
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: ..."
Attachment #273133 - Flags: review?(sdwilsh)
Attachment #273133 - Flags: review?(l10n)
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)
Attachment #273133 - Flags: review?(sdwilsh) → review+
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.
Attachment #273133 - Flags: review?(l10n) → review+
Attached patch l10n notes v1.1Splinter Review
(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.
Attachment #273133 - Attachment is obsolete: true
Keywords: checkin-needed
mozilla/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties  1.6
mozilla/toolkit/mozapps/downloads/content/DownloadProgressListener.js        1.16
Keywords: checkin-needed
Depends on: 389569
Bug was reopened for localization changes which have been checked in.
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
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+
Status: RESOLVED → VERIFIED
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
Blocks: 388517
No longer blocks: 377792
Flags: in-litmus? → in-litmus-
No longer depends on: 426409
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.