Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add documentation, e.g. around bloom usage. #818

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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion druid-shell/src/common_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ impl<F: FnOnce(&dyn Any) + Send> 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);
Expand Down
26 changes: 14 additions & 12 deletions druid/src/bloom.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,12 @@ impl<T: ?Sized + Hash> Bloom<T> {
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.
Copy link
Member

Choose a reason for hiding this comment

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

I think these two lines are a strict restating of the preceding line; I think between the changed method name and the changed first line comment, this should be clear to our expected audience.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's worth stating it twice with different wording. The mistakes that result from not understanding this are extremely subtle and hard to catch.

pub fn may_contain(&self, item: &T) -> bool {
let mask = self.make_bit_mask(item);
self.bits & mask == mask
}
Expand Down Expand Up @@ -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));
}
}

Expand All @@ -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));
}
}
91 changes: 66 additions & 25 deletions druid/src/contexts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -240,49 +240,62 @@ impl<'a> EventCtx<'a> {

/// 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 "is_focused" 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
.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`].
///
/// [`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()));
}

/// 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.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");
Expand All @@ -292,8 +305,12 @@ 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.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");
Expand All @@ -303,8 +320,12 @@ 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.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");
Expand Down Expand Up @@ -414,6 +435,13 @@ 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.
///
/// 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());
}
Expand Down Expand Up @@ -557,20 +585,33 @@ impl<'a, 'b: 'a> PaintCtx<'a, 'b> {
self.base_state.size()
}

/// Query the focus state of the 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`].
///
/// See [`EventCtx::is_focused`] for more information about focus.
///
/// This is true only if this widget has 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 focus status of a widget.
/// 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 [`EventCtx::is_focused`] for more information about focus.
///
/// See [`has_focus`](struct.EventCtx.html#method.has_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.
xStrom marked this conversation as resolved.
Show resolved Hide resolved
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())
}
Expand Down
34 changes: 19 additions & 15 deletions druid/src/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,7 @@ impl<T: Data, W: Widget<T>> WidgetPod<T, W> {
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,
Expand Down Expand Up @@ -446,7 +446,9 @@ impl<T: Data, W: Widget<T>> WidgetPod<T, W> {
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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The bloom filter we're checking can return false positives.

I think the renaming to may_contain should make it clear enough, as this comment has to be applied to too many places to stay consistent (and those were definitly to many tos 😅 ).

Copy link
Member Author

Choose a reason for hiding this comment

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

Just the name may_contain will make it clear enough to someone who already knows the function or will go read its docs. Others who are just glancing at the code at the call site won't necessarily know.

I also don't think the three places this comment was added to is too many. Once we're up to 10 or something we can reconsider.

recurse = child_ctx.base_state.children.may_contain(id);
Event::TargetedCommand(*target, cmd.clone())
}
Target::Global => panic!("Target::Global should be converted before WidgetPod"),
Expand Down Expand Up @@ -496,7 +498,6 @@ impl<T: Data, W: Widget<T>> WidgetPod<T, W> {
self.state.children.clear();
self.state.focus_chain.clear();
}

self.state.children_changed
}
}
Expand All @@ -517,8 +518,10 @@ impl<T: Data, W: Widget<T>> WidgetPod<T, W> {
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)
}
}
Expand All @@ -532,7 +535,9 @@ impl<T: Data, W: Widget<T>> WidgetPod<T, W> {
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)]
Expand All @@ -542,13 +547,12 @@ impl<T: Data, W: Widget<T>> WidgetPod<T, W> {
}
};

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);
}

Expand Down Expand Up @@ -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);
}
}
10 changes: 6 additions & 4 deletions druid/src/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand Down
26 changes: 13 additions & 13 deletions druid/src/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
}
Expand All @@ -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);
})
}
Loading