Closed
Bug 70048
Opened 24 years ago
Closed 24 years ago
Text of modifiers on accelerator keys can't be localized.
Categories
(Core :: XUL, defect)
Core
XUL
Tracking
()
VERIFIED
FIXED
mozilla0.9.3
People
(Reporter: markh, Assigned: lordpixel)
References
Details
(Keywords: l12y, Whiteboard: [nsbranch+,PDT+])
Attachments
(11 files)
6.78 KB,
patch
|
Details | Diff | Splinter Review | |
334 bytes,
text/plain
|
Details | |
457 bytes,
text/plain
|
Details | |
6.78 KB,
patch
|
Details | Diff | Splinter Review | |
6.63 KB,
patch
|
Details | Diff | Splinter Review | |
6.66 KB,
patch
|
Details | Diff | Splinter Review | |
6.85 KB,
patch
|
Details | Diff | Splinter Review | |
469 bytes,
text/plain
|
Details | |
6.85 KB,
patch
|
Details | Diff | Splinter Review | |
6.80 KB,
patch
|
Details | Diff | Splinter Review | |
11.93 KB,
patch
|
Details | Diff | Splinter Review |
The modifiers for accelerator keys (eg, "Ctrl", Shift" etc.) are currently hard-
coded into
http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsMenuFrame.cpp
Discussions with jag indicate the appropriate solution is to use the same
string-bundle for the modifiers as we do for other key names. The best way to
arrange this would be to have a single, static nsIStringBundle shared (with ref
counting) among all instances of nsMenuFrame.
It is worth noting that this same restructing would also allow us to
translate " " to "Space" in a less intrusive manner.
Assigned to jag simply because he wrote that code, but I am happy to roll up
some patches as time permits.
Assignee | ||
Comment 2•24 years ago
|
||
Assignee | ||
Comment 3•24 years ago
|
||
Patch loads all modifier keys from a string bundle stored in xpfe/global/
resources/locale/en-US/platformKeys.properties (and en-US/mac/
platformKeys.properties).
Assignee | ||
Comment 4•24 years ago
|
||
Assignee | ||
Comment 5•24 years ago
|
||
Comment 6•24 years ago
|
||
I think that instead of using the destructor you should use |Destroy| to do the
cleaning up. Not sure though, so I'll see if I can find someone who might...
Some nits:
Index: mozilla/layout/xul/base/src/nsMenuFrame.h
+ virtual ~nsMenuFrame(void);
The |void| there is rather quaint and not needed.
Index: mozilla/layout/xul/base/src/nsMenuFrame.cpp
+ rv =
bundleService->CreateBundle("chrome://global/locale/platformKeys.properties",
+ nsnull,
+ getter_AddRefs(bundle));
Not sure if you can see it in this comment, but in the patch it looks like your
indentation is off by two for the second and third parameter.
@@ -1215,4 +1277,6 @@
}
+
+
// See if we have a key node and use that instead.
nsAutoString keyValue;
??
Index: mozilla/xpfe/global/resources/locale/en-US/mac/jar.mn
@@ -2,2 +2,4 @@
locale/en-US/global/platformGlobalOverlay.dtd
locale/en-US/global/platformDialogOverlay.dtd
++ locale/en-US/global/platformKeys.properties
+
\ No newline at end of file
You shouldn't be adding those. I don't think they hurt in jar.mn, in .h/.cpp
they would, so keep an eye out :-)
Looks fine to me otherwise. r=jag if you fix those nits.
cc'ing dbaron with a request for review of the string code. He's quite a bit
more skilled in these things and might also be able to answer the Destroy vs.
destructor question.
I don't think it really matters whether you use the destructor or |Destroy|.
|Destroy| calls the destructor, so there shouldn't be any cases where they're
called different numbers of times. You should ask whoever owns the code...
The string code looks fine, although I wonder if you really want to store 5
nsString objects (at 20 bytes each?) in memory when you could get away with just
raw PRUnichar* (or am I evil to suggest that?). Not that 100 bytes really makes
a difference...
Assignee | ||
Comment 8•24 years ago
|
||
>Some nits:
> // See if we have a key node and use that instead.
> nsAutoString keyValue;
>??
This is cause the diff was done diff -u -2 (i.e. 2 lines of context. The lines in
the patch with no + or - signs are existing lines near the changes. I did this
cause I got slapped on the original patch for not doing it! :)
Index: mozilla/xpfe/global/resources/locale/en-US/mac/jar.mn
>@@ -2,2 +2,4 @@
> locale/en-US/global/platformGlobalOverlay.dtd
> locale/en-US/global/platformDialogOverlay.dtd
>++ locale/en-US/global/platformKeys.properties
>+
>\ No newline at end of file
>You shouldn't be adding those. I don't think they hurt in jar.mn, in .h/.cpp
>they would, so keep an eye out :-)
Um, the newline at the end? I thought we specifically said we *do* have newlines
at the end of files:
http://www.mozilla.org/hacking/portable-cpp.html#put_new_line_at_eof
I'll get to the other stuff shortly.
Its been suggested Pinkerton owns XPMenus... Mike guilty/not guilty? ;-)
Assignee | ||
Comment 9•24 years ago
|
||
The two comments jag made that you had questions on above were:
* why did you insert two extra blank lines?
* You shouldn't have "no newline at end of file". This is probably because
you put spaces after your last newline.
Assignee | ||
Comment 11•24 years ago
|
||
Assignee | ||
Comment 12•24 years ago
|
||
Mike P just asked me to add a note saying what we want him to look at:
* should we cleanup in ~MenuFrame or in Destroy()
* should we store static nsString objects or raw PRUnichar* ?
Comment 13•24 years ago
|
||
No wonder I didn't see anything about this bug lately!
So anyway, I guess the choice for nsString* vs PRUnichar* is that the latter
will make us create a wrapper object for each |Equals| (which is an
implementation detail, but in this case nice to know, I think).
Assignee | ||
Comment 14•24 years ago
|
||
Comment 15•24 years ago
|
||
a couple of comments:
- cleanup in dtor is fine by me
- how long are the strings being kept in the nsString's? if they're short, we
should be using nsAutoString to avoid the alloc for the object and then another
alloc for the string buffer.
- since there aren't that many of these strings, i don't think it matters if you
use nsString's or PRUnichar*'s. Does String have much space overhead? Probably
not enough to fret over in this instance. If ever frame had these strings, the
answer would be obvious, but since they're a one-time hit...
r=pink with those answers
Comment 16•24 years ago
|
||
The strings are kept from the moment the first nsMenuFrame is created till the
moment the last one is destroyed.
I'd say let's go with the patch as is :-)
Comment 17•24 years ago
|
||
- how long are the strings being kept in the nsString's?
Doh! That's a question about the size, not the timespan.
Small, much smaller than the 64 bytes reserved by nsAutoString (< 10 chars
typically).
But, can nsAutoString be used with statics?
Comment 18•24 years ago
|
||
Yes it can. But, per jst, it's cheaper to just use nsString in this case. For an
nsAutoString, the string object (including its internal 64 chars buffer) would
be allocated, and since we're typically only using <= 5 chars of that, we'd be
throwing away ~59 chars.
Comment 19•24 years ago
|
||
r=pink
lordpixel: are you sure that the glyphs you picked for the mac properties file
exist in all fonts? Or at least any of the fonts a user might have as their
system font? have you tried this in modern, where they use some totally random
font that I still dont quite like? ;)
Assignee | ||
Comment 20•24 years ago
|
||
Assignee | ||
Comment 21•24 years ago
|
||
Assignee | ||
Comment 22•24 years ago
|
||
Current patch is now attachment 28572 [details] [diff] [review], along with new files:
28573=xpfe/global/resources/locale/en-US/mac/platformKeys.properties and
26704=xpfe/global/resources/locale/en-US/platformKeys.properties
Pinkerton, with the classic skin, the system font is used as so the user is
limited to the magic 7 fonts with the right glyphs (Charcoal, Chicago, Gadget
etc).
With the Modern Skin, on Mac OS at least, the font used is also one of the "magic
7" so it displays right too.
There's still the issue of third party skins, but Jag says he thinks some work is
going in to allow platform specific files to easily be added to those skins by
their authors. At this point I think this has simply become an advocacy issue.
Since this is correct for the 9X% of users who'll never use more than Modern or
Classic I think it should go in as it is, since none of us can think of a way to
force sensible behaviour (i.e. printing something better than a box char []) on
3rd party skins that don't use the Menu font...
Comment 23•24 years ago
|
||
locale/en-US/global-platform/platformDialogOverlay.dtd
+#this should not go into global-platform as its an override of the XP version
used for
+#windows/os2/unix. See platformKeys.properties in the parent directory
+en-US.jar:
++ locale/en-US/global/platformKeys.properties
It looks like your indent is off by one :)
r=jag
Comment 24•24 years ago
|
||
you're right about the 9x% cases. sounds good to me. r=pink
Assignee | ||
Comment 25•24 years ago
|
||
Assignee | ||
Comment 26•24 years ago
|
||
*** Bug 77832 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 27•24 years ago
|
||
Comment 28•24 years ago
|
||
*** Bug 87626 has been marked as a duplicate of this bug. ***
Comment 29•24 years ago
|
||
Adding KW l12y,nsbeta1 nominating for 0.9.2
This is a big deal for localised client.
Comment 30•24 years ago
|
||
Adding Vishy to cc: list.
Vishy - Who should I talk to about this one?
Comment 31•24 years ago
|
||
jag and saari.
Assignee | ||
Comment 32•24 years ago
|
||
Or me! Last time I tried this it still compiled. If desired I can respin the
patch sometime this week.
[ I've been travelling for most of the last month but I'm back now ]
Assignee | ||
Comment 33•24 years ago
|
||
Retargetting as Asa asked.
I have no notes on what needs to happen next to get this in. Is Ben G module
owner for this? (that's a vague memory)
Target Milestone: mozilla0.9.2 → mozilla0.9.3
Comment 34•24 years ago
|
||
I think the code is in XPToolkit so saari or jag could help get this in.
Comment 35•24 years ago
|
||
Yep, patch still works great.
Comment 36•24 years ago
|
||
Comment 37•24 years ago
|
||
That was me (Ben Goodger)
Assignee | ||
Comment 38•24 years ago
|
||
I still can't checkin (unavoidable, I've been travelling for most of the last
month and meetings and paperwork simply haven't happened). So someone else will
have to do the deed. I'll mail drivers@mozilla now...
Assignee | ||
Comment 39•24 years ago
|
||
Just noticed we don't need a= for trunk checkins anymore.
I asked drivers to approve checkin to 0.9.2 since I think that's what the l10n
team want?
Comment 40•24 years ago
|
||
I'd like to get this into the branch. It has a big impact on Euro localisation.
Particularly German. Jaime, can you see if we can get nsbranch?
Comment 41•24 years ago
|
||
CC to msanz. Jaime is currently not in the office.
Comment 42•24 years ago
|
||
I'll check this in on the trunk later this evening. I can do the branch when needed.
Comment 43•24 years ago
|
||
danielmc, just to make sure we're talking about the same thing:
this patch allows us to make menu items etc. say "Shift-Control-C" through a
localizable platformKeys.properties, instead of the currently hardcoded
"Shift+Ctrl+C".
I think there is also a bug (and if not, it _should_ be filed) on moving the
accelerator key modifiers out of XUL into DTD, but this patch doesn't address that.
Again, just to make sure we're talking about the same thing, as the summary on
this bug was misleading. I'll update the summary to something better (I hope).
Summary: Modifiers on accelerator keys can't be localized. → Text of modifiers on accelerator keys can't be localized.
Comment 44•24 years ago
|
||
Saari, jag, can you mark it nsbranch so that it can be checked in the branch later?
Updated•24 years ago
|
Comment 45•24 years ago
|
||
Bah, the platform l10n mechanism changed from underneath us. Somehow that didn't
show up in my testing until after I landed (darn you, Murphy).
And of course no one spotted that appending the modifier texts for meta and alt
were switched.
I've backed out the changes to nsMenuFrame.* for now. Fixing this bug correctly
is easy enough for Windows and Linux (already have it working there), but I have
some questions about the Mac part of my fix.
Comment 46•24 years ago
|
||
So my "plan" is to move/copy the generic platformKeys.properties into the os2,
win and unix directories, adjust the jar.mn files appropriately, put this file
in global-platform/locale/ instead of global/locale/, fix nsMenuFrame.cpp to
look in global-platform and fix the alt/meta swap. That covers unix and Windows,
but on Mac I believe this will result in the incorrect symbol being displayed
(command when we want option, and option when we want command). And I'm not
quite sure how to fix that, since that currently seems to be "broken" too.
Assignee | ||
Comment 47•24 years ago
|
||
Jag, I'm not following what you mean is broken? On what level have alt and meta
been swapped, and is this purely an accident or was it a design change?
What problem do you perceive on Mac OS specifically? The symbols use depend on
the file in this attachment:
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=28573
But I'm at a loss because I don't understand what you mean when you say they're
swapped...
Comment 48•24 years ago
|
||
Comment 49•24 years ago
|
||
So this patch implements my above "plan". It turns out that there's no problem
with the mac at all, merely some slight confusion due to exposure to that
day star.
Comment 50•24 years ago
|
||
r=pink
Comment 51•24 years ago
|
||
sr=blake
Comment 52•24 years ago
|
||
Checked in on the trunk.
Comment 53•24 years ago
|
||
I believe this needs a PDT+ to get it into the branch. How should we go about
getting this?
Comment 54•24 years ago
|
||
Montse can you or Frank nominate this. I really think we need this fix for the
German market.
Comment 55•24 years ago
|
||
Daniel, can you verify the fix in the trunk? That will help to get it into the
branch. Do so asap!
Comment 56•24 years ago
|
||
Verified fixed in Trunk 2001-07-05-06-trunk
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
danielmc: Wasn't this meant to be kept open until checked in to the branch?
Comment 59•24 years ago
|
||
Verifying it so we can get it a PDT+ and have it checked into the branch
Comment 60•24 years ago
|
||
yes, reopening for checking into the branch;lordpixel, can you mark it nsbranch+?
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Comment 61•24 years ago
|
||
I'll add nsbranch+ myself since I want to get this in the branch checkins radar
Whiteboard: nsbranch+
Comment 63•24 years ago
|
||
mark PDT+ to check into the branch. If possible, pls get this in for Friday am
builds.
lordpixel - can you check into the branch? If not, pls ask ftang@netscape to
assist you to do so. Thanks.
Whiteboard: nsbranch+ → [nsbranch+,PDT+]
Comment 64•24 years ago
|
||
I'll check this in to the branch...
Comment 65•24 years ago
|
||
And so I did last Friday. Marking this bug fixed.
Status: REOPENED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
Comment 66•24 years ago
|
||
verified fixed for _XP_ menus on mac/linux/win32 7/20 branch builds.
I take that this is not intended to cover the native Mac system menu (I think
that is what jag had mentioned to me).
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 67•24 years ago
|
||
Yes, this is XP menus only.
Native menus are seperate, and work fine.
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: jrgmorrison → xptoolkit.widgets
You need to log in
before you can comment on or make changes to this bug.
Description
•