any for scoreDisplayConstructor and scoreDisplayOptions

[pixelzoom]

A couple of uses of any that need to be addressed, identical in FiniteStatusBar.ts and InfniteStatusBar.ts:

  scoreDisplayConstructor?: any;
  scoreDisplayOptions?: any;

scoreDisplayConstructor should be a constructor, one of the values in VALID_SCORE_DISPLAY_CONSTRUCTORS.

scoreDisplayOptions should be the options type that matches scoreDisplayConstructor.

I'm not sure how to address this, will investigate.

[pixelzoom]

Another case in LevelSectionButton.ts:

  scoreDisplayConstructor?: any; //TODO https://github.com/phetsims/vegas/issues/102
  scoreDisplayOptions?: any; //TODO https://github.com/phetsims/vegas/issues/102

[pixelzoom]

As @jbphet noted in #105 (comment), it would be nice to remove this kind of thing:

// Valid values for scoreDisplayConstructor. These are the types that are relevant for this status bar.
// All constructors must have the same signature!
const VALID_SCORE_DISPLAY_CONSTRUCTORS = [
  ScoreDisplayLabeledNumber, ScoreDisplayNumberAndStar
];

... and use an interface instead.

Then there's still the problem of how to say "this option must have a value that is a constructor with this signature".

[samreid]

This is the TypeScript syntax for indicating the presence of a constructor:

scoreDisplayConstructor?: { new( scoreProperty: IProperty<number>, providedOptions?: any ): Node };

I'm not sure what to do about the options type. And perhaps a factory function (closure) instead of passing around constructors would be clearer.

[jbphet]

I have committed changes to address this issue. The basic idea is to have an option that is a creator function for the score display instead of providing a constructor and separate options. I had @samreid take a look at the approach before making the commits, and he said he found it reasonable.

This works, and is much more type safe, so it solves the problem where any had to be used in several places. Having said that, I don't absolutely love this solution because it means that the client is now fully responsible for configuring the score display, whereas the previous implementation used some of the options from the status bars and merged them into the score display options, making the API perhaps a little easier to use.

An alternative that I considered was to create a factory object that could take a score display style as an enum value and options as a superset of all possible options for the various styles of score display. This struck me as cool, but potentially more time consuming to implement, so I went the route that I did as a bit of a compromise to get it done in a reasonable amount of time.

Assigning to @samreid to review.

[jbphet]

@samreid - I also requested that QA run a regression test these changes on master, see phetsims/qa#817.

[samreid]

I reviewed the commits and this seems a good direction to go in. I have 3 main feedback points:

• Change the return type to Node. There are many places where the return type is identified as specific subtypes of score displays, such as:

  createScoreDisplay?: ( scoreProperty: IProperty<number> ) => ScoreDisplayLabeledNumber | ScoreDisplayLabeledStars;

and

// valid types for the score display in level selection buttons
type ValidScoreDisplayTypes =
  ScoreDisplayLabeledNumber |
  ScoreDisplayLabeledStars |
  ScoreDisplayStars |
  ScoreDisplayNumberAndStar;

However, none of the usage sites need to know those are score displays--all they need to know is they are scenery Nodes. So I tried this patch and it type checked OK:

Index: js/LevelSelectionButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/LevelSelectionButton.ts b/js/LevelSelectionButton.ts
--- a/js/LevelSelectionButton.ts	(revision 03b2e4f4eb4ae8082fcbe5c04b887435fcd2ae4d)
+++ b/js/LevelSelectionButton.ts	(date 1656286269586)
@@ -34,13 +34,6 @@
 const DEFAULT_BEST_TIME_FONT = new PhetFont( 24 );
 const SCALING_TOLERANCE = 1E-4; // Empirically chosen as something the human eye is unlikely to notice.
 
-// valid types for the score display in level selection buttons
-type ValidScoreDisplayTypes =
-  ScoreDisplayLabeledNumber |
-  ScoreDisplayLabeledStars |
-  ScoreDisplayStars |
-  ScoreDisplayNumberAndStar;
-
 type SelfOptions = {
 
   // Used to size the content
@@ -48,7 +41,7 @@
   buttonHeight?: number;
 
   // score display
-  createScoreDisplay?: ( scoreProperty: IProperty<number> ) => ValidScoreDisplayTypes;
+  createScoreDisplay?: ( scoreProperty: IProperty<number> ) => Node;
   scoreDisplayProportion?: number; // percentage of the button height occupied by scoreDisplay, (0,0.5]
   scoreDisplayMinXMargin?: number; // horizontal margin between scoreDisplay and its background
   scoreDisplayMinYMargin?: number;  // vertical margin between scoreDisplay and its background
Index: js/FiniteStatusBar.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/FiniteStatusBar.ts b/js/FiniteStatusBar.ts
--- a/js/FiniteStatusBar.ts	(revision 03b2e4f4eb4ae8082fcbe5c04b887435fcd2ae4d)
+++ b/js/FiniteStatusBar.ts	(date 1656286269590)
@@ -41,7 +41,7 @@
   textFill?: IColor;
 
   // score display
-  createScoreDisplay?: ( scoreProperty: IProperty<number> ) => ScoreDisplayLabeledNumber | ScoreDisplayLabeledStars;
+  createScoreDisplay?: ( scoreProperty: IProperty<number> ) => Node;
 
   // nested options for 'Start Over' button, filled in below
   startOverButtonOptions?: TextPushButtonOptions;
Index: js/InfiniteStatusBar.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/InfiniteStatusBar.ts b/js/InfiniteStatusBar.ts
--- a/js/InfiniteStatusBar.ts	(revision 03b2e4f4eb4ae8082fcbe5c04b887435fcd2ae4d)
+++ b/js/InfiniteStatusBar.ts	(date 1656286269581)
@@ -27,7 +27,7 @@
   spacing?: number;
 
   // score display
-  createScoreDisplay?: ( scoreProperty: IProperty<number> ) => ScoreDisplayLabeledNumber | ScoreDisplayNumberAndStar;
+  createScoreDisplay?: ( scoreProperty: IProperty<number> ) => Node;
 };
 
 export type InfiniteStatusBarOptions = SelfOptions & StrictOmit<StatusBarOptions, 'children' | 'barHeight'>;

If we (in the future?) need more fine-grained type information about those score display subtypes, consider creating a base class, type or interface for it. But Node seems best until we need something more.

• The ability to pass through the default font and text fill to the score display was supported previously, but dropped as described in the preceding comment.

    // nested options for score display
    options.scoreDisplayOptions = merge( {
      font: options.font,
      textFill: options.textFill
    }, options.scoreDisplayOptions );

It seems reasonable to pass through the main font and textFill as options to the createScoreDisplay functions, if we want to keep that ability. But if that is more complicated, it seems OK to drop that feature.

• Finally, it seems a good way to test this change for regressions would be to use the MultiSnapshotComparison tool that @jonathanolson demonstrated recently. That may not be a perfect fit, if fuzzing doesn't visit the games too well, but if it is just checking the style of the scoreboards, maybe it would be helpful. But QA also seems a reasonable cross-check here.

[jbphet]

I've incorporated @samreid's recommendation to broaden the return type for createScoreDisplay (thanks for the patch @samreid !), and it looks good. I did some regression testing in a couple of sims, and everything looks fine.

I decided not to add the options for font and text fill to createScoreDisplay, since it does indeed seem more complicated, as @samreid noted above.

As for regressions testing using MultiSnapshotComparison, I started looking into it and bailed. It didn't seem worth the time to me given that QA is already testing in phetsims/qa#817.

I think that should cover it. Closing.