Remove openHelpMac.commandkey, and hardcode modifiers

RESOLVED FIXED in Firefox 3.6a1

Status

()

defect
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: masayuki, Assigned: steffen.wilberg)

Tracking

Trunk
Firefox 3.6a1
All
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

spinning off from bug 432112.

openHelpMac.commandkey and openHelpMac.modifiers are not needed now.

And openHelpMac.frontendCommandkey and openHelpMac.frontendModifiers should be renamved.
Posted patch patch (obsolete) — Splinter Review
As you requested in bug 432112 comment 6.
Assignee: nobody → steffen.wilberg
Attachment #341755 - Flags: review?(gavin.sharp)
Attachment #341755 - Attachment is patch: true
Attachment #341755 - Attachment mime type: application/octet-stream → text/plain
Status: NEW → ASSIGNED
Every locale uses "accel" for openHelpMac.frontendModifiers, preferencesCmdMac.modifiers, and hideThisAppCmdMac.modifiers,
apart from those which managed to translate "accel" to
"engetela", "അക്സെല്‍", "முடுக்கப்படு", "gijimisa", or "ఎక్సెల్"...
http://mxr.mozilla.org/l10n-central/search?string=frontendModifiers
http://mxr.mozilla.org/l10n-central/search?string=preferencesCmdMac.modifiers
http://mxr.mozilla.org/l10n-central/search?string=hideThisAppCmdMac.modifiers
So I hardcoded "accel" here.

hideOtherAppsCmdMac.modifiers is mostly "accel,alt", except el (Greek) and he (Hebrew), which have "accel,shift". That might be a bug or intention, so I'll leave that alone.
Attachment #341755 - Attachment is obsolete: true
Attachment #341766 - Flags: review?(gavin.sharp)
Attachment #341755 - Flags: review?(gavin.sharp)
We could also hardcode "VK_F1" and "?", which are used by every locale.
At least these didn't get translated...
Comment on attachment 341766 [details] [diff] [review]
also hardcodes a bunch of "accel" modifiers

You missed the fact that "be" uses accel,shift for preferencesCmdMac.modifiers and hideThisAppCmdMac.modifiers.

I'm not sure that relying on MXR results to determine whether something needs to be localizable is a good thing - these might just be bugs in the locales (the translations of "accel" obviously are). Axel should make the call on this, I guess. The l10n note is a good idea either way.

You changed the id of key_openHelpMacFrontend but didn't update menu_openHelp's reference to it.
Attachment #341766 - Flags: review?(gavin.sharp) → review-
You overlooked "be".

I see a change that explicitly changes from accel to accel,shift for hideThisAppCmdMac.modifiers and others in http://bonsai-l10n.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/l10n&subdir=/l10n/be/browser/chrome/browser&command=DIFF_FRAMESET&file=baseMenuOverlay.dtd&rev2=1.4&rev1=1.3

Siarhei, do you remember why you did that change?

That said, I'm not the best advisor when it comes down to international keyboard layouts, that's one of the things I hardly know anything about. Commandkey l10n is an "interesting" topic in itself, so while it's usually a good idea to not change anything about those, some locales need to do so. Whether the localizability should include the modifier or not is something that I don't know. I'd say "if one can, the other one should, too", so in case you want to expose hideAllOthers' modifier keys, you should expose them for hideThisApp's, too.

Simon, any opinions from your side?
(In reply to comment #6)
> Siarhei, do you remember why you did that change?

I don't remember why I did the change. Probably, autotranslate proposed the "translation" and I didn't check it. I will revert the values to the original ones.
(In reply to comment #5)
> You missed the fact that "be" uses accel,shift for preferencesCmdMac.modifiers
> and hideThisAppCmdMac.modifiers.
"be" now uses "accel" like everyone else: http://hg.mozilla.org/l10n-central/be/rev/4139f74032ff

And "el" changed to "accel,alt" for hideOtherAppsCmdMac.modifiers as well: http://hg.mozilla.org/l10n-central/el/rev/de93db7dbb70

So apart from the bogus translations of "accel", "he" is the only locale which uses a different modifier, i.e. "accel,shift" instead of "accel,alt" for hideOtherAppsCmdMac.modifiers.
And that is a bug as well, since "he" only differs in browser code, but uses the correct "accel,shift" in mail and calender code:
http://mxr.mozilla.org/l10n-central/search?string=hideOtherAppsCmdMac.modifiers&find=%2Fhe%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=l10n-central

So I'm going to hardcode each and every keyboard modifier.
Posted patch harcode all modifiers (obsolete) — Splinter Review
(In reply to comment #5)
> You changed the id of key_openHelpMacFrontend but didn't update menu_openHelp's
> reference to it.
Grrr, fixed.
Attachment #341766 - Attachment is obsolete: true
Attachment #345801 - Flags: review?(gavin.sharp)
Changing the meaning of entities without renaming them is just asking for trouble, so I'm changing openHelpMac.commandkey (which is "/" right now), to helpMac.commandkey (which will be "?").
Attachment #345801 - Attachment is obsolete: true
Attachment #345846 - Flags: review?(gavin.sharp)
Attachment #345801 - Flags: review?(gavin.sharp)
Attachment #345846 - Flags: review?(gavin.sharp) → review+
Posted patch unbitrottedSplinter Review
Patch context has changed.
Attachment #345846 - Attachment is obsolete: true
Keywords: checkin-needed
Summary: openHelpMac.commandkey and openHelpMac.modifiers should be removed → Remove openHelpMac.commandkey, and hardcode modifiers
http://hg.mozilla.org/mozilla-central/rev/30c742669856
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
You need to log in before you can comment on or make changes to this bug.