Closed Bug 412793 Opened 17 years ago Closed 16 years ago

update.properties could use comments

Categories

(Toolkit :: Application Update, defect)

defect
Not set
minor

Tracking

()

RESOLVED DUPLICATE of bug 357145

People

(Reporter: Pike, Unassigned)

References

()

Details

Attachments

(1 file)

update.properties is a pretty cryptic file to localize, it's full of string and parameter substitutions without any comment whatsoever.

This is somewhat split off of bug 391598, but that one just added a few more undocumented strings.
Attached patch add notesSplinter Review
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #297938 - Flags: review?(benjamin)
Attachment #297938 - Flags: review?(l10n)
Comment on attachment 297938 [details] [diff] [review]
add notes

You're removing some of the strings, not just adding l10n notes:

+# LOCALIZATION NOTE (updateMoreInfoContentDownloading): First %s: product name (brandShortName), second %s: version identifier (e.g. 3.0.1)
 updateMoreInfoContentDownloading=Getting more details about %S %S…
-statusSucceededFormat=Installed on: %S
-statusFailed=Install Failed
...
-rateFormat= at #1
-progressFormat=#1#2
bug 405044 removed the use of statusSucceededFormat and statusFailed, AFAICT (and mxr) without removing the entities. The other two formats show up as removed lines in the diff, but are added, too, so those are just artifacts of diff.
Is our update naming story really as inconsistent as lxr makes me think? I.e., nsIUpdate.name is defined to be what updateName is, or what <update> said in the xml, but we're not using that value most of the time, and head back to format via updateName again? Or is that intentional?

updateFullName on the other hand is used in http://mxr.mozilla.org/mozilla/source/toolkit/mozapps/update/content/history.js#66, and does reference update.name, i.e., could, if we set it, show something different to "Firefox (3.0.1)".
(In reply to comment #5)
> Is our update naming story really as inconsistent as lxr makes me think? I.e.,
> nsIUpdate.name is defined to be what updateName is, or what <update> said in
> the xml, but we're not using that value most of the time, and head back to
> format via updateName again? Or is that intentional?

I think we are actually using update.name most of the time, but maybe not all over where we could. The direct use of updateName instead of update.name in update.js does look wrong to me.

> updateFullName on the other hand is used in
> http://mxr.mozilla.org/mozilla/source/toolkit/mozapps/update/content/history.js#66,
> and does reference update.name, i.e., could, if we set it, show something
> different to "Firefox (3.0.1)".

updateFullName should look like this: "Firefox 3.0.1 (20080401)", unless the update server sets a name: "April Security Update for Firefox 3 (20080401)".
Comment on attachment 297938 [details] [diff] [review]
add notes

r-, most importantly, the comments should all talk about %S instead of %s.

I'm wondering, would you mind adding explicit numbering to the entries with multiple %S? It's easier to read the comment "%1$S is brandShortName", in particular when you're updating a localization that changed the order. Doing so will be purely internal to this file, it will blow up the patch size, though.

Could we come up with a better story for updateName? Something like is updateName or the name sent from AUS. Or so. I'm not happy with that myself, I just feel awkward hiding the variety of input that AUS can be configured to insert here.

Mind filing the bug on the update.js updateName use?
Attachment #297938 - Flags: review?(l10n) → review-
Attachment #297938 - Flags: review?(benjamin)
Product: Firefox → Toolkit
Assignee: dao → nobody
Status: ASSIGNED → NEW
There are a decent amount of comments now and the remaining work can be finished up in bug 357145
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: