Refine the tandems for color support

[samreid]

From phetsims/scenery-phet#515, there was a TODO

const colorProfileProperty = new StringProperty( initialProfileName, {

  // TODO: Should we move global.view.colorProfile.profileNameProperty  to global.view.colorProfileProperty ? https://github.com/phetsims/scenery-phet/issues/515
  tandem: Tandem.GLOBAL_VIEW.createTandem( 'colorProfile' ).createTandem( 'profileNameProperty' ),
  validValues: colorProfiles
} );

I have left the tandem alone for the color profile (thus maintaining backward compatibility), but it no longer matches the code and this would be a good opportunity to decide what the tandems should be like, both for the colorProfileProperty and for the individual ProfileColorProperty instances. @zepumph can you please make an an initial recommendation, and escalate to a phet-io design meeting if necessary?

[zepumph]

Instrumented ProfileColorProperty instances should be suffixed with ColorProperty, or be called colorProperty. Can we assert that if you agree?

I think we should go ahead and rename the property to general.view.colorProfileProperty, because every sim is going to have one now right?

[samreid]

In phetsims/scenery-phet#515 (comment) @pixelzoom said:

Re "// TODO: Should we move global.view.colorProfile.profileNameProperty to global.view.colorProfileProperty"... Ask designers. Personally, I'd like to see colorProfileProperty and all instrumented instances of ProfileColorProperty under global.view.colors. That will require a new tandem like Tandem.GLOBAL_COLORS.

[samreid]

Instrumented ProfileColorProperty instances should be suffixed with ColorProperty, or be called colorProperty. Can we assert that if you agree?

I added assertions for that in the preceding commits.

[samreid]

I moved the property to e.g., gravityAndOrbits.global.view.colorProfileProperty. I was about to create e.g., gravityAndOrbits.global.colors for the ProfileColorProperty instances, but realized that gravityAndOrbits.global.view.colorProfileProperty is the only thing under global (at least for gravity and orbits). I wonder if it should be in general instead? Also, it should colorProfileProperty be under model or view? The other concern is that it seems the sim colors are general not global since they are sim specific, but I don't want the colorProfileProperty to get too far away from them. @zepumph can you please recommend, and escalate to Thursday meeting if necessary?

[zepumph]

Every sim now has one, so I believe you should put it in general. I think it's very unlikely we will feature it. Once you do that, I'd just pass this issue to a designer to review the studio tree. Pick a sim that has an instrumented color Property.

[zepumph]

In #1257 (comment) you mention Tandem.COLORS pointing to this issue, but I see no reference of that here. If you do intend on having a central place in the Tandem tree that houses all colors, then maybe global is the correct spot for this. I had thought we were moving to a pattern where colors were were to be added to the tandem tree where they were created, like backgroundNode.colorProperty. This is idiomatic to how we support all other view Properties. If a developer decides on placing all colors in one file, then using Tandem.GLOBAL_VIEW is probably the way to go. Tandem.COLORS feels like an antipattern.

[samreid]

Thanks, after reading this documentation in Tandem I see that general is appropriate for these sim-specific colors:

/**
 * Many simulation elements are nested under "general". This tandem is for elements that exists in all sims. For a
 * place to put simulation specific globals, see `GLOBAL`
 *
 * @constant
 * @type {Tandem}
 */
const GENERAL = Tandem.ROOT.createTandem( window.phetio.PhetioIDUtils.GENERAL_COMPONENT_NAME );

By that logic, it seems like general.view.colorProfileProperty is correct since every sim has one. Is it unfortunate that the colorProfileProperty is so far away from the colors? (one in general and others in global)? Maybe that is OK.

I had thought we were moving to a pattern where colors were were to be added to the tandem tree where they were created, like backgroundNode.colorProperty

As described in #1257, many ProfileColorProperties will need to be static, and we do not have a way to pass tandems statically.

you mention Tandem.COLORS pointing to this issue, but I see no reference of that here.

How about Tandem.COLORS = Tandem.GLOBAL_VIEW.createTandem('colors')?

[zepumph]

is so far away from the colors? (one in general and others in global)? Maybe that is OK.

I feel this seems antithetical to the primary points we (@samreid and @zepumph) are making over in #1257. We want to prefer putting colors in the spots they are used. If they are declared statically, then they probably just belong in a constants collection or something. Wouldn't it be based on sim-specific needs? Tandem.COLORS assumes that in every sim there will be a set of instrumented ProfileColorProperties in each sim that are dispersed (like x.colorProperty), and then another set that are statically created and collected in single place (global.view.colors). Doesn't it seem like the wrong thing to have both?

[samreid]

@zepumph and I discussed, and agreed:

• colorProfileProperty should move to general since it appears in every sim.
• Sim colors should be in global, since they are sim specific.
• We will go with Tandem.COLORS = Tandem.GLOBAL_VIEW.createTandem('colors')

[pixelzoom]

Is the tandem strucuture flat for colors, all elements appearing under global.colors? Or is it hierarchical, with colors grouped under components?

Suppose that we have default colors for Slider, defined as static ProfileColorProperty instances in Slider.js. Which of these did you have in mind?

// (1)
global.colors.sliderDefaultThumbFillProperty
global.colors.sliderDefaultThumbStrokeProperty
global.colors.sliderDefaultTrackFillProperty
global.colors.sliderDefaultTrackStrokeProperty

// (2)
global.colors.Slider.defaultThumbFillProperty
global.colors.Slider.defaultThumbStrokeProperty
global.colors.Slider.defaultTrackFillProperty
global.colors.Slider.defaultTrackStrokeProperty

// (3)
global.colors.Slider.thumb.defaultFillProperty
global.colors.Slider.thumb.defaultStrokeProperty
global.colors.Slider.track.defaultTrackFillProperty
global.colors.Slider.track.defaultStrokeProperty

(3) makes the most sense to me.

Hierarchical structure is more likely to also avoid name collisions. For example, what if I have a custom Slider, with its own sliderDefaultThumbFillProperty?

A flat structure also has the problem that colors related to "slider" will not be grouped together in the tandem tree, unless you use a "slider" prefix on every tandem name, as I've done in (1) above.

[pixelzoom]

I'd like to see a convention something like this for instances that are defined at usage sites:

global.colors.{{CLASS_NAME}}.*Property

e.g.:

global.colors.Slider.defaultThumbFillProperty

But that's even problematic, because class names are not unique across all repos. Maybe this?

global.colors.{{REPO}}.{{CLASS_NAME}}.*Property

e.g.:

global.colors.sun.Slider.defaultThumbFillProperty

[pixelzoom]

For instances that are defined in RepoColor.js, I think we also need a convention that prevents namespace collisions with colors in other repos. Maybe something like this:

global.colors.{{REPO}}.*Property

e.g.

global.colors.fourierMakingWaves.panelFillProperty

I really don't want to have to rename FMWColor panelFillProperty:

  // Fill for all Panels
  panelFillProperty: new ProfileColorProperty( 'panelFill', {
    default: Color.grayColor( 245 )
  } ),

... to something ugly/verbose in order to avoid name collision, like:

  // Fill for all Panels
  fourieMakingWavesPanelFillProperty: new ProfileColorProperty( 'fourieMakingWavesPanelFillProperty', {
    default: Color.grayColor( 245 ),
    tandem: Tandem.COLORS.createTandem( 'fourieMakingWavesPanelFillProperty' )
  } ),

[pixelzoom]

Also note in the above example that I've had to duplicate fourieMakingWavesPanelFillProperty three times. ProfileColorProperty name and tandem name both have the same "uniqueness" requirements. Maybe we should ditch name, use the tandem name, and require all ProfileColorProperty to be instrumented if we want them to show up in ColorEditor?

[samreid]

The last few comments have apparently been moved to related issue #1259

[samreid]

I addressed the recommendations from #1251 (comment), @zepumph can you please review? The subsequent comments will be addressed in #1259

[zepumph]

All looks good to me. Thanks

[samreid]

Thanks, closing.

[samreid]

Reopening since CT is showing several errors like this:

graphing-quadratics : phet-io-api-compatibility : unbuilt
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1638198860357/graphing-quadratics/graphing-quadratics_en.html?continuousTest=%7B%22test%22%3A%5B%22graphing-quadratics%22%2C%22phet-io-api-compatibility%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1638198860357%22%2C%22timestamp%22%3A1638200211140%7D&ea&brand=phet-io&phetioStandalone&phetioCompareAPI&randomSeed=332211
Query: ea&brand=phet-io&phetioStandalone&phetioCompareAPI&randomSeed=332211
Uncaught Error: Assertion failed: Designed API changes detected, please roll them back or revise the reference API:

New PhET-iO Element not in reference: graphingQuadratics.general.view.colorProfileProperty
Error: Assertion failed: Designed API changes detected, please roll them back or revise the reference API:

New PhET-iO Element not in reference: graphingQuadratics.general.view.colorProfileProperty
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1638198860357/assert/js/assert.js:25:13)
at XMLHttpRequest.<anonymous> (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1638198860357/chipper/dist/phet-io/js/phetioEngine.js:267:35)
id: Bayes Chrome
Snapshot from 11/29/2021, 8:14:20 AM

[samreid]

This has been on CT since at least Nov 27. I confirmed that running grunt generate-phet-io-api corrects the problem for graphing quadratics. It adds this new part to the API:

          "colorProfileProperty": {
            "_data": {
              "initialState": {
                "units": null,
                "validValues": [
                  "default"
                ],
                "value": "default"
              }
            },
            "_metadata": {
              "phetioTypeName": "PropertyIO<StringIO>"
            }
          },

[samreid]

I saw that the API files were never regenerated after the work in this issue. They were generated many other times for other issues, but somehow we never picked up the colorProfileProperty. I ran grunt generate-phet-io-api --simList=../perennial/data/phet-io-api-stable and the only changes seem to be the new colorProfileProperty. It seems that should be in the API file, so I'll commit those changes. Still not sure why they were missed for so long.

@pixelzoom also hypothesized that the error may be related to the recent port of ProfileColorProperty and colorProfileProperty to TypeScript. I tried reverting ProfileColorProperty but I received the same error.

[samreid]

I added the colorProfileProperty in the commit, and will request review from @zepumph since he was the original reviewer and may have ideas about what happened here.

[zepumph]

I'm quite tired of needing to specify the list to the stable phet-io sims, so I to help me review this issue, I made a --stable flag that will automatically use the stable api sims and regenerate each.

After calling this, I found no differences in the api files. Ready to close?