Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle mnemonic escaping #68196

Merged
merged 9 commits into from
Feb 19, 2019
Merged

Handle mnemonic escaping #68196

merged 9 commits into from
Feb 19, 2019

Conversation

sbatten
Copy link
Member

@sbatten sbatten commented Feb 8, 2019

fixes #61139

replaces #66761

@sbatten sbatten requested a review from bpasero February 8, 2019 04:48
@sbatten sbatten self-assigned this Feb 8, 2019
@sbatten sbatten added this to the February 2019 milestone Feb 8, 2019
@sbatten
Copy link
Member Author

sbatten commented Feb 18, 2019

@bpasero ping

@bpasero bpasero self-assigned this Feb 19, 2019
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbatten I am confused by the new (?) notion of &&& to present a literal & in the menu labels. I would prefer a solution that is consistent with our current approach where && is a mnemonic and & is the literal &. Otherwise we have competing notations for buttons vs menus.

See for example #64999 for a discussion around this for extensions.

@sbatten
Copy link
Member Author

sbatten commented Feb 19, 2019

@bpasero Electron's methodology is && => & and & => mnemonic. Our current menus do not have a method of presenting & literally. The &&& was a way to get us to a working state where we could escape mnemonics, but ideally we would eventually move all our commands to use & and &&. We supported & in menus for mnemonics as well because localizations were using this and it was getting sent to Electron that way and handled as a mnemonic.

@bpasero
Copy link
Member

bpasero commented Feb 19, 2019

@sbatten I just checked how the old menu used to be and found e.g. this: https://github.com/Microsoft/vscode/blob/1.20.1/src/vs/code/electron-main/menus.ts#L382 which seems to indicate to me that for translations we always said that && is a mnemonic and & is the literal &.

Can we get to a solution where & is the literal & in both custom and native menu and && is the ampersand which gets removed on platforms that do not support it or if this is disabled?

@sbatten
Copy link
Member Author

sbatten commented Feb 19, 2019

@bpasero please verify on macOS

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better, some comments.

@@ -383,7 +383,7 @@ MenuRegistry.appendMenuItem(MenuId.MenubarHelpMenu, {
group: '2_reference',
command: {
id: 'workbench.action.openTipsAndTricksUrl',
title: nls.localize({ key: 'miTipsAndTricks', comment: ['&& denotes a mnemonic'] }, "Tips and Tri&&cks")
title: nls.localize({ key: 'miTipsAndTricks', comment: ['&& denotes a mnemonic'] }, "Tips &&& Tri&&cks")
Copy link
Member

@bpasero bpasero Feb 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbatten should this be & instead of &&&?

@@ -767,6 +770,10 @@ class MenuSeparatorActionItem extends ActionItem {
}
}

/**
Copy link
Member

@bpasero bpasero Feb 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbatten is this something you still work on? The TODO I mean.

@sbatten sbatten merged commit e5b1616 into microsoft:master Feb 19, 2019
@sbatten sbatten deleted the escapeMnemonics branch February 19, 2019 16:47
sandy081 pushed a commit to vldmrkl/vscode that referenced this pull request Feb 22, 2019
* handle mnemonic escapes

* Update src/vs/workbench/electron-browser/main.contribution.ts

* update custom menu and recent items list to support escaped mnemonics

* remove need for &&&

* qfix
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing ampersands in new "custom" title bar
3 participants