From a545c57f362b45f3608a9f779e895aaca913d7f6 Mon Sep 17 00:00:00 2001 From: zepumph Date: Wed, 13 Apr 2022 21:27:50 -0600 Subject: [PATCH] Both hands context responses to respect DistanceResponseType, add inProportionOverridesDistanceProgress and so that both hands still get distance progress, https://github.com/phetsims/ratio-and-proportion/issues/459 https://github.com/phetsims/ratio-and-proportion/issues/432 --- js/common/view/RatioHalf.ts | 6 +- .../view/describers/BothHandsDescriber.ts | 11 ++- .../view/describers/HandPositionsDescriber.ts | 95 ++++++++++++++----- 3 files changed, 82 insertions(+), 30 deletions(-) diff --git a/js/common/view/RatioHalf.ts b/js/common/view/RatioHalf.ts index ec5ec55b..b9f6f0d3 100644 --- a/js/common/view/RatioHalf.ts +++ b/js/common/view/RatioHalf.ts @@ -29,7 +29,7 @@ import RatioHandNode from './RatioHandNode.js'; import ViewSounds from './sound/ViewSounds.js'; import TickMarkView from './TickMarkView.js'; import BothHandsDescriber from './describers/BothHandsDescriber.js'; -import HandPositionsDescriber, { SingleHandContextResponseOptions } from './describers/HandPositionsDescriber.js'; +import HandPositionsDescriber, { HandContextResponseOptions } from './describers/HandPositionsDescriber.js'; import RAPRatioTuple from '../model/RAPRatioTuple.js'; import CueArrowsState from './CueArrowsState.js'; import RatioDescriber from './describers/RatioDescriber.js'; @@ -496,11 +496,11 @@ class RatioHalf extends Rectangle { */ getSingleHandContextResponse( handPositionsDescriber: HandPositionsDescriber, bothHandsDescriber: BothHandsDescriber, - options?: SingleHandContextResponseOptions ): string { + options?: HandContextResponseOptions ): string { // When locked, give a description of both-hands, instead of just a single one. if ( this.ratio.lockedProperty.value ) { - return bothHandsDescriber.getBothHandsContextResponse(); + return bothHandsDescriber.getBothHandsContextResponse( options ); } return handPositionsDescriber.getSingleHandContextResponse( this.ratioTerm, this.tickMarkViewProperty.value, options ); diff --git a/js/common/view/describers/BothHandsDescriber.ts b/js/common/view/describers/BothHandsDescriber.ts index f3c61c28..8e893c9c 100644 --- a/js/common/view/describers/BothHandsDescriber.ts +++ b/js/common/view/describers/BothHandsDescriber.ts @@ -14,11 +14,12 @@ import Property from '../../../../../axon/js/Property.js'; import RAPRatioTuple from '../../model/RAPRatioTuple.js'; import Range from '../../../../../dot/js/Range.js'; import RatioDescriber from './RatioDescriber.js'; -import HandPositionsDescriber from './HandPositionsDescriber.js'; +import HandPositionsDescriber, { HandContextResponseOptions } from './HandPositionsDescriber.js'; import TickMarkView from '../TickMarkView.js'; import EnumerationProperty from '../../../../../axon/js/EnumerationProperty.js'; import IReadOnlyProperty from '../../../../../axon/js/IReadOnlyProperty.js'; import TickMarkDescriber from './TickMarkDescriber.js'; +import DistanceResponseType from './DistanceResponseType.js'; const ratioDistancePositionContextResponsePatternString = ratioAndProportionStrings.a11y.ratio.distancePositionContextResponse; @@ -50,7 +51,7 @@ class BothHandsDescriber { this.previousConsequentAtExtremity = false; } - getBothHandsContextResponse(): string { + getBothHandsContextResponse( options?: HandContextResponseOptions ): string { // only applicable if the ratio is locked const ratioLockedEdgeResponse = this.getRatioLockedEdgeCaseContextResponse(); @@ -59,7 +60,7 @@ class BothHandsDescriber { } return StringUtils.fillIn( ratioDistancePositionContextResponsePatternString, { - distance: this.handPositionsDescriber.getBothHandsDistance( true, true ), + distance: this.handPositionsDescriber.getBothHandsDistance( true, options ), position: this.getBothHandsPosition() } ); } @@ -69,7 +70,9 @@ class BothHandsDescriber { */ getBothHandsDynamicDescription(): string { return StringUtils.fillIn( ratioDistancePositionContextResponsePatternString, { - distance: this.handPositionsDescriber.getBothHandsDistance( false, true ), + distance: this.handPositionsDescriber.getBothHandsDistance( true, { + distanceResponseType: DistanceResponseType.DISTANCE_REGION + } ), position: this.getBothHandsPosition() } ); } diff --git a/js/common/view/describers/HandPositionsDescriber.ts b/js/common/view/describers/HandPositionsDescriber.ts index 674ad4a4..106cbc58 100644 --- a/js/common/view/describers/HandPositionsDescriber.ts +++ b/js/common/view/describers/HandPositionsDescriber.ts @@ -4,6 +4,13 @@ * Description for the positions of each hand, as well as their positional relationship like the distance between each * hand, and if they have gotten closer to or farther from each other ("distance progress"). * + * In general, responses are split into implementaiton based on the DistanceResponseType. Whether it be distance + * region (qualitative regions), distance progress (closer/farther), or a combo algorithm to default to distance region, + * but you distance progress to prevent repeating the same region many times. + * + * `getSingleHandContextResponse` and `getBothHandsDistance` use a similar algorithm to accomplish these variations, but + * could not be factored out completely due to natural language requirements. + * * @author Michael Kauzmann (PhET Interactive Simulations) */ @@ -100,6 +107,9 @@ const POSITION_REGIONS_DATA: PositionRegionsData[] = [ ]; type GetDistanceProgressStringOptions = { + + // set to false if it is desired to get distanceProgress even when in proportion + inProportionOverridesDistanceProgress?: boolean; closerString?: string; fartherString?: string; }; @@ -282,7 +292,6 @@ class HandPositionsDescriber { * distance regions. */ getSingleHandComboDistance( ratioTerm: RatioTerm ): string { - const otherHand = ratioTerm === RatioTerm.ANTECEDENT ? rightHandLowerString : leftHandLowerString; const distanceRegion = this.getDistanceRegion( false ); @@ -305,29 +314,44 @@ class HandPositionsDescriber { return distanceRegion; } - getBothHandsDistance( overrideWithDistanceProgress = false, capitalized = false ): string { - const distanceRegion = this.getDistanceRegion( true ); + // TODO: capitalized is currently always used, but it would be nice to improve the implementation for voicing context responses, https://github.com/phetsims/ratio-and-proportion/issues/461 + getBothHandsDistance( capitalized: boolean, providedOptions?: HandContextResponseOptions ): string { + const options = optionize( { - if ( overrideWithDistanceProgress ) { - if ( distanceRegion === this.previousDistanceRegionBoth ) { - assert && assert( capitalized, 'overriding with distance-progress not supported for capitalized strings' ); + // By default, let the describer decide if we should have distance progress or region + distanceResponseType: DistanceResponseType.COMBO + }, providedOptions ); - const distanceProgressPhrase = this.getDistanceProgressString( { - closerString: ratioAndProportionStrings.a11y.handPosition.closerTogether, - fartherString: ratioAndProportionStrings.a11y.handPosition.fartherApart - } ); - if ( distanceProgressPhrase ) { - const distanceProgressDescription = StringUtils.fillIn( ratioAndProportionStrings.a11y.bothHands.handsDistanceProgressPattern, { - distanceProgress: distanceProgressPhrase - } ); - - // Count closer/farther as a previous so that we don't ever get two of them at the same time - this.previousDistanceRegionBoth = distanceProgressDescription; - return distanceProgressDescription; - } - } - this.previousDistanceRegionBoth = distanceRegion; + switch( options.distanceResponseType ) { + case DistanceResponseType.COMBO: + return this.getBothHandsComboDistance( capitalized ); + case DistanceResponseType.DISTANCE_PROGRESS: + return this.getBothHandsDistanceProgress( capitalized ); + case DistanceResponseType.DISTANCE_REGION: + return this.getBothHandsDistanceRegion( capitalized ); + default: + assert && assert( false, 'This is not how enums work' ); + } + assert && assert( false, 'We should always have a distance case above' ); + return 'A serious logic error occurred'; + } + + getBothHandsDistanceProgress( capitalized: boolean ): string { + const distanceProgressPhrase = this.getDistanceProgressString( { + inProportionOverridesDistanceProgress: false, + closerString: ratioAndProportionStrings.a11y.handPosition.closerTogether, + fartherString: ratioAndProportionStrings.a11y.handPosition.fartherApart + } ); + if ( distanceProgressPhrase ) { + return StringUtils.fillIn( ratioAndProportionStrings.a11y.bothHands.handsDistanceProgressPattern, { + distanceProgress: distanceProgressPhrase + } ); } + return this.getBothHandsDistanceRegion( capitalized ); + } + + getBothHandsDistanceRegion( capitalized: boolean ): string { + const distanceRegion = this.getDistanceRegion( true ); const pattern = capitalized ? ratioAndProportionStrings.a11y.bothHands.handsDistancePatternCapitalized : ratioAndProportionStrings.a11y.bothHands.handsDistancePattern; @@ -335,16 +359,41 @@ class HandPositionsDescriber { return StringUtils.fillIn( pattern, { distance: distanceRegion } ); } + getBothHandsComboDistance( capitalized = false ): string { + const distanceRegion = this.getDistanceRegion( true ); + + if ( distanceRegion === this.previousDistanceRegionBoth ) { + assert && assert( capitalized, 'overriding with distance-progress not supported for capitalized strings' ); + + const distanceProgressPhrase = this.getDistanceProgressString( { + inProportionOverridesDistanceProgress: false, + closerString: ratioAndProportionStrings.a11y.handPosition.closerTogether, + fartherString: ratioAndProportionStrings.a11y.handPosition.fartherApart + } ); + if ( distanceProgressPhrase ) { + const distanceProgressDescription = StringUtils.fillIn( ratioAndProportionStrings.a11y.bothHands.handsDistanceProgressPattern, { + distanceProgress: distanceProgressPhrase + } ); + + // Count closer/farther as a previous so that we don't ever get two of them at the same time + this.previousDistanceRegionBoth = distanceProgressDescription; + return distanceProgressDescription; + } + } + this.previousDistanceRegionBoth = distanceRegion; + return this.getBothHandsDistanceRegion( capitalized ); + } + private getDistanceProgressString( providedOptions?: GetDistanceProgressStringOptions ): null | string { const options = optionize( { + inProportionOverridesDistanceProgress: true, closerString: ratioAndProportionStrings.a11y.handPosition.closerTo, fartherString: ratioAndProportionStrings.a11y.handPosition.fartherFrom }, providedOptions ); - // No distance progress if in proportion TODO: this shouldn't occur for both hands in description, not clear if the same in voicing. https://github.com/phetsims/ratio-and-proportion/issues/459 - if ( this.inProportionProperty.value ) { + if ( options.inProportionOverridesDistanceProgress && this.inProportionProperty.value ) { return null; }