Odd implementation of zoom Properties.

[pixelzoom]

In https://github.com/phetsims/geometric-optics/issues/337, @arouinfar requested that I look at CCK for how zoomLevelProperty was instrumented. Specifically, she referred me to circuitConstructionKitDc.introScreen.model.zoomProperty:

We ultimately decided not to make geometric-optics look like CCK. But in the process, I noticed a couple of things that I think are worth pointing out.

Looking at the implementation in CircuitConstructionKitModel.ts, I see:

    // Scaling applied to the circuit node so the user can zoom out and make larger circuits.
    // 0 = zoomed out fully, 1 = zoomed in fully.  This NumberProperty is an implementation detail necessary to wire up
    // to MagnifyingGlassZoomButton
    this.selectedZoomProperty = new NumberProperty( 1, {
      range: new Range( 0, 1 ),
      validValues: [ 0, 1 ],
      phetioDocumentation: 'Zoom level for the sim.  0=zoomed out, 1=zoomed in (magnified)'
    } );

    // For PhET-iO: Use an enumeration pattern for the API
    this.zoomProperty = new EnumerationProperty( ZoomLevel.NORMAL, {
      tandem: tandem.createTandem( 'zoomProperty' ),
      phetioDocumentation: 'Selected zoom level for the simulation'
    } );
    this.selectedZoomProperty.lazyLink( selectedZoom => this.zoomProperty.set( selectedZoom === 0 ? ZoomLevel.ZOOMED_OUT : ZoomLevel.NORMAL ) );
    this.zoomProperty.lazyLink( zoom => this.selectedZoomProperty.set( zoom === ZoomLevel.ZOOMED_OUT ? 0 : 1 ) );

This looks very odd to me, for a number of reasons:

(1) selectedZoomProperty is a number, as required by common-code zoom control. It's not derived, and not iO instrumented, which seems questionable.

(2) zoomProperty is an enumeration, yet has a name similar to selectedZoomProperty -- which is confusing. Based on its named, I'd expect it to have the same value type as selectedZoomProperty.

(3) It seems like zoomProperty should be derived from selectedZoomProperty, but it isn't. Instead, there's an odd relationship where they keep each other in-sync by linking to each other. Imo, it's questionable whether this relationship is really needed or a good idea. And DynamicProperty is typically used when there's this kind of relationship.

[samreid]

I tried to make selectedZoomProperty derived from zoomProperty but received this error from ZoomButtonGroup: Assertion failed: invalid zoomLevelProperty.

[samreid]

@arouinfar what is the desired PhET-iO API for this? It was designed so that the PhET-iO API is "NORMAL" vs "ZOOMED_OUT". Do you want something more like https://github.com/phetsims/geometric-optics/issues/337 ?

[arouinfar]

@matthew-blackman let's review this in #917

[matthew-blackman]

@arouinfar and I spoke and agreed that the PhET-iO instrumentation should match that of Geometric Optics which is as follows:

zoomScaleProperty is a readonly array of scaling factors that are cycled between by clicking the zoom buttons. The default value is 1, and the possible values should not be restricted to any set mathematical pattern.

zoomLevelProperty is an integer that can be set via PhET-iO, corresponding to the index being using in the array of zoomScaleProperty's possible values. The default value will be the indexOf 1 in the zoomScaleProperty array.

[samreid]

I'll add some // REVIEW comments at points in the code, and I'll correct small wording or typos. I'll write other notes and observations here.

• Things are working great for the most part, nice work!
• Large things are so large they cannot be dropped in the toolbox.

• I observed an unrelated issue, which I opened a side issue for: https://github.com/phetsims/circuit-construction-kit-common/issues/924
• Now that there are 4 zoom levels, it may be confusing to some users how to get back to the "default" zoom. That may be corrected by having 3 zoom levels, if it is intuitive that "the middle one" is the default. I don't feel we should have another button like "reset zoom". Or maybe this is not a significant problem.
• We have been careful to describe the currentZoomScaleProperty as the animated zoom level. Should we just rename it to animatedZoomScaleProperty?
• Everything else is great, nice work!

[matthew-blackman]

Updated available zoom scales to 0.5, 1, and 1.8. This removes the "Large things are so large they cannot be dropped in the toolbox." issue when zoomed in.

@arouinfar ready for review - let me know how you like the three zoom options as they can be easily changed.

[arouinfar]

Thanks @matthew-blackman. The tree looks good and behaves well.

Updated available zoom scales to 0.5, 1, and 1.8. This removes the "Large things are so large they cannot be dropped in the toolbox." issue when zoomed in.

Not quite. I can't put the dollar bill away at this zoom level, so we need to take it down further. Maybe try 1.6?

[matthew-blackman]

Closing this issue and continuing circuit dropping behavior in #938