missingStringPhetioElements "density.general.model.strings.densityBuoyancyCommon.averageStringProperty"

[samreid]

In #153 the averageStringProperty became systemAStringProperty in eecee72. But now the migration wrapper says:

missingStringPhetioElements
"density.general.model.strings.densityBuoyancyCommon.averageStringProperty"

Does density 1.1 use the string average? How should we proceed?

[zepumph]

Ok. I understand the problem. It is annoying but likely not too troublesome.

Info:

• Bottle is where the "averageString" was used.
• the bottle as a feature is only used in Buoyancy, but it is imported in DensityBuoyancyScreenView here:

  density-buoyancy-common/js/common/view/DensityBuoyancyScreenView.ts

  Lines 510 to 552 in 322fd8d

  	if ( mass instanceof Cuboid ) {
  	massView = new CuboidView( mass, this, model.showDepthLinesProperty,
  	model.showGravityForceProperty, model.showBuoyancyForceProperty, model.showContactForceProperty,
  	model.showForceValuesProperty, model.vectorZoomProperty, model.showMassValuesProperty );
  	}
  	else if ( mass instanceof Scale ) {
  	massView = new ScaleView( mass, this, model.gravityProperty );
  	}
  	else if ( mass instanceof Cone ) {
  	massView = new ConeView( mass, this, model.showGravityForceProperty,
  	model.showBuoyancyForceProperty, model.showContactForceProperty, model.showForceValuesProperty,
  	model.vectorZoomProperty, model.showMassValuesProperty );
  	}
  	else if ( mass instanceof Ellipsoid ) {
  	massView = new EllipsoidView( mass, this, model.showGravityForceProperty,
  	model.showBuoyancyForceProperty, model.showContactForceProperty, model.showForceValuesProperty,
  	model.vectorZoomProperty, model.showMassValuesProperty );
  	}
  	else if ( mass instanceof HorizontalCylinder ) {
  	massView = new HorizontalCylinderView( mass, this,
  	model.showGravityForceProperty, model.showBuoyancyForceProperty, model.showContactForceProperty,
  	model.showForceValuesProperty, model.vectorZoomProperty, model.showMassValuesProperty );
  	}
  	else if ( mass instanceof VerticalCylinder ) {
  	massView = new VerticalCylinderView( mass, this, model.showGravityForceProperty,
  	model.showBuoyancyForceProperty, model.showContactForceProperty, model.showForceValuesProperty,
  	model.vectorZoomProperty, model.showMassValuesProperty );
  	}
  	else if ( mass instanceof Bottle ) {
  	massView = new BottleView( mass, this, model.showGravityForceProperty,
  	model.showBuoyancyForceProperty, model.showContactForceProperty, model.showForceValuesProperty,
  	model.vectorZoomProperty, model.showMassValuesProperty );
  	}
  	else if ( mass instanceof Boat ) {
  	massView = new BoatView( mass, this, model.pool.liquidYInterpolatedProperty,
  	model.showGravityForceProperty, model.showBuoyancyForceProperty, model.showContactForceProperty,
  	model.showForceValuesProperty, model.vectorZoomProperty, model.showMassValuesProperty );
  	}
  	else if ( mass instanceof Duck ) {
  	massView = new DuckView( mass, this,
  	model.showGravityForceProperty, model.showBuoyancyForceProperty, model.showContactForceProperty,
  	model.showForceValuesProperty, model.vectorZoomProperty, model.showMassValuesProperty );
  	}

• That is how the averageString (only used in buoyancy) is published in Density and has been getting translated for Density even though it isn't used.

To proceed:

• We can just ignore this problem, and don't need to support migration for this. Perhaps with a report processor, or a simple rename or something.
• That said it is interesting to note how this COULD have been a big issue, because density-buoyancy-common is common code, at least as it pertains to translation, in which the available keys to translate are taken from main, not from the DBC sha upon publication. So when this string was deleted, it stopped being served as a string to translate for Density 1.1. This is scary, and I want to make sure that @samreid and @AgustinVallejo understand this nuance, since it is definitely a potential drawback to using the common sim-suite repos. Again. There are no problems here that I'm aware of that need active managing, but to me it seems like a near miss, and it is likely worth investigating to see if we have deleted any other strings that were actually used by density. I'll do that next here.
• Where any other DBC strings used in density 1.1 deleted on main?

[zepumph]

Where any other DBC strings used in density 1.1 deleted on main?

Using this script I found one other problem with material.cement.

const fs = require( 'fs' );
const axios = require( 'axios' );

axios.get( 'https://phet.colorado.edu/sims/html/density/latest/english-string-map.json' ).then( response => {
  const releaseBranchStrings = response.data;
  const raw = fs.readFileSync( '../density-buoyancy-common/density-buoyancy-common-strings_en.json' );
  const mainStrings = JSON.parse( raw );

  const requireJSNamespace = 'DENSITY_BUOYANCY_COMMON/'

  Object.keys( releaseBranchStrings ).forEach( releaseBranchString => {
    if ( releaseBranchString.startsWith( requireJSNamespace ) ) {
      const stringKey = releaseBranchString.replace( requireJSNamespace, '' );
      if ( !mainStrings.hasOwnProperty( stringKey ) ) {
        console.log( 'not found on main: ', stringKey );
      }
    }
  } );
} )

I'll go ahead and add back that string, and we can get rid of it when we publish Density 1.2

[zepumph]

@samreid can you think of anything else here?

[samreid]

Thanks for reminding me that the strings are taken from main, I forgot about that when advising @AgustinVallejo to rename keys. I'm ready to close the issue, but let's make sure @AgustinVallejo is aware of #175 (comment) first.

[AgustinVallejo]

Really interesting read, I double and triple checked that 'Average' was not used in density, because I'm aware of how stringKeys are really sensitive to deletions or changes due to translations. Will be very careful with DBC from now on, thanks! closing