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

Problems on Reset #172

Closed
samreid opened this issue May 4, 2023 · 10 comments
Closed

Problems on Reset #172

samreid opened this issue May 4, 2023 · 10 comments
Labels
dev:help-wanted Extra attention is needed priority:2-high

Comments

@samreid
Copy link
Member

samreid commented May 4, 2023

Sim can get frozen.

card arrow can show over nothingness

image

@samreid samreid self-assigned this May 5, 2023
@samreid
Copy link
Member Author

samreid commented May 6, 2023

One time after resetting there was a leftover card in the card panel

image

samreid added a commit that referenced this issue May 6, 2023
@samreid
Copy link
Member Author

samreid commented May 6, 2023

Something like this may help with that unresetted card:

Subject: [PATCH] Clear card cells on inactive, see https://github.com/phetsims/center-and-variability/issues/172
---
Index: js/median/view/CardNodeContainer.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/median/view/CardNodeContainer.ts b/js/median/view/CardNodeContainer.ts
--- a/js/median/view/CardNodeContainer.ts	(revision 6e8d554607922202e5631e08fba478dc76a69a6b)
+++ b/js/median/view/CardNodeContainer.ts	(date 1683383199141)
@@ -207,8 +207,22 @@
         }
         else if ( !isActive ) {
           const index = this.cardNodeCells.indexOf( cardNode );
-          this.cardNodeCells.splice( index, 1 );
-          this.cardNodeCellsChangedEmitter.emit();
+
+          if ( index >= 0 ) {
+            this.cardNodeCells.splice( index, 1 );
+            this.cardNodeCellsChangedEmitter.emit();
+          }
+        }
+      } );
+
+      cardModel.soccerBall.valueProperty.link( value => {
+        if ( value === null ) {
+          const index = this.cardNodeCells.indexOf( cardNode );
+
+          if ( index >= 0 ) {
+            this.cardNodeCells.splice( index, 1 );
+            this.cardNodeCellsChangedEmitter.emit();
+          }
         }
       } );
 

samreid added a commit that referenced this issue May 6, 2023
@samreid
Copy link
Member Author

samreid commented May 6, 2023

Fixed in the commit, with one TODO for discussion.

@samreid samreid removed their assignment May 6, 2023
@samreid samreid added the dev:help-wanted Extra attention is needed label May 6, 2023
@samreid
Copy link
Member Author

samreid commented May 8, 2023

Can't test this until #189 is fixed. So perhaps I'll increase its priority.

@samreid
Copy link
Member Author

samreid commented May 9, 2023

Reset seems pretty nice in my testing. @marlitas or @matthew-blackman want to review/test?

@samreid samreid added status:ready-for-review and removed dev:help-wanted Extra attention is needed labels May 9, 2023
@marlitas
Copy link
Contributor

Played around with some different scenarios and I am not seeing any remaining issues related to reset functionality. Commits are small, and seem appropriate. No concerns here. Ready to close if @samreid is.

@marlitas marlitas assigned samreid and unassigned marlitas and matthew-blackman May 10, 2023
@samreid
Copy link
Member Author

samreid commented May 10, 2023

Thanks! Closing.

@samreid samreid closed this as completed May 10, 2023
@phet-dev phet-dev reopened this May 11, 2023
@phet-dev
Copy link
Contributor

Reopening because there is a TODO marked for this issue.

@samreid
Copy link
Member Author

samreid commented May 11, 2023

I could use help on the remaining TODO.

@samreid samreid removed their assignment May 11, 2023
@samreid samreid added dev:help-wanted Extra attention is needed and removed status:ready-for-review labels May 11, 2023
@matthew-blackman
Copy link
Contributor

@samreid and I tested the behavior without the dragPositionProperty being reset, and found no issues. The valueProperty of the soccerBall is now being reset correctly. We feel that this issue can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev:help-wanted Extra attention is needed priority:2-high
Projects
None yet
Development

No branches or pull requests

4 participants