Skip to content
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

Guarantee that WidgetAdded is the first thing a widget sees. #1259

Merged
merged 3 commits into from
Nov 9, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ You can find its changes [documented below](#060---2020-06-01).
- `Checkbox::set_text` to update the label. ([#1346] by [@finnerale])
- `Event::should_propagate_to_hidden` and `Lifecycle::should_propagate_to_hidden` to determine whether an event should be sent to hidden widgets (e.g. in `Tabs` or `Either`). ([#1351] by [@andrewhickman])
- `set_cursor` can be called in the `update` method. ([#1361] by [@jneem])
- `WidgetPod::is_initialized` to check if a widget has received `WidgetAdded`. ([#1259] by [@finnerale])

### Changed

Expand Down Expand Up @@ -118,6 +119,7 @@ You can find its changes [documented below](#060---2020-06-01).
- Improve Windows 7 DXGI compatibility ([#1311] by [@raphlinus])
- Fixed `Either` not passing events to its hidden child correctly. ([#1351] by [@andrewhickman])
- Don't drop events while showing file dialogs ([#1302], [#1328] by [@jneem])
- Ensure that `LifeCycle::WidgetAdded` is the first thing a widget sees. ([#1259] by [@finnerale])

### Visual

Expand All @@ -138,6 +140,7 @@ You can find its changes [documented below](#060---2020-06-01).

- Standardized web targeting terminology. ([#1013] by [@xStrom])
- X11: Ported the X11 backend to [`x11rb`](https://github.com/psychon/x11rb). ([#1025] by [@jneem])
- Add `debug_panic` macro for when a backtrace is useful but a panic unnecessary. ([#1259] by [@finnerale])

### Outside News

Expand Down Expand Up @@ -524,6 +527,7 @@ Last release without a changelog :(
[#1328]: https://github.com/linebender/druid/pull/1328
[#1346]: https://github.com/linebender/druid/pull/1346
[#1351]: https://github.com/linebender/druid/pull/1351
[#1259]: https://github.com/linebender/druid/pull/1259
[#1361]: https://github.com/linebender/druid/pull/1361

[Unreleased]: https://github.com/linebender/druid/compare/v0.6.0...master
Expand Down
14 changes: 2 additions & 12 deletions druid/src/app_delegate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,7 @@ impl<'a> DelegateCtx<'a> {
.to(Target::Global),
);
} else {
const MSG: &str = "WindowDesc<T> - T must match the application data type.";
if cfg!(debug_assertions) {
panic!(MSG);
} else {
log::error!("DelegateCtx::new_window: {}", MSG)
}
debug_panic!("DelegateCtx::new_window<T> - T must match the application data type.");
}
}

Expand All @@ -78,12 +73,7 @@ impl<'a> DelegateCtx<'a> {
.to(Target::Window(window)),
);
} else {
const MSG: &str = "MenuDesc<T> - T must match the application data type.";
if cfg!(debug_assertions) {
panic!(MSG);
} else {
log::error!("DelegateCtx::set_menu: {}", MSG)
}
debug_panic!("DelegateCtx::set_menu<T> - T must match the application data type.");
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion druid/src/command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ impl Command {
panic!(
"The selector \"{}\" exists twice with different types. See druid::Command::get for more information",
selector.symbol()
)
);
}))
} else {
None
Expand Down
23 changes: 5 additions & 18 deletions druid/src/contexts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -391,12 +391,7 @@ impl EventCtx<'_, '_> {
.to(Target::Global),
);
} else {
const MSG: &str = "WindowDesc<T> - T must match the application data type.";
if cfg!(debug_assertions) {
panic!(MSG);
} else {
log::error!("EventCtx::new_window: {}", MSG)
}
debug_panic!("EventCtx::new_window<T> - T must match the application data type.");
}
}

Expand All @@ -412,12 +407,9 @@ impl EventCtx<'_, '_> {
.to(Target::Window(self.state.window_id)),
);
} else {
const MSG: &str = "ContextMenu<T> - T must match the application data type.";
if cfg!(debug_assertions) {
panic!(MSG);
} else {
log::error!("EventCtx::show_context_menu: {}", MSG)
}
debug_panic!(
"EventCtx::show_context_menu<T> - T must match the application data type."
);
}
}

Expand Down Expand Up @@ -736,12 +728,7 @@ impl<'a> ContextState<'a> {
.to(Target::Window(self.window_id)),
);
} else {
const MSG: &str = "MenuDesc<T> - T must match the application data type.";
if cfg!(debug_assertions) {
panic!(MSG);
} else {
log::error!("EventCtx::set_menu: {}", MSG)
}
debug_panic!("EventCtx::set_menu<T> - T must match the application data type.");
}
}

Expand Down
55 changes: 44 additions & 11 deletions druid/src/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,13 @@ impl<T, W: Widget<T>> WidgetPod<T, W> {
&self.state
}

/// Returns `true` if the widget has received [`LifeCycle::WidgetAdded`].
///
/// [`LifeCycle::WidgetAdded`]: ./enum.LifeCycle.html#variant.WidgetAdded
pub fn is_initialized(&self) -> bool {
self.old_data.is_some()
}

/// Query the "active" state of the widget.
pub fn is_active(&self) -> bool {
self.state.is_active
Expand Down Expand Up @@ -423,6 +430,14 @@ impl<T: Data, W: Widget<T>> WidgetPod<T, W> {
return;
}

if !self.is_initialized() {
debug_panic!(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I think debug_panic! is an excellent addition, I wonder if this shouldn't actually be a debug_assert!?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

debug_assert! does nothing in release, while debug_panic! at least logs an error. Thus, I would prefer debug_panic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense!

"{:?}: paint method called before receiving WidgetAdded.",
ctx.widget_id()
);
return;
}

ctx.with_save(|ctx| {
let layout_origin = self.layout_rect().origin().to_vec2();
ctx.transform(Affine::translate(layout_origin));
Expand Down Expand Up @@ -489,6 +504,14 @@ impl<T: Data, W: Widget<T>> WidgetPod<T, W> {
data: &T,
env: &Env,
) -> Size {
if !self.is_initialized() {
debug_panic!(
"{:?}: layout method called before receiving WidgetAdded.",
ctx.widget_id()
);
return Size::ZERO;
}

self.state.needs_layout = false;

let child_mouse_pos = match ctx.mouse_pos {
Expand Down Expand Up @@ -541,20 +564,20 @@ impl<T: Data, W: Widget<T>> WidgetPod<T, W> {
///
/// [`event`]: trait.Widget.html#tymethod.event
pub fn event(&mut self, ctx: &mut EventCtx, event: &Event, data: &mut T, env: &Env) {
if self.old_data.is_none() {
log::error!(
"widget {:?} is receiving an event without having first \
received WidgetAdded.",
if !self.is_initialized() {
debug_panic!(
"{:?}: event method called before receiving WidgetAdded.",
ctx.widget_id()
);
return;
}

// log if we seem not to be laid out when we should be
if self.state.layout_rect.is_none() && !event.should_propagate_to_hidden() {
log::warn!(
"Widget '{}' received an event ({:?}) without having been laid out. \
debug_panic!(
"{:?} received an event ({:?}) without having been laid out. \
This likely indicates a missed call to set_layout_rect.",
self.inner.type_name(),
ctx.widget_id(),
event,
);
}
Expand Down Expand Up @@ -807,6 +830,14 @@ impl<T: Data, W: Widget<T>> WidgetPod<T, W> {

true
}
_ if !self.is_initialized() => {
debug_panic!(
"{:?}: received LifeCycle::{:?} before WidgetAdded.",
self.id(),
event
);
return;
}
LifeCycle::Size(_) => {
// We are a descendant of a widget that received the Size event.
// This event was meant only for our parent, so don't recurse.
Expand Down Expand Up @@ -858,13 +889,15 @@ impl<T: Data, W: Widget<T>> WidgetPod<T, W> {
if !self.state.request_update {
match (self.old_data.as_ref(), self.env.as_ref()) {
(Some(d), Some(e)) if d.same(data) && e.same(env) => return,
(Some(_), None) => self.env = Some(env.clone()),
(None, _) => {
log::warn!("old_data missing in {:?}, skipping update", self.id());
self.old_data = Some(data.clone());
self.env = Some(env.clone());
debug_panic!(
"{:?} is receiving an update without having first received WidgetAdded.",
self.id()
);
return;
}
_ => (),
(Some(_), Some(_)) => {}
}
}

Expand Down
4 changes: 3 additions & 1 deletion druid/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,9 @@ pub use im;
#[macro_use]
pub mod lens;

#[macro_use]
mod util;

mod app;
mod app_delegate;
mod bloom;
Expand All @@ -165,7 +168,6 @@ pub mod scroll_component;
mod tests;
pub mod text;
pub mod theme;
mod util;
pub mod widget;
mod win_handler;
mod window;
Expand Down
25 changes: 25 additions & 0 deletions druid/src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,31 @@ use std::collections::HashMap;
use std::hash::Hash;
use std::mem;

/// Panic in debug and log::error in release mode.
///
/// This macro is in some way a combination of `panic` and `debug_assert`,
/// 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is an excellent addition.

() => { ... };
($msg:expr) => {
if cfg!(debug_assertions) {
panic!($msg);
} else {
log::error!($msg);
}
};
($msg:expr,) => { debug_panic!($msg) };
($fmt:expr, $($arg:tt)+) => {
if cfg!(debug_assertions) {
panic!($fmt, $($arg)*);
} else {
log::error!($fmt, $($arg)*);
}
};
}

/// Fast path for equal type extend + drain.
pub trait ExtendDrain {
/// Extend the collection by draining the entries from `source`.
Expand Down