Requiring ProfileColorProperty names to be unique is problematic.

[pixelzoom]

In #1257 (comment), @samreid said:

ProfileColorProperty names must be unique, so declaring one in a constructor would throw a runtime assertion.

I agree that the names need to be unique.

But this name must be unqiue across ALL repos. That seems like a big problem. I'm going to add a new ProfileColorProperty, to common code or my sim. How do I verify that I'm choosing a unique name? Run all sims to find out? And I need to do this every time I add a ProfileColorProperty to my sim?

Are developers going to choose good names? Are they going to be thinking about name conflicts? Do we need a naming convention that helps mitigate conflicts? Should the naming be hierarchical? Should we use tandem names, since they also have to be unique? ....

This requires more thought.

[pixelzoom]

Looking ahead... As the number of ProfileColorProperty instances grows, Color Editor is going to become unusable if the structure is flat -- the same way that the Studio tree would be unusable if it were flat. We'll need some sort of structure/hierarchy in order to "collapse" things we're not interested in. For example, if the designer is only interested in sim-specific colors, they should be able to hide common-code colors.

[pixelzoom]

Discussed briefly with @samreid on Zoom. The general idea that we dicussed was:

• delete the name parameter to ProfileColorProperty
• use tandem name to provide structure and enforce uniqueness of names
• use Studio for editing colors
• delete Color Editor

The obvious issue with this idea is that colors will only be editable if they are instrumented. And not all sims are instrumented. So we'll have to run this idea past the team.

The set of ProfileColorProperty instances that PhET designers are interested in may be different than the set that instructional designers are interested in. But we can address that by using phetioFeatured.

[samreid]

Let's discuss this on Thursday, unassigning until then.

[samreid]

Moving comments from @pixelzoom from #1251 (comment)

@pixelzoom said:

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.

[samreid]

@pixelzoom said:

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

[samreid]

@pixelzoom said:

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' )
  } ),

[samreid]

@pixelzoom said:

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]

From today's design meeting:

@pixelzoom: we should not support a flat structure going forward. There is also opportunity for name collisions, and we have to specify the names twice independently.
@kathy-phet: We do not want all colors to appear in studio, as they do in the Color Editor. Most colors should not be phet-io instrumented.

Maybe we will have to stick with the current pattern for now, or maybe add prefixes, or maybe add a ProfileColorProperty that auto-prefxes.

@pixelzoom says in the long run we should maybe have different sections be collapse-able in the color editor.

@arouinfar says she uses the Color Editor around 1-2 hours per sim. Maybe that will help us know how much effort this is worth.

@pixelzoom agrees the current Color Editor is OK to have it as a flat structure. But once someone realizes there are too many colors, we can re-evaluate and decide how to improve the usability.

@zepumph is wondering if it is sufficient to use repo prefixes to organize things, and collapse-able sections.

But we do need to avoid the namespace collisions somehow. Back to @zepumph and @samreid to discuss, I guess, even though it is no longer a phet-io problem.

[pixelzoom]

@pixelzoom: we should not support a flat structure going forward. There is also opportunity for name collisions, and we have to specify the names twice independently.

A clarification... I said we should not do anything that precludes a hierarchical structure going forward. If we can support a hierarchical structure, we can support a flat structure. And a hierarchical structure is relevant to the Color Editor only - it's essential to avoiding namespace collisions.

[samreid]

Unassigning and adding a 6 month reminder on my personal calendar to re-evaluate (if it hasn't been requested before then).

[pixelzoom]

Requesting that this be reevaluated now. Requiring color name to be unqiue across all repos makes no sense, and will certainly cause problems as ProfileColorProperty instances are added. Imo it's a mistake to defer addressing this until it becomes a problem. A developer will either need to address it when they hit a name collision, or (more likely) use a non-optimal color name to avoid the name collision.

This could be addressed relatively easily now by changing the constructor signature of ProfileColorProperty to:

  /**
   * @param {string} repoName - name of the repository that this ProfileColorProperty belongs to
   * @param {string} colorName - name of this color, must be unique within the repository
   * @param {Object} colorProfileMap - object literal that maps keys (profile names) to ColorDef
   * @param {Object} [options]
   */
  constructor( repoName, colorName, colorProfileMap, options ) {

[pixelzoom]

For now, ProfileColorProperty can just take repoName and colorName, and concatenate them, with a separator, and register that with colorProfilePropery. Recommended to use the '.' separator, like tandems.

[samreid]

How would you like to factor out the repoName in simulations? A repo-specific ColorProfileProperty subclass?

[pixelzoom]

Rather than @param {string} repoName, let's use @param {Namespace} namespace}. Then in ProfileColorProperty:

const SEPARATOR = '.';
const colorProfileName = `${namespace.name}${SEPARATOR}${colorName}`;

How would you like to factor out the repoName in simulations? A repo-specific ColorProfileProperty subclass?

I would be fine with (e.g. in FMWColors.js):

import fourierMakingWaves from '../fourierMakingWaves.js';
...
const FMWColors = {

  // Background colors for screens.
  discreteScreenBackgroundColorProperty: new ProfileColorProperty( fourierMakingWaves, 'discreteScreenBackgroundColorProperty', {
    default: new Color( 236, 255, 255 )
  } ),
...

[pixelzoom]

I prefer using @param {Namespace} namespace} instead of @param {string} repoName because:

• Every .js file (expept main.js) has a Namespace import.
• It avoids the question of repoName format -- is it 'fourier-making-wave' or 'fourierMakingWaves'? It uses the Namespace name format, which has served PhET well.
• It avoid potential typos like 'foueir-making-waves'.

And yes, the ProfileColorProporty API may still need to change at some time in the future. But at least we'll be starting with something that avoids name collisions by creating a per-repo namespace (by using Namespace!) And it's unlikely we'll need to change @param colorName arguments in the future, because they can be chosen to make sense for the repo, not perverted to avoid name collisions.

[pixelzoom]

How would you like to factor out the repoName in simulations? A repo-specific ProfileColorProperty subclass?

A developer could easily create a repo-specific ColorProfileProperty subclass, e.g:

import ProfileColorProperty from '../../../scenery/js/util/ProfileColorProperty.js';
import fourierMakingWaves from '../fourierMakingWaves.js';
...
class FMWColorProfileProperty extends ProfileColorProperty {
  constructor( colorName, colorProfileMap, options ) {
    super( fourierMakingWaves, colorName, colorProfileMap, options );
  }
}

But I don't think that's necessary. Namespace is (conveniently) an import in every .js file (expect main.js). I don't think it's worth creating a subclass just to avoid 1 constructor arg.

[pixelzoom]

Here's a patch that was tested with FMWColors.js. Note the "TODO" for this.name in ProfileColorProperty.

Patch

Index: scenery/js/util/ProfileColorProperty.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/util/ProfileColorProperty.js b/scenery/js/util/ProfileColorProperty.js
--- a/scenery/js/util/ProfileColorProperty.js	(revision cacb390e636c006c8947d9d07cc6a52d8f0540b9)
+++ b/scenery/js/util/ProfileColorProperty.js	(date 1627778463532)
@@ -7,6 +7,7 @@
  */
 import arrayRemove from '../../../phet-core/js/arrayRemove.js';
 import merge from '../../../phet-core/js/merge.js';
+import Namespace from '../../../phet-core/js/Namespace.js';
 import Tandem from '../../../tandem/js/Tandem.js';
 import scenery from '../scenery.js';
 import SceneryConstants from '../SceneryConstants.js';
@@ -14,24 +15,29 @@
 import ColorProperty from '../util/ColorProperty.js';
 import colorProfileProperty from './colorProfileProperty.js';
 
+// constant
+const NAME_SEPARATOR = '.';
+
 // static instances are tracked for iframe communication with the HTML color editor
 const instances = [];
 
 class ProfileColorProperty extends ColorProperty {
 
   /**
-   * @param {string} name - name that appears in the HTML color editor
+   * @param {Namespace} namespace - namespace that this color belongs to
+   * @param {string} colorName - name of the color, unique within namespace
    * @param {Object} colorProfileMap - object literal that maps keys (profile names) to ColorDef
    * @param {Object} [options]
    */
-  constructor( name, colorProfileMap, options ) {
+  constructor( namespace, colorName, colorProfileMap, options ) {
+
+    assert && assert( namespace instanceof Namespace );
+    assert && assert( typeof colorName === 'string' );
 
     options = merge( {
       tandem: Tandem.OPTIONAL
     }, options );
 
-    assert && assert( !!name, 'ProfileColorProperty.options.name is required' );
-
     // All values are eagerly coerced to Color instances for efficiency (so it only has to be done once) and simplicity
     // (so the types are uniform)
     colorProfileMap = _.mapValues( colorProfileMap, Color.toColor );
@@ -54,8 +60,9 @@
       this.value = this.colorProfileMap[ colorProfileName ] || this.colorProfileMap[ SceneryConstants.DEFAULT_COLOR_PROFILE ];
     } );
 
+    //TODO Should this.name be private? Or have a more descriptive name in case we need to locate usages?
     // @public (read-only)
-    this.name = name;
+    this.name = `${namespace.name}${NAME_SEPARATOR}${colorName}`;
 
     // On initialization and when the color changes, send a message to the parent frame identifying the color value.
     // The HTML color editor wrapper listens for these messages and displays the color values.
Index: fourier-making-waves/js/common/FMWColors.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/fourier-making-waves/js/common/FMWColors.js b/fourier-making-waves/js/common/FMWColors.js
--- a/fourier-making-waves/js/common/FMWColors.js	(revision 9599bcc61de41cb4a6620d80ed2fbba603134c83)
+++ b/fourier-making-waves/js/common/FMWColors.js	(date 1627778463538)
@@ -15,74 +15,74 @@
 const FMWColors = {
 
   // Background colors for screens.
-  discreteScreenBackgroundColorProperty: new ProfileColorProperty( 'discreteScreenBackgroundColorProperty', {
+  discreteScreenBackgroundColorProperty: new ProfileColorProperty( fourierMakingWaves, 'discreteScreenBackgroundColorProperty', {
     default: new Color( 236, 255, 255 )
   } ),
-  waveGameScreenBackgroundColorProperty: new ProfileColorProperty( 'waveGameScreenBackgroundColorProperty', {
+  waveGameScreenBackgroundColorProperty: new ProfileColorProperty( fourierMakingWaves, 'waveGameScreenBackgroundColorProperty', {
     default: new Color( 236, 255, 255 )
   } ),
-  wavePacketScreenBackgroundColorProperty: new ProfileColorProperty( 'wavePacketScreenBackgroundColor', {
+  wavePacketScreenBackgroundColorProperty: new ProfileColorProperty( fourierMakingWaves, 'wavePacketScreenBackgroundColor', {
     default: new Color( 255, 250, 227 )
   } ),
 
   // Fill for all Panels
-  panelFillProperty: new ProfileColorProperty( 'panelFill', {
+  panelFillProperty: new ProfileColorProperty( fourierMakingWaves, 'panelFill', {
     default: Color.grayColor( 245 )
   } ),
 
   // Stroke for all Panels
-  panelStrokeProperty: new ProfileColorProperty( 'panelStroke', {
+  panelStrokeProperty: new ProfileColorProperty( fourierMakingWaves, 'panelStroke', {
     default: Color.grayColor( 160 )
   } ),
 
   // Stroke for horizontal separators in Panels
-  separatorStrokeProperty: new ProfileColorProperty( 'separatorStroke', {
+  separatorStrokeProperty: new ProfileColorProperty( fourierMakingWaves, 'separatorStroke', {
     default: Color.grayColor( 200 )
   } ),
 
   // Grid line stroke for all charts except the Amplitude chart in the 'Discrete' and 'Wave Game' screens
-  chartGridLinesStrokeProperty: new ProfileColorProperty( 'chartGridLinesStroke', {
+  chartGridLinesStrokeProperty: new ProfileColorProperty( fourierMakingWaves, 'chartGridLinesStroke', {
     default: Color.grayColor( 200 )
   } ),
 
   // Stroke for all x and y axes
-  axisStrokeProperty: new ProfileColorProperty( 'axisStroke', {
+  axisStrokeProperty: new ProfileColorProperty( fourierMakingWaves, 'axisStroke', {
     default: Color.grayColor( 170 )
   } ),
 
   // Stroke for the sum plot in the Discrete and Wave Packet screens
-  sumPlotStrokeProperty: new ProfileColorProperty( 'sumStroke', {
+  sumPlotStrokeProperty: new ProfileColorProperty( fourierMakingWaves, 'sumStroke', {
     default: 'black'
   } ),
 
   // Stroke used to plot answer to a challenge in Sum chart of the Wave Game screen.
   // If you're thinking of changing this to something other than pink, note that the UI says "Match the pink waveform..."
-  answerSumPlotStrokeProperty: new ProfileColorProperty( 'answerSumPlotStroke', {
+  answerSumPlotStrokeProperty: new ProfileColorProperty( fourierMakingWaves, 'answerSumPlotStroke', {
     default: new Color( 255, 0, 255 )
   } ),
 
   // Stoke used to plot the user's guess to a challenge in the Sum chart of the Wave Game screen.
-  guessSumPlotStrokeProperty: new ProfileColorProperty( 'guessSumPlotStroke', {
+  guessSumPlotStrokeProperty: new ProfileColorProperty( fourierMakingWaves, 'guessSumPlotStroke', {
     default: 'black'
   } ),
 
   // Stroke used to plot the Sum for infinite harmonics in the Discrete screen
-  infiniteHarmonicsStrokeProperty: new ProfileColorProperty( 'infiniteHarmonicsStroke', {
+  infiniteHarmonicsStrokeProperty: new ProfileColorProperty( fourierMakingWaves, 'infiniteHarmonicsStroke', {
     default: Color.grayColor( 189 )
   } ),
 
   // Fill for the level-selection buttons AND the scoreboard in the Wave Game screen
-  levelSelectionButtonFillProperty: new ProfileColorProperty( 'levelSelectionButtonFill', {
+  levelSelectionButtonFillProperty: new ProfileColorProperty( fourierMakingWaves, 'levelSelectionButtonFill', {
     default: new Color( 255, 214, 228 )
   } ),
 
   // Fill for the width indicators in the Wave Packet screen
-  widthIndicatorsFillProperty: new ProfileColorProperty( 'widthIndicatorsColor', {
+  widthIndicatorsFillProperty: new ProfileColorProperty( fourierMakingWaves, 'widthIndicatorsColor', {
     default: 'red'
   } ),
 
   // Stroke for the Continuous Waveform in the Wave Packet screen
-  continuousWaveformStrokeProperty: new ProfileColorProperty( 'continuousWaveformStroke', {
+  continuousWaveformStrokeProperty: new ProfileColorProperty( fourierMakingWaves, 'continuousWaveformStroke', {
     default: Color.grayColor( 189 )
   } )
 };
@@ -103,7 +103,7 @@
   new Color( 255, 105, 180 )
 ];
 FMWColors.HARMONIC_COLOR_PROPERTIES = _.map( HARMONIC_COLORS,
-  ( color, index ) => new ProfileColorProperty( `harmonic${index + 1}Color`, {
+  ( color, index ) => new ProfileColorProperty( fourierMakingWaves, `harmonic${index + 1}Color`, {
     default: HARMONIC_COLORS[ index ]
   } ) );
 

Here's how Color Editor looks for Fourier:

[chrisklus]

From 8/5/21 dev meeting:

We decided to move this to a sub team discussion with @samreid, @pixelzoom, and @zepumph.

[samreid]

I'll apply the changes above and update other sim occurrences. We will keep the duplication for now, and not worry about tandems/etc.

[samreid]

I added the Namespace parameter to ProfileColorProperty and marked ProfileColorProperty.name as private to the file. @pixelzoom would you like to review?

[pixelzoom]

I reviewed changes to ProfileColorProperty.js, how it looks in FMWColors.js, and took Color Editor for a test drive. Looks nice! Closing.