CODAP-1505: store point shape per legend category

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

@@ -1,7 +1,8 @@
 import {
   changeAttributeColorNotification, changeLegendBinCountNotification, changeLegendBinsTypeNotification,
   changeLegendRangeNotification, changePointColorAndAlphaNotification, changePointColorNotification,
-  changePointSizeNotification, changeStrokeColorAndAlphaNotification, swapCategoriesNotification,
+  changePointShapeNotification, changePointSizeNotification, changeStrokeColorAndAlphaNotification,
+  swapCategoriesNotification,
   toggleStrokeSameAsFillNotification
 } from "./data-display-notifications"
 
@@ -57,6 +58,38 @@ describe("swapCategoriesNotification", () => {
   })
 })
 
+describe("changePointShapeNotification", () => {
+  it("emits 'change point shape' with the category when one is assigned", () => {
+    const tile = { id: "GRAPH1", content: { type: "Graph" } } as any
+    const notification = changePointShapeNotification(tile, "star", "water")
+    expect(notification?.message.values.operation).toBe("change point shape")
+    expect(notification?.message.values.shape).toBe("star")
+    expect(notification?.message.values.category).toBe("water")
+    expect(notification?.message.values.type).toBe("DG.GraphView")
+  })
+
+  it("omits the category when the shape applies to every point", () => {
+    // no legend attribute: one shape for the whole display, so there is no category to name
+    const tile = { id: "GRAPH1", content: { type: "Graph" } } as any
+    const notification = changePointShapeNotification(tile, "diamond")
+    expect(notification?.message.values.operation).toBe("change point shape")
+    expect(notification?.message.values.shape).toBe("diamond")
+    expect(notification?.message.values.category).toBeUndefined()
+  })
+
+  it("emits on map as well as graph, since both display shapes", () => {
+    const tile = { id: "MAP1", content: { type: "Map" } } as any
+    const notification = changePointShapeNotification(tile, "plus", "land")
+    expect(notification?.message.values.operation).toBe("change point shape")
+    expect(notification?.message.values.type).toBe("DG.MapView")
+    expect(notification?.message.values.diType).toBe("map")
+  })
+
+  it("returns undefined when the tile is missing", () => {
+    expect(changePointShapeNotification(undefined, "star")).toBeUndefined()
+  })
+})
+
 describe("changePointSizeNotification", () => {
   it("emits 'change point size' with the new multiplier on map", () => {
     const tile = { id: "MAP1", content: { type: "Map" } } as any

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

@@ -1,6 +1,7 @@
 import { AttributeBinningType } from "../../models/shared/data-set-metadata"
 import { ITileModel } from "../../models/tiles/tile-model"
 import { updateTileNotification } from "../../models/tiles/tile-notifications"
+import { PointShape } from "../../utilities/point-shape-utils"
 import { GraphPlace } from "../axis-graph-shared"
 
 // Shared notification helpers for the graph and map tiles (the V3 "data-display" tiles).
@@ -56,6 +57,14 @@ export function changePointColorNotification(
   return updateTileNotification("change point color", { color, category }, tile)
 }
 
+// Shape has no V2 counterpart, so there is no legacy op string to match. `category` is undefined
+// when no legend attribute is assigned and the shape applies to every point.
+export function changePointShapeNotification(
+  tile: ITileModel | undefined, shape: PointShape, category?: string
+) {
+  return updateTileNotification("change point shape", { shape, category }, tile)
+}
+
 // V2 emits the COMPOUND op string `"change " + <internalName>` from the factory
 // `createSetColorAndAlphaCommand` at apps/dg/components/map/map_controller.js (~:327, op at
 // :337). For the non-categorical point color picker, V2 invokes the factory with the

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

@@ -22,6 +22,7 @@ import { numericSortComparator } from "../../../utilities/data-utils"
 import { stringValuesToDateSeconds } from "../../../utilities/date-utils"
 import {hashStringSets, typedId, uniqueId} from "../../../utilities/js-utils"
 import { equalFrequencyBins, isFiniteNumber } from "../../../utilities/math-utils"
+import { kDefaultPointShape, PointShape } from "../../../utilities/point-shape-utils"
 import { cachedFnWithArgsFactory, onAnyAction } from "../../../utilities/mst-utils"
 import { AxisPlace } from "../../axis/axis-types"
 import {GraphPlace} from "../../axis-graph-shared"
@@ -740,6 +741,11 @@ export const DataConfigurationModel = types
         return categorySet?.colorForCategory(cat) ?? missingColor
       },
 
+      getLegendShapeForCategory(cat: string): PointShape {
+        const categorySet = self.categorySetForAttrRole('legend')
+        return categorySet?.shapeForCategory(cat) ?? kDefaultPointShape
+      },
+
       getLegendColorForNumericValue(value: number): string {
         // A log scale is undefined for values <= 0; they are always missing, including when the log
         // domain is degenerate (<= 1 distinct positive value) and legendDisplayRange is empty.
@@ -1097,6 +1103,10 @@ export const DataConfigurationModel = types
       const categorySet = self.categorySetForAttrRole('legend')
       categorySet?.setColorForCategory(cat, color)
     },
+    setLegendShapeForCategory(cat: string, shape: PointShape) {
+      const categorySet = self.categorySetForAttrRole('legend')
+      categorySet?.setShapeForCategory(cat, shape)
+    },
     setNumberOfCategoriesLimitForRole(role: AttrRole, limit: number | undefined) {
       if (limit !== undefined && limit <= 0) {
         limit = undefined

v3/src/components/data-display/models/display-item-description-model.test.ts

@@ -0,0 +1,66 @@
+import { applySnapshot, getSnapshot } from "mobx-state-tree"
+import { DisplayItemDescriptionModel } from "./display-item-description-model"
+
+describe("DisplayItemDescriptionModel point shape", () => {
+  it("defaults to circle, so existing documents render unchanged", () => {
+    const description = DisplayItemDescriptionModel.create()
+    expect(description.pointShape).toBe("circle")
+    expect(description.itemShape).toBe("circle")
+  })
+
+  it("stores an assigned shape", () => {
+    const description = DisplayItemDescriptionModel.create()
+    description.setPointShape("triangle")
+    expect(description.pointShape).toBe("triangle")
+  })
+
+  it("stores the default as absence, so an unused document does not gain the field", () => {
+    /*
+     * V3 saves the serialized snapshot, so a materialized default would mean that merely opening
+     * and saving a document that never used shapes changes it. Asserted against the JSON rather
+     * than the snapshot object: `maybe` leaves the key present with an undefined value in memory,
+     * and it is stringify dropping it that keeps the saved document unchanged.
+     */
+    const description = DisplayItemDescriptionModel.create()
+    const serialized = JSON.parse(JSON.stringify(getSnapshot(description)))
+    expect(serialized).not.toHaveProperty("_itemShape")
+    expect(description.pointShape).toBe("circle")
+  })
+
+  it("removes the stored shape when it is set back to the default", () => {
+    const description = DisplayItemDescriptionModel.create()
+    description.setPointShape("star")
+    expect(getSnapshot(description)._itemShape).toBe("star")
+
+    description.setPointShape("circle")
+    expect(JSON.parse(JSON.stringify(getSnapshot(description)))).not.toHaveProperty("_itemShape")
+    expect(description.pointShape).toBe("circle")
+  })
+
+  it("persists the shape in the snapshot", () => {
+    const description = DisplayItemDescriptionModel.create()
+    description.setPointShape("plus")
+    expect(getSnapshot(description)._itemShape).toBe("plus")
+
+    const restored = DisplayItemDescriptionModel.create(getSnapshot(description))
+    expect(restored.pointShape).toBe("plus")
+  })
+
+  it("resolves a shape it does not recognize to the default", () => {
+    const description = DisplayItemDescriptionModel.create()
+    // simulates a document written by a build that knows a shape this one does not
+    applySnapshot(description, { ...getSnapshot(description), _itemShape: "hexagon" })
+    expect(description.pointShape).toBe("circle")
+  })
+
+  it("is independent of point color", () => {
+    // shape is a second encoding channel, so setting one must not disturb the other
+    const description = DisplayItemDescriptionModel.create()
+    const originalColor = description.pointColor
+    description.setPointShape("star")
+    expect(description.pointColor).toBe(originalColor)
+
+    description.setPointColor("#123456")
+    expect(description.pointShape).toBe("star")
+  })
+})

v3/src/components/data-display/models/display-item-description-model.ts

@@ -1,10 +1,20 @@
 import {Instance, types} from "mobx-state-tree"
 import {applyModelChange} from "../../../models/history/apply-model-change"
 import {defaultPointColor, defaultStrokeColor, kellyColors} from "../../../utilities/color-utils"
+import {kDefaultPointShape, PointShape, pointShapeOrDefault} from "../../../utilities/point-shape-utils"
 
 export const DisplayItemDescriptionModel = types
   .model("DisplayItemDescriptionModel", {
     _itemColors: types.optional(types.array(types.string), [defaultPointColor]),
+    /*
+     * The shape used when no legend attribute assigns one per category. Scalar rather than an
+     * array like _itemColors: color varies per plot index for multi-y plots, shape does not.
+     *
+     * `maybe` rather than `optional` with a default, so the default is stored as absence and a
+     * document that never used shapes does not gain the field when it is opened and saved. That
+     * matches how per-category shapes are stored, and the getter below resolves the absence.
+     */
+    _itemShape: types.maybe(types.string),
     _itemStrokeColor: defaultStrokeColor,
     _itemStrokeSameAsFill: false,
     _pointSizeMultiplier: 1, // Not used when item is a polygon in which case it is set to -1
@@ -17,6 +27,10 @@ export const DisplayItemDescriptionModel = types
     setPointColor(color: string, plotIndex = 0) {
       self._itemColors[plotIndex] = color
     },
+    setPointShape(shape: PointShape) {
+      // Absence means the default, as it does for a category's shape.
+      self._itemShape = shape === kDefaultPointShape ? undefined : shape
+    },
     setPointStrokeColor(color: string) {
       self._itemStrokeColor = color
     },
@@ -44,6 +58,9 @@ export const DisplayItemDescriptionModel = types
     get itemColor() {
       return this.itemColorAtIndex(0)
     },
+    get itemShape(): PointShape {
+      return pointShapeOrDefault(self._itemShape)
+    },
     get itemStrokeColor() {
       return self._itemStrokeSameAsFill ? this.itemColor : self._itemStrokeColor
     },
@@ -59,6 +76,9 @@ export const DisplayItemDescriptionModel = types
     get pointColor() {
       return self.itemColor
     },
+    get pointShape(): PointShape {
+      return self.itemShape
+    },
     get pointStrokeColor() {
       return self.itemStrokeColor
     },

v3/src/components/graph/models/graph-data-configuration-model.test.ts

@@ -1078,3 +1078,46 @@ describe("DataConfigurationModel legend range overrides", () => {
     expect(t.config.legendBinDataExtents).toBeUndefined()
   })
 })
+
+describe("DataConfigurationModel legend point shapes", () => {
+  beforeEach(() => {
+    tree = TreeModel.create({ data: {}, metadata: {}, config: {} })
+    tree.data.addAttribute({ id: "legId", name: "leg" })
+    tree.metadata.setData(tree.data)
+    tree.data.addCases(toCanonical(tree.data, [
+      { __id__: "c1", leg: "land" },
+      { __id__: "c2", leg: "water" }
+    ]))
+    tree.config.setDataset(tree.data, tree.metadata)
+    tree.config.setAttribute("legend", { attributeID: "legId" })
+  })
+
+  it("reports the default shape before anything is assigned", () => {
+    expect(tree.config.attributeType("legend")).toBe("categorical")
+    expect(tree.config.getLegendShapeForCategory("land")).toBe("circle")
+    expect(tree.config.getLegendShapeForCategory("water")).toBe("circle")
+  })
+
+  it("round-trips a shape through the category set", () => {
+    tree.config.setLegendShapeForCategory("land", "star")
+    expect(tree.config.getLegendShapeForCategory("land")).toBe("star")
+    // sibling categories are unaffected
+    expect(tree.config.getLegendShapeForCategory("water")).toBe("circle")
+  })
+
+  it("stores the shape on the shared category set, not on the configuration", () => {
+    // two configurations over the same legend attribute must agree, which is the reason
+    // per-category shape lives on the attribute's category set
+    tree.config.setLegendShapeForCategory("land", "diamond")
+    const categorySet = tree.metadata.getCategorySet("legId")
+    expect(categorySet?.shapeForCategory("land")).toBe("diamond")
+  })
+
+  it("falls back to the default when there is no legend attribute", () => {
+    tree.config.setAttribute("legend", { attributeID: "" })
+    // no category set to consult, so reads resolve rather than returning undefined
+    expect(tree.config.getLegendShapeForCategory("land")).toBe("circle")
+    // and assignment is a no-op rather than a crash
+    expect(() => tree.config.setLegendShapeForCategory("land", "star")).not.toThrow()
+  })
+})

v3/src/components/graph/v2-graph-exporter.test.ts

@@ -396,3 +396,71 @@ describe("V2 graph legend bin count round-trip", () => {
     expect(dataConfig.metadata!.getAttributeBinCount(legendId)).toBeUndefined()
   })
 })
+
+describe("V2 graph point shape round-trip", () => {
+  beforeEach(() => resetMocks())
+
+  // Imports the diet-legend document and returns the first graph tile with a categorical legend.
+  function importCategoricalLegendGraph() {
+    const { v2Document } = loadCodapDocument("mammals-all-diet-legends.codap")
+    const v2GraphTiles = v2Document.components.filter(c => c.type === "DG.GraphView")
+    for (const v2GraphTile of v2GraphTiles) {
+      const tile = v2GraphImporter({ v2Component: v2GraphTile, v2Document, ...mockImporterArgs })
+      const content = isGraphContentModel(tile?.content) ? tile.content : undefined
+      if (content?.dataConfiguration.attributeType("legend") === "categorical") {
+        return { tile: tile!, content }
+      }
+    }
+    throw new Error("no categorical-legend graph found in fixture")
+  }
+
+  it("writes no v3 pointShape when the graph uses the default", () => {
+    const { tile, content } = importCategoricalLegendGraph()
+    expect(content.pointDescription.pointShape).toBe("circle")
+    const out = v2GraphExporter({ tile })
+    // documents that never used the feature gain nothing
+    expect((out?.componentStorage as any)?.v3?.pointShape).toBeUndefined()
+  })
+
+  it("exports a non-default shape into the v3 namespace", () => {
+    const { tile, content } = importCategoricalLegendGraph()
+    content.pointDescription.setPointShape("star")
+    const out = v2GraphExporter({ tile })
+    expect((out?.componentStorage as any)?.v3?.pointShape).toBe("star")
+  })
+
+  it("keeps the shape out of v2's own storage keys", () => {
+    // v2 has no field for shape; anything we add outside the v3 namespace would be unrecognized
+    const { tile, content } = importCategoricalLegendGraph()
+    content.pointDescription.setPointShape("diamond")
+    const storage = v2GraphExporter({ tile })?.componentStorage as any
+    const { v3, ...v2Native } = storage
+    expect(JSON.stringify(v2Native)).not.toContain("diamond")
+  })
+
+  it("imports a shape written by v3", () => {
+    const { v2Document } = loadCodapDocument("mammals-all-diet-legends.codap")
+    const v2GraphTile = v2Document.components.find(c => c.type === "DG.GraphView")!
+    const withShape = {
+      ...v2GraphTile,
+      componentStorage: { ...v2GraphTile.componentStorage, v3: { pointShape: "plus" } }
+    } as typeof v2GraphTile
+    const tile = v2GraphImporter({ v2Component: withShape, v2Document, ...mockImporterArgs })
+    const content = isGraphContentModel(tile?.content) ? tile.content : undefined
+    expect(content?.pointDescription.pointShape).toBe("plus")
+  })
+
+  it("imports as the default when v2 wrote the document", () => {
+    // a document v2 saved has no v3 namespace at all, and one v2 re-saved has dropped it
+    const { v2Document } = loadCodapDocument("mammals-all-diet-legends.codap")
+    const v2GraphTile = v2Document.components.find(c => c.type === "DG.GraphView")!
+    const { v3, ...storageWithoutV3 } = v2GraphTile.componentStorage as any
+    const tile = v2GraphImporter({
+      v2Component: { ...v2GraphTile, componentStorage: storageWithoutV3 },
+      v2Document,
+      ...mockImporterArgs
+    })
+    const content = isGraphContentModel(tile?.content) ? tile.content : undefined
+    expect(content?.pointDescription.pointShape).toBe("circle")
+  })
+})

v3/src/components/graph/v2-graph-exporter.ts

@@ -335,7 +335,10 @@ export const v2GraphExporter: V2TileExportFn = ({ tile }) => {
     // plot models
     ...getPlotModels(graph),
     // v3 extensions
-    ...exportV3Properties(graph.dataConfiguration, { axisTypes: getV3AxisTypes(graph) })
+    ...exportV3Properties(graph.dataConfiguration, {
+      axisTypes: getV3AxisTypes(graph),
+      pointShape: graph.pointDescription.pointShape
+    })
   }
 
   return { type: "DG.GraphView", componentStorage }

v3/src/components/graph/v2-graph-importer.ts

@@ -4,6 +4,7 @@ import {AxisPlace} from "../axis/axis-types"
 import { ITileModel, ITileModelSnapshotIn } from "../../models/tiles/tile-model"
 import {toV3AttrId, toV3Id} from "../../utilities/codap-utils"
 import {defaultBackgroundColor, parseColorToHex} from "../../utilities/color-utils"
+import {pointShapeOrDefault} from "../../utilities/point-shape-utils"
 import {v3TypeFromV2TypeIndex} from "../../v2/codap-v2-data-context-types"
 import {V2TileImportArgs} from "../../v2/codap-v2-tile-importers"
 import { IGuidLink, isV2GraphComponent } from "../../v2/codap-v2-types"
@@ -219,7 +220,9 @@ export function v2GraphImporter({v2Component, v2Document, getCaseData, insertTil
       _itemStrokeColor: strokeColor ? parseColorToHex(strokeColor, {colorNames: true, alpha: strokeTransparency})
                                     : strokeColor,
       _pointSizeMultiplier: pointSizeMultiplier,
-      _itemStrokeSameAsFill: strokeSameAsFill
+      _itemStrokeSameAsFill: strokeSameAsFill,
+      // Absent for documents v2 wrote, and for anything v2 re-saved; those import as the default.
+      _itemShape: pointShapeOrDefault(v3?.pointShape)
     },
     layers: [{
       type: kGraphPointLayerType,

v3/src/data-interactive/data-interactive-type-utils.ts

@@ -122,6 +122,10 @@ export function convertAttributeToV2(attribute: IAttribute, dataContext?: IDataS
     ...(high ? { "high-attribute-color": high } : {})
   } as ICodapV2CategoryMap
   const categoryMap = categorySet ? { _categoryMap } : undefined
+  // Omitted entirely when no category carries a shape, so a document that never used the feature
+  // gains nothing.
+  const shapeMap = categorySet?.shapeMap ?? {}
+  const v3 = Object.keys(shapeMap).length > 0 ? { v3: { categoryShapes: shapeMap } } : undefined
 
   return {
     name,
@@ -131,6 +135,7 @@ export function convertAttributeToV2(attribute: IAttribute, dataContext?: IDataS
     description,
     ...defaultRange,
     ...categoryMap,
+    ...v3,
     editable: (attribute && !metadata?.isEditProtected(attribute.id)) ?? true,
     hidden: (attribute && metadata?.isHidden(attribute.id)) ?? false,
     renameable: (attribute && !metadata?.isRenameProtected(attribute.id)) ?? true,