Code Review GroupSortInteraction

[zepumph]

This new interaction was implemented in https://github.com/phetsims/scenery-phet/issues/815.

From discussions, it is going to be best to have @jbphet @marlitas code review this work as part of their use of it in Mean Share and Balance over in https://github.com/phetsims/mean-share-and-balance/issues/137. I recommend that in addition to using it, they are able to take a look at the common code here and let me know what improvements they recommend.

Files to review:

• GroupSortInteractionModel
• GroupSortInteractionView
• To a lesser extent:SoccerCommonGroupSortInteractionModel and SoccerCommonGroupSortInteractionView and CAVGroupSortInteractionModel

General questions:

• I'd appreciate updates or comments about the overall documentation and implementation steps
• Do Property and method names make sense?
• There are a few TODOs pointing to https://github.com/phetsims/mean-share-and-balance/issues/137 that may be more related to this issue.

Thanks so much, and let me know if you have questions.

[marlitas]

I read through both files one last time and just had one question come up.

It is strange to me that showMouseCueProperty is part of a derived Property called hasGroupItemBeenSortedProperty. If I remove my knowledge of the code I would not understand how a clients setting to show mouse cue or not determines wether a group item has been sorted or not. However, as someone who is familiar with the code I also know that hasGroupItemBeenSortedProperty drives the cueing visibility. Perhaps this can be addressed via a code comment or name change?

this.hasGroupItemBeenSortedProperty = new DerivedProperty( [
      this.hasMouseSortedGroupItemProperty,
      this.hasKeyboardSortedGroupItemProperty,
      this.showMouseCueProperty
    ], ( hasMouseSortedGroupItem, hasKeyboardSortedGroupItem, showMouseCue ) => {
      return hasMouseSortedGroupItem || hasKeyboardSortedGroupItem || !showMouseCue;
    } );

[zepumph]

Ok. @marlitas and I discussed the above topic, and found 4 other improvements. I tried to keep separate commits so that logic is very clean when it comes to cherry picking. Sorry though if that means more cherry picking!! Please review the above, test MSAB (better than I can), let me know if you think there is anything else, and proceed with cherrypick how you see fit. Thanks!

[zepumph]

• [x] Also please note how we added another Property check in https://github.com/phetsims/soccer-common/commit/a1bc6b891f2c3d75cfbc42069c06c6b49c7f6417, we don't think that this means we should add the hasKeyboardGrabbedGroupItemProperty to the multilink dependencies, but it would be good to double check and test this in MSaB while reviewing this issue.

[marlitas]

I reviewed the above commits and also tested in MSaB. It looks good!

[marlitas]

I cherry picked the above.

QA please verify that the showMouseCueProperty is still working as expected with PhET-iO.

[Nancy-Salpepi]

@marlitas showMouseCueProperty is working well on the Balance Point screen. For the Distribute screen, odd behavior occurs in studio, but once I launch the sim things work correctly in the Standard PhET-iO wrapper.

Steps:

1. In Studio, set showMouseCueProperty to false for the Distribute Screen.
2. Press the Reset All button--cueing arrow will reappear in studio
3. Press Test -- cueing arrow is absent 🎉

It looks really odd in studio if before step 1, you increased the number of plates.

[marlitas]

Okay great. Thanks @Nancy-Salpepi. I'm going to investigate to see if this is a sim specific bug, or is related to our recent changes in Group Sort.

[marlitas]

This was a groupSort bug. We were resetting the mouseSortCueVisibleProperty without respecting the showMouseCueProperty. @zepumph fix applied above. Over to you for review, but this is also ready for cherry pick.

[marlitas]

QA, please keep this issue open for @zepumph to finish code review.

[Nancy-Salpepi]

This looks fixed in MSaB rc.4. Keeping open for code review.

[zepumph]

https://github.com/phetsims/scenery-phet/commit/0b0527fc6e01e949fa67392c1cea3c77aca67b39 seems good and reasonably, but would you prefer to just set the value to this.mouseSortCueShouldBeVisible() which ensures that reset holds the correct value generally without duplicating the logic?

Subject: [PATCH] renames and remove query parameter, https://github.com/phetsims/density-buoyancy-common/issues/132
---
Index: js/accessibility/group-sort/model/GroupSortInteractionModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/accessibility/group-sort/model/GroupSortInteractionModel.ts b/js/accessibility/group-sort/model/GroupSortInteractionModel.ts
--- a/js/accessibility/group-sort/model/GroupSortInteractionModel.ts    (revision 0b0527fc6e01e949fa67392c1cea3c77aca67b39)
+++ b/js/accessibility/group-sort/model/GroupSortInteractionModel.ts    (date 1724280886430)
@@ -239,9 +239,7 @@

     // If a PhET-iO client has set showMouseCueProperty to false, then the mouseSortCueVisibleProperty
     // needs to respect that.
-    if ( !this.showMouseCueProperty.value ) {
-      this.mouseSortCueVisibleProperty.value = false;
-    }
+    this.mouseSortCueVisibleProperty.value = this.mouseSortCueShouldBeVisible();
   }

   // Clear the selection state for the interaction (setting to null)

[marlitas]

Yeah, that's way better. Thanks for the clean up. Committed above. I think we can close!