Warn on Unhandled commands #1533
Conversation
There was a problem hiding this comment.
Interesting.
I don't dislike this, but it doesn't totally fit how I use commands myself.
Basically: is_handled is way to optionally suppress the continued delivery of an event. I do not expect that all events are handled; and I wouldn't expect a widget to bother calling 'set_handled' in a situation where it was the only target of a command, for instance.
It's possible that we might want to rethink this, though? I'm certainly open to discussing, and this patch seems reasonable on its own...
|
Yeah, I also intended this for more of a discussion but patch was simple enough that I submitted the PR.
I agree but do you think it is a good practice to I have also thought of making warning opt in, i.e. replace |
|
I feel like if we are going to have options on the selector to control them they should be proper fields and not bits of a string? |
|
I think that having this be opt-in instead of opt-out does definitely make sense, I could see really wanting to log if certain events aren't handled. I agree that stringly-typing here isn't great; a reasonable alternative would be adding a field to |
maybe adding field to |
|
I guess it feels like, semantically, whether or not a given command needs to be handled is something related to the command, not to the selector? As in I could at least theoretically imagine a situation where I want the same selector to be used in cases where it is both expected to be handled and not. Although when I actually write this it sounds bad, so maybe not! |
|
I guess this is a use site vs declaration site dichotomy. I'd normally go for declaration site so most users don't have to worry about it. |
|
Ready for Review |
druid/src/command.rs
Outdated
| @@ -384,6 +405,11 @@ impl Command { | |||
| .default_to(Target::Global) | |||
| } | |||
|
|
|||
| /// Checks if this command must be used. | |||
| pub fn is_must_use(&self) -> bool { | |||
There was a problem hiding this comment.
hmm, I find this name awkward, but the name I'd propose ("must_use") is used by the constructor. We could call the construct new_must_use, maybe, although that doesn't feel great either? We could also call this must_be_used which at least flows a bit better grammatically. I don't love any of these ideas... 😕
There was a problem hiding this comment.
I followed the second one because I expect Selector::must_use("..") to used much more than Command::must_be_used
druid/src/command.rs
Outdated
| selector.symbol(), | ||
| self.symbol | ||
| selector.symbol().str, | ||
| self.symbol.str |
There was a problem hiding this comment.
instead of these changes I would manually implement Display for SelectorSymbol. Something like,
fn fmt(&self, f: Formatter) -> Result<_, _> {
let must_use = if self.must_use {" (must use" } else { "" };
write!(f, "{}{}", self.str, must_use)
}There was a problem hiding this comment.
I manually implemented Debug now. This output is not meant for end user
If more than one widgets have to handle, you can not `set_handled` because it will stop the propogation of the command. So this still shows the warning, which is incorrect.
If more than one widgets have to handle, you can not `set_handled` because it will stop the propogation of the command. So this still shows the warning, which is incorrect.
No description provided.