Replace generic commands with methods on EventCtx and DelegateCtx #931
Conversation
This file contains 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
xStrom
reviewed
May 15, 2020
xStrom
added
S-waiting-on-author
xStrom
added
S-needs-review
|
Thanks for the suggestions! |
cmyr
requested changes
May 15, 2020
cmyr
reviewed
May 15, 2020
| ); | ||
| } else { | ||
| const MSG: &str = "ContextMenu<T> - T must match the application state."; | ||
| if cfg!(debug_assertions) { |
There was a problem hiding this comment.
if this becomes a pattern we could consider a custom macro, like log_or_panic!(...).
There was a problem hiding this comment.
I would like it if this becomes a pattern (or log_or_panic!) as panic! makes debugging easier due to the back trace while log::error! does not crash the app once you ship it.
cmyr
approved these changes
May 15, 2020
xStrom
added
S-waiting-on-author
luleyleo
added
S-needs-review
|
@cmyr Thanks for the review, I think I've addressed everything. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Third part of #908 .
Replaces
NEW_WINDOW,SET_MENUandSHOW_CONTEXT_MENUcommands with methods onEventCtxandDelegateCtx.This will get rid of any generic commands which caused quite some trouble for typed selectors.
When calling any of the methods with the wrong data type, it will
panic!in debug mode andlog::error!in a release build.I've not added
show_context_menutoDelegateCtxas I could not imagine a real world usecase.Thus I had to change
multiwina bit to use aControllerfor the context menu, which seemed to be much closer to actual usage of a context menu.