Guarantee that WidgetAdded is the first thing a widget sees. #1259
Conversation
luleyleo
force-pushed
the
debug-panic
branch
2 times, most recently
from
71a92a7 to
55bb1f0
Compare
druid/src/core.rs
Outdated
| // Should commands require layout? | ||
| // If yes, it will cause problems with our test suite. | ||
| Event::Command(_) => (), | ||
| _ => { | ||
| log::warn!( | ||
| "Widget '{}' received an event ({:?}) without having been laid out. \ |
There was a problem hiding this comment.
I had to exclude Event::Command here, because otherwise our tests would break.
There we often send test commands without doing any layout.
This can of course be changed back, but I wanted to push this now without meddling too much with our test stuff.
There was a problem hiding this comment.
This came up in #1160 recently, because unviewed tabs don't get laid out but they might still receive commands. I think there's a bigger decision that needs to be made (and documented) about which events and lifecycle events hidden widgets should expect to receive, but for now I think it's completely reasonable for them to get commands.
There was a problem hiding this comment.
I agree, it's even totally reasonable for a Command to be sent while handling Event::WindowConnected, and in that case layout should not have been called.
There was a problem hiding this comment.
There are methods in the tabs code that try to say whether an event or lifecycle should be delivered to hidden widgets. Possibly should be moved to the enums themselves.
|
@raphlinus does this help you with #1258? |
| @@ -411,6 +418,14 @@ impl<T: Data, W: Widget<T>> WidgetPod<T, W> { | |||
| return; | |||
| } | |||
|
|
|||
| if !self.is_initialized() { | |||
| debug_panic!( | |||
There was a problem hiding this comment.
Although I think debug_panic! is an excellent addition, I wonder if this shouldn't actually be a debug_assert!?
There was a problem hiding this comment.
debug_assert! does nothing in release, while debug_panic! at least logs an error. Thus, I would prefer debug_panic.
druid/src/core.rs
Outdated
| // Should commands require layout? | ||
| // If yes, it will cause problems with our test suite. | ||
| Event::Command(_) => (), | ||
| _ => { | ||
| log::warn!( | ||
| "Widget '{}' received an event ({:?}) without having been laid out. \ |
There was a problem hiding this comment.
I agree, it's even totally reasonable for a Command to be sent while handling Event::WindowConnected, and in that case layout should not have been called.
| /// but it will log the provided message instead of ignoring it in release builds. | ||
| /// | ||
| /// It's useful when a backtrace would aid debugging but a crash can be avoided in release. | ||
| macro_rules! debug_panic { |
luleyleo
force-pushed
the
debug-panic
branch
from
55bb1f0 to
a6a8fd1
Compare
|
Just checking in, is this ready to go? If not let us know what you need. |
|
I'm waiting for @raphlinus to see if this helps with #1258 |
|
What's the status here? does it still hopelessly break crochet? |
This will panic in debug mode and log in release mode. It's useful when a backtrace would be valuable but a crash can be avoided.
|
No, Crochet would work just fine with it, since I fixed the context handling. This is just waiting for someone to say yay or nay. |
|
Ok. Once the checks are done, I'll merge it. |
Closes #1127 and hopefully closes #1258 as well.
With this,
WidgetPodwill panic whenever a widget receives anything butLifeCycle::WidgetAddedas its first event.The advantage of a panic is that the backtrace makes it in my experience very easy to track down where things went wrong.