From 4e9bc5431bdbaa6df317fc7c827a6a0766934584 Mon Sep 17 00:00:00 2001 From: Marla Schulz Date: Thu, 6 Jun 2024 09:37:12 -0700 Subject: [PATCH] Self code review for distribute directory, see: https://github.com/phetsims/mean-share-and-balance/issues/258 --- js/distribute/model/CandyBar.ts | 4 +++- js/distribute/model/DistributeModel.ts | 28 +++++++++++----------- js/distribute/view/DistributeScreenView.ts | 17 ++++++++----- js/distribute/view/NotepadCandyBarNode.ts | 6 ++--- 4 files changed, 31 insertions(+), 24 deletions(-) diff --git a/js/distribute/model/CandyBar.ts b/js/distribute/model/CandyBar.ts index da90014c..aa12347d 100644 --- a/js/distribute/model/CandyBar.ts +++ b/js/distribute/model/CandyBar.ts @@ -2,7 +2,9 @@ /** * Individual candy bars in the notepad snackType. - * These candy bars are draggable therefore their position and parentPlate are important. + * These candy bars are draggable. + * + * TODO: JB, do we just need this for clarity? It doesn't seem to add any functionality. https://github.com/phetsims/mean-share-and-balance/issues/258 * * @author Marla Schulz (PhET Interactive Simulations) * @author Sam Reid (PhET Interactive Simulations) diff --git a/js/distribute/model/DistributeModel.ts b/js/distribute/model/DistributeModel.ts index 63297f5f..7ab9b2a6 100644 --- a/js/distribute/model/DistributeModel.ts +++ b/js/distribute/model/DistributeModel.ts @@ -1,8 +1,7 @@ // Copyright 2022-2024, University of Colorado Boulder /** - * Model for the Distribute Screen which includes people, candy bars, visual mean snackType, and a numerical - * mean snackType. + * Model for the Distribute Screen which includes people, candy bars, and mean prediction. * * @author Marla Schulz (PhET Interactive Simulations) * @author Sam Reid (PhET Interactive Simulations) @@ -52,8 +51,6 @@ export default class DistributeModel extends SharingModel { public readonly meanPredictionProperty: Property; public readonly predictMeanDragRange = new Range( 0, MeanShareAndBalanceConstants.MAX_NUMBER_OF_SNACKS_PER_PLATE ); - public readonly meanWithRemainderProperty: TReadOnlyProperty; - // Tracks whether the snacks are distributed evenly across the plates or at least distributed as much as is possible // with the data provided. public readonly snacksDistributedProperty: TReadOnlyProperty; @@ -66,6 +63,11 @@ export default class DistributeModel extends SharingModel { // phet-io specific Properties public readonly successIndicatorsOperatingProperty: Property; + // TODO: This is does not need to be a class member, but I'm not sure how to get around linting errors since + // it is needed for PhET-iO studio. https://github.com/phetsims/mean-share-and-balance/issues/258 + public readonly meanWithRemainderProperty: TReadOnlyProperty; + + public constructor( providedOptions?: DistributeModelOptions ) { const createCandyBar = ( options: SnackOptions ) => new CandyBar( options ); @@ -94,7 +96,6 @@ export default class DistributeModel extends SharingModel { this.predictMeanVisibleProperty = new BooleanProperty( false, { tandem: options.tandem.createTandem( 'predictMeanVisibleProperty' ) } ); - this.meanPredictionProperty = new NumberProperty( 0, { range: this.predictMeanDragRange, tandem: options.tandem.createTandem( 'meanPredictionProperty' ) @@ -124,7 +125,6 @@ export default class DistributeModel extends SharingModel { // Initialize the plates and set up plate-related behavior that is specific to the Distribute screen. this.plates.forEach( plate => { - plate.snacksOnNotepadPlate.addItemAddedListener( snack => { snack.isActiveProperty.value = true; const index = plate.snacksOnNotepadPlate.indexOf( snack ); @@ -165,9 +165,7 @@ export default class DistributeModel extends SharingModel { // Remove notepad snacks from this plate, or from another if this plate is empty. _.times( Math.abs( delta ), () => { - - // REVIEW: We should probably be checking the min number of snacks on a plate, not hard coding 0. - if ( plate.getNumberOfNotepadSnacks() > 0 ) { + if ( plate.getNumberOfNotepadSnacks() > MeanShareAndBalanceConstants.MIN_NUMBER_OF_SNACKS_PER_PLATE ) { plate.removeTopSnack(); } else { @@ -177,19 +175,16 @@ export default class DistributeModel extends SharingModel { } } ); } - this.stackChangedEmitter.emit(); } ); // Monitor the isActiveProperty for each plate and do any redistribution of candy bars that is necessary when // changes occur. plate.isActiveProperty.link( isActive => { - if ( !isActive ) { // Handle the situation where this plate went inactive and had excess candy bars on it. The inverse // situation, i.e. when goes inactive with a deficit, is handled elsewhere. - // REVIEW QUESTION: Where is that handled? const excess = Math.max( plate.getNumberOfNotepadSnacks() - plate.tableSnackNumberProperty.value, 0 ); if ( excess > 0 ) { @@ -198,7 +193,9 @@ export default class DistributeModel extends SharingModel { plate.removeTopSnack(); const plateWithFewestSnacks = this.getPlateWithFewestSnacks(); - // REVIEW: Should we confirm that there are indeed plates with space before adding a snack? + assert && assert( plateWithFewestSnacks && + plateWithFewestSnacks.snacksOnNotepadPlate.length < MeanShareAndBalanceConstants.MAX_NUMBER_OF_SNACKS_PER_PLATE, + 'A plate should have space to add another snack.' ); plateWithFewestSnacks!.addASnack(); } ); } @@ -210,6 +207,8 @@ export default class DistributeModel extends SharingModel { } ); } ); + // Track whether the snacks are distributed evenly across the plates or at least distributed as much as is possible + // to trigger success indicators for the predict mean tool. const activePlateDependencies = this.plates.map( plate => plate.isActiveProperty ); const notepadPlateSnackCountDependencies = this.plates.map( plate => plate.snacksOnNotepadPlate.lengthProperty ); this.snacksDistributedProperty = DerivedProperty.deriveAny( [ @@ -227,6 +226,7 @@ export default class DistributeModel extends SharingModel { phetioValueType: BooleanIO } ); + // For phet-io client use only. this.meanWithRemainderProperty = new DerivedProperty( [ this.numberOfPlatesProperty, this.totalSnacksProperty @@ -241,7 +241,7 @@ export default class DistributeModel extends SharingModel { phetioValueType: ObjectLiteralIO } ); - // For phet-io client use only. + this.successIndicatorsOperatingProperty = new BooleanProperty( true, { tandem: options.tandem.createTandem( 'successIndicatorsOperatingProperty' ) } ); diff --git a/js/distribute/view/DistributeScreenView.ts b/js/distribute/view/DistributeScreenView.ts index f309dddf..04502436 100644 --- a/js/distribute/view/DistributeScreenView.ts +++ b/js/distribute/view/DistributeScreenView.ts @@ -2,7 +2,7 @@ /** * Representation for the Distribute Screen. Contains a table with people, each of whom have a plate with candy bars - * on them. It also includes a notepad that also show plates and candy bars that can be dragged and 'leveled out'. + * on them. It also includes a notepad that also show plates and candy bars that can be dragged and 'leveled out'. * * @author Marla Schulz (PhET Interactive Simulations) * @author Sam Reid (PhET Interactive Simulations) @@ -142,6 +142,7 @@ export default class DistributeScreenView extends SharingScreenView { model.stackChangedEmitter.addListener( this.updateMouseSortCueNode.bind( this ) ); model.stackChangedEmitter.addListener( this.updateKeyboardSortCueNode.bind( this ) ); + // Create the notepad plates and candy bars. const notepadPlateNodes = model.plates.map( plate => { plate.xPositionProperty.link( this.updateMouseSortCueNode.bind( this ) ); plate.xPositionProperty.link( this.updateKeyboardSortCueNode.bind( this ) ); @@ -163,7 +164,7 @@ export default class DistributeScreenView extends SharingScreenView { } ) ); - const notepadCandyBarsNode = new InteractiveHighlightingNode( { + const notepadCandyBarsHighlightNode = new InteractiveHighlightingNode( { focusable: true, tagName: 'div', children: notepadCandyBarNodes, @@ -171,7 +172,7 @@ export default class DistributeScreenView extends SharingScreenView { } ); this.notepadSnackLayerNode.addChild( this.cueingHighlight ); - this.notepadSnackLayerNode.addChild( notepadCandyBarsNode ); + this.notepadSnackLayerNode.addChild( notepadCandyBarsHighlightNode ); this.notepadSnackLayerNode.addChild( mouseSortCueNode ); this.notepadSnackLayerNode.addChild( this.keyboardSortCueNode ); @@ -258,12 +259,11 @@ export default class DistributeScreenView extends SharingScreenView { lastPlate.xPositionProperty.value + MeanShareAndBalanceConstants.NOTEPAD_PLATE_DIMENSION.width / 2 ); } ); - this.notepadSnackLayerNode.addChild( meanPredictionSlider ); this.groupSortInteractionView = new GroupSortInteractionView( model.groupSortInteractionModel, - notepadCandyBarsNode, + notepadCandyBarsHighlightNode, { getNextSelectedGroupItem: ( delta, candyBar ) => { const platesWithSnacks = model.getPlatesWithSnacks(); @@ -331,7 +331,9 @@ export default class DistributeScreenView extends SharingScreenView { } ); } - // Handle a candy bar being dropped in the notepad. + /** + * Handle a candy bar being dropped in the notepad by mouse or touch. + */ private candyBarDropped( candyBarNode: NotepadCandyBarNode ): void { const candyBar = candyBarNode.candyBar; @@ -416,6 +418,9 @@ export default class DistributeScreenView extends SharingScreenView { } } + /** + * Update the visibility and position of the keyboard sort cue node based on the model's state. + */ private updateKeyboardSortCueNode(): void { const selectedCandyBar = this.groupSortInteractionModel.selectedGroupItemProperty.value; if ( !this.groupSortInteractionModel.hasGroupItemBeenSortedProperty.value && selectedCandyBar ) { diff --git a/js/distribute/view/NotepadCandyBarNode.ts b/js/distribute/view/NotepadCandyBarNode.ts index 8b77539d..efbdda04 100644 --- a/js/distribute/view/NotepadCandyBarNode.ts +++ b/js/distribute/view/NotepadCandyBarNode.ts @@ -129,10 +129,10 @@ export default class NotepadCandyBarNode extends InteractiveHighlighting( Node ) /** * A pattern is used for the outline of the candy bar. Because of this, the pattern must be rotated and translated to * match the bounds of the rectangle. This method returns the nodes that make up the outline of the candy bar. - * - * When creating partial candy bars the width and rightYTranslation may need to adjust accordingly. */ - public static getSketchOutline( candyBarWidth = MeanShareAndBalanceConstants.CANDY_BAR_WIDTH, rightYTranslation = 0.975 ): Node[ ] { + public static getSketchOutline( candyBarWidth = MeanShareAndBalanceConstants.CANDY_BAR_WIDTH ): Node[ ] { + + const rightYTranslation = 0.975; const horizontalStrokePattern = new Pattern( graphiteTexture_png ).setTransformMatrix( Matrix3.affine( 0.15, 0, 0, 0, 0.15, 0.9 ) );