-
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
GH-5100: Update the menu if the frontend is ready. #5140
Conversation
The code looks good to me, which desktop platforms does it need testing on? |
I see no issue on MacOS |
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.
packages/core/src/electron-browser/menu/electron-menu-contribution.ts
Outdated
Show resolved
Hide resolved
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.
Seems to be a good approximation, won't work though if commands should be enabled/disabled based on internal widget state.
This. I will investigate VS Code's solution a bit more thoroughly.
Thanks for the feedback. I will try how VS Code behaves on my Ubuntu image and will compare it with Theia. Perhaps, I have to add some debouncing logic. |
Closes #5100. Signed-off-by: Akos Kitta <kittaakos@typefox.io>
@akosyakov, I have dropped the dynamic electron menu part. |
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.
LGTM no more flickering on my side.
Closes #5100.
Signed-off-by: Akos Kitta kittaakos@typefox.io
What it does
ready
.How to test
Review checklist
Reminder for reviewers
🌞