-
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
Support dynamic menu contributions #13720
Conversation
cc @kittaakos this is maybe interesting for you. |
@msujew, on my machine (Linux), I tried both electron and browser version (also both on original branch and branch rebased on latest master). |
@rschnekenbu I've mentioned this in the follow-ups of the PR:
The main issue is that on the browser, the whole HTML element is replaced, which in turn leads to losing some state information. I would try looking into this in a separate PR as indicated above. In Electron, we're bound by the limits of the underlying OS. I'm not even sure you can add/remove native window menu entries while they're open on Windows (my OS) or other OS'. I'll take a look at this as well, but I can't promise anything here. |
Other than these rendering issues, I am OK with this PR. I will approve it. |
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.
Looks good with of course the current known limitations :) Thanks @msujew!
What it does
Closes #13719
Adds a new
onDidChange
event to theMenuModelRegistry
. Any changes to the menus (i.e. registering, unregistering, disposal) fire this event now. Adds a listener to this event in the menu factories to ensure that the menus are correctly recreated.How to test
sample-menu-contribution.ts
file:Sample Menu > Sample sub menu
Follow-ups
I've noticed that the menu flashes when the model is changed (i.e. any open main sub menus are closed). That's something that could maybe be addressed in a separate PR. A smart diffing of the menu bar HTML element would probably be appropriate here.
Review checklist
Reminder for reviewers