refactor and test `ui_focus_system` #4264

pubrrr · 2022-03-20T15:05:41Z

Objective

While digging through the bevy code, I found that ui_focus_system overly complex and quite hard to understand. -> make it easier to understand.

Solution

write unit tests
refactor
I tried to introduce no logical changes

pubrrr · 2022-03-20T17:58:13Z

Ok, apparently the ci fails:

 = rustc_version v0.2.3
  └── (build) stdweb v0.4.20
      └── gilrs-core v0.3.2
 = rustc_version v0.4.0
  └── (build) rstest v0.12.0
      └── (dev) bevy_ui v0.7.0-dev

It seems this will depend on koute/stdweb#418

alice-i-cecile · 2022-03-20T18:01:40Z

For the CI, the workaround is to whitelist the duplicate dependencies. That said, I'm reluctant to pull a unit testing framework into our dependency tree just for this.

What benefit does it offer here?

crates/bevy_ui/src/focus.rs

alice-i-cecile · 2022-03-20T18:19:56Z

crates/bevy_ui/src/focus.rs

-            *interaction = Interaction::None;
-        }
-    }
+    focus_ui(


I don't think it's useful to have a system whose contents are a single function call: that seems like needless layering.

This is to make it testable. Or do you have other suggestions how to mock Windows?

I.e. ui_focus_system is actually meant as a public export of focus_ui::<Windows>

Ah. I think we should try to tackle that directly: I don't want to have to replicate this strategy everywhere.

alice-i-cecile · 2022-03-20T18:20:31Z

crates/bevy_ui/src/focus.rs

+    touches_input: Res<Touches>,
+    mut node_query: Query<NodeQuery>,
+) {
+    reset_interactions(


I like this living in a separate system at least.

Should I extract it? This system currently does two things:

resetting Interactions to None

checking the input to set some Interactions to Hovered / Clicked

The coupling between those two things is quite hard as they rely on each other. This becomes quite obvious from the Local<State>. Actually I wanted to get rid of it, but I did not have a good idea without changing the logic.

alice-i-cecile · 2022-03-20T18:21:35Z

crates/bevy_ui/src/focus.rs


-    let mouse_clicked =
-        mouse_button_input.just_pressed(MouseButton::Left) || touches_input.just_released(0);
+    let cursor_position = match windows.get_cursor_position() {


I think you should be able to use the ? operator here.

The compiler says: the ? operator can only be used in a function that returns Result or Option (or another type that implements FromResidual)

Or do you mean I should return an Option here?

Ah my bad; I'd misread what the function does.

alice-i-cecile · 2022-03-20T18:23:04Z

crates/bevy_ui/src/focus.rs

+    )
+}
+
+fn contains_cursor(


This feels like it should be a method on Node or something to that effect?

I also thought that this function looks a bit odd as a standalone. But is Node the right place as Node is actually indifferent to positions?

Yeah 🤔 TBH, this feels like functionality that should be pub that we reuse internally, but I can't immediately figure out the best design for that.

alice-i-cecile · 2022-03-20T18:23:37Z

crates/bevy_ui/src/focus.rs

-                *interaction = Interaction::None;
+}
+
+trait CursorResource: Resource {


I really dislike the use of the generic plus trait here.

Ah I see: it's to work around the difficulty mocking Windows?

See my other comment - how else would I make Windows do what I want in a unit test? Everything I tried ended up with a dependency on hardware.

alice-i-cecile · 2022-03-20T18:24:30Z

crates/bevy_ui/src/focus.rs

+
+#[cfg(test)]
+mod tests {
+    use rstest::*;


I don't think that this adds enough value to pull in a testing library on a one-off basis. These tests should be feasibel to rewrite with a helper method instead.

I kinda handcoded it myself now

Excellent, thanks!

alice-i-cecile · 2022-03-20T18:25:58Z

crates/bevy_ui/src/focus.rs

+}
+
+#[cfg(test)]
+mod tests {


These tests look good, and we definitely need tests for this piece of code.

Is there actually a common practice how to test systems? This TestApp approach is something I came up with in one of my own projects from the Cheatbook suggesting Direct World Access. But I didn't see many tests around in bevy itself.

You can see how I did it in one of my external crates here: https://github.com/Leafwing-Studios/leafwing_input_manager/blob/main/tests/integration.rs

alice-i-cecile

Overall, I think that this region could use code quality improvements to break up the mega-function, and it definitely needs tests!

I pretty strongly dislike rstest here (I don't think it's clearer, and it pulls in a new dependency). The refactor for the code itself needs a bit more iteration, but it feels like breaking this out into smaller pieces is the right path.

alice-i-cecile · 2022-03-20T19:00:31Z

So, WRT the testing strategy: #3835 is the blocker here. IMO we should hold off on this until that's merged, then use the headless windows there to enable us to write proper tests for this code.

pubrrr · 2022-03-20T21:57:10Z

crates/bevy_ui/src/focus.rs

            }
+            *interaction = Interaction::Clicked;


I might need to be careful here concerning change detection. Double check.

crates/bevy_ui/Cargo.toml

nicopap · 2022-03-21T07:17:22Z

You might be interested in bevyengine/rfcs#41 and https://github.com/nicopap/ui-navigation. I'm planning to start the bevy PR next week (I still need to update the RFC with the second round of changes). I'm not sold on rtest either, to be perfectly honest, the test code was completely obtuse to me 😀. Looks great now though!

The ui-navigation crate is sorely lacking when it comes to tests. And unlike bevy's focus system, it's fairly well documented.

…y mutable object

…ewer spots: - reset everything that should be reset - find hovered nodes - write Hovered/Clicked states

pubrrr · 2022-03-23T21:20:25Z

I guess after adding a test that checks change detection, unfortunately most of my changes won't work.

At least there are a couple more tests. The "Option<Interaction> -> Interaction" change can be kept, and maybe a refactoring of cutting it into smaller functions. But the approach "set everything to None, then check the state" won't work. Or does anyone have an idea how to make it work?

Anyway, I'll leave this PR around. Ping me once headless windows are there. I believe the tests can easily be adapted to work without the extra trait then.

NthTensor · 2024-09-17T18:19:07Z

This looks like it has gone stale, and we are now planning to deprecate Interaction anyway. Closing as not planned.

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 20, 2022

pubrrr force-pushed the refactorFocus2 branch from 3c00ed9 to db78199 Compare March 20, 2022 15:25

alice-i-cecile added A-UI Graphical user interfaces, styles, layouts, and widgets C-Code-Quality A section of code that is hard to understand or change and removed S-Needs-Triage This issue needs to be labelled labels Mar 20, 2022

alice-i-cecile self-requested a review March 20, 2022 17:59

alice-i-cecile reviewed Mar 20, 2022

View reviewed changes

crates/bevy_ui/src/focus.rs Show resolved Hide resolved

alice-i-cecile reviewed Mar 20, 2022

View reviewed changes

alice-i-cecile requested changes Mar 20, 2022

View reviewed changes

pubrrr force-pushed the refactorFocus2 branch 2 times, most recently from 54d4d59 to 99a737e Compare March 20, 2022 21:47

pubrrr commented Mar 20, 2022

View reviewed changes

alice-i-cecile reviewed Mar 20, 2022

View reviewed changes

crates/bevy_ui/Cargo.toml Outdated Show resolved Hide resolved

document current behavior by unit tests

9ec11c6

pubrrr force-pushed the refactorFocus2 branch from 99a737e to dafaa04 Compare March 21, 2022 19:19

pubrrr added 5 commits March 23, 2022 22:08

add a test for change detection

a4e9c8f

the focus system does nothing if interaction is none as it is the onl…

af21aac

…y mutable object

simplify logic: if there is no cursor, then there is no interaction

cd22b81

refactor - remove now unnecessary logic, write interaction state in f…

7f6460e

…ewer spots: - reset everything that should be reset - find hovered nodes - write Hovered/Clicked states

refactor - extract some functions

c6704a8

pubrrr force-pushed the refactorFocus2 branch from dafaa04 to c6704a8 Compare March 23, 2022 21:14

alice-i-cecile mentioned this pull request Mar 7, 2024

Upstream and use bevy_mod_picking #12365

Closed

16 tasks

NthTensor closed this Sep 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor and test `ui_focus_system` #4264

refactor and test `ui_focus_system` #4264

pubrrr commented Mar 20, 2022

pubrrr commented Mar 20, 2022 •

edited

Loading

alice-i-cecile commented Mar 20, 2022

alice-i-cecile Mar 20, 2022

pubrrr Mar 20, 2022

alice-i-cecile Mar 20, 2022

alice-i-cecile Mar 20, 2022

pubrrr Mar 20, 2022

alice-i-cecile Mar 20, 2022

pubrrr Mar 20, 2022

alice-i-cecile Mar 20, 2022

alice-i-cecile Mar 20, 2022

pubrrr Mar 20, 2022

alice-i-cecile Mar 20, 2022

alice-i-cecile Mar 20, 2022

alice-i-cecile Mar 20, 2022

pubrrr Mar 20, 2022

alice-i-cecile Mar 20, 2022

pubrrr Mar 20, 2022

alice-i-cecile Mar 20, 2022

alice-i-cecile Mar 20, 2022

pubrrr Mar 20, 2022

alice-i-cecile Mar 20, 2022

alice-i-cecile left a comment

alice-i-cecile commented Mar 20, 2022

pubrrr Mar 20, 2022 •

edited

Loading

nicopap commented Mar 21, 2022

pubrrr commented Mar 23, 2022

NthTensor commented Sep 17, 2024

refactor and test ui_focus_system #4264

refactor and test ui_focus_system #4264

Conversation

pubrrr commented Mar 20, 2022

Objective

Solution

pubrrr commented Mar 20, 2022 • edited Loading

alice-i-cecile commented Mar 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Mar 20, 2022

pubrrr Mar 20, 2022 • edited Loading

Choose a reason for hiding this comment

nicopap commented Mar 21, 2022

pubrrr commented Mar 23, 2022

NthTensor commented Sep 17, 2024

refactor and test `ui_focus_system` #4264

refactor and test `ui_focus_system` #4264

pubrrr commented Mar 20, 2022 •

edited

Loading

pubrrr Mar 20, 2022 •

edited

Loading