Skip to content

Commit

Permalink
Refactor function update_accessibility_nodes (bevyengine#10911)
Browse files Browse the repository at this point in the history
# Objective

`update_accessibility_nodes` is one of the most nested functions in the
entire Bevy repository, with a maximum of 9 levels of indentations. This
PR refactors it down to 3 levels of indentations, while improving
readability on other fronts. The result is a function that is actually
understandable at a first glance.

- This is a proof of concept to demonstrate that it is possible to
gradually lower the nesting limit proposed by bevyengine#10896.

PS: I read AccessKit's documentation, but I don't have experience with
it. Therefore, naming of variables and subroutines may be a bit off.

PS2: I don't know if the test suite covers the functionality of this
system, but since I've spent quite some time on it and the changes were
simple, I'm pretty confident the refactor is functionally equivalent to
the original.

## Solution

I documented each change with a commit, but as a summary I did the
following to reduce nesting:

- I moved from `if-let` blocks to `let-else` statements where
appropriate to reduce rightward shift
- I extracted the closure body to a new function `update_adapter`
- I factored out parts of `update_adapter` into new functions
`queue_node_for_update` and `add_children_nodes`

**Note for reviewers:** GitHub's diff viewer is not the greatest in
showing horizontal code shifts, therefore you may want to use a
different tool like VSCode to review some commits, especially the second
one (anyway, that commit is very straightforward, despite changing many
lines).
  • Loading branch information
Nilirad committed Dec 10, 2023
1 parent 4a46f27 commit 47fa620
Showing 1 changed file with 87 additions and 49 deletions.
136 changes: 87 additions & 49 deletions crates/bevy_winit/src/accessibility.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,55 +86,93 @@ fn update_accessibility_nodes(
)>,
node_entities: Query<Entity, With<AccessibilityNode>>,
) {
if let Ok((primary_window_id, primary_window)) = primary_window.get_single() {
if let Some(adapter) = adapters.get(&primary_window_id) {
let should_run = focus.is_changed() || !nodes.is_empty();
if should_run {
adapter.update_if_active(|| {
let mut to_update = vec![];
let mut name = None;
if primary_window.focused {
let title = primary_window.title.clone();
name = Some(title.into_boxed_str());
}
let focus_id = (*focus).unwrap_or_else(|| primary_window_id).to_bits();
let mut root_children = vec![];
for (entity, node, children, parent) in &nodes {
let mut node = (**node).clone();
if let Some(parent) = parent {
if !node_entities.contains(**parent) {
root_children.push(NodeId(entity.to_bits()));
}
} else {
root_children.push(NodeId(entity.to_bits()));
}
if let Some(children) = children {
for child in children {
if node_entities.contains(*child) {
node.push_child(NodeId(child.to_bits()));
}
}
}
to_update.push((
NodeId(entity.to_bits()),
node.build(&mut NodeClassSet::lock_global()),
));
}
let mut root = NodeBuilder::new(Role::Window);
if let Some(name) = name {
root.set_name(name);
}
root.set_children(root_children);
let root = root.build(&mut NodeClassSet::lock_global());
let window_update = (NodeId(primary_window_id.to_bits()), root);
to_update.insert(0, window_update);
TreeUpdate {
nodes: to_update,
tree: None,
focus: NodeId(focus_id),
}
});
}
let Ok((primary_window_id, primary_window)) = primary_window.get_single() else {
return;
};
let Some(adapter) = adapters.get(&primary_window_id) else {
return;
};
if focus.is_changed() || !nodes.is_empty() {
adapter.update_if_active(|| {
update_adapter(
nodes,
node_entities,
primary_window,
primary_window_id,
focus,
)
});
}
}

fn update_adapter(
nodes: Query<(
Entity,
&AccessibilityNode,
Option<&Children>,
Option<&Parent>,
)>,
node_entities: Query<Entity, With<AccessibilityNode>>,
primary_window: &Window,
primary_window_id: Entity,
focus: Res<Focus>,
) -> TreeUpdate {
let mut to_update = vec![];
let mut window_children = vec![];
for (entity, node, children, parent) in &nodes {
let mut node = (**node).clone();
queue_node_for_update(entity, parent, &node_entities, &mut window_children);
add_children_nodes(children, &node_entities, &mut node);
let node_id = NodeId(entity.to_bits());
let node = node.build(&mut NodeClassSet::lock_global());
to_update.push((node_id, node));
}
let mut window_node = NodeBuilder::new(Role::Window);
if primary_window.focused {
let title = primary_window.title.clone();
window_node.set_name(title.into_boxed_str());
}
window_node.set_children(window_children);
let window_node = window_node.build(&mut NodeClassSet::lock_global());
let node_id = NodeId(primary_window_id.to_bits());
let window_update = (node_id, window_node);
to_update.insert(0, window_update);
TreeUpdate {
nodes: to_update,
tree: None,
focus: NodeId(focus.unwrap_or(primary_window_id).to_bits()),
}
}

#[inline]
fn queue_node_for_update(
node_entity: Entity,
parent: Option<&Parent>,
node_entities: &Query<Entity, With<AccessibilityNode>>,
window_children: &mut Vec<NodeId>,
) {
let should_push = if let Some(parent) = parent {
node_entities.contains(parent.get())
} else {
true
};
if should_push {
window_children.push(NodeId(node_entity.to_bits()));
}
}

#[inline]
fn add_children_nodes(
children: Option<&Children>,
node_entities: &Query<Entity, With<AccessibilityNode>>,
node: &mut NodeBuilder,
) {
let Some(children) = children else {
return;
};
for child in children {
if node_entities.contains(*child) {
node.push_child(NodeId(child.to_bits()));
}
}
}
Expand Down

0 comments on commit 47fa620

Please sign in to comment.