Closed Bug 407481 Opened 12 years ago Closed 12 years ago

Unicode ellipsis to be used in code as well

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 3 beta3

People

(Reporter: hendrik, Assigned: masa141421356)

References

Details

Attachments

(2 files, 1 obsolete file)

Following bug 373623, also in the places where ellipses are introduced programmatically, to shorten long strings, the unicode symbol should be used.
Attachment #292213 - Flags: review?(mconnor)
So, you should probably use the new "intl.ellipsis" pref here instead of hardcoding the ellipsis.
Assignee: nobody → hendrik.maryns
No longer blocks: 373623
Blocks: 373623
Yes, this sounds like a case for intl.ellipsis - esp. as there's actually something shortended there. And all such characters should go through some place in localization anyways, as in some languages (like Japanese), the unicode ellipsis is not rendered in the way we want it and needs to be replaced with dots (which, of course, can nicely be done through intl.ellipsis).
Comment on attachment 292213 [details] [diff] [review]
introduces … in nsContextMenu.js

per previous comments
Attachment #292213 - Flags: review?(mconnor) → review-
Duplicate of this bug: 408982
Attached patch Patch using intl.ellipsis rv1.0 (obsolete) — Splinter Review
(In reply to comment #5)
> Created an attachment (id=298444) [details]
> Patch using intl.ellipsis rv1.0

Looks good to me, in as far as I can tell with my limited knowledge of JS.  You’ll want to ask someone to review this, Gavin, I suppose.

I am sorry not to have tackled this myself.  I had a try, but it didn’t work, and then christmas holidays and computer trouble hindered me from getting to it.
Attachment #298444 - Flags: review?(gavin.sharp)
Comment on attachment 298444 [details] [diff] [review]
Patch using intl.ellipsis rv1.0

>Index: browser/base/content/nsContextMenu.js

I'd just use: 
>+    this.ellipsis = "\u2026"; // default value
>+    try {
>+      this.ellipsis = gPrefService.getComplexValue("intl.ellipsis",
>+                                            Ci.nsIPrefLocalizedString).data;
>+    } catch (e) { }

and put it directly in the nsContextMenu constructor.

The rest looks fine. I'll r+ a new patch with that change made.
Attachment #298444 - Flags: review?(gavin.sharp)
Attachment #298444 - Attachment is obsolete: true
Attachment #298659 - Flags: review?(gavin.sharp)
Attachment #298659 - Flags: review?(gavin.sharp) → review+
Comment on attachment 298659 [details] [diff] [review]
Patch using intl.ellipsis rv2.0

Use the correct ellipsis in code, too.
Attachment #298659 - Flags: approval1.9?
(In reply to comment #9)
> (From update of attachment 298659 [details] [diff] [review])
> Use the correct ellipsis in code, too.
> 
Is it needed to replace "\u2026" as "…" ?
(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 298659 [details] [diff] [review] [details])
> > Use the correct ellipsis in code, too.
> Is it needed to replace "\u2026" as "…" ?

Ah, that comment was addressed to drivers, not to you. Don't worry about that comment. :)
Assignee: hendrik.maryns → masa141421356
Status: NEW → ASSIGNED
Sorry my patch contains CR-LF, please replace it as LF.
(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 298659 [details] [diff] [review] [details])
> > Use the correct ellipsis in code, too.
> > 
> Is it needed to replace "\u2026" as "…" ?

Maybe not, but I don’t see why not?
Gavin, is that change ok?
The current patch is fine.
Comment on attachment 298659 [details] [diff] [review]
Patch using intl.ellipsis rv2.0

a=beltzner for 1.9 ... :)
Attachment #298659 - Flags: approval1.9? → approval1.9+
Checking in browser/base/content/nsContextMenu.js;
/cvsroot/mozilla/browser/base/content/nsContextMenu.js,v  <--  nsContextMenu.js
new revision: 1.32; previous revision: 1.31
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 M11
Verified at Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008012404 Minefield/3.0b3pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.