From fe93a1b72b1c34377ea56c4405073630077b15c5 Mon Sep 17 00:00:00 2001 From: xarvic Date: Sat, 20 Mar 2021 13:51:27 +0100 Subject: [PATCH 01/35] - added LifeCycle::DisabledChanged and InternalLifeCycle::RouteDisabledChanged - implemented the disabled state in WidgetPod - changed call to focus_change from event to post event processing - implemented disabled handling in window.rs and core.rs --- druid/src/contexts.rs | 30 ++++++++++++++ druid/src/core.rs | 92 +++++++++++++++++++++++++++++++++++++++++++ druid/src/event.rs | 14 ++++++- druid/src/window.rs | 30 ++++++++------ 4 files changed, 153 insertions(+), 13 deletions(-) diff --git a/druid/src/contexts.rs b/druid/src/contexts.rs index 17da3f5c60..f13107792c 100644 --- a/druid/src/contexts.rs +++ b/druid/src/contexts.rs @@ -271,6 +271,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() + } } ); @@ -364,6 +381,19 @@ impl_context_method!(EventCtx<'_, '_>, UpdateCtx<'_, '_>, LifeCycleCtx<'_, '_>, self.request_layout(); } + /// Sets the explicitly disabled state of this widget. + /// The widget may still be disabled if `set_disabled(false)` was called. See [`is_disabled`] + /// + /// 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; + } + /// Set the menu of the window containing the current widget. /// `T` must be the application's root `Data` type (the type provided to [`AppLauncher::launch`]). /// diff --git a/druid/src/core.rs b/druid/src/core.rs index c1c1343f1c..2f010af950 100644 --- a/druid/src/core.rs +++ b/druid/src/core.rs @@ -104,6 +104,20 @@ pub(crate) struct WidgetState { pub(crate) viewport_offset: Vec2, // TODO: consider using bitflags for the booleans. + // Any child of this widget changed the disabled state and should either receive + // LifeCycle::DisabledChanged or InternalLifeCycle::RouteDisabledChanged + pub(crate) children_disabled_changed: bool, + + // Any of our ancestors are disabled. Therefore we are also disabled. + pub(crate) ancestor_disabled: bool, + + // explicitly disabled through the usage of EventCtx::set_disabled(true) on this widget. + pub(crate) is_explicitly_disabled: bool, + + // A buffer for the value of EventCtx::set_disabled() before this widget receives + // LifeCycle::DisabledChanged or InternalLifeCycle::RouteDisabledChanged + pub(crate) is_explicitly_disabled_new: bool, + pub(crate) is_hot: bool, pub(crate) is_active: bool, @@ -818,6 +832,9 @@ impl> WidgetPod { // Always merge even if not needed, because merging is idempotent and gives us simpler code. // Doing this conditionally only makes sense when there's a measurable performance boost. ctx.widget_state.merge_up(&mut self.state); + + ctx.widget_state.children_disabled_changed |= (self.state.is_explicitly_disabled_new + != self.state.is_explicitly_disabled); } /// Send notifications originating from this widget's children to this @@ -938,6 +955,30 @@ impl> WidgetPod { f.call(&self.state); true } + InternalLifeCycle::RouteDisabledChanged => { + //If our children changed update the focus-chain + if self.state.children_disabled_changed { + self.state.focus_chain.clear(); + } + + let was_disabled = self.state.is_disabled(); + + self.state.is_explicitly_disabled = self.state.is_explicitly_disabled_new; + + 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); + } + + 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 + } + } }, LifeCycle::WidgetAdded => { assert!(self.old_data.is_none()); @@ -960,6 +1001,33 @@ impl> WidgetPod { // This event was meant only for our parent, so don't recurse. false } + LifeCycle::DisabledChanged(ancestors_disabled) => { + //If our children changed update the focus-chain + if self.state.children_disabled_changed { + self.state.focus_chain.clear(); + } + + let was_disabled = self.state.is_disabled(); + + self.state.is_explicitly_disabled = self.state.is_explicitly_disabled_new; + self.state.ancestor_disabled = *ancestors_disabled; + + if was_disabled != self.state.is_disabled() { + extra_event = Some(LifeCycle::DisabledChanged(self.state.is_disabled())); + } else if self.state.children_disabled_changed { + extra_event = Some(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); + } + + 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); + } + + //It is easier to always use the extra event since we can disable our self's while + //our parent was enabled and vice versa. + false + }, //NOTE: this is not sent here, but from the special set_hot_state method LifeCycle::HotChanged(_) => false, LifeCycle::FocusChanged(_) => { @@ -982,6 +1050,18 @@ impl> WidgetPod { self.inner.lifecycle(&mut child_ctx, event, data, env); } + if let LifeCycle::DisabledChanged(_) | LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged) = event { + self.state.children_disabled_changed = false; + ctx.widget_state.focus_chain.extend(&self.state.focus_chain); + + //Delete changes of disabled state that happened during DisabledChanged + self.state.is_explicitly_disabled_new = self.state.is_explicitly_disabled; + + } else if self.state.is_explicitly_disabled != self.state.is_explicitly_disabled_new { + ctx.widget_state.children_disabled_changed = true; + + } + ctx.widget_state.merge_up(&mut self.state); // we need to (re)register children in case of one of the following events @@ -1055,6 +1135,9 @@ impl> WidgetPod { self.state.request_update = false; ctx.widget_state.merge_up(&mut self.state); +> + ctx.widget_state.children_disabled_changed |= (self.state.is_explicitly_disabled_new + != self.state.is_explicitly_disabled); } } @@ -1091,6 +1174,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, @@ -1107,9 +1193,14 @@ impl WidgetState { cursor_change: CursorChange::Default, cursor: None, sub_window_hosts: Vec::new(), + is_explicitly_disabled_new: false } } + pub(crate) fn is_disabled(&self) -> bool { + self.is_explicitly_disabled || self.ancestor_disabled + } + pub(crate) fn add_timer(&mut self, timer_token: TimerToken) { self.timers.insert(timer_token, self.id); } @@ -1139,6 +1230,7 @@ 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.has_active |= child_state.has_active; self.has_focus |= child_state.has_focus; self.children_changed |= child_state.children_changed; diff --git a/druid/src/event.rs b/druid/src/event.rs index d2e5e9fcca..3fde459c16 100644 --- a/druid/src/event.rs +++ b/druid/src/event.rs @@ -254,6 +254,13 @@ 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. + /// + /// [`is_disabled`](struct.EventCtx.html#method.is_disabled) returns if the widget is disabled. + /// + /// [`set_disabled`](struct.EventCtx.html#method.set_disabled) to change the widgets disabled + /// state. + DisabledChanged(bool), /// Called when the "hot" status changes. /// /// This will always be called _before_ the event that triggered it; that is, @@ -299,6 +306,8 @@ pub enum InternalLifeCycle { /// the widget that is gaining focus, if any new: Option, }, + /// 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. @@ -395,7 +404,7 @@ 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(_) => true, LifeCycle::Size(_) | LifeCycle::HotChanged(_) | LifeCycle::FocusChanged(_) => false, } } @@ -406,7 +415,8 @@ 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 { .. } => { + InternalLifeCycle::RouteWidgetAdded | InternalLifeCycle::RouteFocusChanged { .. } | + InternalLifeCycle::RouteDisabledChanged => { true } InternalLifeCycle::ParentWindowOrigin => false, diff --git a/druid/src/window.rs b/druid/src/window.rs index 095a616c45..776a720d80 100644 --- a/druid/src/window.rs +++ b/druid/src/window.rs @@ -154,6 +154,25 @@ impl Window { false, ); } + + // Update the disabled state if necessary + // Always do this before sending focus change, since this event updates the focus chain. + if self.root.state().is_disabled() { + let event = LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged); + self.lifecycle(queue, &event, data, env, false); + } + + if let Some(focus_req) = widget_state.request_focus.take() { + let old = self.focus; + let new = self.widget_for_focus_request(focus_req); + // Only send RouteFocusChanged in case there's actual change + if old != new { + let event = LifeCycle::Internal(InternalLifeCycle::RouteFocusChanged { old, new }); + self.lifecycle(queue, &event, data, env, false); + self.focus = new; + } + } + // Add all the requested timers to the window's timers map. self.timers.extend_drain(&mut widget_state.timers); @@ -242,17 +261,6 @@ impl Window { self.timers.remove(&token); } - if let Some(focus_req) = widget_state.request_focus.take() { - let old = self.focus; - let new = self.widget_for_focus_request(focus_req); - // Only send RouteFocusChanged in case there's actual change - if old != new { - let event = LifeCycle::Internal(InternalLifeCycle::RouteFocusChanged { old, new }); - self.lifecycle(queue, &event, data, env, false); - self.focus = new; - } - } - if let Some(cursor) = &widget_state.cursor { self.handle.set_cursor(&cursor); } else if matches!( From 3e1874b36fe92171967b44587f82739d300b1872 Mon Sep 17 00:00:00 2001 From: xarvic Date: Sat, 20 Mar 2021 15:47:18 +0100 Subject: [PATCH 02/35] created tests for disable --- druid/src/tests/mod.rs | 209 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 209 insertions(+) diff --git a/druid/src/tests/mod.rs b/druid/src/tests/mod.rs index 2faf91fa8c..5c823845f1 100644 --- a/druid/src/tests/mod.rs +++ b/druid/src/tests/mod.rs @@ -23,6 +23,7 @@ use std::cell::Cell; use std::env; use std::fs; use std::rc::Rc; +use std::collections::HashMap; use crate::widget::*; use crate::*; @@ -350,6 +351,214 @@ fn focus_changed() { }) } +#[test] +fn simple_disable() { + const CHANGE_DISABLED: Selector = Selector::new("druid-tests.change-disabled"); + + let test_widget_factory = |auto_focus: bool, id: WidgetId, state: Rc>>|{ + let mut widget = ModularWidget::new(state) + .lifecycle_fn(move |state, ctx, event, _, _|{ + match event { + LifeCycle::WidgetAdded => { + if auto_focus { + ctx.register_for_focus(); + } + }, + LifeCycle::DisabledChanged(disabled) => { + *state.get_mut() = Some(*disabled); + } + _ => {} + } + }) + .event_fn(|_, ctx, event, _, _|{ + if let Event::Command(cmd) = event { + if let Some(disabled) = cmd.get(CHANGE_DISABLED) { + ctx.set_disabled(*disabled); + } + } + }) + .with_id(id); + widget + }; + + let disabled_0: Rc>> = Default::default(); + let disabled_1: Rc>> = Default::default(); + let disabled_2: Rc>> = Default::default(); + let disabled_3: Rc>> = Default::default(); + + let check_states = |desired: [Option; 4]| { + assert_eq!(desired[0], disabled_0.get()); + assert_eq!(desired[1], disabled_1.get()); + assert_eq!(desired[2], disabled_2.get()); + assert_eq!(desired[3], disabled_3.get()); + }; + + let id_0 = WidgetId::next(); + let id_1 = WidgetId::next(); + let id_2 = WidgetId::next(); + let id_3 = WidgetId::next(); + + let root = Flex::row() + .with_child(test_widget_factory(true, id_0, disabled_0.clone())) + .with_child(test_widget_factory(true, id_1, disabled_1.clone())) + .with_child(test_widget_factory(true, id_2, disabled_2.clone())) + .with_child(test_widget_factory(true, id_3, disabled_3.clone())); + + Harness::create_simple((), root, |harness| { + harness.send_initial_events(); + check_states([None, None, None, None]); + assert_eq!(harness.window().focus_chain().len(), 4); + harness.submit_command(Command::new(CHANGE_DISABLED, true, id_0)); + check_states([Some(true), None, None, None]); + assert_eq!(harness.window().focus_chain().len(), 3); + harness.submit_command(Command::new(CHANGE_DISABLED, true, id_2)); + check_states([Some(true), None, Some(true), None]); + assert_eq!(harness.window().focus_chain().len(), 2); + harness.submit_command(Command::new(CHANGE_DISABLED, true, id_3)); + check_states([Some(true), None, Some(true), Some(true)]); + assert_eq!(harness.window().focus_chain().len(), 1); + harness.submit_command(Command::new(CHANGE_DISABLED, false, id_2)); + check_states([Some(true), None, Some(false), Some(true)]); + assert_eq!(harness.window().focus_chain().len(), 2); + //This is intended the widget should not receive an event! + harness.submit_command(Command::new(CHANGE_DISABLED, true, id_2)); + check_states([Some(true), None, Some(false), Some(true)]); + assert_eq!(harness.window().focus_chain().len(), 2); + //This is intended the widget should not receive an event! + harness.submit_command(Command::new(CHANGE_DISABLED, true, id_1)); + check_states([Some(true), None, Some(false), Some(true)]); + assert_eq!(harness.window().focus_chain().len(), 2); + }) +} + +#[test] +fn disable_tree() { + const MULTI_CHANGE_DISABLED: Selector> = Selector::new("druid-tests.multi-change-disabled"); + + let leaf_factory = |state: Rc>>|{ + ModularWidget::new(state) + .lifecycle_fn(move |state, ctx, event, _, _|{ + match event { + LifeCycle::WidgetAdded => { + if auto_focus { + ctx.register_for_focus(); + } + }, + LifeCycle::DisabledChanged(disabled) => { + *state.get_mut() = Some(*disabled); + } + _ => {} + } + }) + }; + + let wrapper = |id: WidgetId, widget: Box>|{ + ModularWidget::new(WidgetPod::new(widget)) + .lifecycle_fn(|inner, ctx, event, data, env| { + inner.lifecycle(ctx, event, data, env); + }) + .event_fn(|inner, ctx, event, data, env| { + if let Event::Command(cmd) = event { + if let Some(map) = cmd.get(MULTI_CHANGE_DISABLED) { + if let Some(disabled) = map.get(ctx.widget_id()) { + ctx.set_disabled(*disabled); + } + } + } else { + inner.event(ctx, event, data, env); + } + }) + .with_id(id) + }; + + fn multi_update(states: &[(WidgetId, bool)]) -> Command { + let payload = states.iter().collect::>(); + Command::new(MULTI_CHANGE_DISABLED, payload, Target::Global) + } + + let disabled_0: Rc>> = Default::default(); + let disabled_1: Rc>> = Default::default(); + let disabled_2: Rc>> = Default::default(); + let disabled_3: Rc>> = Default::default(); + let disabled_4: Rc>> = Default::default(); + let disabled_5: Rc>> = Default::default(); + + let check_states = |desired: [Option; 6]| { + assert_eq!(desired[0], disabled_0.get()); + assert_eq!(desired[1], disabled_1.get()); + assert_eq!(desired[2], disabled_2.get()); + assert_eq!(desired[3], disabled_3.get()); + assert_eq!(desired[4], disabled_4.get()); + assert_eq!(desired[5], disabled_5.get()); + }; + + let outer_id = WidgetId::next(); + let inner_id = WidgetId::next(); + let single_id = WidgetId::next(); + let root_id = WidgetId::next(); + + let node0 = Flex::row() + .with_child(leaf_factory(disabled_0.clone())) + .with_child(leaf_factory(disabled_1.clone())) + .boxed(); + + let node1 = leaf_factory(disabled_2.clone()).boxed(); + + let node2 = Flex::row() + .with_child(wrapper(outer_id, wrapper(inner_id, node0).boxed())) + .with_child(wrapper(single_id, node1)) + .with_child(leaf_factory(disabled_3.clone())) + .with_child(leaf_factory(disabled_4.clone())) + .with_child(leaf_factory(disabled_5.clone())) + .boxed(); + + let root = wrapper(root_id, node2); + + Harness::create_simple((), root, |harness| { + harness.send_initial_events(); + check_states([None, None, None, None, None, None]); + assert_eq!(harness.window().focus_chain().len(), 6); + + harness.submit_command(multi_update(&[(root_id, true)])); + check_states([Some(true), Some(true), Some(true), Some(true), Some(true), Some(true)]); + assert_eq!(harness.window().focus_chain().len(), 0); + + harness.submit_command(multi_update(&[(inner_id, true)])); + check_states([Some(true), Some(true), Some(true), Some(true), Some(true), Some(true)]); + assert_eq!(harness.window().focus_chain().len(), 0); + + // Node 0 should not be affected + harness.submit_command(multi_update(&[(root_id, false)])); + check_states([Some(true), Some(true), Some(false), Some(false), Some(false), Some(false)]); + assert_eq!(harness.window().focus_chain().len(), 4); + + // Changing inner and outer in different directions should not affect the leaves + harness.submit_command(multi_update(&[(inner_id, false), (outer_id, true)])); + check_states([Some(true), Some(true), Some(false), Some(false), Some(false), Some(false)]); + assert_eq!(harness.window().focus_chain().len(), 4); + + // Changing inner and outer in different directions should not affect the leaves + harness.submit_command(multi_update(&[(inner_id, true), (outer_id, false)])); + check_states([Some(true), Some(true), Some(false), Some(false), Some(false), Some(false)]); + assert_eq!(harness.window().focus_chain().len(), 4); + + // Changing two widgets on the same level + harness.submit_command(multi_update(&[(single_id, true), (inner_id, false)])); + check_states([Some(false), Some(false), Some(true), Some(false), Some(false), Some(false)]); + assert_eq!(harness.window().focus_chain().len(), 5); + + // Disabling the root should disable all widgets + harness.submit_command(multi_update(&[(root_id, true)])); + check_states([Some(true), Some(true), Some(true), Some(true), Some(true), Some(true)]); + assert_eq!(harness.window().focus_chain().len(), 0); + + // Enabling a widget in a disabled tree should not affect the enclosed widgets + harness.submit_command(multi_update(&[(single_id, false)])); + check_states([Some(true), Some(true), Some(true), Some(true), Some(true), Some(true)]); + assert_eq!(harness.window().focus_chain().len(), 0); + }) +} + #[test] fn simple_lifecyle() { let record = Recording::default(); From d6e34b22d3e7e9d71e4f351497c763d73b98e1ee Mon Sep 17 00:00:00 2001 From: xarvic Date: Sat, 20 Mar 2021 15:57:49 +0100 Subject: [PATCH 03/35] fixed tests --- druid/src/tests/mod.rs | 135 ++++++++++++++++++++++++++++------------- 1 file changed, 92 insertions(+), 43 deletions(-) diff --git a/druid/src/tests/mod.rs b/druid/src/tests/mod.rs index 5c823845f1..331c324b33 100644 --- a/druid/src/tests/mod.rs +++ b/druid/src/tests/mod.rs @@ -20,10 +20,10 @@ mod invalidation_tests; mod layout_tests; use std::cell::Cell; +use std::collections::HashMap; use std::env; use std::fs; use std::rc::Rc; -use std::collections::HashMap; use crate::widget::*; use crate::*; @@ -355,30 +355,27 @@ fn focus_changed() { fn simple_disable() { const CHANGE_DISABLED: Selector = Selector::new("druid-tests.change-disabled"); - let test_widget_factory = |auto_focus: bool, id: WidgetId, state: Rc>>|{ - let mut widget = ModularWidget::new(state) - .lifecycle_fn(move |state, ctx, event, _, _|{ - match event { - LifeCycle::WidgetAdded => { - if auto_focus { - ctx.register_for_focus(); - } - }, - LifeCycle::DisabledChanged(disabled) => { - *state.get_mut() = Some(*disabled); + let test_widget_factory = |auto_focus: bool, id: WidgetId, state: Rc>>| { + ModularWidget::new(state) + .lifecycle_fn(move |state, ctx, event, _, _| match event { + LifeCycle::WidgetAdded => { + if auto_focus { + ctx.register_for_focus(); } - _ => {} } + LifeCycle::DisabledChanged(disabled) => { + state.set(Some(*disabled)); + } + _ => {} }) - .event_fn(|_, ctx, event, _, _|{ + .event_fn(|_, ctx, event, _, _| { if let Event::Command(cmd) = event { if let Some(disabled) = cmd.get(CHANGE_DISABLED) { ctx.set_disabled(*disabled); } } }) - .with_id(id); - widget + .with_id(id) }; let disabled_0: Rc>> = Default::default(); @@ -433,26 +430,22 @@ fn simple_disable() { #[test] fn disable_tree() { - const MULTI_CHANGE_DISABLED: Selector> = Selector::new("druid-tests.multi-change-disabled"); + const MULTI_CHANGE_DISABLED: Selector> = + Selector::new("druid-tests.multi-change-disabled"); - let leaf_factory = |state: Rc>>|{ - ModularWidget::new(state) - .lifecycle_fn(move |state, ctx, event, _, _|{ - match event { - LifeCycle::WidgetAdded => { - if auto_focus { - ctx.register_for_focus(); - } - }, - LifeCycle::DisabledChanged(disabled) => { - *state.get_mut() = Some(*disabled); - } - _ => {} - } - }) + let leaf_factory = |state: Rc>>| { + ModularWidget::new(state).lifecycle_fn(move |state, ctx, event, _, _| match event { + LifeCycle::WidgetAdded => { + ctx.register_for_focus(); + } + LifeCycle::DisabledChanged(disabled) => { + state.set(Some(*disabled)); + } + _ => {} + }) }; - let wrapper = |id: WidgetId, widget: Box>|{ + let wrapper = |id: WidgetId, widget: Box>| { ModularWidget::new(WidgetPod::new(widget)) .lifecycle_fn(|inner, ctx, event, data, env| { inner.lifecycle(ctx, event, data, env); @@ -460,7 +453,7 @@ fn disable_tree() { .event_fn(|inner, ctx, event, data, env| { if let Event::Command(cmd) = event { if let Some(map) = cmd.get(MULTI_CHANGE_DISABLED) { - if let Some(disabled) = map.get(ctx.widget_id()) { + if let Some(disabled) = map.get(&ctx.widget_id()) { ctx.set_disabled(*disabled); } } @@ -472,7 +465,7 @@ fn disable_tree() { }; fn multi_update(states: &[(WidgetId, bool)]) -> Command { - let payload = states.iter().collect::>(); + let payload = states.iter().cloned().collect::>(); Command::new(MULTI_CHANGE_DISABLED, payload, Target::Global) } @@ -520,41 +513,97 @@ fn disable_tree() { assert_eq!(harness.window().focus_chain().len(), 6); harness.submit_command(multi_update(&[(root_id, true)])); - check_states([Some(true), Some(true), Some(true), Some(true), Some(true), Some(true)]); + check_states([ + Some(true), + Some(true), + Some(true), + Some(true), + Some(true), + Some(true), + ]); assert_eq!(harness.window().focus_chain().len(), 0); harness.submit_command(multi_update(&[(inner_id, true)])); - check_states([Some(true), Some(true), Some(true), Some(true), Some(true), Some(true)]); + check_states([ + Some(true), + Some(true), + Some(true), + Some(true), + Some(true), + Some(true), + ]); assert_eq!(harness.window().focus_chain().len(), 0); // Node 0 should not be affected harness.submit_command(multi_update(&[(root_id, false)])); - check_states([Some(true), Some(true), Some(false), Some(false), Some(false), Some(false)]); + check_states([ + Some(true), + Some(true), + Some(false), + Some(false), + Some(false), + Some(false), + ]); assert_eq!(harness.window().focus_chain().len(), 4); // Changing inner and outer in different directions should not affect the leaves harness.submit_command(multi_update(&[(inner_id, false), (outer_id, true)])); - check_states([Some(true), Some(true), Some(false), Some(false), Some(false), Some(false)]); + check_states([ + Some(true), + Some(true), + Some(false), + Some(false), + Some(false), + Some(false), + ]); assert_eq!(harness.window().focus_chain().len(), 4); // Changing inner and outer in different directions should not affect the leaves harness.submit_command(multi_update(&[(inner_id, true), (outer_id, false)])); - check_states([Some(true), Some(true), Some(false), Some(false), Some(false), Some(false)]); + check_states([ + Some(true), + Some(true), + Some(false), + Some(false), + Some(false), + Some(false), + ]); assert_eq!(harness.window().focus_chain().len(), 4); // Changing two widgets on the same level harness.submit_command(multi_update(&[(single_id, true), (inner_id, false)])); - check_states([Some(false), Some(false), Some(true), Some(false), Some(false), Some(false)]); + check_states([ + Some(false), + Some(false), + Some(true), + Some(false), + Some(false), + Some(false), + ]); assert_eq!(harness.window().focus_chain().len(), 5); // Disabling the root should disable all widgets harness.submit_command(multi_update(&[(root_id, true)])); - check_states([Some(true), Some(true), Some(true), Some(true), Some(true), Some(true)]); + check_states([ + Some(true), + Some(true), + Some(true), + Some(true), + Some(true), + Some(true), + ]); assert_eq!(harness.window().focus_chain().len(), 0); // Enabling a widget in a disabled tree should not affect the enclosed widgets harness.submit_command(multi_update(&[(single_id, false)])); - check_states([Some(true), Some(true), Some(true), Some(true), Some(true), Some(true)]); + check_states([ + Some(true), + Some(true), + Some(true), + Some(true), + Some(true), + Some(true), + ]); assert_eq!(harness.window().focus_chain().len(), 0); }) } From 500954eed4096dcdea9c98a702b02bc01b11146f Mon Sep 17 00:00:00 2001 From: xarvic Date: Sat, 20 Mar 2021 16:35:57 +0100 Subject: [PATCH 04/35] fixed tests --- druid/src/tests/harness.rs | 2 +- druid/src/tests/mod.rs | 265 ++++++++++++++++++++++++------------- 2 files changed, 171 insertions(+), 96 deletions(-) diff --git a/druid/src/tests/harness.rs b/druid/src/tests/harness.rs index c5a2cb9b1b..95974f420c 100644 --- a/druid/src/tests/harness.rs +++ b/druid/src/tests/harness.rs @@ -253,7 +253,7 @@ impl Harness<'_, T> { } } - fn lifecycle(&mut self, event: LifeCycle) { + pub(crate) fn lifecycle(&mut self, event: LifeCycle) { self.inner.lifecycle(event) } diff --git a/druid/src/tests/mod.rs b/druid/src/tests/mod.rs index 331c324b33..84dacda515 100644 --- a/druid/src/tests/mod.rs +++ b/druid/src/tests/mod.rs @@ -20,7 +20,7 @@ mod invalidation_tests; mod layout_tests; use std::cell::Cell; -use std::collections::HashMap; +//use std::collections::HashMap; use std::env; use std::fs; use std::rc::Rc; @@ -383,11 +383,24 @@ fn simple_disable() { let disabled_2: Rc>> = Default::default(); let disabled_3: Rc>> = Default::default(); - let check_states = |desired: [Option; 4]| { - assert_eq!(desired[0], disabled_0.get()); - assert_eq!(desired[1], disabled_1.get()); - assert_eq!(desired[2], disabled_2.get()); - assert_eq!(desired[3], disabled_3.get()); + let check_states = |name: &str, desired: [Option; 4]| { + if desired[0] != disabled_0.get() + || desired[1] != disabled_1.get() + || desired[2] != disabled_2.get() + || desired[3] != disabled_3.get() + { + panic!(format!( + "test \"{}\":\nexpected: {:?}\n got: {:?}", + name, + desired, + [ + disabled_0.get(), + disabled_1.get(), + disabled_2.get(), + disabled_3.get() + ] + )) + } }; let id_0 = WidgetId::next(); @@ -403,54 +416,66 @@ fn simple_disable() { Harness::create_simple((), root, |harness| { harness.send_initial_events(); - check_states([None, None, None, None]); + harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); + check_states("send_initial_events", [None, None, None, None]); assert_eq!(harness.window().focus_chain().len(), 4); harness.submit_command(Command::new(CHANGE_DISABLED, true, id_0)); - check_states([Some(true), None, None, None]); + harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); + check_states("Change 1", [Some(true), None, None, None]); assert_eq!(harness.window().focus_chain().len(), 3); harness.submit_command(Command::new(CHANGE_DISABLED, true, id_2)); - check_states([Some(true), None, Some(true), None]); + harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); + check_states("Change 2", [Some(true), None, Some(true), None]); assert_eq!(harness.window().focus_chain().len(), 2); harness.submit_command(Command::new(CHANGE_DISABLED, true, id_3)); - check_states([Some(true), None, Some(true), Some(true)]); + harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); + check_states("Change 3", [Some(true), None, Some(true), Some(true)]); assert_eq!(harness.window().focus_chain().len(), 1); harness.submit_command(Command::new(CHANGE_DISABLED, false, id_2)); - check_states([Some(true), None, Some(false), Some(true)]); + harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); + check_states("Change 4", [Some(true), None, Some(false), Some(true)]); assert_eq!(harness.window().focus_chain().len(), 2); //This is intended the widget should not receive an event! harness.submit_command(Command::new(CHANGE_DISABLED, true, id_2)); - check_states([Some(true), None, Some(false), Some(true)]); + harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); + check_states("Change 5", [Some(true), None, Some(false), Some(true)]); assert_eq!(harness.window().focus_chain().len(), 2); //This is intended the widget should not receive an event! - harness.submit_command(Command::new(CHANGE_DISABLED, true, id_1)); - check_states([Some(true), None, Some(false), Some(true)]); + harness.submit_command(Command::new(CHANGE_DISABLED, false, id_1)); + harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); + check_states("Change 6", [Some(true), None, Some(false), Some(true)]); assert_eq!(harness.window().focus_chain().len(), 2); }) } -#[test] +/*#[test] fn disable_tree() { const MULTI_CHANGE_DISABLED: Selector> = Selector::new("druid-tests.multi-change-disabled"); let leaf_factory = |state: Rc>>| { - ModularWidget::new(state).lifecycle_fn(move |state, ctx, event, _, _| match event { - LifeCycle::WidgetAdded => { - ctx.register_for_focus(); - } - LifeCycle::DisabledChanged(disabled) => { - state.set(Some(*disabled)); + ModularWidget::new(state).lifecycle_fn(move |state, ctx, event, _, _| { + println!("lc leaf!"); + match event { + LifeCycle::WidgetAdded => { + ctx.register_for_focus(); + } + LifeCycle::DisabledChanged(disabled) => { + state.set(Some(*disabled)); + } + _ => {} } - _ => {} }) }; let wrapper = |id: WidgetId, widget: Box>| { ModularWidget::new(WidgetPod::new(widget)) .lifecycle_fn(|inner, ctx, event, data, env| { + println!("lc!"); inner.lifecycle(ctx, event, data, env); }) .event_fn(|inner, ctx, event, data, env| { + println!("ev!"); if let Event::Command(cmd) = event { if let Some(map) = cmd.get(MULTI_CHANGE_DISABLED) { if let Some(disabled) = map.get(&ctx.widget_id()) { @@ -476,13 +501,30 @@ fn disable_tree() { let disabled_4: Rc>> = Default::default(); let disabled_5: Rc>> = Default::default(); - let check_states = |desired: [Option; 6]| { - assert_eq!(desired[0], disabled_0.get()); - assert_eq!(desired[1], disabled_1.get()); - assert_eq!(desired[2], disabled_2.get()); - assert_eq!(desired[3], disabled_3.get()); - assert_eq!(desired[4], disabled_4.get()); - assert_eq!(desired[5], disabled_5.get()); + let check_states = |name: &str, desired: [Option; 6]| { + if desired[0] != disabled_0.get() + || desired[1] != disabled_1.get() + || desired[2] != disabled_2.get() + || desired[3] != disabled_3.get() + || desired[4] != disabled_4.get() + || desired[5] != disabled_5.get() + { + panic!( + format!( + "test \"{}\":\nexpected: {:?}\n got: {:?}", + name, + desired, + [ + disabled_0.get(), + disabled_1.get(), + disabled_2.get(), + disabled_3.get(), + disabled_4.get(), + disabled_5.get() + ] + ) + ); + } }; let outer_id = WidgetId::next(); @@ -509,104 +551,137 @@ fn disable_tree() { Harness::create_simple((), root, |harness| { harness.send_initial_events(); - check_states([None, None, None, None, None, None]); + harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); + check_states("Send initial events", [None, None, None, None, None, None]); assert_eq!(harness.window().focus_chain().len(), 6); harness.submit_command(multi_update(&[(root_id, true)])); - check_states([ - Some(true), - Some(true), - Some(true), - Some(true), - Some(true), - Some(true), - ]); + harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); + check_states( + "disable root (0)", + [ + Some(true), + Some(true), + Some(true), + Some(true), + Some(true), + Some(true), + ], + ); assert_eq!(harness.window().focus_chain().len(), 0); harness.submit_command(multi_update(&[(inner_id, true)])); - check_states([ - Some(true), - Some(true), - Some(true), - Some(true), - Some(true), - Some(true), - ]); + harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); + check_states( + "disable inner (1)", + [ + Some(true), + Some(true), + Some(true), + Some(true), + Some(true), + Some(true), + ], + ); assert_eq!(harness.window().focus_chain().len(), 0); // Node 0 should not be affected harness.submit_command(multi_update(&[(root_id, false)])); - check_states([ - Some(true), - Some(true), - Some(false), - Some(false), - Some(false), - Some(false), - ]); + harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); + check_states( + "enable root (2)", + [ + Some(true), + Some(true), + Some(false), + Some(false), + Some(false), + Some(false), + ], + ); assert_eq!(harness.window().focus_chain().len(), 4); // Changing inner and outer in different directions should not affect the leaves harness.submit_command(multi_update(&[(inner_id, false), (outer_id, true)])); - check_states([ - Some(true), - Some(true), - Some(false), - Some(false), - Some(false), - Some(false), - ]); + harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); + check_states( + "change inner outer (3)", + [ + Some(true), + Some(true), + Some(false), + Some(false), + Some(false), + Some(false), + ], + ); assert_eq!(harness.window().focus_chain().len(), 4); // Changing inner and outer in different directions should not affect the leaves harness.submit_command(multi_update(&[(inner_id, true), (outer_id, false)])); - check_states([ - Some(true), - Some(true), - Some(false), - Some(false), - Some(false), - Some(false), - ]); + harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); + check_states( + "change inner outer (4)", + [ + Some(true), + Some(true), + Some(false), + Some(false), + Some(false), + Some(false), + ], + ); assert_eq!(harness.window().focus_chain().len(), 4); // Changing two widgets on the same level harness.submit_command(multi_update(&[(single_id, true), (inner_id, false)])); - check_states([ - Some(false), - Some(false), - Some(true), - Some(false), - Some(false), - Some(false), - ]); + harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); + check_states( + "change horizontal (5)", + [ + Some(false), + Some(false), + Some(true), + Some(false), + Some(false), + Some(false), + ], + ); assert_eq!(harness.window().focus_chain().len(), 5); // Disabling the root should disable all widgets harness.submit_command(multi_update(&[(root_id, true)])); - check_states([ - Some(true), - Some(true), - Some(true), - Some(true), - Some(true), - Some(true), - ]); + harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); + check_states( + "disable root (6)", + [ + Some(true), + Some(true), + Some(true), + Some(true), + Some(true), + Some(true), + ], + ); assert_eq!(harness.window().focus_chain().len(), 0); // Enabling a widget in a disabled tree should not affect the enclosed widgets harness.submit_command(multi_update(&[(single_id, false)])); - check_states([ - Some(true), - Some(true), - Some(true), - Some(true), - Some(true), - Some(true), - ]); + harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); + check_states( + "enable single (7)", + [ + Some(true), + Some(true), + Some(true), + Some(true), + Some(true), + Some(true), + ], + ); assert_eq!(harness.window().focus_chain().len(), 0); }) -} +}*/ #[test] fn simple_lifecyle() { From f60619910ce540c2b3b29e6b9373e9dd718e07c4 Mon Sep 17 00:00:00 2001 From: xarvic Date: Sat, 20 Mar 2021 16:37:38 +0100 Subject: [PATCH 05/35] updated core.rs and event.rs --- druid/src/core.rs | 23 ++++++++++++----------- druid/src/event.rs | 7 +++---- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/druid/src/core.rs b/druid/src/core.rs index 2f010af950..b9cb4c0898 100644 --- a/druid/src/core.rs +++ b/druid/src/core.rs @@ -833,8 +833,8 @@ impl> WidgetPod { // Doing this conditionally only makes sense when there's a measurable performance boost. ctx.widget_state.merge_up(&mut self.state); - ctx.widget_state.children_disabled_changed |= (self.state.is_explicitly_disabled_new - != self.state.is_explicitly_disabled); + ctx.widget_state.children_disabled_changed |= + self.state.is_explicitly_disabled_new != self.state.is_explicitly_disabled; } /// Send notifications originating from this widget's children to this @@ -1015,7 +1015,8 @@ impl> WidgetPod { if was_disabled != self.state.is_disabled() { extra_event = Some(LifeCycle::DisabledChanged(self.state.is_disabled())); } else if self.state.children_disabled_changed { - extra_event = Some(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); + extra_event = + Some(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); } if self.state.is_disabled() && self.state.has_focus { @@ -1027,7 +1028,7 @@ impl> WidgetPod { //It is easier to always use the extra event since we can disable our self's while //our parent was enabled and vice versa. false - }, + } //NOTE: this is not sent here, but from the special set_hot_state method LifeCycle::HotChanged(_) => false, LifeCycle::FocusChanged(_) => { @@ -1050,16 +1051,16 @@ impl> WidgetPod { self.inner.lifecycle(&mut child_ctx, event, data, env); } - if let LifeCycle::DisabledChanged(_) | LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged) = event { + if let LifeCycle::DisabledChanged(_) + | LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged) = event + { self.state.children_disabled_changed = false; ctx.widget_state.focus_chain.extend(&self.state.focus_chain); //Delete changes of disabled state that happened during DisabledChanged self.state.is_explicitly_disabled_new = self.state.is_explicitly_disabled; - } else if self.state.is_explicitly_disabled != self.state.is_explicitly_disabled_new { ctx.widget_state.children_disabled_changed = true; - } ctx.widget_state.merge_up(&mut self.state); @@ -1135,9 +1136,9 @@ impl> WidgetPod { self.state.request_update = false; ctx.widget_state.merge_up(&mut self.state); -> - ctx.widget_state.children_disabled_changed |= (self.state.is_explicitly_disabled_new - != self.state.is_explicitly_disabled); + + ctx.widget_state.children_disabled_changed |= + self.state.is_explicitly_disabled_new != self.state.is_explicitly_disabled; } } @@ -1193,7 +1194,7 @@ impl WidgetState { cursor_change: CursorChange::Default, cursor: None, sub_window_hosts: Vec::new(), - is_explicitly_disabled_new: false + is_explicitly_disabled_new: false, } } diff --git a/druid/src/event.rs b/druid/src/event.rs index 3fde459c16..598a1ffab8 100644 --- a/druid/src/event.rs +++ b/druid/src/event.rs @@ -415,10 +415,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 { .. } | - InternalLifeCycle::RouteDisabledChanged => { - true - } + InternalLifeCycle::RouteWidgetAdded + | InternalLifeCycle::RouteFocusChanged { .. } + | InternalLifeCycle::RouteDisabledChanged => true, InternalLifeCycle::ParentWindowOrigin => false, #[cfg(test)] InternalLifeCycle::DebugRequestState { .. } From 6fd0b717bff682c796291b0c7172a6618f1eca66 Mon Sep 17 00:00:00 2001 From: xarvic Date: Mon, 22 Mar 2021 14:27:26 +0100 Subject: [PATCH 06/35] fixed focus-chain bug: - the focus chain was cleared, if the widget was disabled --- druid/src/contexts.rs | 2 +- druid/src/core.rs | 47 ++++++++++++++++++++++++++++++------------- 2 files changed, 34 insertions(+), 15 deletions(-) diff --git a/druid/src/contexts.rs b/druid/src/contexts.rs index f13107792c..0e1c813650 100644 --- a/druid/src/contexts.rs +++ b/druid/src/contexts.rs @@ -697,7 +697,7 @@ impl LifeCycleCtx<'_, '_> { /// [`LifeCycle::WidgetAdded`]: enum.Lifecycle.html#variant.WidgetAdded /// [`EventCtx::is_focused`]: struct.EventCtx.html#method.is_focused pub fn register_for_focus(&mut self) { - self.widget_state.focus_chain.push(self.widget_id()); + self.widget_state.auto_focus = true; } } diff --git a/druid/src/core.rs b/druid/src/core.rs index b9cb4c0898..e16825c88b 100644 --- a/druid/src/core.rs +++ b/druid/src/core.rs @@ -118,6 +118,11 @@ pub(crate) struct WidgetState { // LifeCycle::DisabledChanged or InternalLifeCycle::RouteDisabledChanged pub(crate) is_explicitly_disabled_new: bool, + // This widget get focused automatically. + // We need to store this here since the widget local focus-chain is cleared when the widget is + // disabled + pub(crate) auto_focus: bool, + pub(crate) is_hot: bool, pub(crate) is_active: bool, @@ -956,11 +961,6 @@ impl> WidgetPod { true } InternalLifeCycle::RouteDisabledChanged => { - //If our children changed update the focus-chain - if self.state.children_disabled_changed { - self.state.focus_chain.clear(); - } - let was_disabled = self.state.is_disabled(); self.state.is_explicitly_disabled = self.state.is_explicitly_disabled_new; @@ -972,11 +972,17 @@ impl> WidgetPod { } if was_disabled != self.state.is_disabled() { + // In case we change but none of our children we still need to update the + // focus-chain + self.state.focus_chain.clear(); extra_event = Some(LifeCycle::DisabledChanged(self.state.is_disabled())); //Each widget needs only one of DisabledChanged and RouteDisabledChanged false + } else if self.state.children_disabled_changed { + self.state.focus_chain.clear(); + true } else { - self.state.children_disabled_changed + false } } }, @@ -1002,19 +1008,19 @@ impl> WidgetPod { false } LifeCycle::DisabledChanged(ancestors_disabled) => { - //If our children changed update the focus-chain - if self.state.children_disabled_changed { - self.state.focus_chain.clear(); - } - let was_disabled = self.state.is_disabled(); self.state.is_explicitly_disabled = self.state.is_explicitly_disabled_new; self.state.ancestor_disabled = *ancestors_disabled; if was_disabled != self.state.is_disabled() { + // In case we change but none of our children we still need to update the + // focus-chain + self.state.focus_chain.clear(); extra_event = Some(LifeCycle::DisabledChanged(self.state.is_disabled())); + } else if self.state.children_disabled_changed { + self.state.focus_chain.clear(); extra_event = Some(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); } @@ -1025,8 +1031,8 @@ impl> WidgetPod { self.state.request_focus = Some(FocusChange::Resign); } - //It is easier to always use the extra event since we can disable our self's while - //our parent was enabled and vice versa. + // It is easier to always use the extra event since the widget can disable itself + // while its parent was enabled and vice versa. false } //NOTE: this is not sent here, but from the special set_hot_state method @@ -1055,7 +1061,12 @@ impl> WidgetPod { | LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged) = event { self.state.children_disabled_changed = false; - ctx.widget_state.focus_chain.extend(&self.state.focus_chain); + if !self.state.is_disabled() { + if self.state.auto_focus { + self.state.focus_chain.push(self.state.id); + } + ctx.widget_state.focus_chain.extend(&self.state.focus_chain); + } //Delete changes of disabled state that happened during DisabledChanged self.state.is_explicitly_disabled_new = self.state.is_explicitly_disabled; @@ -1070,6 +1081,9 @@ impl> WidgetPod { LifeCycle::WidgetAdded | LifeCycle::Internal(InternalLifeCycle::RouteWidgetAdded) => { self.state.children_changed = false; ctx.widget_state.children = ctx.widget_state.children.union(self.state.children); + if self.state.auto_focus { + self.state.focus_chain.push(self.state.id); + } ctx.widget_state.focus_chain.extend(&self.state.focus_chain); ctx.register_child(self.id()); } @@ -1195,6 +1209,7 @@ impl WidgetState { cursor: None, sub_window_hosts: Vec::new(), is_explicitly_disabled_new: false, + auto_focus: false, } } @@ -1202,6 +1217,10 @@ impl WidgetState { 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); } From 25d19c98c0c54c4ce8d253bfe14cca2cbcde51a2 Mon Sep 17 00:00:00 2001 From: xarvic Date: Mon, 22 Mar 2021 14:28:01 +0100 Subject: [PATCH 07/35] fix disabled update --- druid/src/window.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/druid/src/window.rs b/druid/src/window.rs index 776a720d80..faf65138a0 100644 --- a/druid/src/window.rs +++ b/druid/src/window.rs @@ -157,7 +157,7 @@ impl Window { // Update the disabled state if necessary // Always do this before sending focus change, since this event updates the focus chain. - if self.root.state().is_disabled() { + if self.root.state().tree_disabled_changed() { let event = LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged); self.lifecycle(queue, &event, data, env, false); } From e3d8b925da388b3c355a7d07906a94d86e600704 Mon Sep 17 00:00:00 2001 From: xarvic Date: Mon, 22 Mar 2021 14:58:59 +0100 Subject: [PATCH 08/35] update tests --- druid/src/tests/mod.rs | 119 ++++++++++++++++++++++++----------------- 1 file changed, 69 insertions(+), 50 deletions(-) diff --git a/druid/src/tests/mod.rs b/druid/src/tests/mod.rs index 84dacda515..0c99dd75c8 100644 --- a/druid/src/tests/mod.rs +++ b/druid/src/tests/mod.rs @@ -20,9 +20,12 @@ mod invalidation_tests; mod layout_tests; use std::cell::Cell; -//use std::collections::HashMap; +use std::collections::HashMap; use std::env; use std::fs; +use std::fs::File; +use std::fs::OpenOptions; +use std::io::Write; use std::rc::Rc; use crate::widget::*; @@ -416,46 +419,59 @@ fn simple_disable() { Harness::create_simple((), root, |harness| { harness.send_initial_events(); - harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); check_states("send_initial_events", [None, None, None, None]); - assert_eq!(harness.window().focus_chain().len(), 4); + assert_eq!(harness.window().focus_chain(), &[id_0, id_1, id_2, id_3]); harness.submit_command(Command::new(CHANGE_DISABLED, true, id_0)); - harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); check_states("Change 1", [Some(true), None, None, None]); - assert_eq!(harness.window().focus_chain().len(), 3); + assert_eq!(harness.window().focus_chain(), &[id_1, id_2, id_3]); harness.submit_command(Command::new(CHANGE_DISABLED, true, id_2)); - harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); check_states("Change 2", [Some(true), None, Some(true), None]); - assert_eq!(harness.window().focus_chain().len(), 2); + assert_eq!(harness.window().focus_chain(), &[id_1, id_3]); harness.submit_command(Command::new(CHANGE_DISABLED, true, id_3)); - harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); check_states("Change 3", [Some(true), None, Some(true), Some(true)]); - assert_eq!(harness.window().focus_chain().len(), 1); + assert_eq!(harness.window().focus_chain(), &[id_1]); harness.submit_command(Command::new(CHANGE_DISABLED, false, id_2)); - harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); check_states("Change 4", [Some(true), None, Some(false), Some(true)]); - assert_eq!(harness.window().focus_chain().len(), 2); - //This is intended the widget should not receive an event! + assert_eq!(harness.window().focus_chain(), &[id_1, id_2]); harness.submit_command(Command::new(CHANGE_DISABLED, true, id_2)); - harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); - check_states("Change 5", [Some(true), None, Some(false), Some(true)]); - assert_eq!(harness.window().focus_chain().len(), 2); + check_states("Change 5", [Some(true), None, Some(true), Some(true)]); + assert_eq!(harness.window().focus_chain(), &[id_1]); //This is intended the widget should not receive an event! harness.submit_command(Command::new(CHANGE_DISABLED, false, id_1)); - harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); - check_states("Change 6", [Some(true), None, Some(false), Some(true)]); - assert_eq!(harness.window().focus_chain().len(), 2); + check_states("Change 6", [Some(true), None, Some(true), Some(true)]); + assert_eq!(harness.window().focus_chain(), &[id_1]); }) } -/*#[test] +#[test] fn disable_tree() { const MULTI_CHANGE_DISABLED: Selector> = Selector::new("druid-tests.multi-change-disabled"); + fn write_direct(text: &str) { + static mut FILE: Option = None; + + #[allow(unsafe_code)] + unsafe { + if FILE.is_none() { + FILE = Some( + OpenOptions::new() + .write(true) + .create(true) + .open("./test_log") + .unwrap(), + ); + } + let file = FILE.as_mut().unwrap(); + file.write_all(text.as_bytes()).unwrap(); + file.write_all("\n".as_bytes()).unwrap(); + file.flush().unwrap(); + } + } + let leaf_factory = |state: Rc>>| { ModularWidget::new(state).lifecycle_fn(move |state, ctx, event, _, _| { - println!("lc leaf!"); + write_direct("lc leaf!"); match event { LifeCycle::WidgetAdded => { ctx.register_for_focus(); @@ -471,20 +487,21 @@ fn disable_tree() { let wrapper = |id: WidgetId, widget: Box>| { ModularWidget::new(WidgetPod::new(widget)) .lifecycle_fn(|inner, ctx, event, data, env| { - println!("lc!"); + write_direct("lc"); inner.lifecycle(ctx, event, data, env); + write_direct("lc finished"); }) .event_fn(|inner, ctx, event, data, env| { - println!("ev!"); + write_direct("ev"); if let Event::Command(cmd) = event { if let Some(map) = cmd.get(MULTI_CHANGE_DISABLED) { if let Some(disabled) = map.get(&ctx.widget_id()) { + write_direct("change state"); ctx.set_disabled(*disabled); } } - } else { - inner.event(ctx, event, data, env); } + inner.event(ctx, event, data, env); }) .with_id(id) }; @@ -509,21 +526,19 @@ fn disable_tree() { || desired[4] != disabled_4.get() || desired[5] != disabled_5.get() { - panic!( - format!( - "test \"{}\":\nexpected: {:?}\n got: {:?}", - name, - desired, - [ - disabled_0.get(), - disabled_1.get(), - disabled_2.get(), - disabled_3.get(), - disabled_4.get(), - disabled_5.get() - ] - ) - ); + panic!(format!( + "test \"{}\":\nexpected: {:?}\n got: {:?}", + name, + desired, + [ + disabled_0.get(), + disabled_1.get(), + disabled_2.get(), + disabled_3.get(), + disabled_4.get(), + disabled_5.get() + ] + )); } }; @@ -549,14 +564,17 @@ fn disable_tree() { let root = wrapper(root_id, node2); + write_direct("\n\n================= TEST DISABLE =====================\n"); + Harness::create_simple((), root, |harness| { harness.send_initial_events(); - harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); + write_direct("run lifecycle"); check_states("Send initial events", [None, None, None, None, None, None]); assert_eq!(harness.window().focus_chain().len(), 6); harness.submit_command(multi_update(&[(root_id, true)])); - harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); + write_direct("run lifecycle"); + write_direct("test1"); check_states( "disable root (0)", [ @@ -568,10 +586,12 @@ fn disable_tree() { Some(true), ], ); + write_direct("test2"); assert_eq!(harness.window().focus_chain().len(), 0); - + write_direct("test3"); harness.submit_command(multi_update(&[(inner_id, true)])); - harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); + + write_direct("run lifecycle"); check_states( "disable inner (1)", [ @@ -587,7 +607,6 @@ fn disable_tree() { // Node 0 should not be affected harness.submit_command(multi_update(&[(root_id, false)])); - harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); check_states( "enable root (2)", [ @@ -603,7 +622,7 @@ fn disable_tree() { // Changing inner and outer in different directions should not affect the leaves harness.submit_command(multi_update(&[(inner_id, false), (outer_id, true)])); - harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); + //harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); check_states( "change inner outer (3)", [ @@ -619,7 +638,7 @@ fn disable_tree() { // Changing inner and outer in different directions should not affect the leaves harness.submit_command(multi_update(&[(inner_id, true), (outer_id, false)])); - harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); + //harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); check_states( "change inner outer (4)", [ @@ -635,7 +654,7 @@ fn disable_tree() { // Changing two widgets on the same level harness.submit_command(multi_update(&[(single_id, true), (inner_id, false)])); - harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); + //harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); check_states( "change horizontal (5)", [ @@ -651,7 +670,7 @@ fn disable_tree() { // Disabling the root should disable all widgets harness.submit_command(multi_update(&[(root_id, true)])); - harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); + //harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); check_states( "disable root (6)", [ @@ -667,7 +686,7 @@ fn disable_tree() { // Enabling a widget in a disabled tree should not affect the enclosed widgets harness.submit_command(multi_update(&[(single_id, false)])); - harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); + //harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); check_states( "enable single (7)", [ @@ -681,7 +700,7 @@ fn disable_tree() { ); assert_eq!(harness.window().focus_chain().len(), 0); }) -}*/ +} #[test] fn simple_lifecyle() { From 77d4f8f1ca931c6345f2b90451933f2d4ea047c7 Mon Sep 17 00:00:00 2001 From: xarvic Date: Mon, 22 Mar 2021 15:33:39 +0100 Subject: [PATCH 09/35] fixed code (all tests succeed) --- druid/src/core.rs | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/druid/src/core.rs b/druid/src/core.rs index e16825c88b..7233a4de86 100644 --- a/druid/src/core.rs +++ b/druid/src/core.rs @@ -912,6 +912,9 @@ impl> WidgetPod { if self.state.children_changed { self.state.children.clear(); self.state.focus_chain.clear(); + if self.state.auto_focus { + self.state.focus_chain.push(self.state.id); + } } self.state.children_changed } @@ -975,11 +978,17 @@ impl> WidgetPod { // In case we change but none of our children we still need to update the // focus-chain self.state.focus_chain.clear(); + if self.state.auto_focus { + self.state.focus_chain.push(self.state.id); + } extra_event = Some(LifeCycle::DisabledChanged(self.state.is_disabled())); //Each widget needs only one of DisabledChanged and RouteDisabledChanged false } else if self.state.children_disabled_changed { self.state.focus_chain.clear(); + if self.state.auto_focus { + self.state.focus_chain.push(self.state.id); + } true } else { false @@ -1017,10 +1026,15 @@ impl> WidgetPod { // In case we change but none of our children we still need to update the // focus-chain self.state.focus_chain.clear(); + if self.state.auto_focus { + self.state.focus_chain.push(self.state.id); + } extra_event = Some(LifeCycle::DisabledChanged(self.state.is_disabled())); - } else if self.state.children_disabled_changed { self.state.focus_chain.clear(); + if self.state.auto_focus { + self.state.focus_chain.push(self.state.id); + } extra_event = Some(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); } @@ -1062,9 +1076,6 @@ impl> WidgetPod { { self.state.children_disabled_changed = false; if !self.state.is_disabled() { - if self.state.auto_focus { - self.state.focus_chain.push(self.state.id); - } ctx.widget_state.focus_chain.extend(&self.state.focus_chain); } @@ -1077,13 +1088,18 @@ impl> WidgetPod { ctx.widget_state.merge_up(&mut self.state); // we need to (re)register children in case of one of the following events + if let LifeCycle::WidgetAdded = event { + // If this widget is not already present, autofocus is not set before the event, + // since we call register_for_focus in widget added. + if self.state.auto_focus { + self.state.focus_chain.push(self.state.id); + } + } + match event { LifeCycle::WidgetAdded | LifeCycle::Internal(InternalLifeCycle::RouteWidgetAdded) => { self.state.children_changed = false; ctx.widget_state.children = ctx.widget_state.children.union(self.state.children); - if self.state.auto_focus { - self.state.focus_chain.push(self.state.id); - } ctx.widget_state.focus_chain.extend(&self.state.focus_chain); ctx.register_child(self.id()); } @@ -1218,7 +1234,8 @@ impl WidgetState { } pub(crate) fn tree_disabled_changed(&self) -> bool { - self.children_disabled_changed || self.is_explicitly_disabled != self.is_explicitly_disabled_new + self.children_disabled_changed + || self.is_explicitly_disabled != self.is_explicitly_disabled_new } pub(crate) fn add_timer(&mut self, timer_token: TimerToken) { From 4d48eddbd3691e59cb29c268c9989003b3b23244 Mon Sep 17 00:00:00 2001 From: xarvic Date: Tue, 23 Mar 2021 09:49:28 +0100 Subject: [PATCH 10/35] refactored core.rs and tests/mod.rs --- druid/src/core.rs | 32 ++++++++++++-------------------- druid/src/tests/mod.rs | 30 ------------------------------ 2 files changed, 12 insertions(+), 50 deletions(-) diff --git a/druid/src/core.rs b/druid/src/core.rs index 7233a4de86..1853721c1b 100644 --- a/druid/src/core.rs +++ b/druid/src/core.rs @@ -911,10 +911,7 @@ impl> WidgetPod { } else { if self.state.children_changed { self.state.children.clear(); - self.state.focus_chain.clear(); - if self.state.auto_focus { - self.state.focus_chain.push(self.state.id); - } + self.state.reset_focus_chain(); } self.state.children_changed } @@ -977,18 +974,12 @@ impl> WidgetPod { if was_disabled != self.state.is_disabled() { // In case we change but none of our children we still need to update the // focus-chain - self.state.focus_chain.clear(); - if self.state.auto_focus { - self.state.focus_chain.push(self.state.id); - } + self.state.reset_focus_chain(); extra_event = Some(LifeCycle::DisabledChanged(self.state.is_disabled())); //Each widget needs only one of DisabledChanged and RouteDisabledChanged false } else if self.state.children_disabled_changed { - self.state.focus_chain.clear(); - if self.state.auto_focus { - self.state.focus_chain.push(self.state.id); - } + self.state.reset_focus_chain(); true } else { false @@ -1025,16 +1016,10 @@ impl> WidgetPod { if was_disabled != self.state.is_disabled() { // In case we change but none of our children we still need to update the // focus-chain - self.state.focus_chain.clear(); - if self.state.auto_focus { - self.state.focus_chain.push(self.state.id); - } + self.state.reset_focus_chain(); extra_event = Some(LifeCycle::DisabledChanged(self.state.is_disabled())); } else if self.state.children_disabled_changed { - self.state.focus_chain.clear(); - if self.state.auto_focus { - self.state.focus_chain.push(self.state.id); - } + self.state.reset_focus_chain(); extra_event = Some(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); } @@ -1242,6 +1227,13 @@ impl WidgetState { self.timers.insert(timer_token, self.id); } + pub(crate) fn reset_focus_chain(&mut self) { + self.focus_chain.clear(); + if self.auto_focus { + self.focus_chain.push(self.id); + } + } + /// Update to incorporate state changes from a child. /// /// This will also clear some requests in the child state. diff --git a/druid/src/tests/mod.rs b/druid/src/tests/mod.rs index 0c99dd75c8..8ee1c8bf86 100644 --- a/druid/src/tests/mod.rs +++ b/druid/src/tests/mod.rs @@ -448,30 +448,8 @@ fn disable_tree() { const MULTI_CHANGE_DISABLED: Selector> = Selector::new("druid-tests.multi-change-disabled"); - fn write_direct(text: &str) { - static mut FILE: Option = None; - - #[allow(unsafe_code)] - unsafe { - if FILE.is_none() { - FILE = Some( - OpenOptions::new() - .write(true) - .create(true) - .open("./test_log") - .unwrap(), - ); - } - let file = FILE.as_mut().unwrap(); - file.write_all(text.as_bytes()).unwrap(); - file.write_all("\n".as_bytes()).unwrap(); - file.flush().unwrap(); - } - } - let leaf_factory = |state: Rc>>| { ModularWidget::new(state).lifecycle_fn(move |state, ctx, event, _, _| { - write_direct("lc leaf!"); match event { LifeCycle::WidgetAdded => { ctx.register_for_focus(); @@ -487,12 +465,9 @@ fn disable_tree() { let wrapper = |id: WidgetId, widget: Box>| { ModularWidget::new(WidgetPod::new(widget)) .lifecycle_fn(|inner, ctx, event, data, env| { - write_direct("lc"); inner.lifecycle(ctx, event, data, env); - write_direct("lc finished"); }) .event_fn(|inner, ctx, event, data, env| { - write_direct("ev"); if let Event::Command(cmd) = event { if let Some(map) = cmd.get(MULTI_CHANGE_DISABLED) { if let Some(disabled) = map.get(&ctx.widget_id()) { @@ -622,7 +597,6 @@ fn disable_tree() { // Changing inner and outer in different directions should not affect the leaves harness.submit_command(multi_update(&[(inner_id, false), (outer_id, true)])); - //harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); check_states( "change inner outer (3)", [ @@ -638,7 +612,6 @@ fn disable_tree() { // Changing inner and outer in different directions should not affect the leaves harness.submit_command(multi_update(&[(inner_id, true), (outer_id, false)])); - //harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); check_states( "change inner outer (4)", [ @@ -654,7 +627,6 @@ fn disable_tree() { // Changing two widgets on the same level harness.submit_command(multi_update(&[(single_id, true), (inner_id, false)])); - //harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); check_states( "change horizontal (5)", [ @@ -670,7 +642,6 @@ fn disable_tree() { // Disabling the root should disable all widgets harness.submit_command(multi_update(&[(root_id, true)])); - //harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); check_states( "disable root (6)", [ @@ -686,7 +657,6 @@ fn disable_tree() { // Enabling a widget in a disabled tree should not affect the enclosed widgets harness.submit_command(multi_update(&[(single_id, false)])); - //harness.lifecycle(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); check_states( "enable single (7)", [ From 9b2c8ba86c63c838702e95524db7a228e03550f2 Mon Sep 17 00:00:00 2001 From: xarvic Date: Tue, 23 Mar 2021 13:28:27 +0100 Subject: [PATCH 11/35] updated tests --- druid/src/tests/mod.rs | 103 ++++++++++++++++++++++++++++++++--------- 1 file changed, 82 insertions(+), 21 deletions(-) diff --git a/druid/src/tests/mod.rs b/druid/src/tests/mod.rs index 8ee1c8bf86..856c566454 100644 --- a/druid/src/tests/mod.rs +++ b/druid/src/tests/mod.rs @@ -23,9 +23,6 @@ use std::cell::Cell; use std::collections::HashMap; use std::env; use std::fs; -use std::fs::File; -use std::fs::OpenOptions; -use std::io::Write; use std::rc::Rc; use crate::widget::*; @@ -443,22 +440,94 @@ fn simple_disable() { }) } +#[test] +fn resign_focus_on_disable() { + const CHANGE_DISABLED: Selector = Selector::new("druid-tests.change-disabled-disable"); + const REQUEST_FOCUS: Selector<()> = Selector::new("druid-tests.change-disabled-focus"); + + let test_widget_factory = |auto_focus: bool, id: WidgetId, inner: Option>>| { + ModularWidget::new(inner.map(|widget|WidgetPod::new(widget))) + .lifecycle_fn(move |state, ctx, event, data, env| { + match event { + LifeCycle::WidgetAdded => { + if auto_focus { + ctx.register_for_focus(); + } + } + _ => {} + } + if let Some(inner) = state { + inner.lifecycle(ctx, event, data, env); + } + }) + .event_fn(|state, ctx, event, data, env| { + if let Event::Command(cmd) = event { + if let Some(disabled) = cmd.get(CHANGE_DISABLED) { + ctx.set_disabled(*disabled); + return; + } + if cmd.is(REQUEST_FOCUS) { + ctx.request_focus(); + return; + } + } + if let Some(inner) = state { + inner.event(ctx, event, data, env); + } + }) + .with_id(id) + }; + + let id_0 = WidgetId::next(); + let id_1 = WidgetId::next(); + let id_2 = WidgetId::next(); + + let root = Flex::row() + .with_child(test_widget_factory(true, id_0, Some(test_widget_factory(true, id_1, None).boxed()))) + .with_child(test_widget_factory(true, id_2, None)); + + Harness::create_simple((), root, |harness| { + harness.send_initial_events(); + assert_eq!(harness.window().focus_chain(), &[id_0, id_1, id_2]); + assert_eq!(harness.window().focus, None); + harness.submit_command(Command::new(REQUEST_FOCUS, (), id_2)); + assert_eq!(harness.window().focus_chain(), &[id_0, id_1, id_2]); + assert_eq!(harness.window().focus, Some(id_2)); + harness.submit_command(Command::new(CHANGE_DISABLED, true, id_0)); + assert_eq!(harness.window().focus_chain(), &[id_2]); + assert_eq!(harness.window().focus, Some(id_2)); + harness.submit_command(Command::new(CHANGE_DISABLED, true, id_2)); + assert_eq!(harness.window().focus_chain(), &[]); + assert_eq!(harness.window().focus, None); + harness.submit_command(Command::new(CHANGE_DISABLED, false, id_0)); + assert_eq!(harness.window().focus_chain(), &[id_0, id_1]); + assert_eq!(harness.window().focus, None); + harness.submit_command(Command::new(REQUEST_FOCUS, (), id_1)); + assert_eq!(harness.window().focus_chain(), &[id_0, id_1]); + assert_eq!(harness.window().focus, Some(id_1)); + harness.submit_command(Command::new(CHANGE_DISABLED, false, id_2)); + assert_eq!(harness.window().focus_chain(), &[id_0, id_1, id_2]); + assert_eq!(harness.window().focus, Some(id_1)); + harness.submit_command(Command::new(CHANGE_DISABLED, true, id_0)); + assert_eq!(harness.window().focus_chain(), &[id_2]); + assert_eq!(harness.window().focus, None); + }) +} + #[test] fn disable_tree() { const MULTI_CHANGE_DISABLED: Selector> = Selector::new("druid-tests.multi-change-disabled"); let leaf_factory = |state: Rc>>| { - ModularWidget::new(state).lifecycle_fn(move |state, ctx, event, _, _| { - match event { - LifeCycle::WidgetAdded => { - ctx.register_for_focus(); - } - LifeCycle::DisabledChanged(disabled) => { - state.set(Some(*disabled)); - } - _ => {} + ModularWidget::new(state).lifecycle_fn(move |state, ctx, event, _, _| match event { + LifeCycle::WidgetAdded => { + ctx.register_for_focus(); } + LifeCycle::DisabledChanged(disabled) => { + state.set(Some(*disabled)); + } + _ => {} }) }; @@ -471,8 +540,8 @@ fn disable_tree() { if let Event::Command(cmd) = event { if let Some(map) = cmd.get(MULTI_CHANGE_DISABLED) { if let Some(disabled) = map.get(&ctx.widget_id()) { - write_direct("change state"); ctx.set_disabled(*disabled); + return; } } } @@ -539,17 +608,12 @@ fn disable_tree() { let root = wrapper(root_id, node2); - write_direct("\n\n================= TEST DISABLE =====================\n"); - Harness::create_simple((), root, |harness| { harness.send_initial_events(); - write_direct("run lifecycle"); check_states("Send initial events", [None, None, None, None, None, None]); assert_eq!(harness.window().focus_chain().len(), 6); harness.submit_command(multi_update(&[(root_id, true)])); - write_direct("run lifecycle"); - write_direct("test1"); check_states( "disable root (0)", [ @@ -561,12 +625,9 @@ fn disable_tree() { Some(true), ], ); - write_direct("test2"); assert_eq!(harness.window().focus_chain().len(), 0); - write_direct("test3"); harness.submit_command(multi_update(&[(inner_id, true)])); - write_direct("run lifecycle"); check_states( "disable inner (1)", [ From d5116de975c75e1251eb3c3b1773ec6d34f7db82 Mon Sep 17 00:00:00 2001 From: xarvic Date: Tue, 23 Mar 2021 13:28:50 +0100 Subject: [PATCH 12/35] fixed focus-chain bug --- druid/src/core.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/druid/src/core.rs b/druid/src/core.rs index 1853721c1b..b858cb4c7a 100644 --- a/druid/src/core.rs +++ b/druid/src/core.rs @@ -1076,8 +1076,10 @@ impl> WidgetPod { if let LifeCycle::WidgetAdded = event { // If this widget is not already present, autofocus is not set before the event, // since we call register_for_focus in widget added. + // To be consistent with RouteWidgetAdded DisabledChanged and Route DisabledChange we + // insert our id at the first position. if self.state.auto_focus { - self.state.focus_chain.push(self.state.id); + self.state.focus_chain.insert(0, self.state.id); } } From 6c79e1e783a5a4ee8c24f953f13367a4cf3fdc45 Mon Sep 17 00:00:00 2001 From: xarvic Date: Tue, 23 Mar 2021 13:43:26 +0100 Subject: [PATCH 13/35] make clippy happy (i hope) --- druid/src/tests/mod.rs | 56 ++++++++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/druid/src/tests/mod.rs b/druid/src/tests/mod.rs index 856c566454..b705b03620 100644 --- a/druid/src/tests/mod.rs +++ b/druid/src/tests/mod.rs @@ -445,45 +445,47 @@ fn resign_focus_on_disable() { const CHANGE_DISABLED: Selector = Selector::new("druid-tests.change-disabled-disable"); const REQUEST_FOCUS: Selector<()> = Selector::new("druid-tests.change-disabled-focus"); - let test_widget_factory = |auto_focus: bool, id: WidgetId, inner: Option>>| { - ModularWidget::new(inner.map(|widget|WidgetPod::new(widget))) - .lifecycle_fn(move |state, ctx, event, data, env| { - match event { - LifeCycle::WidgetAdded => { + let test_widget_factory = + |auto_focus: bool, id: WidgetId, inner: Option>>| { + ModularWidget::new(inner.map(WidgetPod::new)) + .lifecycle_fn(move |state, ctx, event, data, env| { + if let LifeCycle::WidgetAdded = event { if auto_focus { ctx.register_for_focus(); } } - _ => {} - } - if let Some(inner) = state { - inner.lifecycle(ctx, event, data, env); - } - }) - .event_fn(|state, ctx, event, data, env| { - if let Event::Command(cmd) = event { - if let Some(disabled) = cmd.get(CHANGE_DISABLED) { - ctx.set_disabled(*disabled); - return; + if let Some(inner) = state { + inner.lifecycle(ctx, event, data, env); } - if cmd.is(REQUEST_FOCUS) { - ctx.request_focus(); - return; + }) + .event_fn(|state, ctx, event, data, env| { + if let Event::Command(cmd) = event { + if let Some(disabled) = cmd.get(CHANGE_DISABLED) { + ctx.set_disabled(*disabled); + return; + } + if cmd.is(REQUEST_FOCUS) { + ctx.request_focus(); + return; + } } - } - if let Some(inner) = state { - inner.event(ctx, event, data, env); - } - }) - .with_id(id) - }; + if let Some(inner) = state { + inner.event(ctx, event, data, env); + } + }) + .with_id(id) + }; let id_0 = WidgetId::next(); let id_1 = WidgetId::next(); let id_2 = WidgetId::next(); let root = Flex::row() - .with_child(test_widget_factory(true, id_0, Some(test_widget_factory(true, id_1, None).boxed()))) + .with_child(test_widget_factory( + true, + id_0, + Some(test_widget_factory(true, id_1, None).boxed()), + )) .with_child(test_widget_factory(true, id_2, None)); Harness::create_simple((), root, |harness| { From 40be21f92db8df8fb911b940c1da92ed116b75da Mon Sep 17 00:00:00 2001 From: xarvic Date: Tue, 23 Mar 2021 14:12:46 +0100 Subject: [PATCH 14/35] make clippy happy #2 --- druid/src/contexts.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/druid/src/contexts.rs b/druid/src/contexts.rs index 129aafd89d..933eb28809 100644 --- a/druid/src/contexts.rs +++ b/druid/src/contexts.rs @@ -406,7 +406,7 @@ impl_context_method!(EventCtx<'_, '_>, UpdateCtx<'_, '_>, LifeCycleCtx<'_, '_>, // 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 From 5520e5baa9bb9797a58984054d134703ae5cfb55 Mon Sep 17 00:00:00 2001 From: Christoph Date: Wed, 31 Mar 2021 10:34:09 +0000 Subject: [PATCH 15/35] Apply suggestions from code review Update Documentation Co-authored-by: Colin Rofls --- druid/src/core.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/druid/src/core.rs b/druid/src/core.rs index f675211718..2586ac7dc7 100644 --- a/druid/src/core.rs +++ b/druid/src/core.rs @@ -106,23 +106,24 @@ pub(crate) struct WidgetState { pub(crate) viewport_offset: Vec2, // TODO: consider using bitflags for the booleans. - // Any child of this widget changed the disabled state and should either receive + // `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, - // Any of our ancestors are disabled. Therefore we are also disabled. + // `true` if one of our ancestors is disabled (meaning we are also disabled). pub(crate) ancestor_disabled: bool, - // explicitly disabled through the usage of EventCtx::set_disabled(true) on this widget. + // `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, - // A buffer for the value of EventCtx::set_disabled() before this widget receives + // `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, - // This widget get focused automatically. - // We need to store this here since the widget local focus-chain is cleared when the widget is - // disabled + // `true` if this widget participates in the focus chain. + // We store this as a bool because we need to know how to update the focus chain + // when disabled/enabled state changes. pub(crate) auto_focus: bool, pub(crate) is_hot: bool, From 232e7ee1a573041e1aa79e8d958f0831de2562c9 Mon Sep 17 00:00:00 2001 From: Christoph Date: Wed, 31 Mar 2021 10:35:28 +0000 Subject: [PATCH 16/35] Update druid/src/contexts.rs Update documentation Co-authored-by: Colin Rofls --- druid/src/contexts.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/druid/src/contexts.rs b/druid/src/contexts.rs index 933eb28809..0646ba210b 100644 --- a/druid/src/contexts.rs +++ b/druid/src/contexts.rs @@ -394,8 +394,10 @@ impl_context_method!(EventCtx<'_, '_>, UpdateCtx<'_, '_>, LifeCycleCtx<'_, '_>, self.request_layout(); } - /// Sets the explicitly disabled state of this widget. - /// The widget may still be disabled if `set_disabled(false)` was called. See [`is_disabled`] + /// 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. /// From eccd59651705af41293b661b79a8e4ce4e4764db Mon Sep 17 00:00:00 2001 From: xarvic Date: Wed, 31 Mar 2021 14:19:12 +0200 Subject: [PATCH 17/35] refactor DisabledChanged --- druid/src/core.rs | 68 +++++++++++++++++++++-------------------------- 1 file changed, 31 insertions(+), 37 deletions(-) diff --git a/druid/src/core.rs b/druid/src/core.rs index 2586ac7dc7..8e767031d6 100644 --- a/druid/src/core.rs +++ b/druid/src/core.rs @@ -859,9 +859,6 @@ impl> WidgetPod { // Always merge even if not needed, because merging is idempotent and gives us simpler code. // Doing this conditionally only makes sense when there's a measurable performance boost. ctx.widget_state.merge_up(&mut self.state); - - ctx.widget_state.children_disabled_changed |= - self.state.is_explicitly_disabled_new != self.state.is_explicitly_disabled; } /// Send notifications originating from this widget's children to this @@ -931,7 +928,7 @@ impl> WidgetPod { } else { if self.state.children_changed { self.state.children.clear(); - self.state.reset_focus_chain(); + self.state.focus_chain.clear(); } self.state.children_changed } @@ -985,21 +982,15 @@ impl> WidgetPod { self.state.is_explicitly_disabled = self.state.is_explicitly_disabled_new; - 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); - } - if was_disabled != self.state.is_disabled() { // In case we change but none of our children we still need to update the // focus-chain - self.state.reset_focus_chain(); + self.state.focus_chain.clear(); extra_event = Some(LifeCycle::DisabledChanged(self.state.is_disabled())); //Each widget needs only one of DisabledChanged and RouteDisabledChanged false } else if self.state.children_disabled_changed { - self.state.reset_focus_chain(); + self.state.focus_chain.clear(); true } else { false @@ -1035,22 +1026,16 @@ impl> WidgetPod { self.state.ancestor_disabled = *ancestors_disabled; if was_disabled != self.state.is_disabled() { - // In case we change but none of our children we still need to update the + // In case we change but none of our children does we still need to update the // focus-chain - self.state.reset_focus_chain(); + self.state.focus_chain.clear(); extra_event = Some(LifeCycle::DisabledChanged(self.state.is_disabled())); } else if self.state.children_disabled_changed { - self.state.reset_focus_chain(); + self.state.focus_chain.clear(); extra_event = Some(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); } - 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); - } - // It is easier to always use the extra event since the widget can disable itself // while its parent was enabled and vice versa. false @@ -1077,37 +1062,47 @@ impl> WidgetPod { self.inner.lifecycle(&mut child_ctx, event, data, env); } + // This must happen before merge_up so that the changes to is_explicitly_disabled_new and + // focus_chain get merged up! if let LifeCycle::DisabledChanged(_) | LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged) = event { self.state.children_disabled_changed = false; + if !self.state.is_disabled() { + // Insert our own index at the first position. + // We do this at the end to be consistent with the behaviour in WidgetAdded. + if self.state.auto_focus { + self.state.focus_chain.insert(0, self.state.id); + } ctx.widget_state.focus_chain.extend(&self.state.focus_chain); } - //Delete changes of disabled state that happened during DisabledChanged + 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; - } else if self.state.is_explicitly_disabled != self.state.is_explicitly_disabled_new { - ctx.widget_state.children_disabled_changed = true; } ctx.widget_state.merge_up(&mut self.state); // we need to (re)register children in case of one of the following events - if let LifeCycle::WidgetAdded = event { - // If this widget is not already present, autofocus is not set before the event, - // since we call register_for_focus in widget added. - // To be consistent with RouteWidgetAdded DisabledChanged and Route DisabledChange we - // insert our id at the first position. - if self.state.auto_focus { - self.state.focus_chain.insert(0, self.state.id); - } - } - match event { LifeCycle::WidgetAdded | LifeCycle::Internal(InternalLifeCycle::RouteWidgetAdded) => { self.state.children_changed = false; ctx.widget_state.children = ctx.widget_state.children.union(self.state.children); + + // Insert our own index at the first position. + // We do this at the end since in case of WidgetAdded auto_focus is not set before + // the call. + if self.state.auto_focus { + self.state.focus_chain.insert(0, self.state.id); + } ctx.widget_state.focus_chain.extend(&self.state.focus_chain); ctx.register_child(self.id()); } @@ -1177,9 +1172,6 @@ impl> WidgetPod { self.state.request_update = false; ctx.widget_state.merge_up(&mut self.state); - - ctx.widget_state.children_disabled_changed |= - self.state.is_explicitly_disabled_new != self.state.is_explicitly_disabled; } } @@ -1300,6 +1292,8 @@ impl WidgetState { self.timers.extend_drain(&mut child_state.timers); self.text_registrations .extend(child_state.text_registrations.drain(..)); + ctx.widget_state.children_disabled_changed |= + self.state.is_explicitly_disabled_new != self.state.is_explicitly_disabled; // 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`. From 9754051f5e37953322bdf01170195267b6183804 Mon Sep 17 00:00:00 2001 From: xarvic Date: Wed, 31 Mar 2021 14:22:25 +0200 Subject: [PATCH 18/35] refactor DisabledChanged --- druid/src/core.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/druid/src/core.rs b/druid/src/core.rs index 8e767031d6..4995e712e3 100644 --- a/druid/src/core.rs +++ b/druid/src/core.rs @@ -1246,13 +1246,6 @@ impl WidgetState { self.timers.insert(timer_token, self.id); } - pub(crate) fn reset_focus_chain(&mut self) { - self.focus_chain.clear(); - if self.auto_focus { - self.focus_chain.push(self.id); - } - } - /// Update to incorporate state changes from a child. /// /// This will also clear some requests in the child state. From 74b86b23310599dcedd2eca2ede94ba0917d2aab Mon Sep 17 00:00:00 2001 From: xarvic Date: Wed, 31 Mar 2021 15:32:16 +0200 Subject: [PATCH 19/35] fixed error, revered change of focus_chain --- druid/src/core.rs | 46 +++++++++++++++++++++++++++------------------- 1 file changed, 27 insertions(+), 19 deletions(-) diff --git a/druid/src/core.rs b/druid/src/core.rs index 4995e712e3..b5227af1f2 100644 --- a/druid/src/core.rs +++ b/druid/src/core.rs @@ -928,7 +928,7 @@ impl> WidgetPod { } else { if self.state.children_changed { self.state.children.clear(); - self.state.focus_chain.clear(); + self.state.reset_focus_chain(); } self.state.children_changed } @@ -985,12 +985,12 @@ impl> WidgetPod { if was_disabled != self.state.is_disabled() { // In case we change but none of our children we still need to update the // focus-chain - self.state.focus_chain.clear(); + self.state.reset_focus_chain(); extra_event = Some(LifeCycle::DisabledChanged(self.state.is_disabled())); //Each widget needs only one of DisabledChanged and RouteDisabledChanged false } else if self.state.children_disabled_changed { - self.state.focus_chain.clear(); + self.state.reset_focus_chain(); true } else { false @@ -1028,10 +1028,10 @@ impl> WidgetPod { if was_disabled != self.state.is_disabled() { // In case we change but none of our children does we still need to update the // focus-chain - self.state.focus_chain.clear(); + self.state.reset_focus_chain(); extra_event = Some(LifeCycle::DisabledChanged(self.state.is_disabled())); } else if self.state.children_disabled_changed { - self.state.focus_chain.clear(); + self.state.reset_focus_chain(); extra_event = Some(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); } @@ -1070,11 +1070,6 @@ impl> WidgetPod { self.state.children_disabled_changed = false; if !self.state.is_disabled() { - // Insert our own index at the first position. - // We do this at the end to be consistent with the behaviour in WidgetAdded. - if self.state.auto_focus { - self.state.focus_chain.insert(0, self.state.id); - } ctx.widget_state.focus_chain.extend(&self.state.focus_chain); } @@ -1091,18 +1086,22 @@ impl> WidgetPod { ctx.widget_state.merge_up(&mut self.state); + // If this widget is not already present, autofocus is not set before the event, + // since we call register_for_focus in WidgetAdded. + // To be consistent with RouteWidgetAdded DisabledChanged and Route DisabledChange we + // insert our id at the first position. + // This must be called before updating our parents focus_chain! + if let LifeCycle::WidgetAdded = event { + if self.state.auto_focus { + self.state.focus_chain.insert(0, self.state.id); + } + } + // we need to (re)register children in case of one of the following events match event { LifeCycle::WidgetAdded | LifeCycle::Internal(InternalLifeCycle::RouteWidgetAdded) => { self.state.children_changed = false; ctx.widget_state.children = ctx.widget_state.children.union(self.state.children); - - // Insert our own index at the first position. - // We do this at the end since in case of WidgetAdded auto_focus is not set before - // the call. - if self.state.auto_focus { - self.state.focus_chain.insert(0, self.state.id); - } ctx.widget_state.focus_chain.extend(&self.state.focus_chain); ctx.register_child(self.id()); } @@ -1246,6 +1245,15 @@ impl WidgetState { self.timers.insert(timer_token, self.id); } + /// removes all elements from the focus chain and adds the id of this widget + /// auto_focus is set. + pub(crate) fn reset_focus_chain(&mut self) { + self.focus_chain.clear(); + if self.auto_focus { + self.focus_chain.push(self.id); + } + } + /// Update to incorporate state changes from a child. /// /// This will also clear some requests in the child state. @@ -1285,8 +1293,8 @@ impl WidgetState { self.timers.extend_drain(&mut child_state.timers); self.text_registrations .extend(child_state.text_registrations.drain(..)); - ctx.widget_state.children_disabled_changed |= - self.state.is_explicitly_disabled_new != self.state.is_explicitly_disabled; + self.children_disabled_changed |= + child_state.is_explicitly_disabled_new != child_state.is_explicitly_disabled; // 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`. From c181f51dc36d1f55fa311239401d741efa1fba01 Mon Sep 17 00:00:00 2001 From: xarvic Date: Wed, 31 Mar 2021 15:38:07 +0200 Subject: [PATCH 20/35] refactored tests --- druid/src/tests/mod.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/druid/src/tests/mod.rs b/druid/src/tests/mod.rs index 967b51ecc9..14a3061161 100644 --- a/druid/src/tests/mod.rs +++ b/druid/src/tests/mod.rs @@ -389,7 +389,7 @@ fn simple_disable() { || desired[2] != disabled_2.get() || desired[3] != disabled_3.get() { - panic!(format!( + eprintln!( "test \"{}\":\nexpected: {:?}\n got: {:?}", name, desired, @@ -399,7 +399,8 @@ fn simple_disable() { disabled_2.get(), disabled_3.get() ] - )) + ); + panic!(); } }; @@ -572,7 +573,7 @@ fn disable_tree() { || desired[4] != disabled_4.get() || desired[5] != disabled_5.get() { - panic!(format!( + eprintln!( "test \"{}\":\nexpected: {:?}\n got: {:?}", name, desired, @@ -584,7 +585,8 @@ fn disable_tree() { disabled_4.get(), disabled_5.get() ] - )); + ); + panic!(); } }; From cfc544eba883eb85302030fcf3ce9c037e1fb102 Mon Sep 17 00:00:00 2001 From: xarvic Date: Wed, 31 Mar 2021 16:25:43 +0200 Subject: [PATCH 21/35] reordered lifecycle events --- druid/src/core.rs | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/druid/src/core.rs b/druid/src/core.rs index b5227af1f2..f45e5a7492 100644 --- a/druid/src/core.rs +++ b/druid/src/core.rs @@ -933,6 +933,25 @@ impl> WidgetPod { self.state.children_changed } } + InternalLifeCycle::RouteDisabledChanged => { + 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() { + // In case we change but none of our children we still need to update the + // focus-chain + self.state.reset_focus_chain(); + extra_event = Some(LifeCycle::DisabledChanged(self.state.is_disabled())); + //Each widget needs only one of DisabledChanged and RouteDisabledChanged + false + } else if self.state.children_disabled_changed { + self.state.reset_focus_chain(); + true + } else { + false + } + } InternalLifeCycle::RouteFocusChanged { old, new } => { let this_changed = if *old == Some(self.state.id) { Some(false) @@ -977,25 +996,6 @@ impl> WidgetPod { f.call(&self.state); true } - InternalLifeCycle::RouteDisabledChanged => { - 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() { - // In case we change but none of our children we still need to update the - // focus-chain - self.state.reset_focus_chain(); - extra_event = Some(LifeCycle::DisabledChanged(self.state.is_disabled())); - //Each widget needs only one of DisabledChanged and RouteDisabledChanged - false - } else if self.state.children_disabled_changed { - self.state.reset_focus_chain(); - true - } else { - false - } - } }, LifeCycle::WidgetAdded => { assert!(self.old_data.is_none()); From 05d45fc25fc0814f57a8c9973db350e9e90ee6f5 Mon Sep 17 00:00:00 2001 From: xarvic Date: Wed, 31 Mar 2021 18:02:46 +0200 Subject: [PATCH 22/35] reverted changes to the focus_chain --- druid/src/contexts.rs | 2 +- druid/src/core.rs | 39 ++++++++------------------------------- 2 files changed, 9 insertions(+), 32 deletions(-) diff --git a/druid/src/contexts.rs b/druid/src/contexts.rs index 0646ba210b..1957b4a263 100644 --- a/druid/src/contexts.rs +++ b/druid/src/contexts.rs @@ -745,7 +745,7 @@ impl LifeCycleCtx<'_, '_> { /// [`EventCtx::is_focused`]: struct.EventCtx.html#method.is_focused pub fn register_for_focus(&mut self) { trace!("register_for_focus"); - self.widget_state.auto_focus = true; + self.widget_state.focus_chain.push(self.widget_id()); } /// Register this widget as accepting text input. diff --git a/druid/src/core.rs b/druid/src/core.rs index f45e5a7492..e6aa59efe3 100644 --- a/druid/src/core.rs +++ b/druid/src/core.rs @@ -121,11 +121,6 @@ pub(crate) struct WidgetState { // LifeCycle::DisabledChanged or InternalLifeCycle::RouteDisabledChanged pub(crate) is_explicitly_disabled_new: bool, - // `true` if this widget participates in the focus chain. - // We store this as a bool because we need to know how to update the focus chain - // when disabled/enabled state changes. - pub(crate) auto_focus: bool, - pub(crate) is_hot: bool, pub(crate) is_active: bool, @@ -928,7 +923,7 @@ impl> WidgetPod { } else { if self.state.children_changed { self.state.children.clear(); - self.state.reset_focus_chain(); + self.state.focus_chain.clear(); } self.state.children_changed } @@ -941,12 +936,12 @@ impl> WidgetPod { if was_disabled != self.state.is_disabled() { // In case we change but none of our children we still need to update the // focus-chain - self.state.reset_focus_chain(); + self.state.focus_chain.clear(); extra_event = Some(LifeCycle::DisabledChanged(self.state.is_disabled())); //Each widget needs only one of DisabledChanged and RouteDisabledChanged false } else if self.state.children_disabled_changed { - self.state.reset_focus_chain(); + self.state.focus_chain.clear(); true } else { false @@ -1028,10 +1023,13 @@ impl> WidgetPod { if was_disabled != self.state.is_disabled() { // In case we change but none of our children does we still need to update the // focus-chain - self.state.reset_focus_chain(); + self.state.focus_chain.clear(); extra_event = Some(LifeCycle::DisabledChanged(self.state.is_disabled())); } else if self.state.children_disabled_changed { - self.state.reset_focus_chain(); + // If we are disabled and our Parent changed its state from Enabled to Disabled, + // this doesn't changes our state, but if one of our children changed its + // disabled state we still need to route RouteDisabledChanged. + self.state.focus_chain.clear(); extra_event = Some(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); } @@ -1086,17 +1084,6 @@ impl> WidgetPod { ctx.widget_state.merge_up(&mut self.state); - // If this widget is not already present, autofocus is not set before the event, - // since we call register_for_focus in WidgetAdded. - // To be consistent with RouteWidgetAdded DisabledChanged and Route DisabledChange we - // insert our id at the first position. - // This must be called before updating our parents focus_chain! - if let LifeCycle::WidgetAdded = event { - if self.state.auto_focus { - self.state.focus_chain.insert(0, self.state.id); - } - } - // we need to (re)register children in case of one of the following events match event { LifeCycle::WidgetAdded | LifeCycle::Internal(InternalLifeCycle::RouteWidgetAdded) => { @@ -1227,7 +1214,6 @@ impl WidgetState { cursor: None, sub_window_hosts: Vec::new(), is_explicitly_disabled_new: false, - auto_focus: false, text_registrations: Vec::new(), } } @@ -1245,15 +1231,6 @@ impl WidgetState { self.timers.insert(timer_token, self.id); } - /// removes all elements from the focus chain and adds the id of this widget - /// auto_focus is set. - pub(crate) fn reset_focus_chain(&mut self) { - self.focus_chain.clear(); - if self.auto_focus { - self.focus_chain.push(self.id); - } - } - /// Update to incorporate state changes from a child. /// /// This will also clear some requests in the child state. From b9b9acaed2407ce097bd7ad957845ff40f74d00b Mon Sep 17 00:00:00 2001 From: xarvic Date: Mon, 5 Apr 2021 16:02:45 +0200 Subject: [PATCH 23/35] implemented new focus-chain using LifeCycle::BuildFocusChain --- druid/src/contexts.rs | 1 + druid/src/core.rs | 93 +++++++++++++++++++------------------ druid/src/event.rs | 13 +++++- druid/src/tests/mod.rs | 48 ++++++++++++------- druid/src/widget/textbox.rs | 4 +- druid/src/window.rs | 9 +++- 6 files changed, 103 insertions(+), 65 deletions(-) diff --git a/druid/src/contexts.rs b/druid/src/contexts.rs index 1957b4a263..98f2cb4a19 100644 --- a/druid/src/contexts.rs +++ b/druid/src/contexts.rs @@ -391,6 +391,7 @@ 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(); } diff --git a/druid/src/core.rs b/druid/src/core.rs index e6aa59efe3..6c9cd02c00 100644 --- a/druid/src/core.rs +++ b/druid/src/core.rs @@ -140,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, pub(crate) request_focus: Option, pub(crate) children: Bloom, @@ -923,7 +925,6 @@ impl> WidgetPod { } else { if self.state.children_changed { self.state.children.clear(); - self.state.focus_chain.clear(); } self.state.children_changed } @@ -936,12 +937,12 @@ impl> WidgetPod { if was_disabled != self.state.is_disabled() { // In case we change but none of our children we still need to update the // focus-chain - self.state.focus_chain.clear(); + self.state.update_focus_chain = true; extra_event = Some(LifeCycle::DisabledChanged(self.state.is_disabled())); //Each widget needs only one of DisabledChanged and RouteDisabledChanged false } else if self.state.children_disabled_changed { - self.state.focus_chain.clear(); + self.state.update_focus_chain = true; true } else { false @@ -996,6 +997,8 @@ impl> WidgetPod { 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()); @@ -1020,23 +1023,15 @@ impl> WidgetPod { 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. if was_disabled != self.state.is_disabled() { - // In case we change but none of our children does we still need to update the - // focus-chain - self.state.focus_chain.clear(); - extra_event = Some(LifeCycle::DisabledChanged(self.state.is_disabled())); - } else if self.state.children_disabled_changed { - // If we are disabled and our Parent changed its state from Enabled to Disabled, - // this doesn't changes our state, but if one of our children changed its - // disabled state we still need to route RouteDisabledChanged. self.state.focus_chain.clear(); - extra_event = - Some(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged)); + true + } else { + false } - - // It is easier to always use the extra event since the widget can disable itself - // while its parent was enabled and vice versa. - false } //NOTE: this is not sent here, but from the special set_hot_state method LifeCycle::HotChanged(_) => false, @@ -1045,6 +1040,14 @@ impl> WidgetPod { // 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 { @@ -1060,40 +1063,40 @@ impl> WidgetPod { self.inner.lifecycle(&mut child_ctx, event, data, env); } - // This must happen before merge_up so that the changes to is_explicitly_disabled_new and - // focus_chain get merged up! - if let LifeCycle::DisabledChanged(_) - | LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged) = event - { - self.state.children_disabled_changed = false; - - if !self.state.is_disabled() { - ctx.widget_state.focus_chain.extend(&self.state.focus_chain); - } + // Sync our state with our parent's state after the event! - 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; - } - - ctx.widget_state.merge_up(&mut self.state); - - // 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. @@ -1215,6 +1218,7 @@ impl WidgetState { sub_window_hosts: Vec::new(), is_explicitly_disabled_new: false, text_registrations: Vec::new(), + update_focus_chain: true, } } @@ -1262,6 +1266,8 @@ 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; @@ -1270,8 +1276,7 @@ impl WidgetState { self.timers.extend_drain(&mut child_state.timers); self.text_registrations .extend(child_state.text_registrations.drain(..)); - self.children_disabled_changed |= - child_state.is_explicitly_disabled_new != child_state.is_explicitly_disabled; + 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`. diff --git a/druid/src/event.rs b/druid/src/event.rs index 84b03be0ba..4eaac74fad 100644 --- a/druid/src/event.rs +++ b/druid/src/event.rs @@ -281,6 +281,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`](struct.LifecycleCtx.html#method.register_for_focus) + /// [`focus_next`](struct.LifecycleCtx.html#method.register_for_focus) + /// [`focus_prev`](struct.LifecycleCtx.html#method.register_for_focus) + BuildFocusChain, /// Called when the focus status changes. /// /// This will always be called immediately after a new widget gains focus. @@ -416,7 +426,8 @@ impl LifeCycle { pub fn should_propagate_to_hidden(&self) -> bool { match self { LifeCycle::Internal(internal) => internal.should_propagate_to_hidden(), - LifeCycle::WidgetAdded | LifeCycle::DisabledChanged(_) => true, + LifeCycle::WidgetAdded | LifeCycle::DisabledChanged(_) | LifeCycle::BuildFocusChain + => true, LifeCycle::Size(_) | LifeCycle::HotChanged(_) | LifeCycle::FocusChanged(_) => false, } } diff --git a/druid/src/tests/mod.rs b/druid/src/tests/mod.rs index 14a3061161..041b907edf 100644 --- a/druid/src/tests/mod.rs +++ b/druid/src/tests/mod.rs @@ -355,25 +355,32 @@ fn focus_changed() { fn simple_disable() { const CHANGE_DISABLED: Selector = Selector::new("druid-tests.change-disabled"); - let test_widget_factory = |auto_focus: bool, id: WidgetId, state: Rc>>| { - ModularWidget::new(state) - .lifecycle_fn(move |state, ctx, event, _, _| match event { - LifeCycle::WidgetAdded => { + let test_widget_factory = |auto_focus: bool, id: WidgetId, id_inner: WidgetId, state: Rc>>| { + let inner = WidgetPod::new( + ModularWidget::new(()) + .lifecycle_fn(move |_, ctx, event, _, _|{ + if let LifeCycle::WidgetAdded = event { if auto_focus { ctx.register_for_focus(); } } - LifeCycle::DisabledChanged(disabled) => { - state.set(Some(*disabled)); + }) + .with_id(id_inner) + ); + ModularWidget::new((state, inner)) + .lifecycle_fn(move |state, ctx, event, data, env| { + if let LifeCycle::DisabledChanged(disabled) = event { + state.0.set(Some(*disabled)); } - _ => {} + state.1.lifecycle(ctx, event, data, env); }) - .event_fn(|_, ctx, event, _, _| { + .event_fn(|state, ctx, event, data, env| { if let Event::Command(cmd) = event { if let Some(disabled) = cmd.get(CHANGE_DISABLED) { ctx.set_disabled(*disabled); } } + state.1.event(ctx, event, data, env); }) .with_id(id) }; @@ -409,33 +416,38 @@ fn simple_disable() { let id_2 = WidgetId::next(); let id_3 = WidgetId::next(); + let id_0_o = WidgetId::next(); + let id_1_o = WidgetId::next(); + let id_2_o = WidgetId::next(); + let id_3_o = WidgetId::next(); + let root = Flex::row() - .with_child(test_widget_factory(true, id_0, disabled_0.clone())) - .with_child(test_widget_factory(true, id_1, disabled_1.clone())) - .with_child(test_widget_factory(true, id_2, disabled_2.clone())) - .with_child(test_widget_factory(true, id_3, disabled_3.clone())); + .with_child(test_widget_factory(true, id_0_o, id_0, disabled_0.clone())) + .with_child(test_widget_factory(true, id_1_o, id_1, disabled_1.clone())) + .with_child(test_widget_factory(true, id_2_o, id_2, disabled_2.clone())) + .with_child(test_widget_factory(true, id_3_o, id_3, disabled_3.clone())); Harness::create_simple((), root, |harness| { harness.send_initial_events(); check_states("send_initial_events", [None, None, None, None]); assert_eq!(harness.window().focus_chain(), &[id_0, id_1, id_2, id_3]); - harness.submit_command(Command::new(CHANGE_DISABLED, true, id_0)); + harness.submit_command(Command::new(CHANGE_DISABLED, true, id_0_o)); check_states("Change 1", [Some(true), None, None, None]); assert_eq!(harness.window().focus_chain(), &[id_1, id_2, id_3]); - harness.submit_command(Command::new(CHANGE_DISABLED, true, id_2)); + harness.submit_command(Command::new(CHANGE_DISABLED, true, id_2_o)); check_states("Change 2", [Some(true), None, Some(true), None]); assert_eq!(harness.window().focus_chain(), &[id_1, id_3]); - harness.submit_command(Command::new(CHANGE_DISABLED, true, id_3)); + harness.submit_command(Command::new(CHANGE_DISABLED, true, id_3_o)); check_states("Change 3", [Some(true), None, Some(true), Some(true)]); assert_eq!(harness.window().focus_chain(), &[id_1]); - harness.submit_command(Command::new(CHANGE_DISABLED, false, id_2)); + harness.submit_command(Command::new(CHANGE_DISABLED, false, id_2_o)); check_states("Change 4", [Some(true), None, Some(false), Some(true)]); assert_eq!(harness.window().focus_chain(), &[id_1, id_2]); - harness.submit_command(Command::new(CHANGE_DISABLED, true, id_2)); + harness.submit_command(Command::new(CHANGE_DISABLED, true, id_2_o)); check_states("Change 5", [Some(true), None, Some(true), Some(true)]); assert_eq!(harness.window().focus_chain(), &[id_1]); //This is intended the widget should not receive an event! - harness.submit_command(Command::new(CHANGE_DISABLED, false, id_1)); + harness.submit_command(Command::new(CHANGE_DISABLED, false, id_1_o)); check_states("Change 6", [Some(true), None, Some(true), Some(true)]); assert_eq!(harness.window().focus_chain(), &[id_1]); }) diff --git a/druid/src/widget/textbox.rs b/druid/src/widget/textbox.rs index 9bd43f27bf..f7ddeb96de 100644 --- a/druid/src/widget/textbox.rs +++ b/druid/src/widget/textbox.rs @@ -419,9 +419,11 @@ impl Widget for TextBox { fn lifecycle(&mut self, ctx: &mut LifeCycleCtx, event: &LifeCycle, data: &T, env: &Env) { match event { LifeCycle::WidgetAdded => { + ctx.register_text_input(self.text().input_handler()); + } + LifeCycle::BuildFocusChain => { //TODO: make this a configurable option? maybe? ctx.register_for_focus(); - ctx.register_text_input(self.text().input_handler()); } LifeCycle::FocusChanged(true) => { if self.text().can_write() && !self.multiline && !self.was_focused_from_click { diff --git a/druid/src/window.rs b/druid/src/window.rs index ff2e4c4237..9e07303438 100644 --- a/druid/src/window.rs +++ b/druid/src/window.rs @@ -184,12 +184,19 @@ impl Window { } // Update the disabled state if necessary - // Always do this before sending focus change, since this event updates the focus chain. + // Always do this before updating the focus-chain if self.root.state().tree_disabled_changed() { let event = LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged); self.lifecycle(queue, &event, data, env, false); } + // Update the focus-chain if necessary + // Always do this before sending focus change, since this event updates the focus chain. + if self.root.state().update_focus_chain { + let event = LifeCycle::BuildFocusChain; + self.lifecycle(queue, &event, data, env, false); + } + if let Some(focus_req) = widget_state.request_focus.take() { let old = self.focus; let new = self.widget_for_focus_request(focus_req); From f091bf9ae17060ff901454574d3930b63f3e45a6 Mon Sep 17 00:00:00 2001 From: xarvic Date: Mon, 5 Apr 2021 16:32:16 +0200 Subject: [PATCH 24/35] update tests --- druid/src/tests/mod.rs | 60 ++++++++++++++++++------------------------ 1 file changed, 25 insertions(+), 35 deletions(-) diff --git a/druid/src/tests/mod.rs b/druid/src/tests/mod.rs index 041b907edf..8f0f8409bd 100644 --- a/druid/src/tests/mod.rs +++ b/druid/src/tests/mod.rs @@ -355,32 +355,27 @@ fn focus_changed() { fn simple_disable() { const CHANGE_DISABLED: Selector = Selector::new("druid-tests.change-disabled"); - let test_widget_factory = |auto_focus: bool, id: WidgetId, id_inner: WidgetId, state: Rc>>| { - let inner = WidgetPod::new( - ModularWidget::new(()) - .lifecycle_fn(move |_, ctx, event, _, _|{ - if let LifeCycle::WidgetAdded = event { - if auto_focus { - ctx.register_for_focus(); + let test_widget_factory = |auto_focus: bool, id: WidgetId, state: Rc>>| { + ModularWidget::new(state) + .lifecycle_fn(move |state, ctx, event, _, _| { + match event { + LifeCycle::BuildFocusChain => { + if auto_focus { + ctx.register_for_focus(); + } } + LifeCycle::DisabledChanged(disabled) => { + state.set(Some(*disabled)); + } + _ => {} } }) - .with_id(id_inner) - ); - ModularWidget::new((state, inner)) - .lifecycle_fn(move |state, ctx, event, data, env| { - if let LifeCycle::DisabledChanged(disabled) = event { - state.0.set(Some(*disabled)); - } - state.1.lifecycle(ctx, event, data, env); - }) - .event_fn(|state, ctx, event, data, env| { + .event_fn(|_, ctx, event, _, _| { if let Event::Command(cmd) = event { if let Some(disabled) = cmd.get(CHANGE_DISABLED) { ctx.set_disabled(*disabled); } } - state.1.event(ctx, event, data, env); }) .with_id(id) }; @@ -416,38 +411,33 @@ fn simple_disable() { let id_2 = WidgetId::next(); let id_3 = WidgetId::next(); - let id_0_o = WidgetId::next(); - let id_1_o = WidgetId::next(); - let id_2_o = WidgetId::next(); - let id_3_o = WidgetId::next(); - let root = Flex::row() - .with_child(test_widget_factory(true, id_0_o, id_0, disabled_0.clone())) - .with_child(test_widget_factory(true, id_1_o, id_1, disabled_1.clone())) - .with_child(test_widget_factory(true, id_2_o, id_2, disabled_2.clone())) - .with_child(test_widget_factory(true, id_3_o, id_3, disabled_3.clone())); + .with_child(test_widget_factory(true, id_0, disabled_0.clone())) + .with_child(test_widget_factory(true, id_1, disabled_1.clone())) + .with_child(test_widget_factory(true, id_2, disabled_2.clone())) + .with_child(test_widget_factory(true, id_3, disabled_3.clone())); Harness::create_simple((), root, |harness| { harness.send_initial_events(); check_states("send_initial_events", [None, None, None, None]); assert_eq!(harness.window().focus_chain(), &[id_0, id_1, id_2, id_3]); - harness.submit_command(Command::new(CHANGE_DISABLED, true, id_0_o)); + harness.submit_command(Command::new(CHANGE_DISABLED, true, id_0)); check_states("Change 1", [Some(true), None, None, None]); assert_eq!(harness.window().focus_chain(), &[id_1, id_2, id_3]); - harness.submit_command(Command::new(CHANGE_DISABLED, true, id_2_o)); + harness.submit_command(Command::new(CHANGE_DISABLED, true, id_2)); check_states("Change 2", [Some(true), None, Some(true), None]); assert_eq!(harness.window().focus_chain(), &[id_1, id_3]); - harness.submit_command(Command::new(CHANGE_DISABLED, true, id_3_o)); + harness.submit_command(Command::new(CHANGE_DISABLED, true, id_3)); check_states("Change 3", [Some(true), None, Some(true), Some(true)]); assert_eq!(harness.window().focus_chain(), &[id_1]); - harness.submit_command(Command::new(CHANGE_DISABLED, false, id_2_o)); + harness.submit_command(Command::new(CHANGE_DISABLED, false, id_2)); check_states("Change 4", [Some(true), None, Some(false), Some(true)]); assert_eq!(harness.window().focus_chain(), &[id_1, id_2]); - harness.submit_command(Command::new(CHANGE_DISABLED, true, id_2_o)); + harness.submit_command(Command::new(CHANGE_DISABLED, true, id_2)); check_states("Change 5", [Some(true), None, Some(true), Some(true)]); assert_eq!(harness.window().focus_chain(), &[id_1]); //This is intended the widget should not receive an event! - harness.submit_command(Command::new(CHANGE_DISABLED, false, id_1_o)); + harness.submit_command(Command::new(CHANGE_DISABLED, false, id_1)); check_states("Change 6", [Some(true), None, Some(true), Some(true)]); assert_eq!(harness.window().focus_chain(), &[id_1]); }) @@ -462,7 +452,7 @@ fn resign_focus_on_disable() { |auto_focus: bool, id: WidgetId, inner: Option>>| { ModularWidget::new(inner.map(WidgetPod::new)) .lifecycle_fn(move |state, ctx, event, data, env| { - if let LifeCycle::WidgetAdded = event { + if let LifeCycle::BuildFocusChain = event { if auto_focus { ctx.register_for_focus(); } @@ -536,7 +526,7 @@ fn disable_tree() { let leaf_factory = |state: Rc>>| { ModularWidget::new(state).lifecycle_fn(move |state, ctx, event, _, _| match event { - LifeCycle::WidgetAdded => { + LifeCycle::BuildFocusChain => { ctx.register_for_focus(); } LifeCycle::DisabledChanged(disabled) => { From 4766ceb5583b41c1c3d625feb2ed2c54bd5a3769 Mon Sep 17 00:00:00 2001 From: xarvic Date: Mon, 5 Apr 2021 16:55:33 +0200 Subject: [PATCH 25/35] fixed problems --- druid/src/core.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/druid/src/core.rs b/druid/src/core.rs index 6c9cd02c00..344edf20e0 100644 --- a/druid/src/core.rs +++ b/druid/src/core.rs @@ -1027,7 +1027,7 @@ impl> WidgetPod { // is always the same, or we dont change at all, because we stay disabled if either // we or our parent are disabled. if was_disabled != self.state.is_disabled() { - self.state.focus_chain.clear(); + self.state.update_focus_chain = true; true } else { false @@ -1276,7 +1276,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; + 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`. From a901d1c2f6ccb07a7cfb7705bcb3959246a65e34 Mon Sep 17 00:00:00 2001 From: xarvic Date: Mon, 5 Apr 2021 17:02:31 +0200 Subject: [PATCH 26/35] updated texts --- druid/src/tests/mod.rs | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/druid/src/tests/mod.rs b/druid/src/tests/mod.rs index 8f0f8409bd..2b4b9cac0a 100644 --- a/druid/src/tests/mod.rs +++ b/druid/src/tests/mod.rs @@ -357,18 +357,16 @@ fn simple_disable() { let test_widget_factory = |auto_focus: bool, id: WidgetId, state: Rc>>| { ModularWidget::new(state) - .lifecycle_fn(move |state, ctx, event, _, _| { - match event { - LifeCycle::BuildFocusChain => { - if auto_focus { - ctx.register_for_focus(); - } + .lifecycle_fn(move |state, ctx, event, _, _| match event { + LifeCycle::BuildFocusChain => { + if auto_focus { + ctx.register_for_focus(); } - LifeCycle::DisabledChanged(disabled) => { - state.set(Some(*disabled)); - } - _ => {} } + LifeCycle::DisabledChanged(disabled) => { + state.set(Some(*disabled)); + } + _ => {} }) .event_fn(|_, ctx, event, _, _| { if let Event::Command(cmd) = event { @@ -746,6 +744,10 @@ fn simple_lifecyle() { Harness::create_simple(true, widget, |harness| { harness.send_initial_events(); assert!(matches!(record.next(), Record::L(LifeCycle::WidgetAdded))); + assert!(matches!( + record.next(), + Record::L(LifeCycle::BuildFocusChain) + )); assert!(matches!(record.next(), Record::E(Event::WindowConnected))); assert!(matches!(record.next(), Record::E(Event::WindowSize(_)))); assert!(record.is_empty()); @@ -770,6 +772,10 @@ fn adding_child_lifecycle() { harness.send_initial_events(); assert!(matches!(record.next(), Record::L(LifeCycle::WidgetAdded))); + assert!(matches!( + record.next(), + Record::L(LifeCycle::BuildFocusChain) + )); assert!(matches!(record.next(), Record::E(Event::WindowConnected))); assert!(record.is_empty()); @@ -783,6 +789,10 @@ fn adding_child_lifecycle() { record_new_child.next(), Record::L(LifeCycle::WidgetAdded) )); + assert!(matches!( + record_new_child.next(), + Record::L(LifeCycle::BuildFocusChain) + )); assert!(record_new_child.is_empty()); }) } From eb3cfd7ee3d594b784cb35c1a28d1763d63e7342 Mon Sep 17 00:00:00 2001 From: xarvic Date: Mon, 5 Apr 2021 17:02:43 +0200 Subject: [PATCH 27/35] clippy fix --- druid/src/event.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/druid/src/event.rs b/druid/src/event.rs index 4eaac74fad..43acd5149d 100644 --- a/druid/src/event.rs +++ b/druid/src/event.rs @@ -426,8 +426,9 @@ impl LifeCycle { pub fn should_propagate_to_hidden(&self) -> bool { match self { LifeCycle::Internal(internal) => internal.should_propagate_to_hidden(), - LifeCycle::WidgetAdded | LifeCycle::DisabledChanged(_) | LifeCycle::BuildFocusChain - => true, + LifeCycle::WidgetAdded | LifeCycle::DisabledChanged(_) | LifeCycle::BuildFocusChain => { + true + } LifeCycle::Size(_) | LifeCycle::HotChanged(_) | LifeCycle::FocusChanged(_) => false, } } From fdd510acfc33e3d961608bd35d1f7b46fb9d0180 Mon Sep 17 00:00:00 2001 From: xarvic Date: Mon, 5 Apr 2021 17:32:36 +0200 Subject: [PATCH 28/35] fixed documentation --- druid/src/event.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/druid/src/event.rs b/druid/src/event.rs index 43acd5149d..8b4e03f749 100644 --- a/druid/src/event.rs +++ b/druid/src/event.rs @@ -287,9 +287,9 @@ pub enum LifeCycle { /// 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`](struct.LifecycleCtx.html#method.register_for_focus) - /// [`focus_next`](struct.LifecycleCtx.html#method.register_for_focus) - /// [`focus_prev`](struct.LifecycleCtx.html#method.register_for_focus) + /// [`register_for_focus`]: (struct.LifecycleCtx.html#method.register_for_focus) + /// [`focus_next`]: (struct.LifecycleCtx.html#method.register_for_focus) + /// [`focus_prev`]: (struct.LifecycleCtx.html#method.register_for_focus) BuildFocusChain, /// Called when the focus status changes. /// From 4ef9697622adf64cf7e8bc23bd02a45697f66b19 Mon Sep 17 00:00:00 2001 From: Christoph Date: Mon, 5 Apr 2021 15:35:14 +0000 Subject: [PATCH 29/35] Update druid/src/event.rs Co-authored-by: Colin Rofls --- druid/src/event.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/druid/src/event.rs b/druid/src/event.rs index 8b4e03f749..7578735989 100644 --- a/druid/src/event.rs +++ b/druid/src/event.rs @@ -267,10 +267,12 @@ pub enum LifeCycle { Size(Size), /// Called when the Disabled state of the widgets is changed. /// - /// [`is_disabled`](struct.EventCtx.html#method.is_disabled) returns if the widget is disabled. + /// To check if a widget is disabled, see [`is_disabled`]. /// - /// [`set_disabled`](struct.EventCtx.html#method.set_disabled) to change the widgets disabled - /// state. + /// 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. /// From 4745fe9819a9aa4f3562e6b455cd8f164f9d6f96 Mon Sep 17 00:00:00 2001 From: xarvic Date: Mon, 5 Apr 2021 17:39:22 +0200 Subject: [PATCH 30/35] fixed documentation --- druid/src/event.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/druid/src/event.rs b/druid/src/event.rs index 7578735989..45e9d9d951 100644 --- a/druid/src/event.rs +++ b/druid/src/event.rs @@ -289,9 +289,9 @@ pub enum LifeCycle { /// 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`]: (struct.LifecycleCtx.html#method.register_for_focus) - /// [`focus_next`]: (struct.LifecycleCtx.html#method.register_for_focus) - /// [`focus_prev`]: (struct.LifecycleCtx.html#method.register_for_focus) + /// [`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. /// From 4567f8f67c905e37aaec4d7fc7c29be48f885575 Mon Sep 17 00:00:00 2001 From: xarvic Date: Mon, 5 Apr 2021 17:39:42 +0200 Subject: [PATCH 31/35] made logic simpler --- druid/src/core.rs | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/druid/src/core.rs b/druid/src/core.rs index 344edf20e0..8708e53617 100644 --- a/druid/src/core.rs +++ b/druid/src/core.rs @@ -930,22 +930,18 @@ impl> WidgetPod { } } 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() { - // In case we change but none of our children we still need to update the - // focus-chain - self.state.update_focus_chain = true; extra_event = Some(LifeCycle::DisabledChanged(self.state.is_disabled())); //Each widget needs only one of DisabledChanged and RouteDisabledChanged false - } else if self.state.children_disabled_changed { - self.state.update_focus_chain = true; - true } else { - false + self.state.children_disabled_changed } } InternalLifeCycle::RouteFocusChanged { old, new } => { @@ -1018,6 +1014,8 @@ impl> WidgetPod { 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; @@ -1026,12 +1024,7 @@ impl> WidgetPod { // 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. - if was_disabled != self.state.is_disabled() { - self.state.update_focus_chain = true; - true - } else { - false - } + was_disabled != self.state.is_disabled() } //NOTE: this is not sent here, but from the special set_hot_state method LifeCycle::HotChanged(_) => false, @@ -1218,7 +1211,7 @@ impl WidgetState { sub_window_hosts: Vec::new(), is_explicitly_disabled_new: false, text_registrations: Vec::new(), - update_focus_chain: true, + update_focus_chain: false, } } From ba58aad3915c4a71aab2f71354b94b215e649abf Mon Sep 17 00:00:00 2001 From: xarvic Date: Mon, 5 Apr 2021 18:04:37 +0200 Subject: [PATCH 32/35] refactored post_event_processing --- druid/src/window.rs | 74 ++++++++++++++++++++++++--------------------- 1 file changed, 39 insertions(+), 35 deletions(-) diff --git a/druid/src/window.rs b/druid/src/window.rs index 9e07303438..3d7dd5d228 100644 --- a/druid/src/window.rs +++ b/druid/src/window.rs @@ -197,41 +197,7 @@ impl Window { self.lifecycle(queue, &event, data, env, false); } - if let Some(focus_req) = widget_state.request_focus.take() { - let old = self.focus; - let new = self.widget_for_focus_request(focus_req); - // Only send RouteFocusChanged in case there's actual change - if old != new { - let event = LifeCycle::Internal(InternalLifeCycle::RouteFocusChanged { old, new }); - self.lifecycle(queue, &event, data, env, false); - self.focus = new; - // check if the newly focused widget has an IME session, and - // notify the system if so. - // - // If you're here because a profiler sent you: I guess I should've - // used a hashmap? - let old_was_ime = old - .map(|old| { - self.ime_handlers - .iter() - .any(|(_, sesh)| sesh.widget_id == old) - }) - .unwrap_or(false); - let maybe_active_text_field = self - .ime_handlers - .iter() - .find(|(_, sesh)| Some(sesh.widget_id) == self.focus) - .map(|(token, _)| *token); - // we call this on every focus change; we could call it less but does it matter? - self.ime_focus_change = if maybe_active_text_field.is_some() { - Some(maybe_active_text_field) - } else if old_was_ime { - Some(None) - } else { - None - }; - } - } + self.update_focus(widget_state, queue, data, env); // Add all the requested timers to the window's timers map. self.timers.extend_drain(&mut widget_state.timers); @@ -587,6 +553,44 @@ impl Window { .unwrap() } + fn update_focus(&mut self, widget_state: &mut WidgetState, queue: &mut CommandQueue, data: &T, env: &Env) { + if let Some(focus_req) = widget_state.request_focus.take() { + let old = self.focus; + let new = self.widget_for_focus_request(focus_req); + // Only send RouteFocusChanged in case there's actual change + if old != new { + let event = LifeCycle::Internal(InternalLifeCycle::RouteFocusChanged { old, new }); + self.lifecycle(queue, &event, data, env, false); + self.focus = new; + // check if the newly focused widget has an IME session, and + // notify the system if so. + // + // If you're here because a profiler sent you: I guess I should've + // used a hashmap? + let old_was_ime = old + .map(|old| { + self.ime_handlers + .iter() + .any(|(_, sesh)| sesh.widget_id == old) + }) + .unwrap_or(false); + let maybe_active_text_field = self + .ime_handlers + .iter() + .find(|(_, sesh)| Some(sesh.widget_id) == self.focus) + .map(|(token, _)| *token); + // we call this on every focus change; we could call it less but does it matter? + self.ime_focus_change = if maybe_active_text_field.is_some() { + Some(maybe_active_text_field) + } else if old_was_ime { + Some(None) + } else { + None + }; + } + } + } + /// Create a function that can invalidate the provided widget's text state. /// /// This will be called from outside the main app state in order to avoid From 8d6b78472b3551b1ec0131adc19201c0483a9723 Mon Sep 17 00:00:00 2001 From: xarvic Date: Mon, 5 Apr 2021 18:28:19 +0200 Subject: [PATCH 33/35] updated CHANGELOG.md --- CHANGELOG.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3fbd34c132..22de7d8d86 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,10 @@ You can find its changes [documented below](#070---2021-01-01). - New `TextBox` widget with IME integration ([#1636] by [@cmyr]) - `Notification`s can be submitted while handling other `Notification`s ([#1640] by [@cmyr]) - Added ListIter implementations for OrdMap ([#1641] by [@Lejero]) +- `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 @@ -36,6 +40,7 @@ You can find its changes [documented below](#070---2021-01-01). - Spacers in `Flex` are now implemented by calculating the space in `Flex` instead of creating a widget for it ([#1584] by [@JAicewizard]) - Padding is generic over child widget, impls WidgetWrapper ([#1634] by [@cmyr]) - Menu support was rewritten with support for `Data` ([#1625] by [@jneem]) +- `register_for_focus()` should from now on be called from `LifeCycle::BuildFocusChain` instead of `LifeCycle::WidgetAdded` ([#1632] by [@xarvic]) ### Deprecated @@ -436,6 +441,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 [#599]: https://github.com/linebender/druid/pull/599 [#611]: https://github.com/linebender/druid/pull/611 @@ -642,6 +648,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 From d3e73d12812895f1d2453ae046c2d9cda210587d Mon Sep 17 00:00:00 2001 From: xarvic Date: Mon, 5 Apr 2021 18:31:43 +0200 Subject: [PATCH 34/35] fixed docs --- druid/src/event.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/druid/src/event.rs b/druid/src/event.rs index 45e9d9d951..ef9552eb20 100644 --- a/druid/src/event.rs +++ b/druid/src/event.rs @@ -289,7 +289,7 @@ pub enum LifeCycle { /// 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 + /// [`register_for_focus`]: crate::LifeCycleCtx::register_for_focus /// [`focus_next`]: crate::EventCtx::focus_next /// [`focus_prev`]: crate::EventCtx::focus_prev BuildFocusChain, From a52437c8b8d632e70e6f97e651fbfc809d139a8d Mon Sep 17 00:00:00 2001 From: xarvic Date: Mon, 5 Apr 2021 18:37:41 +0200 Subject: [PATCH 35/35] make clippy happy --- druid/src/window.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/druid/src/window.rs b/druid/src/window.rs index cd1879eb6c..c8ce240881 100644 --- a/druid/src/window.rs +++ b/druid/src/window.rs @@ -552,7 +552,13 @@ impl Window { .unwrap() } - fn update_focus(&mut self, widget_state: &mut WidgetState, queue: &mut CommandQueue, data: &T, env: &Env) { + fn update_focus( + &mut self, + widget_state: &mut WidgetState, + queue: &mut CommandQueue, + data: &T, + env: &Env, + ) { if let Some(focus_req) = widget_state.request_focus.take() { let old = self.focus; let new = self.widget_for_focus_request(focus_req);