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)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla0.9.3

People

(Reporter: markh, Assigned: lordpixel)

References

Details

(Keywords: l12y, Whiteboard: [nsbranch+,PDT+])

Attachments

(11 files)

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.
*** Bug 46191 has been marked as a duplicate of this bug. ***
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: disttsc → lordpixel
Keywords: patch, review
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...
>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? ;-)
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.
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* ?
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).
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
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 :-)
- 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?
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.
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? ;)
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...
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
you're right about the 9x% cases. sounds good to me. r=pink
Attached patch Fixed indentSplinter Review
*** Bug 77832 has been marked as a duplicate of this bug. ***
*** Bug 87626 has been marked as a duplicate of this bug. ***
Adding KW l12y,nsbeta1 nominating for 0.9.2 This is a big deal for localised client.
Keywords: l12y, nsbeta1
Target Milestone: --- → mozilla0.9.2
Adding Vishy to cc: list. Vishy - Who should I talk to about this one?
jag and saari.
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 ]
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
I think the code is in XPToolkit so saari or jag could help get this in.
Yep, patch still works great.
That was me (Ben Goodger)
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...
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?
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?
CC to msanz. Jaime is currently not in the office.
I'll check this in on the trunk later this evening. I can do the branch when needed.
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.
Saari, jag, can you mark it nsbranch so that it can be checked in the branch later?
Severity: minor → normal
Keywords: reviewnsBranch
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.
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.
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...
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.
Depends on: 59892
sr=blake
Checked in on the trunk.
I believe this needs a PDT+ to get it into the branch. How should we go about getting this?
Montse can you or Frank nominate this. I really think we need this fix for the German market.
Daniel, can you verify the fix in the trunk? That will help to get it into the branch. Do so asap!
Verified fixed in Trunk 2001-07-05-06-trunk
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Verified
Status: RESOLVED → VERIFIED
danielmc: Wasn't this meant to be kept open until checked in to the branch?
Verifying it so we can get it a PDT+ and have it checked into the branch
yes, reopening for checking into the branch;lordpixel, can you mark it nsbranch+?
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
I'll add nsbranch+ myself since I want to get this in the branch checkins radar
Whiteboard: nsbranch+
added vtrunk
Keywords: vtrunk
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+]
I'll check this in to the branch...
And so I did last Friday. Marking this bug fixed.
Status: REOPENED → RESOLVED
Closed: 24 years ago24 years ago
Resolution: --- → FIXED
Keywords: vtrunk
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
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.

Attachment

General

Created:
Updated:
Size: