-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
vscode: independent editor/title/run menu #12799
Conversation
@cdamus I agree. IMO, doing things better than VS Code in this case is better than emulating VS Code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @cdamus,
Thank you very much for your change! I tested it with commands and submenus and everything seems to work mostly as expected. I do believe we currently render the menu also on views which should not happen. Could you please have a look at that? (In case you cannot reproduce it, I actually rebased your change on the master branch, maybe there were some changes inbetween.)
Thank you!
@@ -103,9 +103,10 @@ export class TabBarToolbarRegistry implements FrontendApplicationContribution { | |||
} | |||
const result: Array<TabBarToolbarItem | ReactTabBarToolbarItem> = []; | |||
for (const item of this.items.values()) { | |||
const visible = TabBarToolbarItem.is(item) | |||
const visible = TabBarToolbarItem.is(item) && item.command |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this overall condition is flawed as I can see the menu also on views, not only editors. I believe the problem is that we do not evaluate the conditions on menu toolbar items (only if they have content) but we do register our menu in menus-contribution-handler.ts
with a visibility condition. Since this already a rather complex condition, I'd suggest to pull that out into something that is more readable and easier to overwrite, e.g.,:
protected isVisible(item: TabBarToolbarItem | ReactTabBarToolbarItem, widget: Widget): boolean {
if (AnyToolbarItem.isConditional(item)) {
if (item.isVisible && !item.isVisible(widget)) {
return false;
}
if (item.when && !this.contextKeyService.match(item.when, widget.node)) {
return false;
}
}
if (TabBarToolbarItem.is(item) && item.command && !this.commandRegistry.isVisible(item.command, widget)) {
return false;
}
if (MenuToolbarItem.is(item) && !this.hasMenuContent(item, widget)) {
return false;
}
return true;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A welcome refactoring suggestion.
protected renderMenuItem(item: TabBarToolbarItem & MenuToolbarItem): React.ReactNode { | ||
const icon = typeof item.icon === 'function' ? item.icon() : item.icon ?? 'ellipsis'; | ||
return <div key={item.id} className={TabBarToolbar.Styles.TAB_BAR_TOOLBAR_ITEM + ' enabled menu'}> | ||
<div id={item.id} className={codicon(icon, true)} item.menuPath)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add the onClick
listener to the parent? It feels a bit odd that you cannot open the menu by clicking the chevron.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bigger target area for clicking is important for accessibility in any case!
Thanks for the review, @martin-fleck-at ! Indeed, intervening changes merged to |
9c4ddcd
to
4f0c438
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I re-tested the changes and everything looks great to me now, thank you very much Christian! As soon as the CI is happy we can merge this.
In VS Code, contributions to the "editor/title/run" menu contribution point are not added to the ellipsis menu but to a dedicated item on the toolbar. This commit makes Theia do the same, except that unlike VS Code, a pop-up menu is always presented, even if there is only one action available. Fixes eclipse-theia#12687 Signed-off-by: Christian W. Damus <cdamus.ext@eclipsesource.com>
Commit 0e6186f adds the missing return type annotation found by ESLint. |
The merge of eclipse-theia#12799 introduced a regression in the rendering of MenuToolbarItems that aren't pop-up menus but instead just commands that happen to have a menu path: they were rendered as pop-up menus instead of simple buttons. This fix refines the condition for the new pop-up menu rendering to check that the menu to be rendered is not a command to be executed directly on click. Fixes eclipse-theia#12687 Signed-off-by: Christian W. Damus <cdamus.ext@eclipsesource.com>
The merge of #12799 introduced a regression in the rendering of MenuToolbarItems that aren't pop-up menus but instead just commands that happen to have a menu path: they were rendered as pop-up menus instead of simple buttons. This fix refines the condition for the new pop-up menu rendering to check that the menu to be rendered is not a command to be executed directly on click. Fixes #12687 Signed-off-by: Christian W. Damus <cdamus.ext@eclipsesource.com>
What it does
In VS Code, contributions to the
"editor/title/run"
menu contribution point are not added to the ellipsis menu but to a dedicated item on the toolbar. This commit makes Theia do the same, except that unlike VS Code, a pop-up menu is always presented, even if there is only one action available.In my opinion, the inconsistency of the VS Code implementation in not showing the Run/Debug pop-up menu but just the one contributed action on the toolbar, which looks quite different to the Run/Debug pop-up menu action, is confusing. If an action is contributed to an extension point that is a menu of Run/Debug actions, then that should show a menu of Run/Debug actions even if there is only one item on the menu.
If it really is necessary to emulate this detail of the VS Code behaviour, then that should be a follow-up issue to discuss with the user community.
Note also that this PR uses the
debug-alt
icon from the VS Codecodicons
set as the best available alternative from that set to the icon that VS Code uses for its "Run/Debug" menu, which is not in thecodicons
set. If somebody would like to contribute a better license-compatible open-source icon for the purpose, please do!Fixes #12687
How to test
menu_bug_3
branch of the OP's sample repository used for other recent bug reports: Theia Sandbox.myext
extension to the"editor/title/run"
point (it should be checked out as"editor/lineNumber/context"
for another issue report) and remove the"group"
property that isn't needed for this use case."myext.goodbyeWorld"
command from this extension to the"editor/title"
menu so that we can see that this menu exists and doesn't get contributions intended for the"editor/title/run"
menu.myext
extension and link it into yourplugins/
directory....
"more" menu....
menu.Also worth verifying are some related use cases:
"editor/title/run"
menu does not show that menu in the toolbar."when"
clauses not matching the current context, then in this case also the menu does not appear in the toolbar.Review checklist
Reminder for reviewers