CODAP-1504: draw and hit test point shapes in the canvas renderer

v3/src/components/data-display/data-display-types.ts

@@ -4,6 +4,8 @@ import {GraphPlace} from "../axis-graph-shared"
 import { ICase } from "../../models/data/data-set-types"
 
 export type Point = { x: number, y: number }
+// width and height without a position, for sizing something against a box
+export type Extent = { w: number, h: number }
 export type CPLine = { slope: number, intercept: number, pivot1?: Point, pivot2?: Point }
 export const kNullPoint = {x: -999, y: -999}
 

v3/src/components/data-display/data-display-utils.test.ts

@@ -8,7 +8,7 @@ import {
 } from "../../utilities/color-utils"
 import { GraphDataConfigurationModel } from "../graph/models/graph-data-configuration-model"
 import { IPointStyle, PointRendererBase } from "./renderer"
-import { getCasesForDelta, setPointSelection } from "./data-display-utils"
+import { getCasesForDelta, matchCirclesToData, setPointSelection } from "./data-display-utils"
 
 const TreeModel = types.model("Tree", {
   data: DataSet,
@@ -300,3 +300,68 @@ describe("getCasesForDelta", () => {
     expect(getCasesForDelta(null, { x: 0, y: 0, w: 1, h: 1 }, { x: 0, y: 0, w: 1, h: 1 })).toEqual([])
   })
 })
+
+describe("matchCirclesToData", () => {
+  /*
+   * This is where a point is created, and several callers create points without refreshing in the
+   * same breath -- they rely on the resulting case-data change to trigger a refresh later. A point
+   * born without a shape is drawn as a circle until that happens.
+   */
+  it("creates points with the display's shape, as it does with its color", () => {
+    const tree = TreeModel.create({ data: {}, metadata: {}, config: {} })
+    tree.data.addAttribute({ id: "xId", name: "x" })
+    tree.metadata.setData(tree.data)
+    tree.data.addCases(toCanonical(tree.data, [{ __id__: "c1", x: 1 }]))
+    tree.config.setDataset(tree.data, tree.metadata)
+
+    let defaultStyle: Partial<IPointStyle> | undefined
+    const renderer = {
+      matchPointsToData: (_dataId: string, _cases: any, _type: any, style: Partial<IPointStyle>) => {
+        defaultStyle = style
+      }
+    } as unknown as PointRendererBase
+
+    matchCirclesToData({
+      dataConfiguration: tree.config,
+      renderer,
+      pointRadius: 5,
+      pointColor: "#123456",
+      pointShape: "star",
+      pointStrokeColor: "#000000",
+      startAnimation: jest.fn(),
+      stopAnimation: jest.fn(),
+      instanceId: "test"
+    })
+
+    expect(defaultStyle?.shape).toBe("star")
+    expect(defaultStyle?.fill).toBe("#123456")
+  })
+
+  it("creates circles when no shape is supplied", () => {
+    const tree = TreeModel.create({ data: {}, metadata: {}, config: {} })
+    tree.data.addAttribute({ id: "xId", name: "x" })
+    tree.metadata.setData(tree.data)
+    tree.data.addCases(toCanonical(tree.data, [{ __id__: "c1", x: 1 }]))
+    tree.config.setDataset(tree.data, tree.metadata)
+
+    let defaultStyle: Partial<IPointStyle> | undefined
+    const renderer = {
+      matchPointsToData: (_dataId: string, _cases: any, _type: any, style: Partial<IPointStyle>) => {
+        defaultStyle = style
+      }
+    } as unknown as PointRendererBase
+
+    matchCirclesToData({
+      dataConfiguration: tree.config,
+      renderer,
+      pointRadius: 5,
+      pointColor: "#123456",
+      pointStrokeColor: "#000000",
+      startAnimation: jest.fn(),
+      stopAnimation: jest.fn(),
+      instanceId: "test"
+    })
+
+    expect(defaultStyle?.shape).toBe("circle")
+  })
+})

v3/src/components/data-display/data-display-utils.ts

@@ -6,6 +6,7 @@ import {
   defaultStrokeOpacity, defaultStrokeWidth
 } from "../../utilities/color-utils"
 import {between} from "../../utilities/math-utils"
+import { kDefaultPointShape, PointShape } from "../../utilities/point-shape-utils"
 import { IBarCover } from "../graph/graphing-types"
 import {isGraphDataConfigurationModel} from "../graph/models/graph-data-configuration-model"
 import {ISetPointSelection} from "../graph/utilities/graph-utils"
@@ -14,6 +15,7 @@ import {
   pointRadiusSelectionAddend, Rect, rTreeRect
 } from "./data-display-types"
 import {IDataConfigurationModel } from "./models/data-configuration-model"
+import { IDisplayItemDescriptionModel } from "./models/display-item-description-model"
 import {CaseDataWithSubPlot} from "./d3-types"
 import { getRendererForEvent, IPoint, IPointStyle, PointRendererBase } from "./renderer"
 
@@ -76,10 +78,26 @@ export const handleClickOnBar = ({ event, dataConfig, barCover }: IHandleClickOn
   setOrExtendSelection(barCover.caseIDs, dataConfig.dataset, extendSelection)
 }
 
+/*
+ * The shape for each case: the one its legend category carries, and the display's own wherever the
+ * legend assigns none. Every path that draws points needs this, so it is built here rather than
+ * rebuilt identically at each of them.
+ */
+export function legendShapeGetter(
+  dataConfig: IDataConfigurationModel | undefined, displayItemDescription: IDisplayItemDescriptionModel
+): (caseID: string) => PointShape {
+  const shapeIfNoCategory = displayItemDescription.pointShape
+  return (caseID: string) =>
+    dataConfig?.getLegendShapeForCase(caseID, shapeIfNoCategory) ?? shapeIfNoCategory
+}
+
 export interface IMatchCirclesProps {
   dataConfiguration: IDataConfigurationModel
   pointRadius: number
   pointColor: string
+  // The shape a point is created with, as pointColor is the color it is created with. Several
+  // callers create points without refreshing in the same breath.
+  pointShape?: PointShape
   pointDisplayType?: PointDisplayType
   pointStrokeColor: string
   startAnimation: () => void
@@ -90,7 +108,7 @@ export interface IMatchCirclesProps {
 
 export function matchCirclesToData(props: IMatchCirclesProps) {
   const { dataConfiguration, renderer, startAnimation, stopAnimation, pointRadius, pointColor, pointStrokeColor,
-          pointDisplayType = "points" } = props
+          pointShape = kDefaultPointShape, pointDisplayType = "points" } = props
   // TODO: eliminate dependence on GraphDataConfigurationModel
   const allCaseData: CaseDataWithSubPlot[] = isGraphDataConfigurationModel(dataConfiguration)
     ? dataConfiguration.caseDataWithSubPlot
@@ -111,6 +129,7 @@ export function matchCirclesToData(props: IMatchCirclesProps) {
   renderer?.matchPointsToData(dataConfiguration.dataset?.id ?? '', allCaseData, pointDisplayType, {
     radius: pointRadius,
     fill: pointColor,
+    shape: pointShape,
     stroke: pointStrokeColor,
     strokeWidth: defaultStrokeWidth
   })
@@ -130,7 +149,7 @@ export function setPointSelection(
   props: ISetPointSelection, caseIdsToUpdate?: Iterable<string>, numberOfPlots = 1
 ) {
   const { renderer, dataConfiguration, pointRadius, selectedPointRadius,
-    pointColor, pointStrokeColor, getPointColorAtIndex } = props
+    pointColor, pointStrokeColor, pointShape, getPointColorAtIndex } = props
   const dataset = dataConfiguration.dataset
   const legendID = dataConfiguration.attributeID('legend')
   if (!renderer) {
@@ -149,6 +168,7 @@ export function setPointSelection(
     // When there's no legend, use blue fill for selection instead of a colored stroke
     const useSelectionFill = isSelected && !legendID
     const style: Partial<IPointStyle> = {
+      shape: dataConfiguration.getLegendShapeForCase(caseID, pointShape),
       fill: useSelectionFill ? defaultSelectedColor : fill,
       radius: isSelected ? selectedPointRadius : pointRadius,
       stroke: isSelected && !useSelectionFill ? defaultSelectedStroke : pointStrokeColor,

v3/src/components/data-display/inspector/display-item-format-control.scss

@@ -297,6 +297,30 @@
       }
     }
 
+    // A row carrying both a shape control and a color control -- every category row, and the
+    // single Points row shown when there is no legend attribute. Without this the row distributes
+    // with space-between and each control starts wherever its label happens to end, so the rows
+    // do not line up with each other.
+    .cat-color-picker,
+    .color-picker-row.shape-row {
+      gap: 4px;
+
+      // Only the label may shrink; the controls keep their size.
+      .react-aria-Select {
+        flex: 0 0 auto;
+      }
+
+      // Absorbs the free space, which pins both controls to the right edge. Truncates rather than
+      // pushing them out of view.
+      .form-label.color-picker {
+        flex: 1;
+        min-width: 0;
+        overflow: hidden;
+        text-overflow: ellipsis;
+        white-space: nowrap;
+      }
+    }
+
     .cat-color-setting {
       // A flex column, so the rows are flex items whose vertical margins do not collapse and each
       // row occupies the full height the 2.5-row limit below is derived from. The list ends
@@ -333,22 +357,6 @@
       .cat-color-picker {
         flex-shrink: 0; // rows keep their height, so the list scrolls rather than compressing
         margin: vars.$palette-category-row-margin 0;
-        gap: 4px;
-
-        // Only the label may shrink; the controls keep their size.
-        .react-aria-Select {
-          flex: 0 0 auto;
-        }
-
-        // Absorbs the free space so the controls align across rows rather than each starting
-        // wherever its category name happens to end. Truncates rather than pushing them out.
-        .form-label.color-picker {
-          flex: 1;
-          min-width: 0;
-          overflow: hidden;
-          text-overflow: ellipsis;
-          white-space: nowrap;
-        }
       }
     }
 

v3/src/components/data-display/inspector/legend-color-controls.test.tsx

@@ -651,7 +651,7 @@ describe("point shape controls", () => {
       )
 
       expect(screen.getByTestId("point-shape-select")).toBeInTheDocument()
-      // the legend's own colour controls are still there
+      // the legend's own color controls are still there
       expect(screen.getByTestId("color-swatch-DG.Inspector.legendColorLow")).toBeInTheDocument()
     })
 

v3/src/components/data-display/inspector/legend-color-controls.tsx

@@ -182,6 +182,9 @@ export const LegendColorControls = observer(function LegendColorControls(
         categories={categoriesRef.current}
         dataConfiguration={dataConfiguration}
         showShape={showShape}
+        // A category with no shape of its own draws the display's, so the control has to show that
+        // rather than the bare default, or it would disagree with the plot.
+        shapeIfUnset={displayItemDescription.pointShape}
         onCatPointColorChange={handleCatPointColorChange}
         onCatPointShapeChange={handleCatPointShapeChange}
       />
@@ -224,7 +227,8 @@ export const LegendColorControls = observer(function LegendColorControls(
   // every point and sit in a single row.
   const singleRowLabel = showShape ? t("V3.Inspector.points") : t("DG.Inspector.color")
   return (
-    <div className="palette-row color-picker-row">
+    // shape-row marks a row carrying both controls, so it aligns them the way the category rows do
+    <div className={clsx("palette-row", "color-picker-row", { "shape-row": showShape })}>
       <label className="form-label color-picker">{singleRowLabel}</label>
       <If condition={showShape}>
         <PointShapeSetting propertyLabel={t("V3.Inspector.pointShape")}
@@ -243,12 +247,13 @@ interface ICategoricalColorControlsProps {
   categories?: string[]
   dataConfiguration: IDataConfigurationModel
   showShape: boolean
+  shapeIfUnset: PointShape
   onCatPointColorChange: (color: string, cat: string) => void
   onCatPointShapeChange: (shape: PointShape, cat: string) => void
 }
 
 const CategoricalColorControls = observer(function CategoricalColorControls(
-  { categories, dataConfiguration, showShape, onCatPointColorChange, onCatPointShapeChange }:
+  { categories, dataConfiguration, showShape, shapeIfUnset, onCatPointColorChange, onCatPointShapeChange }:
     ICategoricalColorControlsProps
 ) {
   const [scrollVersion, setScrollVersion] = useState(0)
@@ -265,7 +270,7 @@ const CategoricalColorControls = observer(function CategoricalColorControls(
           <If condition={showShape}>
             <PointShapeSetting propertyLabel={category}
               closeTrigger={scrollVersion}
-              shape={dataConfiguration.getLegendShapeForCategory(category)}
+              shape={dataConfiguration.getLegendShapeForCategory(category, shapeIfUnset)}
               color={dataConfiguration.getLegendColorForCategory(category)}
               onShapeChange={(shape) => onCatPointShapeChange(shape, category)}/>
           </If>

v3/src/components/data-display/inspector/point-shape-setting.tsx

@@ -117,7 +117,7 @@ export const PointShapeSetting = observer(function PointShapeSetting({
         * trigger it always appears, clipping and scrolling within its own max-height, which is
         * the lesser failure.
         *
-        * The colour picker in this same palette disables flipping for the same reason, so the
+        * The color picker in this same palette disables flipping for the same reason, so the
         * fault lies in how popovers position within the palette rather than in this control.
         */}
       <Popover shouldFlip={false}

v3/src/components/data-display/models/data-configuration-model.ts

@@ -741,9 +741,11 @@ export const DataConfigurationModel = types
         return categorySet?.colorForCategory(cat) ?? missingColor
       },
 
-      getLegendShapeForCategory(cat: string): PointShape {
+      // `shapeIfUnset` is the display's own shape, so a category the user has not assigned one to
+      // keeps drawing whatever the plot drew before a legend was added.
+      getLegendShapeForCategory(cat: string, shapeIfUnset: PointShape = kDefaultPointShape): PointShape {
         const categorySet = self.categorySetForAttrRole('legend')
-        return categorySet?.shapeForCategory(cat) ?? kDefaultPointShape
+        return categorySet?.shapeForCategory(cat, shapeIfUnset) ?? shapeIfUnset
       },
 
       getLegendColorForNumericValue(value: number): string {
@@ -858,6 +860,32 @@ export const DataConfigurationModel = types
       casesInBinAreSelected(quantile: number): boolean {
         const selection = self.getCasesForLegendBin(quantile)
         return !!(selection.length > 0 && selection?.every((anID: string) => self.dataset?.isCaseSelected(anID)))
+      },
+      /*
+       * Whether the legend assigns per-category state. Note this is narrower than
+       * isCategoricalAttributeType, which counts a color legend too: that one gives each case a
+       * color of its own, so there are no categories to hang a color or a shape on.
+       */
+      get legendHasCategories(): boolean {
+        const legendType = self.attributeType('legend')
+        return legendType === 'categorical' || legendType === 'checkbox'
+      },
+      /*
+       * Whether the legend attribute lives in a collection more childmost than the plotted cases.
+       * A point then stands for several children at once, and resolving the legend through any one
+       * of them would attribute that child's value to the whole group, so callers fall back.
+       *
+       * Must stay a block above the views that use it, so they reach it through `self`. They are
+       * passed around as detached function references -- a plot hands `getLegendColorForCase` to
+       * setPointCoordinates, which calls it bare -- so `this` inside them is undefined.
+       */
+      get legendCollectionIsMoreChildmost(): boolean {
+        const legendID = self.attributeID('legend')
+        const legendCollectionID = self.dataset?.getCollectionForAttribute(legendID)?.id
+        const legendCollectionIndex = self.dataset?.getCollectionIndex(legendCollectionID) ?? 0
+        const childmostCollectionID = idOfChildmostCollectionForAttributes(self.axisAttributeIDs, self.dataset)
+        const childmostCollectionIndex = self.dataset?.getCollectionIndex(childmostCollectionID) ?? 0
+        return legendCollectionIndex > childmostCollectionIndex
       }
     }))
   .views(self => (
@@ -890,6 +918,12 @@ export const DataConfigurationModel = types
        * For categorical it is a map of categories to colors
        * The color type is not handled yet.
        */
+      // Changes identity when any category's shape changes, so a display can react to it the way it
+      // reacts to legendColorDomain.
+      get legendShapeDomain() {
+        if (!self.legendHasCategories) return undefined
+        return self.categorySetForAttrRole('legend')?.shapeMap
+      },
       get legendColorDomain() {
         const legendType = self.attributeType('legend')
         switch (legendType) {
@@ -908,23 +942,14 @@ export const DataConfigurationModel = types
         }
       },
       getLegendColorForCase(id: string, colorIfMissing = missingColor): string {
-
-        const collectionOfLegendIsMoreChildmost = () => {
-          const legendCollectionID = self.dataset?.getCollectionForAttribute(legendID)?.id,
-            legendCollectionIndex = self.dataset?.getCollectionIndex(legendCollectionID) ?? 0,
-            childmostCollectionID = idOfChildmostCollectionForAttributes(self.axisAttributeIDs, self.dataset),
-            childmostCollectionIndex = self.dataset?.getCollectionIndex(childmostCollectionID) ?? 0
-          return legendCollectionIndex > childmostCollectionIndex
-        }
-
         const legendID = self.attributeID('legend')
         // todo: When user deletes we are not currently deleting the legend attribute ID. But we should.
         const legendAttribute = self.dataset?.getAttribute(legendID)
         if (!id || !legendID || !legendAttribute) {
           return ''
         }
         const legendType = self.attributeType('legend')
-        if (collectionOfLegendIsMoreChildmost()) {
+        if (self.legendCollectionIsMoreChildmost) {
           return colorIfMissing
         }
         const legendValue = self.dataset?.getStrValue(id, legendID)
@@ -945,6 +970,22 @@ export const DataConfigurationModel = types
           default:
             return ''
         }
+      },
+      // `shapeIfNoCategory` is the display's own shape, which is what a plot with no legend draws
+      // throughout.
+      getLegendShapeForCase(id: string, shapeIfNoCategory: PointShape = kDefaultPointShape): PointShape {
+        const legendID = self.attributeID('legend')
+        const legendAttribute = self.dataset?.getAttribute(legendID)
+        if (!id || !legendID || !legendAttribute) return shapeIfNoCategory
+
+        if (!self.legendHasCategories) return shapeIfNoCategory
+
+        if (self.legendCollectionIsMoreChildmost) return shapeIfNoCategory
+
+        const legendValue = self.dataset?.getStrValue(id, legendID)
+        if (!legendValue) return shapeIfNoCategory
+
+        return self.getLegendShapeForCategory(legendValue, shapeIfNoCategory)
       }
     }))
   .actions(self => ({