Skip to content

#101995 extend tooltip of menu item that defines 'alt' command#116211

Merged
sbatten merged 4 commits intomicrosoft:mainfrom
gjsjohnmurray:101995
Mar 10, 2021
Merged

#101995 extend tooltip of menu item that defines 'alt' command#116211
sbatten merged 4 commits intomicrosoft:mainfrom
gjsjohnmurray:101995

Conversation

@gjsjohnmurray
Copy link
Contributor

@gjsjohnmurray gjsjohnmurray commented Feb 9, 2021

This PR implements #101995

junk

If a menu item specifies an additional alt commmand the tooltip gets a second line displaying this when the Alt key is not down.

The goal is to improve discoverability of places where two alternative commands exist.

@gjsjohnmurray
Copy link
Contributor Author

gjsjohnmurray commented Feb 9, 2021

ping @sbatten who commented on the original issue I opened

@sbatten sbatten self-assigned this Feb 9, 2021
@sbatten sbatten self-requested a review February 9, 2021 19:51
@sbatten sbatten added this to the February 2021 milestone Feb 9, 2021
@joaomoreno joaomoreno changed the base branch from master to main February 15, 2021 08:51
@gjsjohnmurray
Copy link
Contributor Author

gjsjohnmurray commented Feb 23, 2021

@sbatten any chance of this merging in time for February release?

@sbatten
Copy link
Member

sbatten commented Feb 23, 2021

@gjsjohnmurray probably no and that's my fault for not coming back to the PR in time. I just did the code review and I think it looks good. As with anything, I'd like to see it in insiders for the month in case there are any surprises. I think we can merge this right after release.

/cc @bpasero
one question I have, do we refer to option/alt key differently on macOS here? or is Alt the right terminology?

@sbatten sbatten modified the milestones: February 2021, March 2021 Feb 23, 2021
@bpasero
Copy link
Member

bpasero commented Feb 23, 2021

@sbatten the key needs to be called "Option" on macOS: https://en.wikipedia.org/wiki/Alt_key#macOS

@bpasero
Copy link
Member

bpasero commented Feb 24, 2021

@gjsjohnmurray I think we need to localize the key though, as the name maybe different in other languages.

@gjsjohnmurray
Copy link
Contributor Author

gjsjohnmurray commented Feb 24, 2021

@bpasero what is the right way to do this? I used

vscode/src/vs/workbench/contrib/debug/browser/debugHover.ts

Line 108 in 01cfa2f

tip.textContent = nls.localize({ key: 'quickTip', comment: ['"switch to editor language hover" means to show the programming language hover widget instead of the debug hover'] }, 'Hold {0} key to switch to editor language hover', isMacintosh ? 'Option' : 'Alt');
as my model for the isMacintosh test. Does the keylabel text get localized in that instance?

@bpasero
Copy link
Member

bpasero commented Feb 24, 2021

No, it needs 2 distinct messages.

@gjsjohnmurray
Copy link
Contributor Author

gjsjohnmurray commented Feb 24, 2021 •
edited
Loading

Based on the keyboard images at https://support.apple.com/en-us/HT201794 most keyboard layouts use ⌥ (U+2325). Exceptions on that page are English, Japanese, Korean, Taiwanese and Thai which use the text 'option'. In all cases the text 'alt' also appears on the key.

Having said that, the Magic Keyboard apparently labels it with ⌥ and 'option' on the US layout at least (no mention of 'alt').

Shall we leave it to the translators to decide on a per-language basis? Presumably they'd pick either ⌥ or 'Option' (should that be uncapitalized?).

Are the nls.localize translations file-specific? That is, if I add this in the file I'm changing for this PR

const = nls.localize({ key: 'macOSaltKey', comment: ['label or symbol on the Alt / Command key (commonly U+2325)'] }, 'Command')

and do the same to fix debugHover.ts, will the translators have to duplicate the translation? If yes, is there a central place to export the string from?

@gjsjohnmurray
Copy link
Contributor Author

gjsjohnmurray commented Feb 24, 2021

If the ⌥ symbol gets used in some localizations perhaps the tip should use regular square brackets to denote the key instead of the fancy THREE-D TOP-LIGHTED LEFTWARDS EQUILATERAL ARROWHEAD and THREE-D TOP-LIGHTED RIGHTWARDS EQUILATERAL ARROWHEAD shown in the PR's original screenshot, e.g.

image

image

(screenshots above simulated on my Windows dev platform)

image

@gjsjohnmurray
Copy link
Contributor Author

gjsjohnmurray commented Mar 9, 2021

@bpasero please see my questions at #116211 (comment)

It makes sense to me for there to be one place setting the per-OS (and possibly per-language) text or symbol representing the Alt/Option key when it needs to be referred to in display strings.

I'd like to get this PR into Insiders soon so it can get good exposure.

@bpasero
Copy link
Member

bpasero commented Mar 9, 2021

I don't fully understand why we have to solve this newly for this case? I am quite sure we already have existing tooltips with the Alt-key as part of them, why can we not reuse what we do there?

@sbatten
Copy link
Member

sbatten commented Mar 9, 2021

This might be the right thing to use

vscode/src/vs/base/common/keybindingLabels.ts

Lines 61 to 83 in ed5e512

export const UILabelProvider = new ModifierLabelProvider(
{
ctrlKey: '⌃',
shiftKey: '⇧',
altKey: '⌥',
metaKey: '⌘',
separator: '',
},
{
ctrlKey: nls.localize({ key: 'ctrlKey', comment: ['This is the short form for the Control key on the keyboard'] }, "Ctrl"),
shiftKey: nls.localize({ key: 'shiftKey', comment: ['This is the short form for the Shift key on the keyboard'] }, "Shift"),
altKey: nls.localize({ key: 'altKey', comment: ['This is the short form for the Alt key on the keyboard'] }, "Alt"),
metaKey: nls.localize({ key: 'windowsKey', comment: ['This is the short form for the Windows key on the keyboard'] }, "Windows"),
separator: '+',
},
{
ctrlKey: nls.localize({ key: 'ctrlKey', comment: ['This is the short form for the Control key on the keyboard'] }, "Ctrl"),
shiftKey: nls.localize({ key: 'shiftKey', comment: ['This is the short form for the Shift key on the keyboard'] }, "Shift"),
altKey: nls.localize({ key: 'altKey', comment: ['This is the short form for the Alt key on the keyboard'] }, "Alt"),
metaKey: nls.localize({ key: 'superKey', comment: ['This is the short form for the Super key on the keyboard'] }, "Super"),
separator: '+',
}
);

@gjsjohnmurray
Copy link
Contributor Author

gjsjohnmurray commented Mar 9, 2021

Thanks @sbatten, this is what I was looking for. I have pushed a change to use it. At the same time I switched to using regular square brackets around the altKey representation.

sbatten
sbatten approved these changes Mar 9, 2021
View reviewed changes
@gjsjohnmurray
Copy link
Contributor Author

gjsjohnmurray commented Mar 10, 2021

@sbatten thanks for the feedback. I have pushed the changes you suggested, so I think this is now ready to be merged.

@sbatten sbatten merged commit 75da064 into microsoft:main Mar 10, 2021
@sbatten
Copy link
Member

sbatten commented Mar 10, 2021

@gjsjohnmurray merged! thanks!

aeschli pushed a commit that referenced this pull request Mar 10, 2021
@gjsjohnmurray @aeschli
#101995 extend tooltip of menu item that defines 'alt' command (#116211)
f239e60 
* do #101995 extend tooltip of menu item that defines 'alt' command

* key label is 'Option' on macOS

* use platform-specific representation of altKey

* incorporate PR feedback
@gjsjohnmurray gjsjohnmurray deleted the 101995 branch March 10, 2021 17:13
@gjsjohnmurray gjsjohnmurray mentioned this pull request Mar 12, 2021
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sbatten sbatten sbatten approved these changes

Assignees

@sbatten sbatten

Labels

None yet

Projects

None yet

Milestone

March 2021

Development

Successfully merging this pull request may close these issues.

3 participants

@gjsjohnmurray @sbatten @bpasero