Closed Bug 403484 Opened 12 years ago Closed 12 years ago

The ellipsis for UI should be localizable

Categories

(Core :: Internationalization, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta2

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: jp-critical)

Attachments

(2 files, 16 obsolete files)

1.79 KB, patch
mconnor
: review+
Details | Diff | Splinter Review
14.74 KB, patch
roc
: review+
Details | Diff | Splinter Review
spun out from bug 400237.

Now, XUL label and tree uses Unicode Ellipsis (U+2026) instead of three ASCII dots ("..."). This change is good thing for semantics. But this is wrong for Japanese UI. Because Japanese Ellipsis glyph is the three dots being aligned to middle of ascent and other Japanese applications are using three ASCII dots in UI. So, the ellipsis code point should be localizable.

The patch will be coming soon.
Flags: blocking1.9?
This doesn't block changes to en-US strings (bug 373623)
Blocks: 390282
No longer blocks: 373623
Attached patch Patch rv1.0 (obsolete) — Splinter Review
needs r+sr for content/ and layout/, first.
Attachment #288322 - Flags: superreview?(roc)
Attachment #288322 - Flags: review?(roc)
Attached patch Patch rv1.1 (obsolete) — Splinter Review
Oops, there is a mistake, sorry.
Attachment #288322 - Attachment is obsolete: true
Attachment #288323 - Flags: superreview?(roc)
Attachment #288323 - Flags: review?(roc)
Attachment #288322 - Flags: superreview?(roc)
Attachment #288322 - Flags: review?(roc)
Attachment #288323 - Flags: superreview?(roc)
Attachment #288323 - Flags: superreview+
Attachment #288323 - Flags: review?(roc)
Attachment #288323 - Flags: review+
Axel: Would review the toolkit part?
Attachment #288434 - Flags: review?(l10n)
Attached patch browser part of rv1.1 (obsolete) — Splinter Review
mconnor: Would you review this?
Attachment #288435 - Flags: review?(mconnor)
Attached patch mail part of rv1.1 (obsolete) — Splinter Review
scott: Would you review this?
Attachment #288437 - Flags: review?(mscott)
Attached patch seamonkey part of rv1.1 (obsolete) — Splinter Review
Neil: Would you review the seamonkey part?
Attachment #288438 - Flags: superreview?
Attachment #288438 - Flags: review?
Comment on attachment 288434 [details] [diff] [review]
toolkit part of rv1.1

r=me (doesn't need Axel's r unless I missed something_
Attachment #288434 - Flags: review?(l10n) → review+
Attachment #288435 - Flags: review?(mconnor) → review+
Attachment #288438 - Flags: superreview?(neil)
Attachment #288438 - Flags: superreview?
Attachment #288438 - Flags: review?(neil)
Attachment #288438 - Flags: review?
Attachment #288437 - Flags: review?(mscott) → review+
Excuse me if that's a stupid question, I don't know the i18n infrastructure well -- wouldn't it be easier to add pref("intl.ellipsis", "…") to all.js and let apps / locales override that pref directly? I don't see why the indirect way over the properties file is needed, and why the pref needs to be set at so many places.
Modifying prefs directly is frowned upon, it's just that we try to keep localizers from touching prefs directly as much as possible, the implications are uncomfortable. In that scenario of installed language-packs, it's likely even buggy, as all installed add-ons prefs get read, but only one should be effective.

Re comment 8, no, mconnor didn't miss anything, at least nothing I wouldn't miss myself.

Could we get a localization note to the intl.ellipsis stating where it's used and why it would be changed? Something like

# LOCALIZATION NOTE (intl.ellipsis): Use the unicode ellipsis char, \u2026,
# unless using three dots, ..., is commonly used in Software for your language.
Comment on attachment 288438 [details] [diff] [review]
seamonkey part of rv1.1

Do you need different ellipses in different applications? If so, please ask KaiRo for review on this patch.
Attachment #288438 - Flags: superreview?(neil)
Attachment #288438 - Flags: review?(neil)
(In reply to comment #13)
> Could we get a localization note to the intl.ellipsis stating where it's used
> and why it would be changed? Something like
> 
> # LOCALIZATION NOTE (intl.ellipsis): Use the unicode ellipsis char, \u2026,
> # unless using three dots, ..., is commonly used in Software for your language.

I think it's actually common everywhere. I guess we're probably among the first ones who use \u2026 here.

Perhaps it should rather say "...unless \u2026 doesn't suit traditions in your locale." or "...unless you know \u2026 doesn't suit your locale." or something like that?
(In reply to comment #14)
> (From update of attachment 288438 [details] [diff] [review])
> Do you need different ellipses in different applications?

I'm not sure, it should be decided by the leader of the product.
Comment on attachment 288438 [details] [diff] [review]
seamonkey part of rv1.1

If the product doesn't use Unicode ellipsis, "intl.ellipsis" should be "..."(three ASCII dots.)
Attachment #288438 - Flags: superreview?(kairo)
Attachment #288438 - Flags: review?(kairo)
Attached patch Patch rv1.1.1 (obsolete) — Splinter Review
The localization note is added.
Attachment #288323 - Attachment is obsolete: true
Attachment #288485 - Flags: superreview+
Attachment #288485 - Flags: review+
Comment on attachment 288438 [details] [diff] [review]
seamonkey part of rv1.1

I hate all this in principle, but that doesn't matter. I don't want to understand or use it and I'm no super-reviewer. Passing r+sr to Neil instead.
Attachment #288438 - Flags: superreview?(neil)
Attachment #288438 - Flags: superreview?(kairo)
Attachment #288438 - Flags: review?(neil)
Attachment #288438 - Flags: review?(kairo)
Comment on attachment 288438 [details] [diff] [review]
seamonkey part of rv1.1

And I still don't think we need our own ellipsis.
Attachment #288438 - Flags: superreview?(neil)
Attachment #288438 - Flags: review?(neil)
OK, the case I hate is something different, sorry. Anyways, I think it's wrong to define this for every app. We should have it in all.js and only define it in toolkit.
Comment on attachment 288441 [details] [diff] [review]
minimo part of rv1.1

this is great.

Is there a list of other required intl.* preferences.  It seams that anything that is required should be a default value (not singling intl out, there are a bunch of default prefs that aren't "defaulted" when the pref is missing)
Attachment #288441 - Flags: review?(dougt) → review+
Attached patch Patch rv1.2 (obsolete) — Splinter Review
O.K. Seamonkey should use toolkit default settings too.
Attachment #288438 - Attachment is obsolete: true
Attachment #288485 - Attachment is obsolete: true
Attachment #288505 - Flags: superreview+
Attachment #288505 - Flags: review+
(In reply to comment #22)
> Is there a list of other required intl.* preferences.  It seams that anything
> that is required should be a default value (not singling intl out, there are a
> bunch of default prefs that aren't "defaulted" when the pref is missing)

I don't know such list...
Comment on attachment 288505 [details] [diff] [review]
Patch rv1.2

>Index: browser/app/profile/firefox.js
>Index: mail/app/profile/all-thunderbird.js
>Index: xulrunner/app/xulrunner.js
>Index: calendar/sunbird/app/profile/sunbird.js

I think you probably don't need to patch those files at all when it's in all.js instead, as all of those apps pick up that all.js, IIRC.
Attached patch Patch rv1.3 (obsolete) — Splinter Review
er, right, thank you!

Now, the all needed reviews are cleared.
Attachment #288435 - Attachment is obsolete: true
Attachment #288437 - Attachment is obsolete: true
Attachment #288439 - Attachment is obsolete: true
Attachment #288440 - Attachment is obsolete: true
Attachment #288441 - Attachment is obsolete: true
Attachment #288505 - Attachment is obsolete: true
Attachment #288506 - Flags: superreview+
Attachment #288506 - Flags: review+
Attachment #288506 - Flags: approval1.9?
Attachment #288439 - Flags: review?(benjamin)
Attachment #288440 - Flags: review?(lilmatt)
I think you should let someone re-review this version. The MiniMo all.js part might still be needed, and someone should OK this approach specifically, though I think it's basically the best at least intrusive way to go here.
Doug:

(In reply to comment #27)
> The MiniMo all.js part might still be needed,

Is this right? Doesn't use Minimo modules/libpref/src/init/all.js?
blocking p3
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
Duplicate of this bug: 52978
Attached patch Patch rv1.4 (obsolete) — Splinter Review
o.k. we should use this patch.
Attachment #288506 - Attachment is obsolete: true
Attachment #288793 - Flags: superreview+
Attachment #288793 - Flags: review+
Comment on attachment 288793 [details] [diff] [review]
Patch rv1.4

mconnor:

this is changed that the all.js refers toolkit's intl.properties directly from rv1.0. I need your additional re-review.
Attachment #288793 - Flags: review?(mconnor)
Comment on attachment 288793 [details] [diff] [review]
Patch rv1.4

looks good to me.  Should think about filing a followup to clean up all.js to point to toolkit's version of stuff by default, and deprecate chrome://navigator etc in the core...
Attachment #288793 - Flags: review?(mconnor) → review+
checked-in, I'll file the bug.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
backed-out the patch, the static nsString is leaking...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Patch rv1.5 (obsolete) — Splinter Review
fix the leaking of nsAdoptingString.
Attachment #288793 - Attachment is obsolete: true
Attachment #288834 - Flags: superreview?(roc)
Attachment #288834 - Flags: review?(roc)
I feel this last change is there only to keep the leak detector happy, and nothing more. nsContentUtils::Shutdown will be only called once at application shutdown, right ? (maybe slightly more useful if "Switch Profile" calls it too).

Now if we view this from the point of view of memory fragmentation, it gets more interesting. The first time sLocalizedEllipsis is affected a value, it claims a small buffer of memory that it will never be release in the application lifetime. Typical bad fragmenting behavior if it happens at the wrong time. We know the string will always be three char or less, so it could be optimized to go in a static buffer (though it must be able to dynamically allocate a larger buffer if that assumption is broken), no dynamic allocation and no fragmentation.

I don't believe it would change much in this particular case, because it happens at startup when little of the memory allocated earlier will be released, but I think check-in should consider that kind of situation:
"I'm dynamically allocating a small chunk of memory that will stay forever => BEEP ! Fragmentation !"

Of course, if nsAdoptingString already includes that kind of optimization, I might talking for nothing ;-) Just can't find it's implementation in lxr now.
Maybe we could make the static data just be a PRUnichar[3], truncate the preference if it's longer than 3 characters, and return an nsDependentString?
Attached patch Patch rv1.6 (obsolete) — Splinter Review
Attachment #288834 - Attachment is obsolete: true
Attachment #288971 - Flags: superreview?(roc)
Attachment #288971 - Flags: review?(roc)
Attachment #288834 - Flags: superreview?(roc)
Attachment #288834 - Flags: review?(roc)
That's going to leak an nsDependentString. How about returning an nsDependentString directly (not a const reference)?
Attached patch Patch rv1.7 (obsolete) — Splinter Review
Attachment #288971 - Attachment is obsolete: true
Attachment #289121 - Flags: superreview?(roc)
Attachment #289121 - Flags: review?(roc)
Attachment #288971 - Flags: superreview?(roc)
Attachment #288971 - Flags: review?(roc)
+    PRUint32 len = PR_MIN(tmp.Length(), sizeof(sBuf) / sizeof(PRUnichar) - 1);

Use NS_ARRAY_LENTH(sBuf)

How about using CopyUnicodeTo (from nsReadableUtils.h) instead of the loop.
Attached patch Patch rv1.8 (obsolete) — Splinter Review
Attachment #289121 - Attachment is obsolete: true
Attachment #289201 - Flags: superreview?(roc)
Attachment #289201 - Flags: review?(roc)
Attachment #289121 - Flags: superreview?(roc)
Attachment #289121 - Flags: review?(roc)
Attached patch Patch rv1.8Splinter Review
Sorry, this is new patch.
Attachment #289201 - Attachment is obsolete: true
Attachment #289202 - Flags: superreview?(roc)
Attachment #289202 - Flags: review?(roc)
Attachment #289201 - Flags: superreview?(roc)
Attachment #289201 - Flags: review?(roc)
Attachment #289202 - Flags: superreview?(roc)
Attachment #289202 - Flags: superreview+
Attachment #289202 - Flags: review?(roc)
Attachment #289202 - Flags: review+
checked-in, thanks.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
I know that nsDependentStrings are small objects, and the compiler probably optimizes the copy construction away anyway, but just in case (and because of preferred style) you should probably change these lines:

  const nsDependentString kEllipsis = nsContentUtils::GetLocalizedEllipsis();

to use const references instead, like:

  const nsDependentString& kEllipsis = nsContentUtils::GetLocalizedEllipsis();
(In reply to comment #48)
> (In reply to comment #47)
> See also http://mxr.mozilla.org/firefox/search?string=8230

From that search, only the nsIndexedToHTML.cpp result is pertinent.

The search http://mxr.mozilla.org/firefox/search?string=2026 gives some apparently useful results, but they all correspond to using the default value when the search for the preference fails, so the code's already fixed.

It could be useful still to run those search again from time to time to check that new code that uses directly U+2026 hasn't been checked in.

I think nsIndexedToHTML.cpp would preferably be taken into account by opening a new bug for it, and referencing that bug from this one.
(In reply to comment #49)
> 
> I think nsIndexedToHTML.cpp would preferably be taken into account by opening a
> new bug for it, and referencing that bug from this one.
> 
Ok. I filed it as bug 411854.
Blocks: 411854
Depends on: 460441
Hi,

I submitted bug 603252 to say that "intl.ellipsis" added in this bug doesn't changes the way ellipsis should rendered in menus and labels.

I would like to make zh-TW Firefox uses U+2026 in Mac, and "..." in Windows and Linux, but this seems impossible with only one l10n repo (Japanese uses two repos - ja and ja-JP-Mac). Am I wrong?
This are two different use-cases of the ellipsis, let's have the discussion about menus in the other bug.
You need to log in before you can comment on or make changes to this bug.