Closed Bug 680812 Opened 8 years ago Closed 8 years ago
Excessive space between "280" and "KB" in download dialog
Component: General → File Handling
Product: Core → Firefox
QA Contact: general → file.handling
Patch to remove excessive space between file size and unit. Patch on changeset:75619:482742e6fff7
Huh. Why does it look like there's still a space between "280" and "KB"? The visual effect is correct (as, in ui-r+ for that screenshot), but from looking at the various .properties and APIs involved, I'd expect there to be no space. Could you do a little debugging to see if there is actually still a space in there, and where's in coming from? We may want to kill _that_ space, and leave .properties file alone. (because I suspect you'll need to change the string name, so that localizers know to update their version to no include the space.)
There is a space between 280 and KB and its coming from fileSizeWithType in unknownContentType.properties.
Patch against changeset:75619:482742e6fff7
Screenshot of Patch
Attachment #554796 - Attachment is obsolete: true
Now I'm even more confused. What's the difference between the first two patches in this bug, and why are the screenshots different? The original screenshot (attachment 553174 [details]) shows current Firefox with a whole lot of space between the number and unit. The first patch removes the 1 space in the .properties file. The screenshot (attachment 554796 [details]) looks like the correct fix to me (space between number and unit, but not too much... "just right"). But I was asking in comment 3 where the just-right amount of space is coming from. The second patch looks almost identical to the first patch (removes the 1 space), but now it's screenshot (attachment 555056 [details]) shows the number and unit touching each other (no space at all, looks too tight).
I just double-checked with DOM Inspector on a current trunk OS X nightly. The string in the dialog is "Preview Document (3.2 MB)" -- there are definitely 2 spaces between the number and unit. The string in unknownContentType.properties accounts for 1 of the spaces, where's the other coming from?
The string in unknowContentType.properties was (%2S %3S). One space was coming from the space between %2s and %3S and another was from the number "3". In attachment (attachment 554796 [details]) I removed the space between %2S and %3S. So there was only 1 space left between "280" and "KB". After removing the number "3" from "%3S" that 1 space is also deleted and there is no space at all in "280" and "KB". I also feel second patch looks very tight. First patch (attachment 554794 [details] [diff] [review]) seems correct fix to me.
Hmm, there shouldn't be any difference between "%1S %2S" and %S %S", the number are just to allow disambiguation for locales which need to change the order thof things (eg "%2S %1S").
Yeah, number makes a difference and culprit is "fill2" function in nsTextFormatter.cpp file (line 142: rv = (*ss->stuff)(ss, &space, 1); ). Main cause is: variable 'width' passed to "fill2" as a parameter is calculated as "width = (width * 10) + (c - '0'); " Therefore when 'c' is 3, value of 'width' become 3 and data value of %3S is "KB". That is just 2 chars in length. Therefore, one extra space is inserted. The number of spaces to insert are calculated using expression "width -= srclen;" So if data value of %3S is less than the number 3 then the number of spaces inserted are equal to the difference between the (number - data_value_lenth). In our case the difference is 1 therefore one space is inserted. However, if we don't use number then there will be no extra space insertion occurs.
I suspect if there is any %9S and data value of it is just 2 chars in a length then number of spaces inserted will be 7.
Dolske: I don't know why that Right adjusting done in "fill2" is required?. Are we still going with patch 1 for this bug?
patch with localization note to remove an extra space.
Holy schmoley. I also traced the code flow, and was a little surprised to see we treat these as printf-style formatters! But that's actually the problem. The correct (and normal) way to specify this stuff is like this: label=My %1$S is full of %2$S not label=My %1S is full of %2S No only does the latter lead to formatting glitches like this, but it also doesn't work for localization... "My %2S is full of %1S" won't print the arguments in reverse order. Thus, the correct fix here would be: -fileSizeWithType=%1S (%2S %3S) +fileSizeWithType=%1$S (%2$S %3$S) (This string has been broken since bug 595888 first added it). I did a quick MXR regex search for "%[0-9][^\$]", looks like there are a couple other questionable uses like this in other property files. Pike, should be rev the entity name here to make sure localizers didn't just cut'n'paste the wrong string? I guess so, a quick l10n MXR shows all (17) locales just copied this as-is. More terrifying is that the regex search finds a number of busted-looking strings, though some look unused. I'll file a new bug for that in a moment.
Comment on attachment 558727 [details] [diff] [review] Patch against changeset:75619:482742e6fff7 r- per previous comment. But HUGE KUDOS for helping to discover the root cause of what was happening here! I look forward to the patch I suggested so we can close out this seemingly-trivial issue! :-)
Attachment #558727 - Flags: review?(dolske) → review-
For central, we should do the key change, I suggest something like orderedFileSizeWithType. Sadly, this is already on aurora. Aurora is string frozen, so we can't take that patch on aurora. As the change is cosmetic in nature, I'd be fine with landing the value-only change on aurora and post to .l10n to invite people to fix that in their locales, too. Nothing deep, we'll catch the real fix in the next cycle then.
Comment on attachment 559042 [details] [diff] [review] Patch for Aurora r+ing this as the version suitable to land on Aurora. For mozilla-central we should change the label from "fileSizeWithType" to something else, as Pike suggested in the last comment. So sorry this has dragged out, but at least we're in the final stretch and made an interesting discovery along the way! Can you do the patch for mozilla-central too as described above?
Modification done according to comment 18.
Comment on attachment 559819 [details] [diff] [review] Patch on changset: 75619:482742e6fff7 Thanks!
Attachment #559819 - Flags: review?(dolske) → review+
Landed on inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/b3eb728a2d54
Target Milestone: --- → Firefox 9
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Attachment #559042 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This has been approved for aurora but hasn't landed. Please land there ASAP if you would like to get this in Fx10.
Comment on attachment 559042 [details] [diff] [review] Patch for Aurora Actually, jared pointed out that this made it into central when Fx9 was there, so this is already in the repo. I'm clearing the aurora approval.
Attachment #559042 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.