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

Disable widgets #1632

Merged
merged 38 commits into from
Apr 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
fe93a1b
- added LifeCycle::DisabledChanged and InternalLifeCycle::RouteDisab…
Mar 20, 2021
3e1874b
created tests for disable
Mar 20, 2021
d6e34b2
fixed tests
Mar 20, 2021
500954e
fixed tests
Mar 20, 2021
f606199
updated core.rs and event.rs
Mar 20, 2021
6fd0b71
fixed focus-chain bug:
Mar 22, 2021
25d19c9
fix disabled update
Mar 22, 2021
e3d8b92
update tests
Mar 22, 2021
77d4f8f
fixed code (all tests succeed)
Mar 22, 2021
4d48edd
refactored core.rs and tests/mod.rs
Mar 23, 2021
9b2c8ba
updated tests
Mar 23, 2021
d5116de
fixed focus-chain bug
Mar 23, 2021
6c79e1e
make clippy happy
Mar 23, 2021
1fe6971
Merge branch 'master' into disable-widgets-2
xarvic Mar 23, 2021
40be21f
make clippy happy #2
Mar 23, 2021
5520e5b
Apply suggestions from code review
xarvic Mar 31, 2021
232e7ee
Update druid/src/contexts.rs
xarvic Mar 31, 2021
eccd596
refactor DisabledChanged
Mar 31, 2021
9754051
refactor DisabledChanged
Mar 31, 2021
74b86b2
fixed error, revered change of focus_chain
Mar 31, 2021
c181f51
refactored tests
Mar 31, 2021
cfc544e
reordered lifecycle events
Mar 31, 2021
05d45fc
reverted changes to the focus_chain
Mar 31, 2021
b9b9aca
implemented new focus-chain using LifeCycle::BuildFocusChain
Apr 5, 2021
f091bf9
update tests
Apr 5, 2021
4766ceb
fixed problems
Apr 5, 2021
a901d1c
updated texts
Apr 5, 2021
eb3cfd7
clippy fix
Apr 5, 2021
fdd510a
fixed documentation
Apr 5, 2021
4ef9697
Update druid/src/event.rs
xarvic Apr 5, 2021
4745fe9
fixed documentation
Apr 5, 2021
4567f8f
made logic simpler
Apr 5, 2021
ba58aad
refactored post_event_processing
Apr 5, 2021
8d6b784
updated CHANGELOG.md
Apr 5, 2021
d3e73d1
fixed docs
Apr 5, 2021
7be336c
Merge branch 'master' into disable-widgets-2
xarvic Apr 5, 2021
a52437c
make clippy happy
Apr 5, 2021
2919953
Merge branch 'master' into disable-widgets-2
cmyr Apr 6, 2021
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
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ You can find its changes [documented below](#070---2021-01-01).
- `Notification`s can be submitted while handling other `Notification`s ([#1640] by [@cmyr])
- Added ListIter implementations for OrdMap ([#1641] by [@Lejero])
- `Padding` can now use `Key<Insets>` ([#1662] by [@cmyr])
- `LifeCycle::DisabledChanged`, `InternalLifeCycle::RouteDisabledChanged` and the `set_disabled()` and `is_disabled()`
context-methods to implement disabled ([#1632] by [@xarvic])
- `LifeCycle::BuildFocusChain` to update the focus-chain ([#1632] by [@xarvic])

### Changed

Expand All @@ -40,6 +43,7 @@ You can find its changes [documented below](#070---2021-01-01).
- Padding is generic over child widget, impls WidgetWrapper ([#1634] by [@cmyr])
- Menu support was rewritten with support for `Data` ([#1625] by [@jneem])
- Update to piet v0.4.0 (rich text on linux!) ([#1677] by [@cmyr])
- `register_for_focus()` should from now on be called from `LifeCycle::BuildFocusChain` instead of `LifeCycle::WidgetAdded` ([#1632] by [@xarvic])
- Flex values that are less than 0.0 will default to 0.0 and warn in release. It will panic in debug mode. ([#1691] by [@arthmis])

### Deprecated
Expand Down Expand Up @@ -444,6 +448,7 @@ Last release without a changelog :(
[@SecondFlight]: https://github.com/SecondFlight
[@lord]: https://github.com/lord
[@Lejero]: https://github.com/Lejero
[@xarvic]: https://github.com/xarvic
[@arthmis]: https://github.com/arthmis
[@ccqpein]: https://github.com/ccqpein

Expand Down Expand Up @@ -653,6 +658,7 @@ Last release without a changelog :(
[#1606]: https://github.com/linebender/druid/pull/1606
[#1619]: https://github.com/linebender/druid/pull/1619
[#1625]: https://github.com/linebender/druid/pull/1625
[#1632]: https://github.com/linebender/druid/pull/1632
[#1634]: https://github.com/linebender/druid/pull/1634
[#1635]: https://github.com/linebender/druid/pull/1635
[#1636]: https://github.com/linebender/druid/pull/1636
Expand Down
33 changes: 33 additions & 0 deletions druid/src/contexts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,23 @@ impl_context_method!(
pub fn has_focus(&self) -> bool {
self.widget_state.has_focus
}

/// The disabled state of a widget.
///
/// Returns `true` if this widget or any of its ancestors is explicitly disabled.
/// To make this widget explicitly disabled use [`set_disabled`].
///
/// Disabled means that this widget should not change the state of the application. What
/// that means is not entirely clear but in any it should not change its data. Therefore
/// others can use this as a safety mechanism to prevent the application from entering an
/// illegal state.
/// For an example the decrease button of a counter of type `usize` should be disabled if the
/// value is `0`.
///
/// [`set_disabled`]: EventCtx::set_disabled
pub fn is_disabled(&self) -> bool {
self.widget_state.is_disabled()
}
}
);

Expand Down Expand Up @@ -374,9 +391,25 @@ impl_context_method!(EventCtx<'_, '_>, UpdateCtx<'_, '_>, LifeCycleCtx<'_, '_>,
pub fn children_changed(&mut self) {
trace!("children_changed");
self.widget_state.children_changed = true;
self.widget_state.update_focus_chain = true;
self.request_layout();
}

/// Set the disabled state for this widget.
///
/// Setting this to `false` does not mean a widget is not still disabled; for instance it may
/// still be disabled by an ancestor. See [`is_disabled`] for more information.
///
/// Calling this method during [`LifeCycle::DisabledChanged`] has no effect.
///
/// [`LifeCycle::DisabledChanged`]: struct.LifeCycle.html#variant.DisabledChanged
/// [`is_disabled`]: EventCtx::is_disabled
pub fn set_disabled(&mut self, disabled: bool) {
// widget_state.children_disabled_changed is not set because we want to be able to delete
// changes that happened during DisabledChanged.
self.widget_state.is_explicitly_disabled_new = disabled;
}

/// Indicate that text input state has changed.
///
/// A widget that accepts text input should call this anytime input state
Expand Down
102 changes: 98 additions & 4 deletions druid/src/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,21 @@ pub(crate) struct WidgetState {
pub(crate) viewport_offset: Vec2,

// TODO: consider using bitflags for the booleans.
// `true` if a descendent of this widget changed its disabled state and should receive
// LifeCycle::DisabledChanged or InternalLifeCycle::RouteDisabledChanged
pub(crate) children_disabled_changed: bool,

// `true` if one of our ancestors is disabled (meaning we are also disabled).
pub(crate) ancestor_disabled: bool,

// `true` if this widget has been explicitly disabled.
// A widget can be disabled without being *explicitly* disabled if an ancestor is disabled.
pub(crate) is_explicitly_disabled: bool,

// `true` if this widget has been explicitly disabled, but has not yet seen one of
// LifeCycle::DisabledChanged or InternalLifeCycle::RouteDisabledChanged
pub(crate) is_explicitly_disabled_new: bool,

pub(crate) is_hot: bool,

pub(crate) is_active: bool,
Expand All @@ -125,6 +140,8 @@ pub(crate) struct WidgetState {
/// Any descendant has requested update.
pub(crate) request_update: bool,

pub(crate) update_focus_chain: bool,

pub(crate) focus_chain: Vec<WidgetId>,
pub(crate) request_focus: Option<FocusChange>,
pub(crate) children: Bloom<WidgetId>,
Expand Down Expand Up @@ -908,11 +925,25 @@ impl<T: Data, W: Widget<T>> WidgetPod<T, W> {
} else {
if self.state.children_changed {
self.state.children.clear();
self.state.focus_chain.clear();
}
self.state.children_changed
}
}
InternalLifeCycle::RouteDisabledChanged => {
self.state.update_focus_chain = true;

let was_disabled = self.state.is_disabled();

self.state.is_explicitly_disabled = self.state.is_explicitly_disabled_new;

if was_disabled != self.state.is_disabled() {
extra_event = Some(LifeCycle::DisabledChanged(self.state.is_disabled()));
//Each widget needs only one of DisabledChanged and RouteDisabledChanged
false
} else {
self.state.children_disabled_changed
}
}
InternalLifeCycle::RouteFocusChanged { old, new } => {
let this_changed = if *old == Some(self.state.id) {
Some(false)
Expand Down Expand Up @@ -962,6 +993,8 @@ impl<T: Data, W: Widget<T>> WidgetPod<T, W> {
assert!(self.old_data.is_none());
trace!("Received LifeCycle::WidgetAdded");

self.state.update_focus_chain = true;

self.old_data = Some(data.clone());
self.env = Some(env.clone());

Expand All @@ -980,13 +1013,34 @@ impl<T: Data, W: Widget<T>> WidgetPod<T, W> {
// This event was meant only for our parent, so don't recurse.
false
}
LifeCycle::DisabledChanged(ancestors_disabled) => {
self.state.update_focus_chain = true;

let was_disabled = self.state.is_disabled();

self.state.is_explicitly_disabled = self.state.is_explicitly_disabled_new;
self.state.ancestor_disabled = *ancestors_disabled;

// the change direction (true -> false or false -> true) of our parent and ourself
// is always the same, or we dont change at all, because we stay disabled if either
// we or our parent are disabled.
was_disabled != self.state.is_disabled()
}
//NOTE: this is not sent here, but from the special set_hot_state method
LifeCycle::HotChanged(_) => false,
LifeCycle::FocusChanged(_) => {
// We are a descendant of a widget that has/had focus.
// Descendants don't inherit focus, so don't recurse.
false
}
LifeCycle::BuildFocusChain => {
if self.state.update_focus_chain {
self.state.focus_chain.clear();
true
} else {
false
}
}
};

let mut child_ctx = LifeCycleCtx {
Expand All @@ -1002,18 +1056,40 @@ impl<T: Data, W: Widget<T>> WidgetPod<T, W> {
self.inner.lifecycle(&mut child_ctx, event, data, env);
}

ctx.widget_state.merge_up(&mut self.state);
// Sync our state with our parent's state after the event!

Copy link
Member

Choose a reason for hiding this comment

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

I'm slightly scared of all the little bits of code scattered throughout this method, and I think it is going to be very important to carefully document what our invariants are, and what should go where.

For instance: why does this need to come after merge_up, and why does the preceding block need to come before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I fixed some of them and added documentation to explain the rest.

Copy link
Collaborator Author

@xarvic xarvic Mar 31, 2021

Choose a reason for hiding this comment

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

Hmmm :/ It wasn't as easy as i thought, but i think some of this complexity is necessary.

  • The ID of this widget should be the first in our focus-chain
  • we should add our ID only if auto_focus is set
  • auto_focus is not set until after WidgetAdded
  • we only need to add the value if children_changed or children_disabled_changed is set, otherwise we duplicate the values.
  • some_one could call ctx.children_changed() while routing WidgetAdded, which makes no sense, but therefore after the call we don't know if the value was set

The current implementation always adds the own id when the focus_chain is cleared and focus_chain is set. Only in WidgetAdded the id is always added after the event.
I don't know if there is a better way to satisfy the the conditions i mentioned?

// we need to (re)register children in case of one of the following events
match event {
// we need to (re)register children in case of one of the following events
LifeCycle::WidgetAdded | LifeCycle::Internal(InternalLifeCycle::RouteWidgetAdded) => {
self.state.children_changed = false;
ctx.widget_state.children = ctx.widget_state.children.union(self.state.children);
ctx.widget_state.focus_chain.extend(&self.state.focus_chain);
ctx.register_child(self.id());
}
LifeCycle::DisabledChanged(_)
| LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged) => {
self.state.children_disabled_changed = false;

if self.state.is_disabled() && self.state.has_focus {
// This may gets overwritten. This is ok because it still ensures that a
// FocusChange is routed after we updated the focus-chain.
self.state.request_focus = Some(FocusChange::Resign);
}

// Delete changes of disabled state that happened during DisabledChanged to avoid
// recursions.
self.state.is_explicitly_disabled_new = self.state.is_explicitly_disabled;
}
// Update focus-chain of our parent
LifeCycle::BuildFocusChain => {
self.state.update_focus_chain = false;
if !self.state.is_disabled() {
ctx.widget_state.focus_chain.extend(&self.state.focus_chain);
}
}
_ => (),
}

ctx.widget_state.merge_up(&mut self.state);
}

/// Propagate a data update.
Expand Down Expand Up @@ -1114,6 +1190,9 @@ impl WidgetState {
paint_insets: Insets::ZERO,
invalid: Region::EMPTY,
viewport_offset: Vec2::ZERO,
children_disabled_changed: false,
ancestor_disabled: false,
is_explicitly_disabled: false,
baseline_offset: 0.0,
is_hot: false,
needs_layout: false,
Expand All @@ -1130,10 +1209,21 @@ impl WidgetState {
cursor_change: CursorChange::Default,
cursor: None,
sub_window_hosts: Vec::new(),
is_explicitly_disabled_new: false,
text_registrations: Vec::new(),
update_focus_chain: false,
}
}

pub(crate) fn is_disabled(&self) -> bool {
self.is_explicitly_disabled || self.ancestor_disabled
}

pub(crate) fn tree_disabled_changed(&self) -> bool {
self.children_disabled_changed
|| self.is_explicitly_disabled != self.is_explicitly_disabled_new
}

pub(crate) fn add_timer(&mut self, timer_token: TimerToken) {
self.timers.insert(timer_token, self.id);
}
Expand Down Expand Up @@ -1168,6 +1258,9 @@ impl WidgetState {

self.needs_layout |= child_state.needs_layout;
self.request_anim |= child_state.request_anim;
self.children_disabled_changed |= child_state.children_disabled_changed;
self.children_disabled_changed |=
child_state.is_explicitly_disabled_new != child_state.is_explicitly_disabled;
self.has_active |= child_state.has_active;
self.has_focus |= child_state.has_focus;
self.children_changed |= child_state.children_changed;
Expand All @@ -1176,6 +1269,7 @@ impl WidgetState {
self.timers.extend_drain(&mut child_state.timers);
self.text_registrations
.extend(child_state.text_registrations.drain(..));
self.update_focus_chain |= child_state.update_focus_chain;

// We reset `child_state.cursor` no matter what, so that on the every pass through the tree,
// things will be recalculated just from `cursor_change`.
Expand Down
31 changes: 27 additions & 4 deletions druid/src/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,15 @@ pub enum LifeCycle {
/// [`Size`]: struct.Size.html
/// [`Widget::layout`]: trait.Widget.html#tymethod.layout
Size(Size),
/// Called when the Disabled state of the widgets is changed.
///
/// To check if a widget is disabled, see [`is_disabled`].
///
/// To change a widget's disabled state, see [`set_disabled`].
///
/// [`is_disabled`]: crate::EventCtx::is_disabled
/// [`set_disabled`]: crate::EventCtx::set_disabled
DisabledChanged(bool),
/// Called when the "hot" status changes.
///
/// This will always be called _before_ the event that triggered it; that is,
Expand All @@ -274,6 +283,16 @@ pub enum LifeCycle {
/// See [`is_hot`](struct.EventCtx.html#method.is_hot) for
/// discussion about the hot status.
HotChanged(bool),
/// This is called when the widget-tree changes and druid wants to rebuild the
/// Focus-chain.
///
/// It is the only place from witch [`register_for_focus`] should be called.
/// By doing so the widget can get focused by other widgets using [`focus_next`] or [`focus_prev`].
///
/// [`register_for_focus`]: crate::LifeCycleCtx::register_for_focus
/// [`focus_next`]: crate::EventCtx::focus_next
/// [`focus_prev`]: crate::EventCtx::focus_prev
BuildFocusChain,
/// Called when the focus status changes.
///
/// This will always be called immediately after a new widget gains focus.
Expand Down Expand Up @@ -310,6 +329,8 @@ pub enum InternalLifeCycle {
/// the widget that is gaining focus, if any
new: Option<WidgetId>,
},
/// Used to route the `DisabledChanged` event to the required widgets.
RouteDisabledChanged,
/// The parents widget origin in window coordinate space has changed.
ParentWindowOrigin,
/// Testing only: request the `WidgetState` of a specific widget.
Expand Down Expand Up @@ -407,7 +428,9 @@ impl LifeCycle {
pub fn should_propagate_to_hidden(&self) -> bool {
match self {
LifeCycle::Internal(internal) => internal.should_propagate_to_hidden(),
LifeCycle::WidgetAdded => true,
LifeCycle::WidgetAdded | LifeCycle::DisabledChanged(_) | LifeCycle::BuildFocusChain => {
true
}
LifeCycle::Size(_) | LifeCycle::HotChanged(_) | LifeCycle::FocusChanged(_) => false,
}
}
Expand All @@ -418,9 +441,9 @@ impl InternalLifeCycle {
/// (for example the hidden tabs in a tabs widget).
pub fn should_propagate_to_hidden(&self) -> bool {
match self {
InternalLifeCycle::RouteWidgetAdded | InternalLifeCycle::RouteFocusChanged { .. } => {
true
}
InternalLifeCycle::RouteWidgetAdded
| InternalLifeCycle::RouteFocusChanged { .. }
| InternalLifeCycle::RouteDisabledChanged => true,
InternalLifeCycle::ParentWindowOrigin => false,
#[cfg(test)]
InternalLifeCycle::DebugRequestState { .. }
Expand Down
2 changes: 1 addition & 1 deletion druid/src/tests/harness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ impl<T: Data> Harness<'_, T> {
}
}

fn lifecycle(&mut self, event: LifeCycle) {
pub(crate) fn lifecycle(&mut self, event: LifeCycle) {
self.inner.lifecycle(event)
}

Expand Down
Loading