Skip to content

Commit

Permalink
Refactor TreeArena (#827)
Browse files Browse the repository at this point in the history
Make tests more spread out and more readable.
Add comments detailing the state of the arena being tested.
Fix a lifetime error in unsafe API.

Rename Arena[Ref/Mut]Children to Arena[Ref/Mut]List
Rename a lot of methods.
Rework documentation.
Unify safe and unsafe APIs.
  • Loading branch information
PoignardAzur authored Jan 21, 2025
1 parent 46170fa commit 8276e75
Show file tree
Hide file tree
Showing 10 changed files with 390 additions and 350 deletions.
23 changes: 23 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,9 @@ jobs:
# See also https://github.com/linebender/vello/pull/610
SKIP_RENDER_TESTS: ${{ matrix.skip_gpu }}

- name: Run cargo nextest on unsafe arena
run: cargo nextest run -p tree_arena --locked --no-fail-fast --no-default-features

- name: Upload test results due to failure
uses: actions/upload-artifact@v4
if: failure()
Expand All @@ -302,6 +305,26 @@ jobs:
- name: Run cargo test --doc
run: cargo test --doc --workspace --locked --profile ci --all-features --no-fail-fast

miri:
name: cargo miri (unsafe tree_arena)
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4

- name: Install nightly toolchain
uses: dtolnay/rust-toolchain@master
with:
toolchain: nightly
components: miri

- name: Restore cache
uses: Swatinem/rust-cache@v2
with:
save-if: ${{ github.event_name != 'merge_group' }}

- name: Run cargo miri
run: cargo miri test -p tree_arena --locked --no-default-features --no-fail-fast

test-stable-wasm:
name: cargo test (wasm32)
runs-on: ubuntu-latest
Expand Down
74 changes: 37 additions & 37 deletions masonry/src/contexts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use accesskit::TreeUpdate;
use dpi::LogicalPosition;
use parley::{FontContext, LayoutContext};
use tracing::{trace, warn};
use tree_arena::{ArenaMutChildren, ArenaRefChildren};
use tree_arena::{ArenaMutList, ArenaRefList};
use winit::window::ResizeDirection;

use crate::action::Action;
Expand Down Expand Up @@ -49,8 +49,8 @@ pub struct MutateCtx<'a> {
pub(crate) global_state: &'a mut RenderRootState,
pub(crate) parent_widget_state: Option<&'a mut WidgetState>,
pub(crate) widget_state: &'a mut WidgetState,
pub(crate) widget_state_children: ArenaMutChildren<'a, WidgetState>,
pub(crate) widget_children: ArenaMutChildren<'a, Box<dyn Widget>>,
pub(crate) widget_state_children: ArenaMutList<'a, WidgetState>,
pub(crate) widget_children: ArenaMutList<'a, Box<dyn Widget>>,
}

/// A context provided inside of [`WidgetRef`].
Expand All @@ -60,25 +60,25 @@ pub struct MutateCtx<'a> {
pub struct QueryCtx<'a> {
pub(crate) global_state: &'a RenderRootState,
pub(crate) widget_state: &'a WidgetState,
pub(crate) widget_state_children: ArenaRefChildren<'a, WidgetState>,
pub(crate) widget_children: ArenaRefChildren<'a, Box<dyn Widget>>,
pub(crate) widget_state_children: ArenaRefList<'a, WidgetState>,
pub(crate) widget_children: ArenaRefList<'a, Box<dyn Widget>>,
}

/// A context provided to Widget event-handling methods.
pub struct EventCtx<'a> {
pub(crate) global_state: &'a mut RenderRootState,
pub(crate) widget_state: &'a mut WidgetState,
pub(crate) widget_state_children: ArenaMutChildren<'a, WidgetState>,
pub(crate) widget_children: ArenaMutChildren<'a, Box<dyn Widget>>,
pub(crate) widget_state_children: ArenaMutList<'a, WidgetState>,
pub(crate) widget_children: ArenaMutList<'a, Box<dyn Widget>>,
pub(crate) target: WidgetId,
pub(crate) allow_pointer_capture: bool,
pub(crate) is_handled: bool,
}

/// A context provided to the [`Widget::register_children`] method.
pub struct RegisterCtx<'a> {
pub(crate) widget_state_children: ArenaMutChildren<'a, WidgetState>,
pub(crate) widget_children: ArenaMutChildren<'a, Box<dyn Widget>>,
pub(crate) widget_state_children: ArenaMutList<'a, WidgetState>,
pub(crate) widget_children: ArenaMutList<'a, Box<dyn Widget>>,
#[cfg(debug_assertions)]
pub(crate) registered_ids: Vec<WidgetId>,
}
Expand All @@ -87,42 +87,42 @@ pub struct RegisterCtx<'a> {
pub struct UpdateCtx<'a> {
pub(crate) global_state: &'a mut RenderRootState,
pub(crate) widget_state: &'a mut WidgetState,
pub(crate) widget_state_children: ArenaMutChildren<'a, WidgetState>,
pub(crate) widget_children: ArenaMutChildren<'a, Box<dyn Widget>>,
pub(crate) widget_state_children: ArenaMutList<'a, WidgetState>,
pub(crate) widget_children: ArenaMutList<'a, Box<dyn Widget>>,
}

// TODO - Change this once other layout methods are added.
/// A context provided to [`Widget::layout`] methods.
pub struct LayoutCtx<'a> {
pub(crate) global_state: &'a mut RenderRootState,
pub(crate) widget_state: &'a mut WidgetState,
pub(crate) widget_state_children: ArenaMutChildren<'a, WidgetState>,
pub(crate) widget_children: ArenaMutChildren<'a, Box<dyn Widget>>,
pub(crate) widget_state_children: ArenaMutList<'a, WidgetState>,
pub(crate) widget_children: ArenaMutList<'a, Box<dyn Widget>>,
}

/// A context provided to the [`Widget::compose`] method.
pub struct ComposeCtx<'a> {
pub(crate) global_state: &'a mut RenderRootState,
pub(crate) widget_state: &'a mut WidgetState,
pub(crate) widget_state_children: ArenaMutChildren<'a, WidgetState>,
pub(crate) widget_children: ArenaMutChildren<'a, Box<dyn Widget>>,
pub(crate) widget_state_children: ArenaMutList<'a, WidgetState>,
pub(crate) widget_children: ArenaMutList<'a, Box<dyn Widget>>,
}

/// A context passed to [`Widget::paint`] method.
pub struct PaintCtx<'a> {
pub(crate) global_state: &'a mut RenderRootState,
pub(crate) widget_state: &'a WidgetState,
pub(crate) widget_state_children: ArenaMutChildren<'a, WidgetState>,
pub(crate) widget_children: ArenaMutChildren<'a, Box<dyn Widget>>,
pub(crate) widget_state_children: ArenaMutList<'a, WidgetState>,
pub(crate) widget_children: ArenaMutList<'a, Box<dyn Widget>>,
pub(crate) debug_paint: bool,
}

/// A context passed to [`Widget::accessibility`] method.
pub struct AccessCtx<'a> {
pub(crate) global_state: &'a mut RenderRootState,
pub(crate) widget_state: &'a WidgetState,
pub(crate) widget_state_children: ArenaMutChildren<'a, WidgetState>,
pub(crate) widget_children: ArenaMutChildren<'a, Box<dyn Widget>>,
pub(crate) widget_state_children: ArenaMutList<'a, WidgetState>,
pub(crate) widget_children: ArenaMutList<'a, Box<dyn Widget>>,
pub(crate) tree_update: &'a mut TreeUpdate,
pub(crate) rebuild_all: bool,
}
Expand All @@ -149,7 +149,7 @@ impl_context_method!(
fn get_child<Child: Widget>(&self, child: &'_ WidgetPod<Child>) -> &'_ Child {
let child_ref = self
.widget_children
.get_child(child.id())
.item(child.id())
.expect("get_child: child not found");
child_ref.item.as_dyn_any().downcast_ref::<Child>().unwrap()
}
Expand All @@ -159,7 +159,7 @@ impl_context_method!(
fn get_child_dyn(&self, child: &'_ WidgetPod<impl Widget + ?Sized>) -> &'_ dyn Widget {
let child_ref = self
.widget_children
.get_child(child.id())
.item(child.id())
.expect("get_child: child not found");
child_ref.item.as_dyn()
}
Expand All @@ -169,7 +169,7 @@ impl_context_method!(
fn get_child_state(&self, child: &'_ WidgetPod<impl Widget + ?Sized>) -> &'_ WidgetState {
let child_state_ref = self
.widget_state_children
.get_child(child.id())
.item(child.id())
.expect("get_child_state: child not found");
child_state_ref.item
}
Expand Down Expand Up @@ -198,7 +198,7 @@ impl_context_method!(
) -> &'_ mut WidgetState {
let child_state_mut = self
.widget_state_children
.get_child_mut(child.id())
.item_mut(child.id())
.expect("get_child_state_mut: child not found");
child_state_mut.item
}
Expand All @@ -215,11 +215,11 @@ impl MutateCtx<'_> {
) -> WidgetMut<'c, Child> {
let child_state_mut = self
.widget_state_children
.get_child_mut(child.id())
.item_mut(child.id())
.expect("get_mut: child not found");
let child_mut = self
.widget_children
.get_child_mut(child.id())
.item_mut(child.id())
.expect("get_mut: child not found");
let child_ctx = MutateCtx {
global_state: self.global_state,
Expand Down Expand Up @@ -264,11 +264,11 @@ impl<'w> QueryCtx<'w> {
pub fn get(self, child: WidgetId) -> WidgetRef<'w, dyn Widget> {
let child_state = self
.widget_state_children
.into_child(child)
.into_item(child)
.expect("get: child not found");
let child = self
.widget_children
.into_child(child)
.into_item(child)
.expect("get: child not found");

let ctx = QueryCtx {
Expand Down Expand Up @@ -993,11 +993,11 @@ impl_context_method!(MutateCtx<'_>, EventCtx<'_>, UpdateCtx<'_>, {
let id = child.id();
let _ = self
.widget_state_children
.remove_child(id)
.remove(id)
.expect("remove_child: child not found");
let _ = self
.widget_children
.remove_child(id)
.remove(id)
.expect("remove_child: child not found");
self.global_state.scenes.remove(&child.id());

Expand Down Expand Up @@ -1182,8 +1182,8 @@ impl RegisterCtx<'_> {
let id = child.id();
let state = WidgetState::new(child.id(), widget.short_type_name(), transform);

self.widget_children.insert_child(id, widget.as_box_dyn());
self.widget_state_children.insert_child(id, state);
self.widget_children.insert(id, widget.as_box_dyn());
self.widget_state_children.insert(id, state);
}
}

Expand Down Expand Up @@ -1229,11 +1229,11 @@ macro_rules! impl_get_raw {
{
let child_state_mut = self
.widget_state_children
.get_child_mut(child.id())
.item_mut(child.id())
.expect("get_raw_ref: child not found");
let child_mut = self
.widget_children
.get_child_mut(child.id())
.item_mut(child.id())
.expect("get_raw_ref: child not found");
#[allow(clippy::needless_update)]
let child_ctx = $SomeCtx {
Expand Down Expand Up @@ -1262,11 +1262,11 @@ macro_rules! impl_get_raw {
{
let child_state_mut = self
.widget_state_children
.get_child_mut(child.id())
.item_mut(child.id())
.expect("get_raw_mut: child not found");
let child_mut = self
.widget_children
.get_child_mut(child.id())
.item_mut(child.id())
.expect("get_raw_mut: child not found");
#[allow(clippy::needless_update)]
let child_ctx = $SomeCtx {
Expand Down Expand Up @@ -1302,11 +1302,11 @@ impl<'s> AccessCtx<'s> {
{
let child_state_mut = self
.widget_state_children
.get_child_mut(child.id())
.item_mut(child.id())
.expect("get_raw_ref: child not found");
let child_mut = self
.widget_children
.get_child_mut(child.id())
.item_mut(child.id())
.expect("get_raw_ref: child not found");
let child_ctx = AccessCtx {
widget_state: child_state_mut.item,
Expand Down
14 changes: 7 additions & 7 deletions masonry/src/passes/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ pub(crate) fn run_layout_on<W: Widget + ?Sized>(
bc: &BoxConstraints,
) -> Size {
let id = pod.id();
let mut widget = parent_ctx.widget_children.get_child_mut(id).unwrap();
let mut state = parent_ctx.widget_state_children.get_child_mut(id).unwrap();
let mut widget = parent_ctx.widget_children.item_mut(id).unwrap();
let mut state = parent_ctx.widget_state_children.item_mut(id).unwrap();

let trace = parent_ctx.global_state.trace.layout;
let _span = enter_span_if(
Expand All @@ -42,7 +42,7 @@ pub(crate) fn run_layout_on<W: Widget + ?Sized>(
// We forcefully set request_layout to true for all children.
// This is used below to check that widget.layout(..) visited all of them.
for child_id in widget.item.children_ids() {
let child_state = state.children.get_child_mut(child_id).unwrap().item;
let child_state = state.children.item_mut(child_id).unwrap().item;
if !child_state.is_stashed {
child_state.request_layout = true;
}
Expand Down Expand Up @@ -136,7 +136,7 @@ pub(crate) fn run_layout_on<W: Widget + ?Sized>(
{
let name = widget.item.short_type_name();
for child_id in widget.item.children_ids() {
let child_state = state.children.get_child_mut(child_id).unwrap().item;
let child_state = state.children.item_mut(child_id).unwrap().item;

if child_state.is_stashed {
continue;
Expand Down Expand Up @@ -182,7 +182,7 @@ pub(crate) fn run_layout_on<W: Widget + ?Sized>(
}
}

let state_mut = parent_ctx.widget_state_children.get_child_mut(id).unwrap();
let state_mut = parent_ctx.widget_state_children.item_mut(id).unwrap();
parent_ctx.widget_state.merge_up(state_mut.item);
state_mut.item.size = new_size;
new_size
Expand All @@ -205,8 +205,8 @@ pub(crate) fn run_layout_pass(root: &mut RenderRoot) {
};

let mut dummy_state = WidgetState::synthetic(root.root.id(), root.get_kurbo_size());
let root_state_token = root.widget_arena.states.root_token_mut();
let root_widget_token = root.widget_arena.widgets.root_token_mut();
let root_state_token = root.widget_arena.states.roots_mut();
let root_widget_token = root.widget_arena.widgets.roots_mut();
let mut ctx = LayoutCtx {
global_state: &mut root.global_state,
widget_state: &mut dummy_state,
Expand Down
10 changes: 5 additions & 5 deletions masonry/src/passes/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
//! This file includes utility functions used by multiple passes.
use tracing::span::EnteredSpan;
use tree_arena::{ArenaMut, ArenaMutChildren, ArenaRef};
use tree_arena::{ArenaMut, ArenaMutList, ArenaRef};

use crate::render_root::RenderRootState;
use crate::widget::WidgetArena;
Expand Down Expand Up @@ -55,20 +55,20 @@ pub(crate) fn enter_span(
pub(crate) fn recurse_on_children(
id: WidgetId,
mut widget: ArenaMut<'_, Box<dyn Widget>>,
mut state: ArenaMutChildren<'_, WidgetState>,
mut state: ArenaMutList<'_, WidgetState>,
mut callback: impl FnMut(ArenaMut<'_, Box<dyn Widget>>, ArenaMut<'_, WidgetState>),
) {
let parent_name = widget.item.short_type_name();
let parent_id = id;

for child_id in widget.item.children_ids() {
let widget = widget.children.get_child_mut(child_id).unwrap_or_else(|| {
let widget = widget.children.item_mut(child_id).unwrap_or_else(|| {
panic!(
"Error in '{}' #{}: cannot find child #{} returned by children_ids()",
parent_name, parent_id, child_id
)
});
let state = state.get_child_mut(child_id).unwrap_or_else(|| {
let state = state.item_mut(child_id).unwrap_or_else(|| {
panic!(
"Error in '{}' #{}: cannot find child #{} returned by children_ids()",
parent_name, parent_id, child_id
Expand All @@ -88,7 +88,7 @@ pub(crate) fn merge_state_up(arena: &mut WidgetArena, widget_id: WidgetId) {
};

let mut parent_state_mut = arena.states.find_mut(parent_id).unwrap();
let child_state_mut = parent_state_mut.children.get_child_mut(widget_id).unwrap();
let child_state_mut = parent_state_mut.children.item_mut(widget_id).unwrap();

parent_state_mut.item.merge_up(child_state_mut.item);
}
Expand Down
6 changes: 3 additions & 3 deletions masonry/src/passes/update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ fn update_widget_tree(

#[cfg(debug_assertions)]
for child_id in widget.item.children_ids() {
if widget.children.get_child(child_id).is_none() {
if widget.children.item(child_id).is_none() {
panic!(
"Error in '{}' #{}: method register_children() did not call \
RegisterCtx::register_child() on child #{} returned by children_ids()",
Expand Down Expand Up @@ -181,8 +181,8 @@ pub(crate) fn run_update_widget_tree_pass(root: &mut RenderRoot) {

if root.root.incomplete() {
let mut ctx = RegisterCtx {
widget_state_children: root.widget_arena.states.root_token_mut(),
widget_children: root.widget_arena.widgets.root_token_mut(),
widget_state_children: root.widget_arena.states.roots_mut(),
widget_children: root.widget_arena.widgets.roots_mut(),
#[cfg(debug_assertions)]
registered_ids: Vec::new(),
};
Expand Down
Loading

0 comments on commit 8276e75

Please sign in to comment.