403484 - The ellipsis for UI should be localizable

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P3)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

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.

Comment 1

[Dão Gottwald [::dao]]

This doesn't block changes to en-US strings (bug 373623)

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch rv1.0

needs r+sr for content/ and layout/, first.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch rv1.1

Oops, there is a mistake, sorry.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: toolkit part of rv1.1

Axel: Would review the toolkit part?

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: browser part of rv1.1

mconnor: Would you review this?

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: mail part of rv1.1

scott: Would you review this?

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: seamonkey part of rv1.1

Neil: Would you review the seamonkey part?

Comment 8

[Mike Connor [:mconnor]]

Comment on attachment 288434 [details] [diff] [review]
toolkit part of rv1.1

r=me (doesn't need Axel's r unless I missed something_

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: xulrunner part of rv1.1

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: calendar part of rv1.1

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: minimo part of rv1.1

Comment 12

[Dão Gottwald [::dao]]

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.

Comment 13

[Axel Hecht [:Pike]]

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 14

[[e-mail]]

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.

Comment 15

[Rimas Kudelis [use needinfo to ping]]

(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?

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

(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 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

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.)

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch rv1.1.1

The localization note is added.

Comment 19

[Robert Kaiser]

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 20

[[e-mail]]

Comment on attachment 288438 [details] [diff] [review]
seamonkey part of rv1.1

And I still don't think we need our own ellipsis.

Comment 21

[Robert Kaiser]

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 22

[Doug Turner (:dougt)]

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)

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch rv1.2

O.K. Seamonkey should use toolkit default settings too.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

(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 25

[Robert Kaiser]

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.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch rv1.3

er, right, thank you!

Now, the all needed reviews are cleared.

Comment 27

[Robert Kaiser]

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.

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

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?

Comment 29

[Mike Schroepfer]

blocking p3

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch rv1.4

o.k. we should use this patch.

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

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.

Comment 33

[Mike Connor [: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...

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

checked-in, I'll file the bug.

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

backed-out the patch, the static nsString is leaking...

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch rv1.5

fix the leaking of nsAdoptingString.

Comment 37

[Jean-Marc Desperrier]

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.

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch rv1.6

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That's going to leak an nsDependentString. How about returning an nsDependentString directly (not a const reference)?

Comment 41

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch rv1.7

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    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.

Comment 43

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch rv1.8

Comment 44

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch rv1.8

Sorry, this is new patch.

Comment 45

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

checked-in, thanks.

Comment 46

[jag (Peter Annema)]

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();

Comment 47

[Masahiro YAMADA]

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

Comment 48

[Masahiro YAMADA]

(In reply to comment #47)
See also http://mxr.mozilla.org/firefox/search?string=8230

Comment 49

[Jean-Marc Desperrier]

(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.

Comment 50

[Masahiro YAMADA]

(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.

Comment 51

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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?

Comment 52

[Axel Hecht [:Pike]]

This are two different use-cases of the ellipsis, let's have the discussion about menus in the other bug.