menus: make menus that reference unregistered commands more robust#9886
Merged
vince-fugnitto merged 1 commit intomasterfrom Aug 16, 2021
Merged
menus: make menus that reference unregistered commands more robust#9886vince-fugnitto merged 1 commit intomasterfrom
vince-fugnitto merged 1 commit intomasterfrom
Conversation
vince-fugnitto
added
menus
msujew
approved these changes
Aug 12, 2021
Member
There was a problem hiding this comment.
I can confirm the issue is present on master and is addressed by this change nicely:
- On
masterunregistering those commands crashes the application on startup ❌ - With this change in place, the application starts up nicely ✔️
- The menu entries related to the commands are not visible ✔️
- The missing commands are logged as errors in the console (perhaps a warning would suffice, but I don't really have a strong opinion on that) ✔️
vince-fugnitto
force-pushed
the
vf/menu-robustness
branch
2 times, most recently
from
9328bee to
2caaf89
Compare
Member
Author
|
I updated the pull-request following @paul-marechal's offline comments about having a more permanent way to verify the feature in the framework (previously a dropped commit). I added an api-test, as well as a sample contribution in |
vince-fugnitto
force-pushed
the
vf/menu-robustness
branch
from
2caaf89 to
43394e3
Compare
The commit updates `menus` to not fail the layout when there are no commands attached to them, and instead log the error. Previously, if a command was unregistered and a command referenced it the layout would fail, and possibly also fail to start the application. The change also includes: - a bogus menu registration in `api-samples` to verify the feature in the future. - an api test to verify that registration does not fail for menus with invalid commands. Signed-off-by: vince-fugnitto <vincent.fugnitto@ericsson.com>
vince-fugnitto
force-pushed
the
vf/menu-robustness
branch
from
43394e3 to
55e178a
Compare
paul-marechal
approved these changes
Aug 16, 2021
Member
paul-marechal
left a comment
paul-marechal
left a comment
There was a problem hiding this comment.
LGTM, the application is not blocked because of unknown commands in menus anymore.
This was referenced Aug 16, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What it does
Fixes: #9870
The pull-request updates the registration of menus that reference invalid or missing commands to be more robust.
Rather than fail the layout, or the start of the application, the menus items will be omitted with an error log for extenders to handle.
How to test
electron-menu-robust.mp4
Review checklist
Reminder for reviewers