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

Cueing highlight/arrow can be on nonexistent chocolate bar #319

Closed
Tracked by #1121
Nancy-Salpepi opened this issue Jul 10, 2024 · 6 comments
Closed
Tracked by #1121

Cueing highlight/arrow can be on nonexistent chocolate bar #319

Nancy-Salpepi opened this issue Jul 10, 2024 · 6 comments
Labels

Comments

@Nancy-Salpepi
Copy link

Nancy-Salpepi commented Jul 10, 2024

Test device
MacBook Air M1 chip and Dell

Operating System
macOS14.5 and Win10

Browser
Safari and Chrome

Problem description
For phetsims/qa#1105, in the State Wrapper on the Distribute Screen it is possible to have the cueing arrow/highlight present on a nonexistent chocolate bar in the downstream sim.

Steps to reproduce

  1. In the State Wrapper set state rate = 0
  2. Go to the Distribute Screen
  3. Increase the number of bars for person 2 (e.g. to 7)
  4. In the notepad area, move some of those bars to person one's plate so their stack is higher and the cueing arrow is on that stack now.
  5. Press Set State Now
  6. Press Sync button in downstream sim

Visuals

cueing.highlight.arrow.mp4
@Nancy-Salpepi Nancy-Salpepi added the type:bug Something isn't working label Jul 10, 2024
@marlitas
Copy link
Contributor

Great catch! The stackChangedEmitter was not emitting in state setting because it was being skipped in the tableSnackNumberProperty link. I switched the emitter to fire from when snacks are added or removed from each plate's observable array. This will fire more often, but also is more accurate with our code philosophy in this sim where the observable arrays are the ground truth and engine for the logic of the sim.

This is ready for @Nancy-Salpepi's review. Feel free to close if all looks well!

@Nancy-Salpepi
Copy link
Author

Looks great on main! Closing.

@jbphet
Copy link
Contributor

jbphet commented Jul 22, 2024

Reopening - the firing of stackChangedEmitter on the removal of candy bars from plates caused some regressions that went initially unnoticed, but are pretty significant. @marlitas and I have been working through them today.

@jbphet
Copy link
Contributor

jbphet commented Jul 22, 2024

@marlitas and I worked on this together for a few hours, and we think we've got it worked out. I've committed the changes, and am assigning to @marlitas for a bit more testing and review.

@jbphet jbphet assigned marlitas and unassigned Nancy-Salpepi Jul 22, 2024
marlitas added a commit that referenced this issue Jul 22, 2024
@marlitas
Copy link
Contributor

When looking at this together @jbphet and I talked about being consistent with cue handling in our sim specific code, however the keyboard sort cue visibility is managed through a DerivedProperty in the GroupSortInteractionModel. I don't think it would be wise to mess with that. This just led to one minor documentation change committed above.

I did some additional state wrapper testing and I believe this issue is now ready for RC test.

@Nancy-Salpepi
Copy link
Author

This looks fixed in rc.1.
Closing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants