Closed Bug 403484 Opened 12 years ago Closed 12 years ago
The ellipsis for UI should be localizable
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.
needs r+sr for content/ and layout/, first.
Oops, there is a mistake, sorry.
Axel: Would review the toolkit part?
Attachment #288434 - Flags: review?(l10n)
mconnor: Would you review this?
Attachment #288435 - Flags: review?(mconnor)
scott: Would you review this?
Attachment #288437 - Flags: review?(mscott)
Neil: Would you review the seamonkey part?
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+
12 years ago
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.
(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.)
The localization note is added.
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.
Comment on attachment 288438 [details] [diff] [review] seamonkey part of rv1.1 And I still don't think we need our own ellipsis.
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+
O.K. Seamonkey should use toolkit default settings too.
(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.
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+
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.
12 years ago
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?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
o.k. we should use this patch.
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 → ---
fix the leaking of nsAdoptingString.
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, truncate the preference if it's longer than 3 characters, and return an nsDependentString?
That's going to leak an nsDependentString. How about returning an nsDependentString directly (not a const reference)?
+ 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.
Sorry, this is new patch.
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 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();
I think this code should be fixed too. http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/netwerk/streamconv/converters/nsIndexedToHTML.cpp&rev=1.88#828
(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.
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.