Factor out a parent BambooPlot type

[samreid]

From #34, the duplicated code between e.g., BarPlot and UpDownArrowPlot should be factored out. Using TypeScript, we would probably use an abstract update method.

[samreid]

In #37 @pixelzoom asked:

I have been wondering if there should be a Plot.js base class. There's starting to be duplication, as well as some undesirable variation, amoung the plot classes. And if you're trying to create a new plot class, it's not totally clear what the required interface/API is.

[samreid]

I factored out AbstractPlot and it is working great for BarPlot and will work well for UpDownArrowPlot:

Details

Index: js/AbstractPlot.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/AbstractPlot.js b/js/AbstractPlot.js
new file mode 100644
--- /dev/null	(date 1625952601309)
+++ b/js/AbstractPlot.js	(date 1625952601309)
@@ -0,0 +1,67 @@
+// Copyright 2020-2021, University of Colorado Boulder
+
+/**
+ * Abstract Plot base class that supports ChartTransform, data set and abstract method for update()
+ *
+ * @author Sam Reid (PhET Interactive Simulations)
+ */
+
+import Node from '../../scenery/js/nodes/Node.js';
+import bamboo from './bamboo.js';
+
+class AbstractPlot extends Node {
+
+  /**
+   * @param {ChartTransform} chartTransform
+   * @param {Array.<Vector2|null>} dataSet
+   * @param {Object} [options]
+   */
+  constructor( chartTransform, dataSet, options ) {
+
+    super( options );
+
+    // @private
+    this.chartTransform = chartTransform;
+
+    // @public if you change this directly, you are responsible for calling update
+    this.dataSet = dataSet;
+
+    // Update when the transform changes.
+    const changedListener = () => this.update();
+    chartTransform.changedEmitter.addListener( changedListener );
+
+    // @private
+    this.disposeAbstractPlot = () => chartTransform.changedEmitter.removeListener( changedListener );
+  }
+
+  /**
+   * Sets the dataSet and redraws the plot. If instead the dataSet array is mutated, it is the client's responsibility
+   * to call `update` or make sure `update` is called elsewhere (say, if the chart scrolls in that frame).
+   * @param {Vector2[]} dataSet
+   * @public
+   */
+  setDataSet( dataSet ) {
+    this.dataSet = dataSet;
+    this.update();
+  }
+
+  /**
+   * @public
+   * @abstract
+   */
+  update() {
+    throw new Error( 'update should be implemented in a subtype' );
+  }
+
+  /**
+   * @public
+   * @override
+   */
+  dispose() {
+    this.disposeAbstractPlot();
+    super.dispose();
+  }
+}
+
+bamboo.register( 'AbstractPlot', AbstractPlot );
+export default AbstractPlot;
\ No newline at end of file
Index: js/BarPlot.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/BarPlot.js b/js/BarPlot.js
--- a/js/BarPlot.js	(revision ba5302cf7dd0df74983d8a4b671db0ee8699a6ab)
+++ b/js/BarPlot.js	(date 1625952685151)
@@ -8,19 +8,19 @@
 
 import Vector2 from '../../dot/js/Vector2.js';
 import merge from '../../phet-core/js/merge.js';
-import Node from '../../scenery/js/nodes/Node.js';
 import Paintable from '../../scenery/js/nodes/Paintable.js';
 import Rectangle from '../../scenery/js/nodes/Rectangle.js';
+import AbstractPlot from './AbstractPlot.js';
 import bamboo from './bamboo.js';
 
 // constants
 const DEFAULT_PAINTABLE_OPTIONS = { fill: 'black' };
 
-class BarPlot extends Node {
+class BarPlot extends AbstractPlot {
 
   /**
    * @param {ChartTransform} chartTransform
-   * @param {Vector2[]} dataSet
+   * @param {Array.<Vector2|null>} dataSet
    * @param {Object} [options]
    */
   constructor( chartTransform, dataSet, options ) {
@@ -34,13 +34,7 @@
       pointToPaintableFields: point => DEFAULT_PAINTABLE_OPTIONS
     }, options );
 
-    super( options );
-
-    // @private
-    this.chartTransform = chartTransform;
-
-    // @public if you change this directly, you are responsible for calling update
-    this.dataSet = dataSet;
+    super( chartTransform, dataSet, options );
 
     // @private
     this.barWidth = options.barWidth;
@@ -50,24 +44,7 @@
 
     // @private {Rectangle[]}
     this.rectangles = [];
-    this.setDataSet( dataSet );
-
-    // Update when the transform changes.
-    const changedListener = () => this.update();
-    chartTransform.changedEmitter.addListener( changedListener );
 
-    // @private
-    this.disposeBarPlot = () => chartTransform.changedEmitter.removeListener( changedListener );
-  }
-
-  /**
-   * Sets the dataSet and redraws the plot. If instead the dataSet array is mutated, it is the client's responsibility
-   * to call `update` or make sure `update` is called elsewhere (say, if the chart scrolls in that frame).
-   * @param {Vector2[]} dataSet
-   * @public
-   */
-  setDataSet( dataSet ) {
-    this.dataSet = dataSet;
     this.update();
   }
 
@@ -107,15 +84,6 @@
       this.rectangles[ i ].mutate( providedOptions );
     }
   }
-
-  /**
-   * @public
-   * @override
-   */
-  dispose() {
-    this.disposeBarPlot();
-    super.dispose();
-  }
 }
 
 bamboo.register( 'BarPlot', BarPlot );

However, it cannot be used for any of the other Plots because they already extend something else. class CanvasLinePlot extends CanvasPainter , class LinearEquationPlot extends Line, class LinePlot extends Path, class ScatterPlot extends Path. @pixelzoom how do you recommend to proceed?

[pixelzoom]

I was going to suggest using composition for those other plot classes. That would work for LinearEquationPlot, LinePlot, and ScatterPlot -- and would be an improvement imo, because it wouldn't expose the APIs of Line and Path. But I see that ChartCanvasNode requires its plots to be CanvasPainter, and I'm not sure what to do there.

@samreid what do you think about using composition where possible? And does that spark ideas for how to address CanvasPainter?

[samreid]

I was going to suggest using composition for those other plot classes.

For instance, would LinearEquationPlot extend Plot and contain a Line, or vice versa?

[pixelzoom]

The former.

[samreid]

It also seems nice to keep the Plots as Nodes themselves, so usage reads like:

// Anything you want clipped goes in here
const chartClip = new Node( {
  clipArea: chartRectangle.getShape(),
  children: [
    new BarPlot( chartTransform, dataSet, {
      opacity: 0.1
    } ),
    new LinePlot( chartTransform, dataSet, {
      stroke: 'red',
      lineWidth: 2
    } )
  ]
} );

Instead of:

// Anything you want clipped goes in here
const chartClip = new Node( {
  children: [
    new BarPlot( chartTransform, dataSet, {
      opacity: 0.1
    } ).getNode(),
    new LinePlot( chartTransform, dataSet, {
      stroke: 'red',
      lineWidth: 2
    } ).getNode()
  ]
} );

@pixelzoom does this sound reasonable to you, that we would have a type like PlotNode extends Node and adds content via its Node children? On the other hand, this approach isn't compatible with CanvasPainter which should not be a Node. Another way to factor out this code would be with a mixin, but I don't want to add any more mixins until we are using TypeScript.