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

egui_kittest: Allow passing state to the app closure #5313

Merged
merged 60 commits into from
Nov 6, 2024

Conversation

lucasmerlin
Copy link
Collaborator

The allows us to pass any state to the ui closure. While it is possible to just store state in the closure itself, accessing that state after the harness was created to e.g. read or modify it would require interior mutability. With this change there are new Harness::new_state, Harness::run_state, ... methods that allow passing state on each run.

This builds on top of #5301, which should be merged first

# Conflicts:
#	Cargo.lock
#	crates/egui_demo_lib/src/demo/widget_gallery.rs
#	crates/egui_demo_lib/tests/snapshots/widget_gallery.png
#	crates/egui_kittest/Cargo.toml
#	crates/egui_kittest/README.md
#	crates/egui_kittest/src/builder.rs
#	crates/egui_kittest/src/lib.rs
#	crates/egui_kittest/src/snapshot.rs
#	crates/egui_kittest/src/wgpu.rs
Copy link

Preview available at https://egui-pr-preview.github.io/pr/5313-lucas/kittest_state
Note that it might take a couple seconds for the update to show up after the preview_build workflow has completed.

@lucasmerlin
Copy link
Collaborator Author

I just realized that it would also possible to store the generic state in the Harness, making it accessible via a state_mut function (or just making it public on the Harness struct). That would allow us to get rid of all the extra run_state, step_state etc.. methods. Which way would you prefer @emilk?

@emilk
Copy link
Owner

emilk commented Oct 28, 2024

So we want to have mutable state that is both easily accessible from the ui closure, but that can also be inspected at the end of the test so we can check that the state ended up in the correct, well, state. 🤔

I agree that forcing users to use Rc<RefCell<State>> is too un-ergonomic.

I also agree that the run_state solution proposed in the PR is a bit ugly.

Putting the state in the harness does seem more attractive. We could make Harness generic over state, though that would lead to monomorphization of the whole Harness, probably increasing compile-times. Maybe that's fine, but we could probably work around it with clever use of Any and dynamic-casting, wrapped in ergonomic accessors 🤔 But maybe not worth the effort

@lucasmerlin
Copy link
Collaborator Author

I've updated it so the harness owns the state. I think this is a bit nicer, but I think tests using this are now a bit less readable.
assert_eq!(checked, true) now becomes assert_eq!(*harness.state(), true), which is less obvious and requires the reader to check somewhere else for the meaning of the state.

But still, I think the benefits outweigh the disadvantage here. Also, if you only need to check the state at the end of the test, and not somewhere in between, one could also just reference it in the closure and then drop() the harness and then the state can be accessed again to do a final assert

lucasmerlin and others added 3 commits October 29, 2024 21:40
The check.sh script was broken in #5166, this fixes it
* Related to #5297 
* [x] I have followed the instructions in the PR template

---------

Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
# Conflicts:
#	crates/egui_kittest/src/app_kind.rs
#	crates/egui_kittest/src/builder.rs
#	crates/egui_kittest/src/lib.rs
@lucasmerlin lucasmerlin mentioned this pull request Nov 1, 2024
@lucasmerlin
Copy link
Collaborator Author

Somehow the spell checker suddenly finds some old typos not introduced in this PR 🤔 I've fixed them in #5339

@lucasmerlin lucasmerlin requested a review from emilk November 1, 2024 18:11
emilk pushed a commit that referenced this pull request Nov 4, 2024
The spell check pipeline in #5313 suddenly failed, this fixes these
typos and some more found via my IDEs spell checker tool
crates/egui_kittest/src/lib.rs Outdated Show resolved Hide resolved
crates/egui_kittest/src/lib.rs Outdated Show resolved Hide resolved
@lucasmerlin lucasmerlin merged commit 3c7ad0e into master Nov 6, 2024
46 checks passed
@lucasmerlin lucasmerlin deleted the lucas/kittest_state branch November 6, 2024 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants