Refactor function update_accessibility_nodes
#10911
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Nilirad
added
A-Windowing
|
Haven't taken a closer look yet (I will when I can) but this looks really good! I definitely agree that this is far more readable and takes good advantage of the tools Rust gives us for avoiding things like this. Hope it get's merged soon! |
alice-i-cecile
approved these changes
Dec 9, 2023
|
@JustAnotherCodemonkey when you get a chance, I'd appreciate your review on this :) With two approvals I can merge this in <3 |
|
@alice-i-cecile How do I do a review? I've never done this before and don't see a button. |
JustAnotherCodemonkey
approved these changes
Dec 9, 2023
Nilirad
added
the
S-Ready-For-Final-Review
|
a negation was missed in this refactor: #11206 |
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jan 3, 2024
# Objective - Since #10911, example `button` crashes when clicking the button ``` thread 'main' panicked at .cargo/registry/src/index.crates.io-6f17d22bba15001f/accesskit_consumer-0.16.1/src/tree.rs:139:9: assertion `left == right` failed left: 1 right: 0 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace Encountered a panic in system `bevy_winit::accessibility::update_accessibility_nodes`! Encountered a panic in system `bevy_app::main_schedule::Main::run_main`! ``` ## Solution - Re-add lost negation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
update_accessibility_nodesis 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.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:
if-letblocks tolet-elsestatements where appropriate to reduce rightward shiftupdate_adapterupdate_adapterinto new functionsqueue_node_for_updateandadd_children_nodesNote 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).