From e284990bd78d879782d4ada64e573e4b980f3369 Mon Sep 17 00:00:00 2001 From: Kaur Kuut Date: Wed, 8 Apr 2020 22:22:22 +0300 Subject: [PATCH 1/3] Add documentation, e.g. around bloom usage. --- druid-shell/src/common_util.rs | 2 +- druid/src/bloom.rs | 26 ++++++++++++----------- druid/src/contexts.rs | 38 +++++++++++++++++++--------------- druid/src/core.rs | 34 ++++++++++++++++-------------- druid/src/tests/mod.rs | 26 +++++++++++------------ druid/src/widget/textbox.rs | 10 ++++----- druid/src/widget/widget.rs | 5 +++++ druid/src/window.rs | 6 +++++- 8 files changed, 83 insertions(+), 64 deletions(-) diff --git a/druid-shell/src/common_util.rs b/druid-shell/src/common_util.rs index b72e294f24..5a6cfbc109 100644 --- a/druid-shell/src/common_util.rs +++ b/druid-shell/src/common_util.rs @@ -55,7 +55,7 @@ impl IdleCallback for F { /// /// This can be used safely from multiple threads. /// -/// The counter will overflow if `next()` iscalled 2^64 - 2 times. +/// The counter will overflow if `next()` is called 2^64 - 2 times. /// If this is possible for your application, and reuse would be undesirable, /// use something else. pub struct Counter(AtomicU64); diff --git a/druid/src/bloom.rs b/druid/src/bloom.rs index 578c0852d0..bab3eab65e 100644 --- a/druid/src/bloom.rs +++ b/druid/src/bloom.rs @@ -71,10 +71,12 @@ impl Bloom { self.entry_count += 1; } - /// Return whether an item exists in the filter. + /// Returns `true` if the item may have been added to the filter. /// /// This can return false positives, but never false negatives. - pub fn contains(&self, item: &T) -> bool { + /// Thus `true` means that the item may have been added - or not, + /// while `false` means that the item has definitely not been added. + pub fn may_contain(&self, item: &T) -> bool { let mask = self.make_bit_mask(item); self.bits & mask == mask } @@ -134,11 +136,11 @@ mod tests { let mut bloom = Bloom::default(); for i in 0..100 { bloom.add(&i); - assert!(bloom.contains(&i)); + assert!(bloom.may_contain(&i)); } bloom.clear(); for i in 0..100 { - assert!(!bloom.contains(&i)); + assert!(!bloom.may_contain(&i)); } } @@ -147,18 +149,18 @@ mod tests { let mut bloom1 = Bloom::default(); bloom1.add(&0); bloom1.add(&1); - assert!(!bloom1.contains(&2)); - assert!(!bloom1.contains(&3)); + assert!(!bloom1.may_contain(&2)); + assert!(!bloom1.may_contain(&3)); let mut bloom2 = Bloom::default(); bloom2.add(&2); bloom2.add(&3); - assert!(!bloom2.contains(&0)); - assert!(!bloom2.contains(&1)); + assert!(!bloom2.may_contain(&0)); + assert!(!bloom2.may_contain(&1)); let bloom3 = bloom1.union(bloom2); - assert!(bloom3.contains(&0)); - assert!(bloom3.contains(&1)); - assert!(bloom3.contains(&2)); - assert!(bloom3.contains(&3)); + assert!(bloom3.may_contain(&0)); + assert!(bloom3.may_contain(&1)); + assert!(bloom3.may_contain(&2)); + assert!(bloom3.may_contain(&3)); } } diff --git a/druid/src/contexts.rs b/druid/src/contexts.rs index fb28f07029..7a29d2c148 100644 --- a/druid/src/contexts.rs +++ b/druid/src/contexts.rs @@ -238,7 +238,7 @@ impl<'a> EventCtx<'a> { self.is_handled } - /// The focus status of a widget. + /// The (tree) focus status of a widget. /// /// Focus means that the widget receives keyboard events. /// @@ -251,20 +251,21 @@ impl<'a> EventCtx<'a> { /// hierarchy, all ancestors of that leaf widget are also invoked with /// `FocusChanged(true)`. /// - /// Discussion question: is "is_focused" a better name? + /// Discussion question: is "may_have_focus" a better name? /// /// [`request_focus`]: struct.EventCtx.html#method.request_focus pub fn has_focus(&self) -> bool { + // The bloom filter we're checking can return false positives. let is_child = self .focus_widget - .map(|id| self.base_state.children.contains(&id)) + .map(|id| self.base_state.children.may_contain(&id)) .unwrap_or(false); is_child || self.focus_widget == Some(self.widget_id()) } - /// The (leaf) focus status of a widget. See [`has_focus`]. + /// The (leaf) focus status of a widget. /// - /// [`has_focus`]: struct.EventCtx.html#method.has_focus + /// See [`has_focus`](struct.EventCtx.html#method.has_focus). pub fn is_focused(&self) -> bool { self.focus_widget == Some(self.widget_id()) } @@ -282,7 +283,7 @@ impl<'a> EventCtx<'a> { /// /// This should only be called by a widget that currently has focus. pub fn focus_next(&mut self) { - if self.focus_widget == Some(self.widget_id()) { + if self.is_focused() { self.base_state.request_focus = Some(FocusChange::Next); } else { log::warn!("focus_next can only be called by the currently focused widget"); @@ -293,7 +294,7 @@ impl<'a> EventCtx<'a> { /// /// This should only be called by a widget that currently has focus. pub fn focus_prev(&mut self) { - if self.focus_widget == Some(self.widget_id()) { + if self.is_focused() { self.base_state.request_focus = Some(FocusChange::Previous); } else { log::warn!("focus_prev can only be called by the currently focused widget"); @@ -304,7 +305,7 @@ impl<'a> EventCtx<'a> { /// /// This should only be called by a widget that currently has focus. pub fn resign_focus(&mut self) { - if self.focus_widget == Some(self.widget_id()) { + if self.is_focused() { self.base_state.request_focus = Some(FocusChange::Resign); } else { log::warn!("resign_focus can only be called by the currently focused widget"); @@ -414,6 +415,8 @@ impl<'a> LifeCycleCtx<'a> { } /// Register this widget to be eligile to accept focus automatically. + /// + /// This should only be called in response to a `LifeCycle::WidgetAdded` event. pub fn register_for_focus(&mut self) { self.base_state.focus_chain.push(self.widget_id()); } @@ -557,24 +560,25 @@ impl<'a, 'b: 'a> PaintCtx<'a, 'b> { self.base_state.size() } - /// Query the focus state of the widget. - /// - /// This is true only if this widget has focus. - pub fn is_focused(&self) -> bool { - self.focus_widget == Some(self.widget_id()) - } - - /// The focus status of a widget. + /// The (tree) focus status of a widget. /// /// See [`has_focus`](struct.EventCtx.html#method.has_focus). pub fn has_focus(&self) -> bool { + // The bloom filter we're checking can return false positives. let is_child = self .focus_widget - .map(|id| self.base_state.children.contains(&id)) + .map(|id| self.base_state.children.may_contain(&id)) .unwrap_or(false); is_child || self.focus_widget == Some(self.widget_id()) } + /// The (leaf) focus status of a widget. + /// + /// See [`has_focus`](struct.EventCtx.html#method.has_focus). + pub fn is_focused(&self) -> bool { + self.focus_widget == Some(self.widget_id()) + } + /// Returns the currently visible [`Region`]. /// /// [`Region`]: struct.Region.html diff --git a/druid/src/core.rs b/druid/src/core.rs index cb0bab6b88..2fd88dbb9b 100644 --- a/druid/src/core.rs +++ b/druid/src/core.rs @@ -369,7 +369,7 @@ impl> WidgetPod { let mut child_ctx = EventCtx { cursor: ctx.cursor, command_queue: ctx.command_queue, - window: &ctx.window, + window: ctx.window, window_id: ctx.window_id, base_state: &mut self.state, had_active, @@ -446,7 +446,9 @@ impl> WidgetPod { Target::Window(_) => Event::Command(cmd.clone()), Target::Widget(id) if *id == child_ctx.widget_id() => Event::Command(cmd.clone()), Target::Widget(id) => { - recurse = child_ctx.base_state.children.contains(id); + // Recurse when the target widget could be our descendant. + // The bloom filter we're checking can return false positives. + recurse = child_ctx.base_state.children.may_contain(id); Event::TargetedCommand(*target, cmd.clone()) } Target::Global => panic!("Target::Global should be converted before WidgetPod"), @@ -496,7 +498,6 @@ impl> WidgetPod { self.state.children.clear(); self.state.focus_chain.clear(); } - self.state.children_changed } } @@ -517,8 +518,10 @@ impl> WidgetPod { self.inner.lifecycle(ctx, &event, data, env); false } else { - old.map(|id| self.state.children.contains(&id)) - .or_else(|| new.map(|id| self.state.children.contains(&id))) + // Recurse when the target widgets could be our descendants. + // The bloom filter we're checking can return false positives. + old.map(|id| self.state.children.may_contain(&id)) + .or_else(|| new.map(|id| self.state.children.may_contain(&id))) .unwrap_or(false) } } @@ -532,7 +535,9 @@ impl> WidgetPod { state_cell.set(self.state.clone()); false } else { - self.state.children.contains(&widget) + // Recurse when the target widget could be our descendant. + // The bloom filter we're checking can return false positives. + self.state.children.may_contain(&widget) } } #[cfg(test)] @@ -542,13 +547,12 @@ impl> WidgetPod { } }; - let mut child_ctx = LifeCycleCtx { - command_queue: ctx.command_queue, - base_state: &mut self.state, - window_id: ctx.window_id, - }; - if recurse { + let mut child_ctx = LifeCycleCtx { + command_queue: ctx.command_queue, + base_state: &mut self.state, + window_id: ctx.window_id, + }; self.inner.lifecycle(&mut child_ctx, event, data, env); } @@ -691,9 +695,9 @@ mod tests { let env = Env::default(); widget.lifecycle(&mut ctx, &LifeCycle::WidgetAdded, &None, &env); - assert!(ctx.base_state.children.contains(&ID_1)); - assert!(ctx.base_state.children.contains(&ID_2)); - assert!(ctx.base_state.children.contains(&ID_3)); + assert!(ctx.base_state.children.may_contain(&ID_1)); + assert!(ctx.base_state.children.may_contain(&ID_2)); + assert!(ctx.base_state.children.may_contain(&ID_3)); assert_eq!(ctx.base_state.children.entry_count(), 7); } } diff --git a/druid/src/tests/mod.rs b/druid/src/tests/mod.rs index a57a63034b..0739bcabad 100644 --- a/druid/src/tests/mod.rs +++ b/druid/src/tests/mod.rs @@ -275,13 +275,13 @@ fn child_tracking() { harness.send_initial_events(); let root = harness.get_state(id_4); assert_eq!(root.children.entry_count(), 3); - assert!(root.children.contains(&id_1)); - assert!(root.children.contains(&id_2)); - assert!(root.children.contains(&id_3)); + assert!(root.children.may_contain(&id_1)); + assert!(root.children.may_contain(&id_2)); + assert!(root.children.may_contain(&id_3)); let split = harness.get_state(id_3); - assert!(split.children.contains(&id_1)); - assert!(split.children.contains(&id_2)); + assert!(split.children.may_contain(&id_1)); + assert!(split.children.may_contain(&id_2)); assert_eq!(split.children.entry_count(), 2); }); } @@ -302,18 +302,18 @@ fn register_after_adding_child() { Harness::create(String::new(), widget, |harness| { harness.send_initial_events(); - assert!(harness.get_state(id_5).children.contains(&id_6)); - assert!(harness.get_state(id_5).children.contains(&id_1)); - assert!(harness.get_state(id_5).children.contains(&id_4)); + assert!(harness.get_state(id_5).children.may_contain(&id_6)); + assert!(harness.get_state(id_5).children.may_contain(&id_1)); + assert!(harness.get_state(id_5).children.may_contain(&id_4)); assert_eq!(harness.get_state(id_5).children.entry_count(), 3); harness.submit_command(REPLACE_CHILD, None); - assert!(harness.get_state(id_5).children.contains(&id_6)); - assert!(harness.get_state(id_5).children.contains(&id_4)); - assert!(harness.get_state(id_5).children.contains(&id_7)); - assert!(harness.get_state(id_5).children.contains(&id_2)); - assert!(harness.get_state(id_5).children.contains(&id_3)); + assert!(harness.get_state(id_5).children.may_contain(&id_6)); + assert!(harness.get_state(id_5).children.may_contain(&id_4)); + assert!(harness.get_state(id_5).children.may_contain(&id_7)); + assert!(harness.get_state(id_5).children.may_contain(&id_2)); + assert!(harness.get_state(id_5).children.may_contain(&id_3)); assert_eq!(harness.get_state(id_5).children.entry_count(), 5); }) } diff --git a/druid/src/widget/textbox.rs b/druid/src/widget/textbox.rs index c180ca6774..9576b9980c 100644 --- a/druid/src/widget/textbox.rs +++ b/druid/src/widget/textbox.rs @@ -240,7 +240,7 @@ impl Widget for TextBox { match event { Event::MouseDown(mouse) => { - if !ctx.has_focus() { + if !ctx.is_focused() { ctx.request_focus(); } ctx.set_active(true); @@ -281,7 +281,7 @@ impl Widget for TextBox { } } Event::Command(ref cmd) - if ctx.has_focus() + if ctx.is_focused() && (cmd.selector == crate::commands::COPY || cmd.selector == crate::commands::CUT) => { @@ -392,9 +392,9 @@ impl Widget for TextBox { let placeholder_color = env.get(theme::PLACEHOLDER_COLOR); let cursor_color = env.get(theme::CURSOR_COLOR); - let has_focus = ctx.has_focus(); + let is_focused = ctx.is_focused(); - let border_color = if has_focus { + let border_color = if is_focused { env.get(theme::PRIMARY_LIGHT) } else { env.get(theme::BORDER_DARK) @@ -449,7 +449,7 @@ impl Widget for TextBox { rc.draw_text(&text_layout, text_pos, color); // Paint the cursor if focused and there's no selection - if has_focus && self.cursor_on && self.selection.is_caret() { + if is_focused && self.cursor_on && self.selection.is_caret() { let cursor_x = self.x_for_offset(&text_layout, self.cursor()); let xy = text_pos + Vec2::new(cursor_x, 2. - font_size); let x2y2 = xy + Vec2::new(0., font_size + 2.); diff --git a/druid/src/widget/widget.rs b/druid/src/widget/widget.rs index 764983e58c..c109b2507d 100644 --- a/druid/src/widget/widget.rs +++ b/druid/src/widget/widget.rs @@ -46,6 +46,11 @@ use super::prelude::*; /// [`LifeCycleCtx::widget_id`]: struct.LifeCycleCtx.html#method.widget_id /// [`WidgetExt::with_id`]: trait.WidgetExt.html#method.with_id /// [`IdentityWrapper`]: widget/struct.IdentityWrapper.html +// +// The widget id does not necessarily identify only a single widget +// in the strictest sense. Internally WidgetPod uses its child's id, +// and the window base state uses the root widget's id. +// // this is NonZeroU64 because we regularly store Option #[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)] pub struct WidgetId(NonZeroU64); diff --git a/druid/src/window.rs b/druid/src/window.rs index a8df380c2b..44a82d0b34 100644 --- a/druid/src/window.rs +++ b/druid/src/window.rs @@ -72,8 +72,12 @@ impl Window { &self.root.state().focus_chain } + /// Returns `true` if the provided widget may be in this window, + /// but it may also be a false positive. + /// However when this returns `false` the widget is definitely not in this window. pub(crate) fn may_contain_widget(&self, widget_id: WidgetId) -> bool { - self.root.state().children.contains(&widget_id) + // The bloom filter we're checking can return false positives. + self.root.state().children.may_contain(&widget_id) } pub(crate) fn set_menu(&mut self, mut menu: MenuDesc, data: &T, env: &Env) { From fd0620c0ebe159934d2387b97a51e9ad3a902fae Mon Sep 17 00:00:00 2001 From: Kaur Kuut Date: Fri, 10 Apr 2020 14:42:39 +0300 Subject: [PATCH 2/3] Remove WidgetId note about multiple use. --- druid/src/widget/widget.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/druid/src/widget/widget.rs b/druid/src/widget/widget.rs index c109b2507d..764983e58c 100644 --- a/druid/src/widget/widget.rs +++ b/druid/src/widget/widget.rs @@ -46,11 +46,6 @@ use super::prelude::*; /// [`LifeCycleCtx::widget_id`]: struct.LifeCycleCtx.html#method.widget_id /// [`WidgetExt::with_id`]: trait.WidgetExt.html#method.with_id /// [`IdentityWrapper`]: widget/struct.IdentityWrapper.html -// -// The widget id does not necessarily identify only a single widget -// in the strictest sense. Internally WidgetPod uses its child's id, -// and the window base state uses the root widget's id. -// // this is NonZeroU64 because we regularly store Option #[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)] pub struct WidgetId(NonZeroU64); From 1f177a09eba986ecb9cfefa5368e1985bf0dbfd1 Mon Sep 17 00:00:00 2001 From: Kaur Kuut Date: Fri, 10 Apr 2020 14:42:59 +0300 Subject: [PATCH 3/3] Rewrite focus documentation. --- druid/src/contexts.rs | 89 ++++++++++++++++++++++++++++++------------- druid/src/event.rs | 10 +++-- 2 files changed, 69 insertions(+), 30 deletions(-) diff --git a/druid/src/contexts.rs b/druid/src/contexts.rs index 7a29d2c148..55ecf7a9ed 100644 --- a/druid/src/contexts.rs +++ b/druid/src/contexts.rs @@ -238,22 +238,37 @@ impl<'a> EventCtx<'a> { self.is_handled } - /// The (tree) focus status of a widget. + /// The focus status of a widget. + /// + /// Returns `true` if this specific widget is focused. + /// To check if any descendants are focused use [`has_focus`]. /// /// Focus means that the widget receives keyboard events. /// /// A widget can request focus using the [`request_focus`] method. - /// This will generally result in a separate event propagation of - /// a `FocusChanged` method, including sending `false` to the previous - /// widget that held focus. + /// It's also possible to register for automatic focus via [`register_for_focus`]. /// - /// Only one leaf widget at a time has focus. However, in a container - /// hierarchy, all ancestors of that leaf widget are also invoked with - /// `FocusChanged(true)`. + /// If a widget gains or loses focus it will get a [`LifeCycle::FocusChanged`] event. /// - /// Discussion question: is "may_have_focus" a better name? + /// Only one widget at a time is focused. However due to the way events are routed, + /// all ancestors of that widget will also receive keyboard events. /// /// [`request_focus`]: struct.EventCtx.html#method.request_focus + /// [`register_for_focus`]: struct.LifeCycleCtx.html#method.register_for_focus + /// [`LifeCycle::FocusChanged`]: enum.LifeCycle.html#variant.FocusChanged + /// [`has_focus`]: struct.EventCtx.html#method.has_focus + pub fn is_focused(&self) -> bool { + self.focus_widget == Some(self.widget_id()) + } + + /// The (tree) focus status of a widget. + /// + /// Returns `true` if either this specific widget or any one of its descendants is focused. + /// To check if only this specific widget is focused use [`is_focused`]. + /// + /// See [`is_focused`] for more information about focus. + /// + /// [`is_focused`]: struct.EventCtx.html#method.is_focused pub fn has_focus(&self) -> bool { // The bloom filter we're checking can return false positives. let is_child = self @@ -263,18 +278,11 @@ impl<'a> EventCtx<'a> { is_child || self.focus_widget == Some(self.widget_id()) } - /// The (leaf) focus status of a widget. - /// - /// See [`has_focus`](struct.EventCtx.html#method.has_focus). - pub fn is_focused(&self) -> bool { - self.focus_widget == Some(self.widget_id()) - } - /// Request keyboard focus. /// - /// See [`has_focus`] for more information. + /// See [`is_focused`] for more information about focus. /// - /// [`has_focus`]: struct.EventCtx.html#method.has_focus + /// [`is_focused`]: struct.EventCtx.html#method.is_focused pub fn request_focus(&mut self) { self.base_state.request_focus = Some(FocusChange::Focus(self.widget_id())); } @@ -282,6 +290,10 @@ impl<'a> EventCtx<'a> { /// Transfer focus to the next focusable widget. /// /// This should only be called by a widget that currently has focus. + /// + /// See [`is_focused`] for more information about focus. + /// + /// [`is_focused`]: struct.EventCtx.html#method.is_focused pub fn focus_next(&mut self) { if self.is_focused() { self.base_state.request_focus = Some(FocusChange::Next); @@ -293,6 +305,10 @@ impl<'a> EventCtx<'a> { /// Transfer focus to the previous focusable widget. /// /// This should only be called by a widget that currently has focus. + /// + /// See [`is_focused`] for more information about focus. + /// + /// [`is_focused`]: struct.EventCtx.html#method.is_focused pub fn focus_prev(&mut self) { if self.is_focused() { self.base_state.request_focus = Some(FocusChange::Previous); @@ -304,6 +320,10 @@ impl<'a> EventCtx<'a> { /// Give up focus. /// /// This should only be called by a widget that currently has focus. + /// + /// See [`is_focused`] for more information about focus. + /// + /// [`is_focused`]: struct.EventCtx.html#method.is_focused pub fn resign_focus(&mut self) { if self.is_focused() { self.base_state.request_focus = Some(FocusChange::Resign); @@ -416,7 +436,12 @@ impl<'a> LifeCycleCtx<'a> { /// Register this widget to be eligile to accept focus automatically. /// - /// This should only be called in response to a `LifeCycle::WidgetAdded` event. + /// This should only be called in response to a [`LifeCycle::WidgetAdded`] event. + /// + /// See [`EventCtx::is_focused`] for more information about focus. + /// + /// [`LifeCycle::WidgetAdded`]: enum.Lifecycle.html#variant.WidgetAdded + /// [`EventCtx::is_focused`]: struct.EventCtx.html#method.is_focused pub fn register_for_focus(&mut self) { self.base_state.focus_chain.push(self.widget_id()); } @@ -560,9 +585,28 @@ impl<'a, 'b: 'a> PaintCtx<'a, 'b> { self.base_state.size() } + /// The focus status of a widget. + /// + /// Returns `true` if this specific widget is focused. + /// To check if any descendants are focused use [`has_focus`]. + /// + /// See [`EventCtx::is_focused`] for more information about focus. + /// + /// [`has_focus`]: #method.has_focus + /// [`EventCtx::is_focused`]: struct.EventCtx.html#method.is_focused + pub fn is_focused(&self) -> bool { + self.focus_widget == Some(self.widget_id()) + } + /// The (tree) focus status of a widget. /// - /// See [`has_focus`](struct.EventCtx.html#method.has_focus). + /// Returns `true` if either this specific widget or any one of its descendants is focused. + /// To check if only this specific widget is focused use [`is_focused`]. + /// + /// See [`EventCtx::is_focused`] for more information about focus. + /// + /// [`is_focused`]: #method.is_focused + /// [`EventCtx::is_focused`]: struct.EventCtx.html#method.is_focused pub fn has_focus(&self) -> bool { // The bloom filter we're checking can return false positives. let is_child = self @@ -572,13 +616,6 @@ impl<'a, 'b: 'a> PaintCtx<'a, 'b> { is_child || self.focus_widget == Some(self.widget_id()) } - /// The (leaf) focus status of a widget. - /// - /// See [`has_focus`](struct.EventCtx.html#method.has_focus). - pub fn is_focused(&self) -> bool { - self.focus_widget == Some(self.widget_id()) - } - /// Returns the currently visible [`Region`]. /// /// [`Region`]: struct.Region.html diff --git a/druid/src/event.rs b/druid/src/event.rs index c90daaf26f..4567c8cb4d 100644 --- a/druid/src/event.rs +++ b/druid/src/event.rs @@ -186,11 +186,13 @@ pub enum LifeCycle { }, /// Called when the focus status changes. /// - /// This will always be called immediately after an event where a widget - /// has requested focus. + /// This will always be called immediately after a new widget gains focus. + /// The newly focused widget will receive this with `true` and the widget + /// that lost focus will receive this with `false`. /// - /// See [`has_focus`](struct.EventCtx.html#method.has_focus) for - /// discussion about the focus status. + /// See [`EventCtx::is_focused`] for more information about focus. + /// + /// [`EventCtx::is_focused`]: struct.EventCtx.html#method.is_focused FocusChanged(bool), /// Testing only: request the `BaseState` of a specific widget. ///