Merge pull request #51 from veillette/claude/refactor-view-state-model-CI5az

Refactor view options into per-screen ViewOptionsModel

Author: veillette

src/OpticsLabConstants.ts

@@ -133,6 +133,13 @@ export const RAY_ALPHA_BUCKETS = 20;
 /** Distance threshold (pixels) for image-convergence grid quantization. */
 export const RAY_CONVERGENCE_THRESHOLD = 5;
 
+/** Default length (px) of a ray stub in "ray stubs" display mode. */
+export const RAY_STUB_LENGTH_DEFAULT_PX = 50;
+/** Minimum allowed ray-stub length (px). */
+export const RAY_STUB_LENGTH_MIN_PX = 10;
+/** Maximum allowed ray-stub length (px). */
+export const RAY_STUB_LENGTH_MAX_PX = 200;
+
 // ── 5. Edit-panel UI ──────────────────────────────────────────────────────────
 
 export const PANEL_BOTTOM_MARGIN = 10;

src/common/view/BaseOpticalElementView.ts

@@ -114,8 +114,15 @@ export abstract class BaseOpticalElementView extends Node {
    */
   // biome-ignore lint/suspicious/noExplicitAny: linkAttribute requires any for the target object
   protected trackLinkAttribute<T>(property: TReadOnlyProperty<T>, object: any, attributeName: string): void {
-    const handle = property.linkAttribute(object, attributeName) as unknown as (value: unknown) => void;
-    this._linkedAttributes.push({ property: property as TReadOnlyProperty<unknown>, handle });
+    // linkAttribute() returns void, so we must store the listener explicitly.
+    const listener = (value: T) => {
+      object[attributeName] = value;
+    };
+    property.link(listener as unknown as (value: unknown) => void);
+    this._linkedAttributes.push({
+      property: property as TReadOnlyProperty<unknown>,
+      handle: listener as unknown as (value: unknown) => void,
+    });
   }
 
   /**

src/common/view/ElementRegistry.ts

@@ -12,6 +12,7 @@ import opticsLab from "../../OpticsLabNamespace.js";
 import type { OpticalElement } from "../model/optics/OpticsTypes.js";
 import type { OpticalElementView } from "./ElementRegistry.js";
 import { createOpticalElementView as createFromRegistry } from "./ElementRegistry.js";
+import type { ViewOptionsModel } from "./ViewOptionsModel.js";
 
 // Re-export so callers that previously imported OpticalElementView from this
 // file continue to work without any import-path changes.
@@ -25,8 +26,9 @@ export function createOpticalElementView(
   element: OpticalElement,
   modelViewTransform: ModelViewTransform2,
   tandem: Tandem,
+  viewOptions: ViewOptionsModel,
 ): OpticalElementView | null {
-  return createFromRegistry(element, modelViewTransform, tandem);
+  return createFromRegistry(element, modelViewTransform, tandem, viewOptions);
 }
 
 opticsLab.register("createOpticalElementView", createOpticalElementView);

src/common/view/OpticalElementViewFactory.ts

@@ -34,8 +34,7 @@ import {
 import opticsLab from "../../OpticsLabNamespace.js";
 import type { ViewMode } from "../model/optics/OpticsTypes.js";
 import type { TracedSegment } from "../model/optics/RayTracer.js";
-import { rayArrowsVisibleProperty } from "./RayArrowsVisibleProperty.js";
-import { rayStubLengthPxProperty, rayStubsEnabledProperty } from "./RayStubsProperty.js";
+import type { ViewOptionsModel } from "./ViewOptionsModel.js";
 
 // Cohen–Sutherland region codes
 const CS_INSIDE = 0;
@@ -122,17 +121,33 @@ export class RayPropagationView extends CanvasNode {
   private segments: TracedSegment[] = [];
   private mode: ViewMode = "rays";
   private readonly modelViewTransform: ModelViewTransform2;
-
-  public constructor(canvasBounds: Bounds2, modelViewTransform: ModelViewTransform2, options?: CanvasNodeOptions) {
+  private readonly viewOptions: ViewOptionsModel;
+  /** Shared listener stored so it can be unlinked in dispose(). */
+  private readonly _invalidatePaintListener = () => this.invalidatePaint();
+
+  public constructor(
+    canvasBounds: Bounds2,
+    modelViewTransform: ModelViewTransform2,
+    viewOptions: ViewOptionsModel,
+    options?: CanvasNodeOptions,
+  ) {
     super({
       canvasBounds,
       pickable: false, // rays are non-interactive
       ...options,
     });
     this.modelViewTransform = modelViewTransform;
-    rayArrowsVisibleProperty.lazyLink(() => this.invalidatePaint());
-    rayStubsEnabledProperty.lazyLink(() => this.invalidatePaint());
-    rayStubLengthPxProperty.lazyLink(() => this.invalidatePaint());
+    this.viewOptions = viewOptions;
+    viewOptions.rayArrowsVisibleProperty.lazyLink(this._invalidatePaintListener);
+    viewOptions.rayStubsEnabledProperty.lazyLink(this._invalidatePaintListener);
+    viewOptions.rayStubLengthPxProperty.lazyLink(this._invalidatePaintListener);
+  }
+
+  public override dispose(): void {
+    this.viewOptions.rayArrowsVisibleProperty.unlink(this._invalidatePaintListener);
+    this.viewOptions.rayStubsEnabledProperty.unlink(this._invalidatePaintListener);
+    this.viewOptions.rayStubLengthPxProperty.unlink(this._invalidatePaintListener);
+    super.dispose();
   }
 
   /**
@@ -176,15 +191,15 @@ export class RayPropagationView extends CanvasNode {
     } else {
       this.paintExtensionRays(context, segments, clipRect);
       this.paintForwardRays(context, segments, clipRect);
-      if (rayArrowsVisibleProperty.value && !rayStubsEnabledProperty.value) {
+      if (this.viewOptions.rayArrowsVisibleProperty.value && !this.viewOptions.rayStubsEnabledProperty.value) {
         this.paintArrowheads(context, segments, clipRect);
       }
     }
   }
 
   private paintExtensionRays(context: CanvasRenderingContext2D, segments: TracedSegment[], clipRect: ClipRect): void {
     // When ray stubs are enabled, suppress extension rays entirely.
-    if (rayStubsEnabledProperty.value) {
+    if (this.viewOptions.rayStubsEnabledProperty.value) {
       return;
     }
     const modelViewTransform = this.modelViewTransform;
@@ -241,8 +256,8 @@ export class RayPropagationView extends CanvasNode {
     const modelViewTransform = this.modelViewTransform;
     context.lineWidth = RAY_LINE_WIDTH;
 
-    const stubsEnabled = rayStubsEnabledProperty.value;
-    const stubLengthPx = rayStubLengthPxProperty.value;
+    const stubsEnabled = this.viewOptions.rayStubsEnabledProperty.value;
+    const stubLengthPx = this.viewOptions.rayStubLengthPxProperty.value;
 
     const paintSegment = (seg: TracedSegment, additive: boolean): void => {
       // In stub mode, only draw segments emitted directly from a light source.

src/common/view/RayPropagationView.ts

@@ -25,8 +25,8 @@ import {
 } from "../../OpticsLabConstants.js";
 import type { OpticalElement } from "../model/optics/OpticsTypes.js";
 import type { TracedSegment } from "../model/optics/RayTracer.js";
-import { handlesVisibleProperty } from "./HandlesVisibleProperty.js";
 import { createOpticalElementView, type OpticalElementView } from "./OpticalElementViewFactory.js";
+import { ViewOptionsModel } from "./ViewOptionsModel.js";
 
 /**
  * Scenery may introduce Canvas layers (full-size, cleared opaque) above SVG layers when the subtree mixes
@@ -291,8 +291,8 @@ export function downloadSceneSVG({
   segments,
   viewState,
 }: SceneSVGExportOptions): void {
-  const savedHandlesVisible = handlesVisibleProperty.value;
-  handlesVisibleProperty.value = viewState.showHandles;
+  const exportViewOptions = new ViewOptionsModel();
+  exportViewOptions.handlesVisibleProperty.value = viewState.showHandles;
 
   const exportWidth = Math.ceil(visibleBounds.width);
   const exportHeight = Math.ceil(visibleBounds.height);
@@ -340,7 +340,7 @@ export function downloadSceneSVG({
 
     const elementsLayer = new Node();
     for (const element of elements) {
-      const view = createOpticalElementView(element, exportMVT, Tandem.OPT_OUT);
+      const view = createOpticalElementView(element, exportMVT, Tandem.OPT_OUT, exportViewOptions);
       if (!view) {
         continue;
       }
@@ -419,7 +419,6 @@ export function downloadSceneSVG({
       display.dispose();
     }
   } finally {
-    handlesVisibleProperty.value = savedHandlesVisible;
     for (const overlayNode of exportOverlayNodes) {
       overlayNode.dispose();
     }

src/common/view/SceneSVGExporter.ts

@@ -21,18 +21,15 @@ import { CarouselPanel } from "./CarouselPanel.js";
 import type { ComponentKey } from "./ComponentCarousel.js";
 import { DetectorView } from "./detectors/DetectorView.js";
 import { EditContainerNode } from "./EditContainerNode.js";
-import { focalMarkersVisibleProperty } from "./FocalMarkersVisibleProperty.js";
-import { handlesVisibleProperty } from "./HandlesVisibleProperty.js";
 import { ImageOverlayNode } from "./ImageOverlayNode.js";
 import { InfoDialogNode } from "./InfoDialogNode.js";
 import { ObserverNode } from "./ObserverNode.js";
 import { createOpticalElementView, type OpticalElementView } from "./OpticalElementViewFactory.js";
-import { rayArrowsVisibleProperty } from "./RayArrowsVisibleProperty.js";
 import { RayPropagationView } from "./RayPropagationView.js";
-import { rayStubsEnabledProperty } from "./RayStubsProperty.js";
 import { sceneHistoryRegistry } from "./SceneHistoryRegistry.js";
 import { downloadSceneSVG } from "./SceneSVGExporter.js";
 import { ToolsPanel } from "./ToolsPanel.js";
+import { ViewOptionsModel } from "./ViewOptionsModel.js";
 import { viewSnapState } from "./ViewSnapState.js";
 
 /**
@@ -83,6 +80,10 @@ function tryHandleToolsPanelShortcut(
     extendedRaysProperty: BooleanProperty;
     gridVisibleProperty: BooleanProperty;
     snapToGridProperty: BooleanProperty;
+    handlesVisibleProperty: BooleanProperty;
+    focalMarkersVisibleProperty: BooleanProperty;
+    rayArrowsVisibleProperty: BooleanProperty;
+    rayStubsEnabledProperty: BooleanProperty;
   },
 ): boolean {
   if (isTextInput || event.ctrlKey || event.metaKey || event.altKey || event.key.length !== 1) {
@@ -95,6 +96,10 @@ function tryHandleToolsPanelShortcut(
     extendedRaysProperty,
     gridVisibleProperty,
     snapToGridProperty,
+    handlesVisibleProperty,
+    focalMarkersVisibleProperty,
+    rayArrowsVisibleProperty,
+    rayStubsEnabledProperty,
   } = locals;
   if (letter === "m") {
     measuringTapeVisibleProperty.toggle();
@@ -178,6 +183,9 @@ export class RayTracingCommonView extends ScreenView {
   /** Maps element id → view so we can remove views and provide rebuild callbacks. */
   private readonly elementViewMap = new Map<string, OpticalElementView>();
 
+  /** Per-screen view state (handles visibility, ray arrows, focal markers, etc.). */
+  protected readonly viewOptions: ViewOptionsModel;
+
   /**
    * Maps element id → its Tandem so we can remove it from the global tandem tree on deletion.
    * RichDragListener is not a PhetioObject, so its tandem's dispose() is never triggered
@@ -230,6 +238,8 @@ export class RayTracingCommonView extends ScreenView {
     const tandem = options?.tandem;
     const uiStrings = StringManager.getInstance().getUIStrings();
 
+    this.viewOptions = new ViewOptionsModel(tandem?.createTandem("viewOptions"));
+
     // ── Model-View Transform ────────────────────────────────────────────────
     // Maps model origin (0, 0) to the centre of the visible play area.
     // 100 px = 1 m; y-axis is inverted (model +y = up, view +y = down).
@@ -316,7 +326,11 @@ export class RayTracingCommonView extends ScreenView {
     viewSnapState.setSnapToGrid(snapToGridProperty);
 
     // ── Ray Propagation Layer (behind elements so rays don't block handles) ─
-    this.rayPropagationView = new RayPropagationView(this.visibleBoundsProperty.value, modelViewTransform);
+    this.rayPropagationView = new RayPropagationView(
+      this.visibleBoundsProperty.value,
+      modelViewTransform,
+      this.viewOptions,
+    );
     this.visibleBoundsProperty.link((visibleBounds) => {
       this.rayPropagationView.canvasBounds = visibleBounds;
     });
@@ -398,7 +412,7 @@ export class RayTracingCommonView extends ScreenView {
         // view already exists and skips duplicate creation.
         const tandemName = element.id.replace(/-(\d+)$/, (_, n) => n);
         const elementTandem = tandem?.createTandem(tandemName) ?? Tandem.OPTIONAL;
-        const view = createOpticalElementView(element, modelViewTransform, elementTandem);
+        const view = createOpticalElementView(element, modelViewTransform, elementTandem, this.viewOptions);
         if (view) {
           this.elementTandemMap.set(element.id, elementTandem);
           this._setupView(element, view);
@@ -435,7 +449,7 @@ export class RayTracingCommonView extends ScreenView {
     for (const element of model.scene.getAllElements()) {
       const tandemName = element.id.replace(/-(\d+)$/, (_, n) => n);
       const elementTandem = tandem?.createTandem(tandemName) ?? Tandem.OPTIONAL;
-      const elementView = createOpticalElementView(element, modelViewTransform, elementTandem);
+      const elementView = createOpticalElementView(element, modelViewTransform, elementTandem, this.viewOptions);
       if (elementView) {
         this.elementTandemMap.set(element.id, elementTandem);
         this._setupView(element, elementView);
@@ -452,7 +466,7 @@ export class RayTracingCommonView extends ScreenView {
       }
       const tn = element.id.replace(/-(\d+)$/, (_, n: string) => n);
       const et = tandem?.createTandem(tn) ?? Tandem.OPTIONAL;
-      const view = createOpticalElementView(element, modelViewTransform, et);
+      const view = createOpticalElementView(element, modelViewTransform, et, this.viewOptions);
       if (view) {
         this.elementTandemMap.set(element.id, et);
         this._setupView(element, view);
@@ -492,6 +506,7 @@ export class RayTracingCommonView extends ScreenView {
       this.selectedElementProperty,
       this.visibleBoundsProperty,
       _opticsLabPreferences.snapToGridProperty,
+      this.viewOptions,
       tandem,
     );
     this.addChild(toolsPanel);
@@ -526,7 +541,7 @@ export class RayTracingCommonView extends ScreenView {
           viewState: {
             showGrid: model.scene.showGridProperty.value,
             gridSpacing: model.scene.gridSizeProperty.value,
-            showHandles: handlesVisibleProperty.value,
+            showHandles: this.viewOptions.handlesVisibleProperty.value,
             measuringTape: {
               visible: toolsPanel.measuringTapeVisibleProperty.value,
               basePosition: toolsPanel.measuringTapeNode.basePositionProperty.value.copy(),
@@ -618,6 +633,10 @@ export class RayTracingCommonView extends ScreenView {
           extendedRaysProperty: toolsPanel.extendedRaysProperty,
           gridVisibleProperty: model.scene.showGridProperty,
           snapToGridProperty: _opticsLabPreferences.snapToGridProperty,
+          handlesVisibleProperty: this.viewOptions.handlesVisibleProperty,
+          focalMarkersVisibleProperty: this.viewOptions.focalMarkersVisibleProperty,
+          rayArrowsVisibleProperty: this.viewOptions.rayArrowsVisibleProperty,
+          rayStubsEnabledProperty: this.viewOptions.rayStubsEnabledProperty,
         })
       ) {
         event.preventDefault();
@@ -667,6 +686,7 @@ export class RayTracingCommonView extends ScreenView {
   public override dispose(): void {
     window.removeEventListener("keydown", this._handleKeyDown);
     super.dispose();
+    this.viewOptions.dispose();
   }
 
   public override step(_dt: number): void {

src/common/view/SimScreenView.ts

@@ -58,11 +58,7 @@ import {
 import opticsLabQueryParameters from "../../preferences/opticsLabQueryParameters.js";
 import type { OpticalElement } from "../model/optics/OpticsTypes.js";
 import type { RayTracingCommonModel } from "../model/SimModel.js";
-import { focalMarkersVisibleProperty } from "./FocalMarkersVisibleProperty.js";
-import { handlesVisibleProperty } from "./HandlesVisibleProperty.js";
 import { DEFAULT_OBSERVER } from "./ObserverNode.js";
-import { rayArrowsVisibleProperty } from "./RayArrowsVisibleProperty.js";
-import { rayStubsEnabledProperty } from "./RayStubsProperty.js";
 import {
   dragHandleIcon,
   extendedRaysIcon,
@@ -77,6 +73,7 @@ import {
   showImagesIcon,
   snapToGridIcon,
 } from "./ToolsPanelIcons.js";
+import type { ViewOptionsModel } from "./ViewOptionsModel.js";
 
 export class ToolsPanel extends Node {
   /** Draggable measuring-tape overlay – add to scene in the desired z-order. */
@@ -91,6 +88,8 @@ export class ToolsPanel extends Node {
   public readonly measuringTapeVisibleProperty: BooleanProperty;
   public readonly protractorVisibleProperty: BooleanProperty;
 
+  private readonly viewOptions: ViewOptionsModel;
+
   /**
    * Whether "Extended Rays" mode is active.  Changing this property updates
    * model.scene.modeProperty; conversely, external modeProperty changes update
@@ -104,9 +103,11 @@ export class ToolsPanel extends Node {
     selectedElementProperty: Property<OpticalElement | null>,
     visibleBoundsProperty: ReadOnlyProperty<Bounds2>,
     snapToGridProperty: Property<boolean>,
+    viewOptions: ViewOptionsModel,
     tandem: Tandem | undefined,
   ) {
     super();
+    this.viewOptions = viewOptions;
 
     const strings = StringManager.getInstance();
     const uiStrings = strings.getUIStrings();
@@ -274,22 +275,22 @@ export class ToolsPanel extends Node {
         { ...checkboxOptions, ...cbTandem("observerModeCheckbox") },
       ),
       new Checkbox(
-        handlesVisibleProperty,
+        viewOptions.handlesVisibleProperty,
         makeCheckboxContent(dragHandleIcon(), uiStrings.showHandlesStringProperty, labelOptions),
         { ...checkboxOptions, ...cbTandem("showHandlesCheckbox") },
       ),
       new Checkbox(
-        focalMarkersVisibleProperty,
+        viewOptions.focalMarkersVisibleProperty,
         makeCheckboxContent(focalPointIcon(), uiStrings.focalMarkersStringProperty, labelOptions),
         { ...checkboxOptions, ...cbTandem("focalMarkersCheckbox") },
       ),
       new Checkbox(
-        rayArrowsVisibleProperty,
+        viewOptions.rayArrowsVisibleProperty,
         makeCheckboxContent(rayArrowsIcon(), uiStrings.showRayArrowsStringProperty, labelOptions),
         { ...checkboxOptions, ...cbTandem("rayArrowsCheckbox") },
       ),
       new Checkbox(
-        rayStubsEnabledProperty,
+        viewOptions.rayStubsEnabledProperty,
         makeCheckboxContent(rayStubsIcon(), uiStrings.rayStubsStringProperty, labelOptions),
         { ...checkboxOptions, ...cbTandem("rayStubsCheckbox") },
       ),
@@ -343,10 +344,7 @@ export class ToolsPanel extends Node {
     this.extendedRaysProperty.reset();
     this.measuringTapeVisibleProperty.reset();
     this.protractorVisibleProperty.reset();
-    handlesVisibleProperty.reset();
-    focalMarkersVisibleProperty.reset();
-    rayArrowsVisibleProperty.reset();
-    rayStubsEnabledProperty.reset();
+    this.viewOptions.reset();
     this.measuringTapeNode.basePositionProperty.reset();
     this.measuringTapeNode.tipPositionProperty.reset();
     this.protractorNode.angleProperty.reset();