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

API: submenus extension point #102784

Merged
merged 45 commits into from
Jul 23, 2020
Merged

API: submenus extension point #102784

merged 45 commits into from
Jul 23, 2020

Conversation

joaomoreno
Copy link
Member

@joaomoreno joaomoreno commented Jul 17, 2020

This PR fixes #100172

  • Fix non-editor submenus, fix dropdown submenus: address SubmenuItemAction vs ContextSubMenu confusion
  • Fix native context menus
  • Support nested submenus
  • Prevent submenu cycles
  • Expand the set of menus which have submenu support
  • Support top-level toolbar submenus, including icon for the action item

@joaomoreno joaomoreno added this to the July 2020 milestone Jul 17, 2020
@joaomoreno joaomoreno requested a review from jrieken July 17, 2020 13:19
@joaomoreno joaomoreno self-assigned this Jul 17, 2020
@joaomoreno
Copy link
Member Author

joaomoreno commented Jul 21, 2020

@sbatten Submenus seemed to work nicely in the editor context menu, but not across the rest of the workbench. There was some mixup with SubmenuItemAction vs ContextSubMenu. In 86ec536, I made SubmenuItemAction extends SubmenuAction which resolves the whole mess and makes submenus work everywhere else. Can you please review that commit? The interesting change is in 86ec536#diff-cc0794e8bea870179e50767a30f52d5bR233-R236

@joaomoreno joaomoreno requested a review from sbatten July 21, 2020 15:04
@joaomoreno
Copy link
Member Author

joaomoreno commented Jul 22, 2020

@sbatten @isidorn Native submenus were yet another implementation, dependent on the bogus ContextSubMenu class, so submenus didn't work in that case either. I deleted that class.

@joaomoreno
Copy link
Member Author

@sbatten @jrieken Recursive submenus work in custom UI:

image

But they break down in native UI, since the entire menu hierarchy must be given to Electron at the time of creation:

image


Because of the limitation and the fact that it would simply be a gimmick, I'll write some code to prevent menu cycles.

@joaomoreno
Copy link
Member Author

@jrieken I've refactored menusExtensionPoint.ts heavily to make it easier to maintain the list of API menus, give it a look: c3cef6f

@joaomoreno
Copy link
Member Author

Submenus now work across most menus! Check this out:

Annotation 2020-07-23 152725
Annotation 2020-07-23 153320
Annotation 2020-07-23 153406

@joaomoreno joaomoreno merged commit a7fdaf4 into master Jul 23, 2020
@joaomoreno joaomoreno deleted the joao/submenus branch July 23, 2020 14:56
@isidorn
Copy link
Contributor

isidorn commented Jul 28, 2020

Very cool work 👏

}

addClasses(this.element, ...classNames);
Copy link
Member

Choose a reason for hiding this comment

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

fyi - es6 deprecated, use this.element.classList...

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually didn't know that!

@@ -47,16 +47,15 @@ namespace schema {
case 'explorer/context': return MenuId.ExplorerContext;
case 'editor/title/context': return MenuId.EditorTitleContext;
case 'debug/callstack/context': return MenuId.DebugCallStackContext;
case 'debug/toolbar': return MenuId.DebugToolBar;
case 'debug/toolBar': return MenuId.DebugToolBar;
Copy link
Member

Choose a reason for hiding this comment

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

API breakage?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah we had both for backwards compatibility, even though debug/toolbar was "deprecated". I am not sure if there are extensions which really on this.

Copy link
Member

Choose a reason for hiding this comment

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

High risk. You could know relatively easy (telemetry, search extension usages) or just keep it given the low price of supporting it. btw: deprecated doesn't mean that we will remove it

Copy link
Contributor

@isidorn isidorn Aug 10, 2020

Choose a reason for hiding this comment

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

I just searched top 1000 extensions usages on debug/toolbar usage and only one extension used the deprecated name - the extension is not that popular and I have created an issue for them IBM-Blockchain/blockchain-vscode-extension#2593
So I suggest the following: let's go with the removal, and if needed we can easily reintroduce it as part of a recovery build if somebody complains.
fyi @weinand

Copy link
Member Author

Choose a reason for hiding this comment

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

Great catch @jrieken, I really thought this was an oversight and failed to see the case difference. I'll leave it up to @isidorn and @weinand. Sorry all!

@joaomoreno
Copy link
Member Author

@jrieken Thanks for the post-mergeum review! 🙏

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

Successfully merging this pull request may close these issues.

Add API support for submenus
4 participants