search

phetsims / molecule-polarity

"Molecule Polarity" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
2 stars 6 forks source link

Migration rules for 1.3 #141

Closed pixelzoom closed 1 year ago

pixelzoom commented 2 years ago

140 (add support for dynamic locale) will certainly result in many API changes. According to the PhET-iO spreadsheet, this sim needs to provide migration support from 1.2 to 1.3. So migration rules will be needed.

pixelzoom commented 2 years ago

At 9/1/2022 design meeting, @kathy-phet said:

samreid commented 2 years ago

I'll reassign this when we get closer to that stage. Self-unassigning for now.

zepumph commented 1 year ago

@samreid and I will take a first pass, we are doing this sim!

zepumph commented 1 year ago

First pass at rules are committed above. The only question I had is how sketchy it is that mulitple Text instances can map back to a single i18n string, i.e. 

new TandemFragmentRenamed( 'moleculePolarity.twoAtomsScreen.model.molecule.atomA.labelProperty',
      'moleculePolarity.general.model.strings.moleculePolarity.atomAStringProperty' ),
new TandemFragmentRenamed( 'moleculePolarity.threeAtomsScreen.model.molecule.atomA.labelProperty',
      'moleculePolarity.general.model.strings.moleculePolarity.atomAStringProperty' ),
samreid commented 1 year ago

We had the same problem in gravity and orbits where multiple strings map to one string and the last one takes precedence but we decided it is ok.

pixelzoom commented 1 year ago

Seems fine to me.

samreid commented 1 year ago

I reviewed the rules and they look good. I tested migrating a 1.2 version and it migrated correctly. I observed there are 5 common rules that do not apply to this simulation. I applied this patch for testing:

```diff Subject: [PATCH] Clean up ts-expect-error, see https://github.com/phetsims/chipper/issues/1366 --- Index: common/js/migration/MigrationEngine.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/common/js/migration/MigrationEngine.ts b/common/js/migration/MigrationEngine.ts --- a/common/js/migration/MigrationEngine.ts (revision 9b26258bd8e9a98ef0abc2b7f0def127a48ca8a8) +++ b/common/js/migration/MigrationEngine.ts (date 1671202950476) @@ -48,6 +48,10 @@ this.rules.forEach( migrationRule => { const result = migrationRule.processCommand( command, this.oldAPIFlat, this.newAPIFlat ); command = result.command; + + if ( result.messages.length === 0 ) { + console.log( 'Rule had no messages: ' + migrationRule ); + } messageBatches.push( result.messages ); } ); ```

And it indicated these rules:

``` Rule had no messages: TandemFragmentRenamed: Renamed phetioIDs containing moleculePolarity.global.view.colorProfile.profileNameProperty -> moleculePolarity.general.view.colorProfileProperty. The phetioID was renamed between versions, and the PhET-iO Element's state was forwarded to the new name. Most likely nothing else is needed for this migration rule, but if customizing in the previous version, it would be good to double check that it is still customized in the new version. MigrationEngine.ts:53 Rule had no messages: TandemFragmentRenamed: Renamed phetioIDs containing moleculePolarity.general.view.soundManager.enhancedSoundEnabledProperty -> moleculePolarity.general.view.soundManager.extraSoundEnabledProperty. The phetioID was renamed between versions, and the PhET-iO Element's state was forwarded to the new name. Most likely nothing else is needed for this migration rule, but if customizing in the previous version, it would be good to double check that it is still customized in the new version. MigrationEngine.ts:53 Rule had no messages: TandemFragmentRenamed: Renamed phetioIDs containing moleculePolarity.general.view.navigationBar.phetButton.phetMenu.enhancedSoundMenuItem.visibleProperty -> moleculePolarity.general.view.navigationBar.phetButton.phetMenu.extraSoundMenuItem.visibleProperty. The phetioID was renamed between versions, and the PhET-iO Element's state was forwarded to the new name. Most likely nothing else is needed for this migration rule, but if customizing in the previous version, it would be good to double check that it is still customized in the new version. MigrationEngine.ts:53 Rule had no messages: TandemFragmentRenamed: Renamed phetioIDs containing .textNode. -> .text.. The phetioID was renamed between versions, and the PhET-iO Element's state was forwarded to the new name. Most likely nothing else is needed for this migration rule, but if customizing in the previous version, it would be good to double check that it is still customized in the new version. MigrationEngine.ts:53 Rule had no messages: TandemFragmentRenamed: Renamed phetioIDs containing .readoutTextProperty -> .readoutStringProperty. The phetioID was renamed between versions, and the PhET-iO Element's state was forwarded to the new name. Most likely nothing else is needed for this migration rule, but if customizing in the previous version, it would be good to double check that it is still customized in the new version. ```

We previously agreed to avoid too much abstraction for the migration files for now, but this is making me wonder about these choices:

  1. leave it as it is with unused rules
  2. remove the unused rules

If we leave it as it is with unused rules, I considered factoring those out as a bundle:

```ts const migrationRules: MigrationRules = { '1.6.0': [ ...getCommonCodeRulesBatch1( simName ), ////////////////////////////////////////////////////// // ph-scale specific rules new TandemFragmentDelete( 'phScaleBasics.macroScreen.model.dropper.flowRateProperty.rangeProperty' ), new TandemFragmentDelete( 'phScaleBasics.macroScreen.model.solution.soluteVolumeProperty.rangeProperty' ), new TandemFragmentDelete( 'phScaleBasics.macroScreen.model.solution.waterVolumeProperty.rangeProperty' ), // Use inputEnabledProperty new TandemFragmentRenamed( 'phScaleBasics.macroScreen.view.dropperNode.dragListener.enabledProperty', 'phScaleBasics.macroScreen.view.dropperNode.inputEnabledProperty' ), // Rename waterLabelNode => waterText new TandemFragmentRenamed( 'phScaleBasics.macroScreen.view.waterFaucetNode.waterLabelNode.visibleProperty', 'phScaleBasics.macroScreen.view.waterFaucetNode.waterText.visibleProperty' ), new TandemFragmentRenamed( 'phScaleBasics.macroScreen.view.waterFaucetNode.waterLabelNode.stringProperty', 'phScaleBasics.general.model.strings.phScale.choice.waterStringProperty' ), // Separators no longer instrumented. The scenery layout engine hides and shows them automatically. // NumberDisplay valueText can no longer be made invisible by itself--just hide the whole numberDisplay new TandemFragmentDelete( 'phScaleBasics.macroScreen.view.pHMeterNode.pHIndicatorNode.numberDisplay.valueText.visibleProperty' ) ] }; ```

But after writing this out, I'm feeling the same concerns that we discussed about making sure no new rules are added to getCommonCodeRulesBatch1 (since they would likely be in the wrong order), etc. Anyways, perhaps we should just close the issue. @zepumph what do you think?

pixelzoom commented 1 year ago

Re factoring out rules that are shared across migration files via something like:

 ...getCommonCodeRulesBatch1( simName ),

On 12/6/2022, @samreid, @zepumph and I discussed whether to factor out common-code rules, and this approach specifically. As I recall, @zepumph and I had a negative reaction to this proposal, and thought it would result in headaches and maintenance issues. I still feel that way. I think it's better to keep the migration rules self-contained -- certainly for now, and we can revisit in the future as we have a better understanding of migration.

Re having irrelevant "common" rules in a migration file... In general, I think it's better to only include relevant rules, and we should not just be copy-pasting common rules from one sim to another. And if you've already identified which ones are irrelevant, why leave them in? They are serving no purpose, and may result in confusion/work in the future.

samreid commented 1 year ago

I removed the unused rules. As far as I can tell, this issue can be closed. @zepumph or @pixelzoom please reopen if there's more to do or discuss.

pixelzoom commented 1 year ago

12/22/2022 phet-io meeting

Reopened. Closed prematurely. Has not been reviewed, cleaned up, or match the pattern we decided for ph-scale.

pixelzoom commented 1 year ago

Since molecule-polarity disappeared from priority list for January, I'm removing the high-priority label.

zepumph commented 1 year ago

We now have a tool for testing and helping to create migration rules, over in https://github.com/phetsims/phet-io-wrappers/issues/481. We should use this here. @pixelzoom let's pair on this the next time this becomes a priority, as we will need to make sure that the patch in https://github.com/phetsims/phet-io-wrappers/issues/483 happens first.

pixelzoom commented 1 year ago

molecule-polarity doesn't seem like it's going to become a priority for awhile. And @zepumph seems to indicate in https://github.com/phetsims/molecule-polarity/issues/141#issuecomment-1404137581 that some new tool should be applied before reviewing. So unassigning until this becomes a priority.

zepumph commented 1 year ago

Agreed! Please note that in order to do this issue, we are blocked on https://github.com/phetsims/phet-io-wrappers/issues/483.

zepumph commented 1 year ago

This is now unblocked. You can test migration rules for this sim with a link like:

http://localhost:8080/phet-io-wrappers/version-migration/?sim=molecule-polarity&locales=*&keyboardLocaleSwitcher&phetioDebug=true&oldVersion=https://phet-io.colorado.edu/sims/molecule-polarity/1.2/wrappers/studio

Here is what a basic studio load errors with:

Error: Assertion failed: MigrationRules renamed one or more phetioIDs to something not in new API:
moleculePolarity.general.view.soundManager.enabledProperty
moleculePolarity.general.view.soundManager.extraSoundEnabledProperty
moleculePolarity.general.view.navigationBar.phetButton.phetMenu.visibleProperty
moleculePolarity.general.view.navigationBar.phetButton.visibleProperty
moleculePolarity.general.view.navigationBar.twoAtomsScreenButton.icon.visibleProperty
moleculePolarity.general.view.navigationBar.twoAtomsScreenButton.enabledProperty
moleculePolarity.general.view.navigationBar.threeAtomsScreenButton.icon.visibleProperty
moleculePolarity.general.view.navigationBar.threeAtomsScreenButton.enabledProperty
moleculePolarity.general.view.navigationBar.realMoleculesScreenButton.icon.visibleProperty
moleculePolarity.general.view.navigationBar.realMoleculesScreenButton.enabledProperty
moleculePolarity.general.view.panZoomListener.matrixProperty
moleculePolarity.general.view.panZoomListener.sourceFramePanBoundsProperty
pixelzoom commented 1 year ago

Insufficient information in https://github.com/phetsims/molecule-polarity/issues/141#issuecomment-1526265869.

It took me awhile to figure out how to use http://localhost:8080/phet-io-wrappers/version-migration, and how to obtain the "basic studio load errors" that @zepumph has shown above.

And I have no idea how to go from "basic studio load errors" to migration rules.

Back to @zepumph for a usable description of how to create migration rules. And this should be documented in phet-io/doc/.

zepumph commented 1 year ago

Sorry. My understanding was that this was in @samreid and my court still, just unassigned until this sim was a priority again. If I was expecting you to work on this then I would have included more info.

pixelzoom commented 1 year ago

Migration rules and getting Molecule Polarity into dev testing is on the worklist for the May 18 - June 7, 2023 iteration.

@zepumph let me know how I can help move this forward.

samreid commented 1 year ago

Working on this a bit.

samreid commented 1 year ago

This patch is running, not crashing, and migrating some things (but not all things).

```diff Subject: [PATCH] Round terms to 0.1 and compute MAD based on rounded numbers, see https://github.com/phetsims/center-and-variability/issues/169 --- Index: main/phet-io-wrappers/common/js/migration/IOTypeStateAttributeDelete.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/phet-io-wrappers/common/js/migration/IOTypeStateAttributeDelete.ts b/main/phet-io-wrappers/common/js/migration/IOTypeStateAttributeDelete.ts --- a/main/phet-io-wrappers/common/js/migration/IOTypeStateAttributeDelete.ts (revision 85161550f95844adb59c5c443686ca9ebd61f263) +++ b/main/phet-io-wrappers/common/js/migration/IOTypeStateAttributeDelete.ts (date 1684597926400) @@ -44,29 +44,31 @@ Object.keys( oldState ).forEach( keyPhetioID => { - const ioTypeName = oldFlatAPI[ keyPhetioID ]._metadata.phetioTypeName; + if ( oldFlatAPI.hasOwnProperty( keyPhetioID ) ) { + const ioTypeName = oldFlatAPI[ keyPhetioID ]._metadata.phetioTypeName; - newState[ keyPhetioID ] = oldState[ keyPhetioID ]; + newState[ keyPhetioID ] = oldState[ keyPhetioID ]; - if ( this.nameAppearsAnywhere( ioTypeName ) ) { - if ( this.matchesTopLevelTypeName( ioTypeName ) ) { + if ( this.nameAppearsAnywhere( ioTypeName ) ) { + if ( this.matchesTopLevelTypeName( ioTypeName ) ) { - assert && assert( oldState.hasOwnProperty( keyPhetioID ) && newState[ keyPhetioID ].hasOwnProperty( this.key ), `Expected state for ${keyPhetioID} in oldState` ); + assert && assert( oldState.hasOwnProperty( keyPhetioID ) && newState[ keyPhetioID ].hasOwnProperty( this.key ), `Expected state for ${keyPhetioID} in oldState` ); - delete newState[ keyPhetioID ][ this.key ]; - messages.push( `IOTypeStateAttributeDelete[${this.ioTypeName},${this.key}]: ` + keyPhetioID ); - } - else if ( this.matchesPropertyValueTypeName( ioTypeName ) ) { + delete newState[ keyPhetioID ][ this.key ]; + messages.push( `IOTypeStateAttributeDelete[${this.ioTypeName},${this.key}]: ` + keyPhetioID ); + } + else if ( this.matchesPropertyValueTypeName( ioTypeName ) ) { - // We need to also check for the nested .value attribute if it is a PropertyIO - const presentInNestedValue = oldState.hasOwnProperty( keyPhetioID ) && oldState[ keyPhetioID ].value.hasOwnProperty( this.key ); - assert && assert( presentInNestedValue, `Expected state for ${keyPhetioID} in oldState` ); + // We need to also check for the nested .value attribute if it is a PropertyIO + const presentInNestedValue = oldState.hasOwnProperty( keyPhetioID ) && oldState[ keyPhetioID ].value.hasOwnProperty( this.key ); + assert && assert( presentInNestedValue, `Expected state for ${keyPhetioID} in oldState` ); - delete newState[ keyPhetioID ].value[ this.key ]; - messages.push( `IOTypeStateAttributeDelete[${this.ioTypeName},${this.key}]: ` + keyPhetioID ); - } - else { - assert && assert( false, `Name appeared, but there was no handler: ${ioTypeName} to delete state attributes` ); + delete newState[ keyPhetioID ].value[ this.key ]; + messages.push( `IOTypeStateAttributeDelete[${this.ioTypeName},${this.key}]: ` + keyPhetioID ); + } + else { + assert && assert( false, `Name appeared, but there was no handler: ${ioTypeName} to delete state attributes` ); + } } } } ); Index: main/phet-io-sim-specific/repos/molecule-polarity/js/molecule-polarity-migration-rules.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/phet-io-sim-specific/repos/molecule-polarity/js/molecule-polarity-migration-rules.ts b/main/phet-io-sim-specific/repos/molecule-polarity/js/molecule-polarity-migration-rules.ts --- a/main/phet-io-sim-specific/repos/molecule-polarity/js/molecule-polarity-migration-rules.ts (revision 697e65770dd55bcf9642cb6865df8d796318266d) +++ b/main/phet-io-sim-specific/repos/molecule-polarity/js/molecule-polarity-migration-rules.ts (date 1684597760779) @@ -8,10 +8,13 @@ import TandemFragmentDelete from '../../../../phet-io-wrappers/common/js/migration/TandemFragmentDelete.js'; import PhetioTypeDelete from '../../../../phet-io-wrappers/common/js/migration/PhetioTypeDelete.js'; import EnumerationToStringUnionValue from '../../../../phet-io-wrappers/common/js/migration/EnumerationToStringUnionValue.js'; +import ConvertToStringsStateForLocale from '../../../../phet-io-wrappers/common/js/migration/ConvertToStringsStateForLocale.js'; +import IOTypeStateAttributeDelete from '../../../../phet-io-wrappers/common/js/migration/IOTypeStateAttributeDelete.js'; window.phetio = window.phetio || {}; const simName = 'moleculePolarity'; +const stringsStatePhetioID = 'moleculePolarity.general.model.stringsState'; window.phetio.migrationRules = { '1.3.0': [ @@ -36,11 +39,13 @@ new TandemFragmentRenamed( `${simName}.general.model.simNameProperty`, `${simName}.general.model.strings.${simName}.${simName}.titleStringProperty` ), new TandemFragmentDelete( 'Screen.view.controlPanel.separator' ), - new TandemFragmentRenamed( 'moleculePolarity.twoAtomsScreen.nameProperty', 'moleculePolarity.general.model.strings.moleculePolarity.screen.twoAtomsStringProperty' ), - new TandemFragmentRenamed( 'moleculePolarity.threeAtomsScreen.nameProperty', 'moleculePolarity.general.model.strings.moleculePolarity.screen.threeAtomsStringProperty' ), - new TandemFragmentRenamed( 'moleculePolarity.realMoleculesScreen.nameProperty', 'moleculePolarity.general.model.strings.moleculePolarity.screen.realMoleculesStringProperty' ), - new TandemFragmentRenamed( 'moleculePolarity.homeScreen.view.titleText.stringProperty', 'moleculePolarity.general.model.strings.moleculePolarity.moleculePolarity.titleStringProperty' ), - new TandemFragmentRenamed( 'moleculePolarity.general.view.navigationBar.titleText.stringProperty', 'moleculePolarity.general.model.displayedSimNameProperty' ), + new ConvertToStringsStateForLocale( 'moleculePolarity.twoAtomsScreen.nameProperty', 'moleculePolarity.general.model.strings.moleculePolarity.screen.twoAtomsStringProperty', stringsStatePhetioID ), + new ConvertToStringsStateForLocale( 'moleculePolarity.threeAtomsScreen.nameProperty', 'moleculePolarity.general.model.strings.moleculePolarity.screen.threeAtomsStringProperty', stringsStatePhetioID ), + new ConvertToStringsStateForLocale( 'moleculePolarity.realMoleculesScreen.nameProperty', 'moleculePolarity.general.model.strings.moleculePolarity.screen.realMoleculesStringProperty', stringsStatePhetioID ), + + new TandemFragmentDelete( 'moleculePolarity.homeScreen.view.titleText.stringProperty' ), + new TandemFragmentDelete( 'moleculePolarity.general.view.navigationBar.titleText.stringProperty' ), + new TandemFragmentDelete( 'moleculePolarity.general.model.strings.moleculePolarity.moleculePolarity.titleStringProperty' ), // Used to be read-only so they won't be settable new TandemFragmentDelete( 'moleculePolarity.general.view.navigationBar.twoAtomsScreenButton.text.stringProperty' ), @@ -50,6 +55,22 @@ new TandemFragmentDelete( 'moleculePolarity.homeScreen.view.buttonGroup.threeAtomsScreenButton.text.stringProperty' ), new TandemFragmentDelete( 'moleculePolarity.homeScreen.view.buttonGroup.realMoleculesScreenButton.text.stringProperty' ), + // Support for migration of soundManager Properties + // See https://github.com/phetsims/phet-io/issues/1903 + new TandemFragmentDelete( 'moleculePolarity.general.view.soundManager.enabledProperty' ), + new TandemFragmentDelete( 'moleculePolarity.general.view.soundManager.extraSoundEnabledProperty' ), + + new TandemFragmentDelete( 'moleculePolarity.general.view.navigationBar.phetButton.phetMenu.visibleProperty' ), + new TandemFragmentDelete( 'moleculePolarity.general.view.navigationBar.phetButton.visibleProperty' ), + new TandemFragmentDelete( 'moleculePolarity.general.view.navigationBar.twoAtomsScreenButton.icon.visibleProperty' ), + new TandemFragmentDelete( 'moleculePolarity.general.view.navigationBar.twoAtomsScreenButton.enabledProperty' ), + new TandemFragmentDelete( 'moleculePolarity.general.view.navigationBar.threeAtomsScreenButton.icon.visibleProperty' ), + new TandemFragmentDelete( 'moleculePolarity.general.view.navigationBar.threeAtomsScreenButton.enabledProperty' ), + new TandemFragmentDelete( 'moleculePolarity.general.view.navigationBar.realMoleculesScreenButton.icon.visibleProperty' ), + new TandemFragmentDelete( 'moleculePolarity.general.view.navigationBar.realMoleculesScreenButton.enabledProperty' ), + new TandemFragmentDelete( 'moleculePolarity.general.view.panZoomListener.matrixProperty' ), + new TandemFragmentDelete( 'moleculePolarity.general.view.panZoomListener.sourceFramePanBoundsProperty' ), + new TandemFragmentRenamed( /moleculePolarity\.\w+Screen\.view\.electronegativityPanels\.atom\wElectronegativityPanel\.subtitleText\.stringProperty/, 'moleculePolarity.general.model.strings.moleculePolarity.electronegativityStringProperty' ), new TandemFragmentRenamed( /moleculePolarity\.\w+Screen\.view\.controlPanel\.viewControls\.titleText\.stringProperty/, @@ -70,16 +91,20 @@ new EnumerationToStringUnionValue( 'moleculePolarity.twoAtomsScreen.view.viewProperties.surfaceTypeProperty' ), // TODO: both screens point back to the same model string Property, so what if we want different values for each? https://github.com/phetsims/molecule-polarity/issues/141 - new TandemFragmentRenamed( 'moleculePolarity.twoAtomsScreen.model.molecule.atomA.labelProperty', - 'moleculePolarity.general.model.strings.moleculePolarity.atomAStringProperty' ), - new TandemFragmentRenamed( 'moleculePolarity.twoAtomsScreen.model.molecule.atomB.labelProperty', - 'moleculePolarity.general.model.strings.moleculePolarity.atomBStringProperty' ), - new TandemFragmentRenamed( 'moleculePolarity.threeAtomsScreen.model.molecule.atomA.labelProperty', - 'moleculePolarity.general.model.strings.moleculePolarity.atomAStringProperty' ), - new TandemFragmentRenamed( 'moleculePolarity.threeAtomsScreen.model.molecule.atomB.labelProperty', - 'moleculePolarity.general.model.strings.moleculePolarity.atomBStringProperty' ), - new TandemFragmentRenamed( 'moleculePolarity.threeAtomsScreen.model.molecule.atomC.labelProperty', - 'moleculePolarity.general.model.strings.moleculePolarity.atomCStringProperty' ), + new ConvertToStringsStateForLocale( 'moleculePolarity.twoAtomsScreen.model.molecule.atomA.labelProperty', + 'moleculePolarity.general.model.strings.moleculePolarity.atomAStringProperty', stringsStatePhetioID ), + new ConvertToStringsStateForLocale( 'moleculePolarity.twoAtomsScreen.model.molecule.atomB.labelProperty', + 'moleculePolarity.general.model.strings.moleculePolarity.atomBStringProperty', stringsStatePhetioID ), + new ConvertToStringsStateForLocale( 'moleculePolarity.threeAtomsScreen.model.molecule.atomA.labelProperty', + 'moleculePolarity.general.model.strings.moleculePolarity.atomAStringProperty', stringsStatePhetioID ), + new ConvertToStringsStateForLocale( 'moleculePolarity.threeAtomsScreen.model.molecule.atomB.labelProperty', + 'moleculePolarity.general.model.strings.moleculePolarity.atomBStringProperty', stringsStatePhetioID ), + new ConvertToStringsStateForLocale( 'moleculePolarity.threeAtomsScreen.model.molecule.atomC.labelProperty', + 'moleculePolarity.general.model.strings.moleculePolarity.atomCStringProperty', stringsStatePhetioID ), + + // NumberProperty used to have a step attribute to tell studio how much to increment/decrement sliders, but + // it was removed in https://github.com/phetsims/axon/issues/386 + new IOTypeStateAttributeDelete( 'NumberPropertyIO', 'step' ), new TandemFragmentDelete( 'partialChargeProperty.rangeProperty' ), new TandemFragmentDelete( 'moleculePolarity.twoAtomsScreen.view.moleculeNode.atomANode.labelText.visibleProperty' ), ```

Next steps:

Would be good to work with @zepumph on the next steps. Also, should we commit the patch or improve it first?

zepumph commented 1 year ago

Taking a look now before standup

zepumph commented 1 year ago

UPDATE: This was fixed once all model strings were successfully converted to stringsState instead of to individual string Properties.

zepumph commented 1 year ago

Why did I need to guard IOTypeStateAttributeDelete? There were some IO types not in the object. But why?

Because this rule isn't the very first item in the migrationRules. It was failing on stringsState which is mutated into the "oldState" by the ConvertToStringsStateForLocale rule, but wouldn't be in the old API. Best practice is to guard like you are doing, and also likely put this IOType mutation rule at the top of the list.

zepumph commented 1 year ago

Ok, looks like there were still many TandemFragmentRenames that were renaming to model strings, which is buggy, and should use ConvertToStringsStateForLocale. I added an assertion. I'm ready to pair with @samreid at this time.

``` Subject: [PATCH] so much fun --- Index: phet-io-wrappers/common/js/migration/TandemFragmentRenamed.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-io-wrappers/common/js/migration/TandemFragmentRenamed.ts b/phet-io-wrappers/common/js/migration/TandemFragmentRenamed.ts --- a/phet-io-wrappers/common/js/migration/TandemFragmentRenamed.ts (revision 85161550f95844adb59c5c443686ca9ebd61f263) +++ b/phet-io-wrappers/common/js/migration/TandemFragmentRenamed.ts (date 1684770322154) @@ -73,6 +73,7 @@ const oldPhetioID = oldStatePhetioIDs[ i ]; if ( this.matchesTandem( oldPhetioID ) ) { + assert && assert( !this.newFragment.includes( 'general.model.strings' ), `Model strings are not stateful, instead use ConvertToStringsStateForLocale: ${this.newFragment}` ); const newPhetioID = this.promote( oldPhetioID ); messages.push( 'TandemFragmentRenamed: [setState] - ' + oldPhetioID + ' ==> ' + newPhetioID ); promotedMigratedIDs.push( newPhetioID ); Index: phet-io-wrappers/common/js/migration/IOTypeStateAttributeDelete.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-io-wrappers/common/js/migration/IOTypeStateAttributeDelete.ts b/phet-io-wrappers/common/js/migration/IOTypeStateAttributeDelete.ts --- a/phet-io-wrappers/common/js/migration/IOTypeStateAttributeDelete.ts (revision 85161550f95844adb59c5c443686ca9ebd61f263) +++ b/phet-io-wrappers/common/js/migration/IOTypeStateAttributeDelete.ts (date 1684769106556) @@ -44,29 +44,31 @@ Object.keys( oldState ).forEach( keyPhetioID => { - const ioTypeName = oldFlatAPI[ keyPhetioID ]._metadata.phetioTypeName; + if ( oldFlatAPI.hasOwnProperty( keyPhetioID ) ) { + const ioTypeName = oldFlatAPI[ keyPhetioID ]._metadata.phetioTypeName; - newState[ keyPhetioID ] = oldState[ keyPhetioID ]; + newState[ keyPhetioID ] = oldState[ keyPhetioID ]; - if ( this.nameAppearsAnywhere( ioTypeName ) ) { - if ( this.matchesTopLevelTypeName( ioTypeName ) ) { + if ( this.nameAppearsAnywhere( ioTypeName ) ) { + if ( this.matchesTopLevelTypeName( ioTypeName ) ) { - assert && assert( oldState.hasOwnProperty( keyPhetioID ) && newState[ keyPhetioID ].hasOwnProperty( this.key ), `Expected state for ${keyPhetioID} in oldState` ); + assert && assert( oldState.hasOwnProperty( keyPhetioID ) && newState[ keyPhetioID ].hasOwnProperty( this.key ), `Expected state for ${keyPhetioID} in oldState` ); - delete newState[ keyPhetioID ][ this.key ]; - messages.push( `IOTypeStateAttributeDelete[${this.ioTypeName},${this.key}]: ` + keyPhetioID ); - } - else if ( this.matchesPropertyValueTypeName( ioTypeName ) ) { + delete newState[ keyPhetioID ][ this.key ]; + messages.push( `IOTypeStateAttributeDelete[${this.ioTypeName},${this.key}]: ` + keyPhetioID ); + } + else if ( this.matchesPropertyValueTypeName( ioTypeName ) ) { - // We need to also check for the nested .value attribute if it is a PropertyIO - const presentInNestedValue = oldState.hasOwnProperty( keyPhetioID ) && oldState[ keyPhetioID ].value.hasOwnProperty( this.key ); - assert && assert( presentInNestedValue, `Expected state for ${keyPhetioID} in oldState` ); + // We need to also check for the nested .value attribute if it is a PropertyIO + const presentInNestedValue = oldState.hasOwnProperty( keyPhetioID ) && oldState[ keyPhetioID ].value.hasOwnProperty( this.key ); + assert && assert( presentInNestedValue, `Expected state for ${keyPhetioID} in oldState` ); - delete newState[ keyPhetioID ].value[ this.key ]; - messages.push( `IOTypeStateAttributeDelete[${this.ioTypeName},${this.key}]: ` + keyPhetioID ); - } - else { - assert && assert( false, `Name appeared, but there was no handler: ${ioTypeName} to delete state attributes` ); + delete newState[ keyPhetioID ].value[ this.key ]; + messages.push( `IOTypeStateAttributeDelete[${this.ioTypeName},${this.key}]: ` + keyPhetioID ); + } + else { + assert && assert( false, `Name appeared, but there was no handler: ${ioTypeName} to delete state attributes` ); + } } } } ); Index: phet-io-sim-specific/repos/molecule-polarity/js/molecule-polarity-migration-rules.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-io-sim-specific/repos/molecule-polarity/js/molecule-polarity-migration-rules.ts b/phet-io-sim-specific/repos/molecule-polarity/js/molecule-polarity-migration-rules.ts --- a/phet-io-sim-specific/repos/molecule-polarity/js/molecule-polarity-migration-rules.ts (revision 697e65770dd55bcf9642cb6865df8d796318266d) +++ b/phet-io-sim-specific/repos/molecule-polarity/js/molecule-polarity-migration-rules.ts (date 1684770350997) @@ -8,13 +8,21 @@ import TandemFragmentDelete from '../../../../phet-io-wrappers/common/js/migration/TandemFragmentDelete.js'; import PhetioTypeDelete from '../../../../phet-io-wrappers/common/js/migration/PhetioTypeDelete.js'; import EnumerationToStringUnionValue from '../../../../phet-io-wrappers/common/js/migration/EnumerationToStringUnionValue.js'; +import ConvertToStringsStateForLocale from '../../../../phet-io-wrappers/common/js/migration/ConvertToStringsStateForLocale.js'; +import IOTypeStateAttributeDelete from '../../../../phet-io-wrappers/common/js/migration/IOTypeStateAttributeDelete.js'; window.phetio = window.phetio || {}; const simName = 'moleculePolarity'; +const stringsStatePhetioID = 'moleculePolarity.general.model.stringsState'; window.phetio.migrationRules = { '1.3.0': [ + + // NumberProperty used to have a step attribute to tell studio how much to increment/decrement sliders, but + // it was removed in https://github.com/phetsims/axon/issues/386 + new IOTypeStateAttributeDelete( 'NumberPropertyIO', 'step' ), + new TandemFragmentRenamed( 'enhancedSound', 'extraSound' ), new NumberPropertyNullRangeIsInfinite(), @@ -33,14 +41,20 @@ new PhetioTypeDelete( 'SimInfoIO' ), // must be last after all renames so that the phetioID matches in the newAPI - new TandemFragmentRenamed( `${simName}.general.model.simNameProperty`, `${simName}.general.model.strings.${simName}.${simName}.titleStringProperty` ), + new ConvertToStringsStateForLocale( `${simName}.general.model.simNameProperty`, + `${simName}.general.model.strings.${simName}.${simName}.titleStringProperty`, stringsStatePhetioID ), new TandemFragmentDelete( 'Screen.view.controlPanel.separator' ), - new TandemFragmentRenamed( 'moleculePolarity.twoAtomsScreen.nameProperty', 'moleculePolarity.general.model.strings.moleculePolarity.screen.twoAtomsStringProperty' ), - new TandemFragmentRenamed( 'moleculePolarity.threeAtomsScreen.nameProperty', 'moleculePolarity.general.model.strings.moleculePolarity.screen.threeAtomsStringProperty' ), - new TandemFragmentRenamed( 'moleculePolarity.realMoleculesScreen.nameProperty', 'moleculePolarity.general.model.strings.moleculePolarity.screen.realMoleculesStringProperty' ), - new TandemFragmentRenamed( 'moleculePolarity.homeScreen.view.titleText.stringProperty', 'moleculePolarity.general.model.strings.moleculePolarity.moleculePolarity.titleStringProperty' ), - new TandemFragmentRenamed( 'moleculePolarity.general.view.navigationBar.titleText.stringProperty', 'moleculePolarity.general.model.displayedSimNameProperty' ), + new ConvertToStringsStateForLocale( 'moleculePolarity.twoAtomsScreen.nameProperty', + 'moleculePolarity.general.model.strings.moleculePolarity.screen.twoAtomsStringProperty', stringsStatePhetioID ), + new ConvertToStringsStateForLocale( 'moleculePolarity.threeAtomsScreen.nameProperty', + 'moleculePolarity.general.model.strings.moleculePolarity.screen.threeAtomsStringProperty', stringsStatePhetioID ), + new ConvertToStringsStateForLocale( 'moleculePolarity.realMoleculesScreen.nameProperty', + 'moleculePolarity.general.model.strings.moleculePolarity.screen.realMoleculesStringProperty', stringsStatePhetioID ), + + new TandemFragmentDelete( 'moleculePolarity.homeScreen.view.titleText.stringProperty' ), + new TandemFragmentDelete( 'moleculePolarity.general.view.navigationBar.titleText.stringProperty' ), + new TandemFragmentDelete( 'moleculePolarity.general.model.strings.moleculePolarity.moleculePolarity.titleStringProperty' ), // Used to be read-only so they won't be settable new TandemFragmentDelete( 'moleculePolarity.general.view.navigationBar.twoAtomsScreenButton.text.stringProperty' ), @@ -50,19 +64,35 @@ new TandemFragmentDelete( 'moleculePolarity.homeScreen.view.buttonGroup.threeAtomsScreenButton.text.stringProperty' ), new TandemFragmentDelete( 'moleculePolarity.homeScreen.view.buttonGroup.realMoleculesScreenButton.text.stringProperty' ), - new TandemFragmentRenamed( /moleculePolarity\.\w+Screen\.view\.electronegativityPanels\.atom\wElectronegativityPanel\.subtitleText\.stringProperty/, - 'moleculePolarity.general.model.strings.moleculePolarity.electronegativityStringProperty' ), - new TandemFragmentRenamed( /moleculePolarity\.\w+Screen\.view\.controlPanel\.viewControls\.titleText\.stringProperty/, - 'moleculePolarity.general.model.strings.moleculePolarity.viewStringProperty' ), - new TandemFragmentRenamed( /moleculePolarity\.\w+Screen\.view\.controlPanel\.viewControls\.partialChargesCheckbox\.labelText\.stringProperty/, - 'moleculePolarity.general.model.strings.moleculePolarity.partialChargesStringProperty' ), - new TandemFragmentRenamed( /moleculePolarity\.\w+Screen\.view\.controlPanel\.eFieldControl\.titleText\.stringProperty/, - 'moleculePolarity.general.model.strings.moleculePolarity.electricFieldStringProperty' ), + // Support for migration of soundManager Properties + // See https://github.com/phetsims/phet-io/issues/1903 + new TandemFragmentDelete( 'moleculePolarity.general.view.soundManager.enabledProperty' ), + new TandemFragmentDelete( 'moleculePolarity.general.view.soundManager.extraSoundEnabledProperty' ), + + new TandemFragmentDelete( 'moleculePolarity.general.view.navigationBar.phetButton.phetMenu.visibleProperty' ), + new TandemFragmentDelete( 'moleculePolarity.general.view.navigationBar.phetButton.visibleProperty' ), + new TandemFragmentDelete( 'moleculePolarity.general.view.navigationBar.twoAtomsScreenButton.icon.visibleProperty' ), + new TandemFragmentDelete( 'moleculePolarity.general.view.navigationBar.twoAtomsScreenButton.enabledProperty' ), + new TandemFragmentDelete( 'moleculePolarity.general.view.navigationBar.threeAtomsScreenButton.icon.visibleProperty' ), + new TandemFragmentDelete( 'moleculePolarity.general.view.navigationBar.threeAtomsScreenButton.enabledProperty' ), + new TandemFragmentDelete( 'moleculePolarity.general.view.navigationBar.realMoleculesScreenButton.icon.visibleProperty' ), + new TandemFragmentDelete( 'moleculePolarity.general.view.navigationBar.realMoleculesScreenButton.enabledProperty' ), + new TandemFragmentDelete( 'moleculePolarity.general.view.panZoomListener.matrixProperty' ), + new TandemFragmentDelete( 'moleculePolarity.general.view.panZoomListener.sourceFramePanBoundsProperty' ), - new TandemFragmentRenamed( 'moleculePolarity.twoAtomsScreen.view.controlPanel.viewControls.bondCharacterCheckbox.labelText.stringProperty', - 'moleculePolarity.general.model.strings.moleculePolarity.bondCharacterStringProperty' ), - new TandemFragmentRenamed( 'moleculePolarity.twoAtomsScreen.view.controlPanel.surfaceControl.titleText.stringProperty', - 'moleculePolarity.general.model.strings.moleculePolarity.surfaceStringProperty' ), + new ConvertToStringsStateForLocale( /moleculePolarity\.\w+Screen\.view\.electronegativityPanels\.atom\wElectronegativityPanel\.subtitleText\.stringProperty/, + 'moleculePolarity.general.model.strings.moleculePolarity.electronegativityStringProperty', stringsStatePhetioID ), + new ConvertToStringsStateForLocale( /moleculePolarity\.\w+Screen\.view\.controlPanel\.viewControls\.titleText\.stringProperty/, + 'moleculePolarity.general.model.strings.moleculePolarity.viewStringProperty', stringsStatePhetioID ), + new ConvertToStringsStateForLocale( /moleculePolarity\.\w+Screen\.view\.controlPanel\.viewControls\.partialChargesCheckbox\.labelText\.stringProperty/, + 'moleculePolarity.general.model.strings.moleculePolarity.partialChargesStringProperty', stringsStatePhetioID ), + new ConvertToStringsStateForLocale( /moleculePolarity\.\w+Screen\.view\.controlPanel\.eFieldControl\.titleText\.stringProperty/, + 'moleculePolarity.general.model.strings.moleculePolarity.electricFieldStringProperty', stringsStatePhetioID ), + + new ConvertToStringsStateForLocale( 'moleculePolarity.twoAtomsScreen.view.controlPanel.viewControls.bondCharacterCheckbox.labelText.stringProperty', + 'moleculePolarity.general.model.strings.moleculePolarity.bondCharacterStringProperty', stringsStatePhetioID ), + new ConvertToStringsStateForLocale( 'moleculePolarity.twoAtomsScreen.view.controlPanel.surfaceControl.titleText.stringProperty', + 'moleculePolarity.general.model.strings.moleculePolarity.surfaceStringProperty', stringsStatePhetioID ), new TandemFragmentRenamed( 'moleculePolarity.global.model.dipoleDirectionProperty', 'moleculePolarity.global.model.preferences.dipoleDirectionProperty' ), @@ -70,16 +100,16 @@ new EnumerationToStringUnionValue( 'moleculePolarity.twoAtomsScreen.view.viewProperties.surfaceTypeProperty' ), // TODO: both screens point back to the same model string Property, so what if we want different values for each? https://github.com/phetsims/molecule-polarity/issues/141 - new TandemFragmentRenamed( 'moleculePolarity.twoAtomsScreen.model.molecule.atomA.labelProperty', - 'moleculePolarity.general.model.strings.moleculePolarity.atomAStringProperty' ), - new TandemFragmentRenamed( 'moleculePolarity.twoAtomsScreen.model.molecule.atomB.labelProperty', - 'moleculePolarity.general.model.strings.moleculePolarity.atomBStringProperty' ), - new TandemFragmentRenamed( 'moleculePolarity.threeAtomsScreen.model.molecule.atomA.labelProperty', - 'moleculePolarity.general.model.strings.moleculePolarity.atomAStringProperty' ), - new TandemFragmentRenamed( 'moleculePolarity.threeAtomsScreen.model.molecule.atomB.labelProperty', - 'moleculePolarity.general.model.strings.moleculePolarity.atomBStringProperty' ), - new TandemFragmentRenamed( 'moleculePolarity.threeAtomsScreen.model.molecule.atomC.labelProperty', - 'moleculePolarity.general.model.strings.moleculePolarity.atomCStringProperty' ), + new ConvertToStringsStateForLocale( 'moleculePolarity.twoAtomsScreen.model.molecule.atomA.labelProperty', + 'moleculePolarity.general.model.strings.moleculePolarity.atomAStringProperty', stringsStatePhetioID ), + new ConvertToStringsStateForLocale( 'moleculePolarity.twoAtomsScreen.model.molecule.atomB.labelProperty', + 'moleculePolarity.general.model.strings.moleculePolarity.atomBStringProperty', stringsStatePhetioID ), + new ConvertToStringsStateForLocale( 'moleculePolarity.threeAtomsScreen.model.molecule.atomA.labelProperty', + 'moleculePolarity.general.model.strings.moleculePolarity.atomAStringProperty', stringsStatePhetioID ), + new ConvertToStringsStateForLocale( 'moleculePolarity.threeAtomsScreen.model.molecule.atomB.labelProperty', + 'moleculePolarity.general.model.strings.moleculePolarity.atomBStringProperty', stringsStatePhetioID ), + new ConvertToStringsStateForLocale( 'moleculePolarity.threeAtomsScreen.model.molecule.atomC.labelProperty', + 'moleculePolarity.general.model.strings.moleculePolarity.atomCStringProperty', stringsStatePhetioID ), new TandemFragmentDelete( 'partialChargeProperty.rangeProperty' ), new TandemFragmentDelete( 'moleculePolarity.twoAtomsScreen.view.moleculeNode.atomANode.labelText.visibleProperty' ), @@ -97,18 +127,18 @@ new TandemFragmentDelete( '.noneText.visibleProperty' ), new TandemFragmentDelete( 'moleculePolarity.twoAtomsScreen.view.colorKeyNode.visibleProperty' ), - new TandemFragmentRenamed( 'moleculePolarity.twoAtomsScreen.view.controlPanel.viewControls.bondDipoleCheckbox.labelText.labelText.stringProperty', - 'moleculePolarity.general.model.strings.moleculePolarity.bondDipoleStringProperty' ), - new TandemFragmentRenamed( 'moleculePolarity.threeAtomsScreen.view.controlPanel.viewControls.bondDipolesCheckbox.labelText.labelText.stringProperty', - 'moleculePolarity.general.model.strings.moleculePolarity.bondDipolesStringProperty' ), - new TandemFragmentRenamed( 'moleculePolarity.threeAtomsScreen.view.controlPanel.viewControls.molecularDipoleCheckbox.labelText.labelText.stringProperty', - 'moleculePolarity.general.model.strings.moleculePolarity.molecularDipoleStringProperty' ), + new ConvertToStringsStateForLocale( 'moleculePolarity.twoAtomsScreen.view.controlPanel.viewControls.bondDipoleCheckbox.labelText.labelText.stringProperty', + 'moleculePolarity.general.model.strings.moleculePolarity.bondDipoleStringProperty', stringsStatePhetioID ), + new ConvertToStringsStateForLocale( 'moleculePolarity.threeAtomsScreen.view.controlPanel.viewControls.bondDipolesCheckbox.labelText.labelText.stringProperty', + 'moleculePolarity.general.model.strings.moleculePolarity.bondDipolesStringProperty', stringsStatePhetioID ), + new ConvertToStringsStateForLocale( 'moleculePolarity.threeAtomsScreen.view.controlPanel.viewControls.molecularDipoleCheckbox.labelText.labelText.stringProperty', + 'moleculePolarity.general.model.strings.moleculePolarity.molecularDipoleStringProperty', stringsStatePhetioID ), - new TandemFragmentRenamed( 'moleculePolarity.twoAtomsScreen.view.controlPanel.surfaceControl.radioButtonGroup.noneText.stringProperty', - 'moleculePolarity.general.model.strings.moleculePolarity.noneStringProperty' ), - new TandemFragmentRenamed( 'moleculePolarity.twoAtomsScreen.view.controlPanel.surfaceControl.radioButtonGroup.electrostaticPotentialText.stringProperty', - 'moleculePolarity.general.model.strings.moleculePolarity.electrostaticPotentialStringProperty' ), - new TandemFragmentRenamed( 'moleculePolarity.twoAtomsScreen.view.controlPanel.surfaceControl.radioButtonGroup.electronDensityText.stringProperty', - 'moleculePolarity.general.model.strings.moleculePolarity.electronDensityStringProperty' ) + new ConvertToStringsStateForLocale( 'moleculePolarity.twoAtomsScreen.view.controlPanel.surfaceControl.radioButtonGroup.noneText.stringProperty', + 'moleculePolarity.general.model.strings.moleculePolarity.noneStringProperty', stringsStatePhetioID ), + new ConvertToStringsStateForLocale( 'moleculePolarity.twoAtomsScreen.view.controlPanel.surfaceControl.radioButtonGroup.electrostaticPotentialText.stringProperty', + 'moleculePolarity.general.model.strings.moleculePolarity.electrostaticPotentialStringProperty', stringsStatePhetioID ), + new ConvertToStringsStateForLocale( 'moleculePolarity.twoAtomsScreen.view.controlPanel.surfaceControl.radioButtonGroup.electronDensityText.stringProperty', + 'moleculePolarity.general.model.strings.moleculePolarity.electronDensityStringProperty', stringsStatePhetioID ) ] }; ```
zepumph commented 1 year ago

There is a TODO in here:

// TODO: Why not ConvertToStringsStateForLocale? https://github.com/phetsims/molecule-polarity/issues/141

'moleculePolarity.homeScreen.view.titleText.stringProperty' and 'moleculePolarity.general.view.navigationBar.titleText.stringProperty' are both phetioReadOnly so we shouldn't need to migrate anything.

The third phetioID:: 'moleculePolarity.general.model.strings.moleculePolarity.moleculePolarity.titleStringProperty' is not necessary and can be removed.

zepumph commented 1 year ago

There is another TOOD:

// TODO: both screens point back to the same model string Property, so what if we want different values for each? https://github.com/phetsims/molecule-polarity/issues/141

https://github.com/phetsims/phet-io-sim-specific/blob/a50053f42c82524f2867475b9bad79da212b25c9/repos/molecule-polarity/js/molecule-polarity-migration-rules.ts#L99-L109

In https://github.com/phetsims/phet-io-sim-specific/commit/a049148bf41fa23eb16c08c6021bc114ac87dbf2 there is precedent for choosing the phetioID you most want to overwrite the others (potentially). I will leave it up to @pixelzoom which are potentially more "important" or "likely to be renamed" by clients. Right now any label from the threeAtomsScreen will overwrite those from the twoAtomsScreen because they are after. It think it is pretty low stakes, and would be fine either way.

zepumph commented 1 year ago

I also created a new MigrationRule checker that double checks that there are no superfluous migration rules, and will assert out. I was able to trigger it with this patch:

Subject: [PATCH] Add ensureAllMigrationRulesDidSomething to test, https://github.com/phetsims/molecule-polarity/issues/141
---
Index: repos/molecule-polarity/js/molecule-polarity-migration-rules.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/repos/molecule-polarity/js/molecule-polarity-migration-rules.ts b/repos/molecule-polarity/js/molecule-polarity-migration-rules.ts
--- a/repos/molecule-polarity/js/molecule-polarity-migration-rules.ts   (revision b280b442549d6fe1f9aacf0bc509d1d287221d4d)
+++ b/repos/molecule-polarity/js/molecule-polarity-migration-rules.ts   (date 1684777925442)
@@ -34,6 +34,7 @@

     new TandemFragmentRenamed( '.textProperty', '.stringProperty' ),
     new TandemFragmentRenamed( '.labelNode.', '.labelText.' ),
+    new TandemFragmentRenamed( '.lafdsafsdbelNode.', '.labelTexfdfdt.' ),

     new TandemFragmentDelete( 'ScreenButton.text.visibleProperty' ),
     new TandemFragmentDelete( `${simName}.general.view.navigationBar.titleText.visibleProperty` ),
zepumph commented 1 year ago

Ok. This is ready for double check by @pixelzoom and for QA to hammer on it during dev test.

@pixelzoom:

pixelzoom commented 1 year ago

In the migration wrapper, I opened the About dialog, then pressed the Migrate button. The error below appears in the console. @zepumph thinks the ensureAllStateValuesInNewAPI rule needs to be fixed to map from archetype to dynamic element.

Error: Assertion failed: MigrationRules renamed one or more phetioIDs to something not in new API:
moleculePolarity.general.view.navigationBar.phetButton.phetMenu.aboutDialogCapsule.aboutDialog.termsPrivacyAndLicensingLinkText.visibleProperty
moleculePolarity.general.view.navigationBar.phetButton.phetMenu.aboutDialogCapsule.aboutDialog.termsPrivacyAndLicensingLinkText.stringProperty
moleculePolarity.general.view.navigationBar.phetButton.phetMenu.aboutDialogCapsule.aboutDialog.translationCreditsLinkText.visibleProperty
moleculePolarity.general.view.navigationBar.phetButton.phetMenu.aboutDialogCapsule.aboutDialog.translationCreditsLinkText.stringProperty
moleculePolarity.general.view.navigationBar.phetButton.phetMenu.aboutDialogCapsule.aboutDialog.thirdPartyCreditsLinkText.visibleProperty
moleculePolarity.general.view.navigationBar.phetButton.phetMenu.aboutDialogCapsule.aboutDialog.thirdPartyCreditsLinkText.stringProperty
moleculePolarity.general.view.navigationBar.phetButton.phetMenu.aboutDialogCapsule.aboutDialog
moleculePolarity.general.view.navigationBar.phetButton.phetMenu.aboutDialogCapsule.aboutDialog.visibleProperty
moleculePolarity.general.view.navigationBar.phetButton.phetMenu.aboutDialogCapsule.aboutDialog.isShowingProperty

    at window.assertions.assertFunction (assert.js:28:13)
    at MigrationEngine.ensureAllStateValuesInNewAPI (MigrationEngine.ts:91:17)
    at MigrationEngine.ts:61:56
    at Array.forEach (<anonymous>)
    at MigrationEngine.process (MigrationEngine.ts:61:28)
    at StandardPhetioWrapper.loadStandardPhetioWrapperFromHTML (StandardPhetioWrapper.ts:299:38)
    at async Client.loadStandardPhetioWrapperFromHTML (Client.ts:1477:20)
    at async ?sim=molecule-polarity&locales=*&keyboardLocaleSwitcher&phetioDebug=true&oldVersion=https://phet-io.colorado.edu/sims/molecule-polarity/1.2/wrappers/studio:119:13
(anonymous) @ ?sim=molecule-polarity&locales=*&keyboardLocaleSwitcher&phetioDebug=true&oldVersion=https://phet-io.colorado.edu/sims/molecule-polarity/1.2/wrappers/studio:122
pixelzoom commented 1 year ago

I spent about 15 minutes trying to review the migration rules. The majority of them are undocumented -- something we decided was important when we did this at least 2x previously. So I don't know why many of these rules are necessary (what API change they are addressing, related GitHub issues, ...), and I'm not about to comb through this issue and GitHub commits to try to figure that out.

I'm kicking this back to @zepumph and @samreid for documentation. Let me know if you'd like me to tag along while you add doc.

pixelzoom commented 1 year ago

Re the questions in https://github.com/phetsims/molecule-polarity/issues/141#issuecomment-1557651046 ... I gather that the concern is that the labels "A", "B", and "C" are shared by both screens.

There has never been a requirement or request to make each screen have it's own independent set of strings for these labels. That would in fact seem somewhat unusual and undesirable. So the first question is, do we need to even consider this? Until someone asks for this feature, my feeling is a strong "no".

The second issue raised is:

Right now any label from the threeAtomsScreen will overwrite those from the twoAtomsScreen because they are after.

I believe that should be addressed by making the rules more general. I.e.: 

-    new ConvertToStringsStateForLocale( 'moleculePolarity.twoAtomsScreen.model.molecule.atomA.labelProperty',
      'moleculePolarity.general.model.strings.moleculePolarity.atomAStringProperty', stringsStatePhetioID ),
-    new ConvertToStringsStateForLocale( 'moleculePolarity.twoAtomsScreen.model.molecule.atomB.labelProperty',
      'moleculePolarity.general.model.strings.moleculePolarity.atomBStringProperty', stringsStatePhetioID ),
-    new ConvertToStringsStateForLocale( 'moleculePolarity.threeAtomsScreen.model.molecule.atomA.labelProperty',
      'moleculePolarity.general.model.strings.moleculePolarity.atomAStringProperty', stringsStatePhetioID ),
+    new ConvertToStringsStateForLocale( 'model.molecule.atomA.labelProperty',
      'model.strings.moleculePolarity.atomAStringProperty', stringsStatePhetioID ),
-    new ConvertToStringsStateForLocale( 'moleculePolarity.threeAtomsScreen.model.molecule.atomB.labelProperty',
      'moleculePolarity.general.model.strings.moleculePolarity.atomBStringProperty', stringsStatePhetioID ),
+    new ConvertToStringsStateForLocale( 'model.molecule.atomB.labelProperty',
      'model.strings.moleculePolarity.atomBStringProperty', stringsStatePhetioID ),
-    new ConvertToStringsStateForLocale( 'moleculePolarity.threeAtomsScreen.model.molecule.atomC.labelProperty',
      'moleculePolarity.general.model.strings.moleculePolarity.atomCStringProperty', stringsStatePhetioID ),
+    new ConvertToStringsStateForLocale( 'model.molecule.atomC.labelProperty',
      'model.strings.moleculePolarity.atomCStringProperty', stringsStatePhetioID ),

@zepumph thoughts?

zepumph commented 1 year ago

A meeting this morning with @samreid @pixelzoom and @zepumph yielded the above commits, and @pixelzoom will take it from here with documentation and general improvements to the way we write out rules. We agreed that more explicit rules were better and clearer than confusing things like renaming all labelNode to labelText. We also agree that keeping full phetioIDs as the same strings as seen in the oldVersion is best, so don't rename all textProperty -> stringProperty as the first rule, but instead as the last rule.

pixelzoom commented 1 year ago

These rules are buggy:

    new TandemFragmentDelete( 'moleculePolarity.general.view.panZoomListener.matrixProperty' ),
    new TandemFragmentDelete( 'moleculePolarity.general.view.panZoomListener.sourceFramePanBoundsProperty' ),

Neither of these elements was removed; they were relocated as part of https://github.com/phetsims/phet-io/issues/1913. The rules should be:

    new TandemFragmentRenamed( 'moleculePolarity.general.view.panZoomListener.matrixProperty',
      'moleculePolarity.general.view.display.panZoomListener.matrixProperty' ),
    new TandemFragmentRenamed( 'moleculePolarity.general.view.panZoomListener.sourceFramePanBoundsProperty',
      'moleculePolarity.general.view.display.panZoomListener.sourceFramePanBoundsProperty' ),
pixelzoom commented 1 year ago

These rules seem buggy:

   // It's OK to delete these because they were formerly textProperty, but read-only, and now they do not exist.
    new TandemFragmentDelete( 'moleculePolarity.homeScreen.view.titleText.textProperty' ),
    new TandemFragmentDelete( 'moleculePolarity.general.view.navigationBar.titleText.textProperty' ),
    new TandemFragmentDelete( 'moleculePolarity.general.view.navigationBar.twoAtomsScreenButton.text.textProperty' ),
    new TandemFragmentDelete( 'moleculePolarity.general.view.navigationBar.threeAtomsScreenButton.text.textProperty' ),
    new TandemFragmentDelete( 'moleculePolarity.general.view.navigationBar.realMoleculesScreenButton.text.textProperty' ),
    new TandemFragmentDelete( 'moleculePolarity.homeScreen.view.buttonGroup.twoAtomsScreenButton.text.textProperty' ),
    new TandemFragmentDelete( 'moleculePolarity.homeScreen.view.buttonGroup.threeAtomsScreenButton.text.textProperty' ),
    new TandemFragmentDelete( 'moleculePolarity.homeScreen.view.buttonGroup.realMoleculesScreenButton.text.textProperty' ),

Is it really OK to delete these? Why is the fact that they were read-only relevant? Couldn't someone be trying to read them?

Each of these has an associated StringProperty, so shouldn't they be ConvertToStringsStateForLocale rules?

EDIT: Discussed with @zepumph and @samreid via Zoom. Since we only need to support Studio for migration now, the above TandemFragmentDelete would have been sufficient. But since I had already done the work, we decided it was OK to replace with 7 ConvertToStringsStateForLocale and 1 TandemFragmentRename in the commit below https://github.com/phetsims/phet-io-sim-specific/commit/31209f30fd10d8dfd05664dd01b1e3b44c4dec3a.

pixelzoom commented 1 year ago

Commits above were incorrectly tagged with https://github.com/phetsims/molecule-polarity/issues/140.

Rules have been reviewed, revised, and documented. I deleted most (all?) of what I started with. This took me all day.

pixelzoom commented 1 year ago

Completion is now blocked by:

Over to @zepumph for next steps:

zepumph commented 1 year ago

Please note that from https://github.com/phetsims/phet-io-wrappers/issues/513, I had to add migration rules from changes in the AboutDialog and the Options->Preferences conversion. The most important part of that was migrating the sim-specific controls. the dipoleDirectionControl was pretty straight forward, but it isn't clear to me what the expected way of handling the surfaceColorControl is, so I only did a minimal amount of testing by toggling the control visible in studio.

zepumph commented 1 year ago

I removed the TODO about PhetioTypeDelete( 'SimInfoIO' ). I tested it by removing the migration and saw that it worked without it, still I think it is safer and better to not have SimInfo objects being set at all (!!!), let alone across versions. It would at the least be noisy and confusing, at at its worst it would be buggy to think you had different stats (like URL/Referrer/sim version/etc).

I glanced through the remaining rules and everything looks really nice. Thanks for your work.

Thank you!@

pixelzoom commented 1 year ago

Please note that from https://github.com/phetsims/phet-io-wrappers/issues/513, I had to add migration rules from changes in the AboutDialog and the Options->Preferences conversion.

These need work. In the above commit, I did some cleanup and added TODOs.

pixelzoom commented 1 year ago

This rule was added for OptionsDialog -> PreferencesDialog:

    new TandemFragmentDelete( 'optionsDialogCapsule.optionsDialog' ),

Removing this rule results in these errors in the Migration wrapper:

moleculePolarity.general.view.navigationBar.phetButton.phetMenu.optionsDialogCapsule.optionsDialog.title.visibleProperty
moleculePolarity.general.view.navigationBar.phetButton.phetMenu.optionsDialogCapsule.optionsDialog.title.textProperty
moleculePolarity.general.view.navigationBar.phetButton.phetMenu.optionsDialogCapsule.optionsDialog.content.visibleProperty
moleculePolarity.general.view.navigationBar.phetButton.phetMenu.optionsDialogCapsule.optionsDialog.visibleProperty
moleculePolarity.general.view.navigationBar.phetButton.phetMenu.optionsDialogCapsule.optionsDialog.isShowingProperty

At lease one of these (isShowingDialog) can be migrated. So adding this rule seems overly general, maybe even buggy.

pixelzoom commented 1 year ago

@zepumph and I discussed the rules related to optionsDialog. We think it's reasonable to replace all rules related to optionsDialog with this one rule, which I've done in https://github.com/phetsims/phet-io-sim-specific/commit/6b3370a478206b54c7ebdaaa50bf286f712462c8. 

    // Element optionsDialog no longer exists, and was replaced by preferencesDialog, which is very different.
    // Trying to migrate specific sub-elements from optionsDialog to preferencesDialog is too complicated.
    // So delete everything related to optionsDialog. The client will need to do work to customize preferencesDialog.
    // See https://github.com/phetsims/joist/issues/837
    new TandemFragmentDelete( 'optionsDialogCapsule.optionsDialog' ),

This will also be easy to transfer to other sims, since we won't need to attempt migration of sub-elements of optionsDialog to sub-elements of preferencesDialog.

pixelzoom commented 1 year ago

Rules are now complete, and I haven't seen any problems with fuzz testing.

When @samreid has completed review and closed https://github.com/phetsims/phet-io-wrappers/issues/513, this issue can then be closed, and the dev-test issue for the 1.3 release can be created.

pixelzoom commented 1 year ago

https://github.com/phetsims/phet-io-wrappers/issues/513, so this issue is now complete, and ready for dev testing.

For dev testing, I'm not familiar with how QA tests migration rules. I see Testing Batch Forward Migration in the QA Book, but I'm not familar with that process, and it does not mention the new Migration Wrapper. @KatieWoe if you have questions about how QA should test migration rules, please contact @zepumph and @samreid.

KatieWoe commented 1 year ago

Migration was tested in dev.3. If nothing else needs to be done with this issue it can be closed

pixelzoom commented 1 year ago

Thanks @KatieWoe. Closing.