diff --git a/v3/src/components/data-display/data-display-notifications.test.ts b/v3/src/components/data-display/data-display-notifications.test.ts index f651aec58e..983b2df6f2 100644 --- a/v3/src/components/data-display/data-display-notifications.test.ts +++ b/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 diff --git a/v3/src/components/data-display/data-display-notifications.ts b/v3/src/components/data-display/data-display-notifications.ts index 556420c553..55ceb05382 100644 --- a/v3/src/components/data-display/data-display-notifications.ts +++ b/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 " + ` 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 diff --git a/v3/src/components/data-display/models/data-configuration-model.ts b/v3/src/components/data-display/models/data-configuration-model.ts index a17463b3d4..9fa1fc7b1f 100644 --- a/v3/src/components/data-display/models/data-configuration-model.ts +++ b/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 diff --git a/v3/src/components/data-display/models/display-item-description-model.test.ts b/v3/src/components/data-display/models/display-item-description-model.test.ts new file mode 100644 index 0000000000..391f28f372 --- /dev/null +++ b/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") + }) +}) diff --git a/v3/src/components/data-display/models/display-item-description-model.ts b/v3/src/components/data-display/models/display-item-description-model.ts index 1bfa0a63a0..2a1c7fbe57 100644 --- a/v3/src/components/data-display/models/display-item-description-model.ts +++ b/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 }, diff --git a/v3/src/components/graph/models/graph-data-configuration-model.test.ts b/v3/src/components/graph/models/graph-data-configuration-model.test.ts index 813667c0bb..b7d9c106a9 100644 --- a/v3/src/components/graph/models/graph-data-configuration-model.test.ts +++ b/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() + }) +}) diff --git a/v3/src/components/graph/v2-graph-exporter.test.ts b/v3/src/components/graph/v2-graph-exporter.test.ts index 389aa36870..20ca06947a 100644 --- a/v3/src/components/graph/v2-graph-exporter.test.ts +++ b/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") + }) +}) diff --git a/v3/src/components/graph/v2-graph-exporter.ts b/v3/src/components/graph/v2-graph-exporter.ts index e02c9996f5..72d5aeb3c5 100644 --- a/v3/src/components/graph/v2-graph-exporter.ts +++ b/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 } diff --git a/v3/src/components/graph/v2-graph-importer.ts b/v3/src/components/graph/v2-graph-importer.ts index b4a018ba09..fadf8de64b 100644 --- a/v3/src/components/graph/v2-graph-importer.ts +++ b/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, diff --git a/v3/src/data-interactive/data-interactive-type-utils.ts b/v3/src/data-interactive/data-interactive-type-utils.ts index 300770440c..ee9bfa7467 100644 --- a/v3/src/data-interactive/data-interactive-type-utils.ts +++ b/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, diff --git a/v3/src/data-interactive/point-shape-v2-export.test.ts b/v3/src/data-interactive/point-shape-v2-export.test.ts new file mode 100644 index 0000000000..79f5277d2b --- /dev/null +++ b/v3/src/data-interactive/point-shape-v2-export.test.ts @@ -0,0 +1,75 @@ +import { createCodapDocument } from "../models/codap/create-codap-document" +import { DataBroker } from "../models/data/data-broker" +import { DataSet } from "../models/data/data-set" +import { getMetadataFromDataSet } from "../models/shared/shared-data-utils" +import { getSharedModelManager } from "../models/tiles/tile-environment" +import { convertAttributeToV2 } from "./data-interactive-type-utils" + +// Shapes are exported in the attribute's v3 namespace rather than in _categoryMap; see the v3 field +// on ICodapV2Attribute for why that is a constraint rather than a preference. +describe("point shape v2 export", () => { + let dataSet: ReturnType + + beforeEach(() => { + const doc = createCodapDocument() + const sharedModelManager = getSharedModelManager(doc)! + const dataBroker = new DataBroker({ sharedModelManager }) + dataSet = DataSet.create({ collections: [{ name: "Cases" }] }) + const attr = dataSet.addAttribute({ name: "a" }) + dataSet.addCases([{ [attr.id]: "land" }, { [attr.id]: "water" }]) + dataBroker.addDataSet(dataSet) + }) + + const categorySet = () => getMetadataFromDataSet(dataSet)!.getCategorySet(dataSet.attributes[0].id)! + + it("writes nothing when no category carries a shape", () => { + categorySet().setColorForCategory("land", "#ff0000") + const v2Attr = convertAttributeToV2(dataSet.attributes[0], dataSet) + + // no v3 namespace at all, so documents that never used the feature are unchanged + expect(v2Attr.v3).toBeUndefined() + }) + + it("writes assigned shapes into the attribute's v3 namespace", () => { + categorySet().setShapeForCategory("land", "star") + categorySet().setShapeForCategory("water", "diamond") + const v2Attr = convertAttributeToV2(dataSet.attributes[0], dataSet) + + expect(v2Attr.v3?.categoryShapes).toEqual({ land: "star", water: "diamond" }) + }) + + it("keeps shapes out of _categoryMap, which v2 would read as categories", () => { + categorySet().setColorForCategory("land", "#ff0000") + categorySet().setShapeForCategory("land", "star") + const v2Attr = convertAttributeToV2(dataSet.attributes[0], dataSet) + + const categoryMap = v2Attr._categoryMap as Record + expect(categoryMap).toBeDefined() + + // No shape-bearing key of any name may appear in the category map. + expect(categoryMap.__shapes).toBeUndefined() + expect(categoryMap.shapes).toBeUndefined() + expect(categoryMap.categoryShapes).toBeUndefined() + + // Every key is either a real category or one of the three v2 tolerates. + const v2Ignorable = ["__order", "stroke-color", "stroke-transparency"] + const realCategories = categorySet().valuesArray + const v2NumericColorKeys = ["attribute-color", "low-attribute-color", "high-attribute-color"] + Object.keys(categoryMap).forEach(key => { + const isKnown = v2Ignorable.includes(key) || realCategories.includes(key) || + v2NumericColorKeys.includes(key) + expect({ key, isKnown }).toEqual({ key, isKnown: true }) + }) + + // and the shape did not leak into __order, which is what v2 would corrupt + expect(categoryMap.__order).toEqual(realCategories) + }) + + it("omits categories left at the default so the export stays minimal", () => { + categorySet().setShapeForCategory("land", "star") + const v2Attr = convertAttributeToV2(dataSet.attributes[0], dataSet) + + expect(v2Attr.v3?.categoryShapes).toEqual({ land: "star" }) + expect(v2Attr.v3?.categoryShapes?.water).toBeUndefined() + }) +}) diff --git a/v3/src/models/data/category-set.test.ts b/v3/src/models/data/category-set.test.ts index c4fea95776..f233a29a61 100644 --- a/v3/src/models/data/category-set.test.ts +++ b/v3/src/models/data/category-set.test.ts @@ -5,7 +5,6 @@ import { jestSpyConsole } from "../../test/jest-spy-console" import { Attribute, IAttribute } from "./attribute" import { CategorySet, ICategorySet, createProvisionalCategorySet, getProvisionalDataSet } from "./category-set" import { DataSet } from "./data-set" -import { onAnyAction } from "../../utilities/mst-utils" describe("CategorySet", () => { const Tree = types.model("Tree", { @@ -353,25 +352,121 @@ describe("CategorySet", () => { expect(handleAttributeInvalidated).toHaveBeenCalledWith(cId) }) - it("identifies actions that indicate user modification of category sets", () => { - const a = Attribute.create({ name: "a", values: ["a", "b", "c"] }) - const tree = Tree.create({ attribute: a, categories: { attribute: a.id } }) + describe("per-category point shapes", () => { + const makeSet = (values: string[]) => { + const tree = Tree.create({ + attribute: Attribute.create({ id: "aId", name: "a" }), + categories: { attribute: "aId" } + }) + values.forEach(v => tree.attribute.addValue(v)) + return tree.categories + } + + it("defaults every category to circle", () => { + const categories = makeSet(["a", "b"]) + expect(categories.shapeForCategory("a")).toBe("circle") + expect(categories.shapeForCategory("b")).toBe("circle") + // a category that does not exist still resolves rather than returning undefined + expect(categories.shapeForCategory("nope")).toBe("circle") + }) + + it("stores an assigned shape", () => { + const categories = makeSet(["a", "b"]) + runInAction(() => categories.setShapeForCategory("a", "star")) + expect(categories.shapeForCategory("a")).toBe("star") + // assigning one category leaves the others alone + expect(categories.shapeForCategory("b")).toBe("circle") + }) - const fn = jest.fn() - const disposer = onAnyAction(tree.categories, action => { - if (tree.categories.userActionNames.includes(action.name)) { + it("stores the default as absence, so unused documents carry nothing", () => { + const categories = makeSet(["a"]) + runInAction(() => categories.setShapeForCategory("a", "star")) + expect(getSnapshot(categories).shapes).toEqual({ a: "star" }) + + // reverting to the default removes the entry rather than recording "circle" + runInAction(() => categories.setShapeForCategory("a", "circle")) + expect(getSnapshot(categories).shapes).toEqual({}) + expect(categories.shapeForCategory("a")).toBe("circle") + }) + + it("resolves an unrecognized stored shape to the default", () => { + const categories = makeSet(["a"]) + // simulates a document written by a build that knows a shape this one does not + applySnapshot(categories, { ...getSnapshot(categories), shapes: { a: "hexagon" } }) + expect(categories.shapeForCategory("a")).toBe("circle") + }) + + it("exposes only explicitly assigned shapes via shapeMap", () => { + const categories = makeSet(["a", "b", "c"]) + expect(categories.shapeMap).toEqual({}) + + runInAction(() => categories.setShapeForCategory("b", "diamond")) + // b only -- a and c are at the default and must not appear + expect(categories.shapeMap).toEqual({ b: "diamond" }) + }) + + + it("is observable, so the renderer re-reads when a shape changes", () => { + const categories = makeSet(["a"]) + const fn = jest.fn() + const disposer = autorun(() => { + categories.shapeForCategory("a") fn() - } + }) + expect(fn).toHaveBeenCalledTimes(1) + + runInAction(() => categories.setShapeForCategory("a", "plus")) + expect(fn).toHaveBeenCalledTimes(2) + + disposer() }) - tree.categories.move("c", "a") - expect(fn).toHaveBeenCalledTimes(1) - tree.categories.setColorForCategory("a", "red") - expect(fn).toHaveBeenCalledTimes(2) - expect(tree.categories.colorForCategory("a")).toBe("red") - expect(fn).toHaveBeenCalledTimes(2) - tree.categories.storeAllCurrentColors() - expect(fn).toHaveBeenCalledTimes(4) - - disposer() }) + + + describe("category values that collide with object keys", () => { + // Category values come from the data and can be any string, including ones that mean something + // to a plain JavaScript object. + const makeSet = (values: string[]) => { + const tree = Tree.create({ + attribute: Attribute.create({ id: "aId", name: "a" }), + categories: { attribute: "aId" } + }) + values.forEach(v => tree.attribute.addValue(v)) + return tree.categories + } + + it("keeps a color assigned to a category named __proto__", () => { + const categories = makeSet(["__proto__", "b"]) + runInAction(() => categories.setColorForCategory("__proto__", "#ff0000")) + + expect(categories.colorForCategory("__proto__")).toBe("#ff0000") + expect(Object.keys(categories.colorMap)).toContain("__proto__") + }) + + it("keeps a shape assigned to a category named __proto__", () => { + const categories = makeSet(["__proto__", "b"]) + runInAction(() => { + categories.setShapeForCategory("__proto__", "star") + categories.setShapeForCategory("b", "plus") + }) + + expect(categories.shapeForCategory("__proto__")).toBe("star") + expect(Object.keys(categories.shapeMap).sort()).toEqual(["__proto__", "b"]) + expect(categories.shapeMap.__proto__).toBe("star") + }) + + it("does not report an inherited member as a color", () => { + // on a normal object, colorMap["constructor"] would return a function + const categories = makeSet(["a"]) + expect(typeof categories.colorForCategory("constructor")).not.toBe("function") + expect(categories.colorForCategory("constructor")).toBeUndefined() + }) + + it("assigns a color to a category named constructor like any other", () => { + const categories = makeSet(["constructor", "b"]) + runInAction(() => categories.setColorForCategory("constructor", "#00ff00")) + expect(categories.colorForCategory("constructor")).toBe("#00ff00") + }) + }) + }) diff --git a/v3/src/models/data/category-set.ts b/v3/src/models/data/category-set.ts index b5f2762429..1b59bad9a6 100644 --- a/v3/src/models/data/category-set.ts +++ b/v3/src/models/data/category-set.ts @@ -4,6 +4,7 @@ import { } from "mobx-state-tree" import { kellyColors } from "../../utilities/color-utils" import { compareValues } from "../../utilities/data-utils" +import { kDefaultPointShape, PointShape, pointShapeOrDefault } from "../../utilities/point-shape-utils" import { gLocale } from "../../utilities/translation/locale" import { Attribute, IAttribute } from "./attribute" import { IDataSet } from "./data-set" @@ -139,6 +140,8 @@ export const CategorySet = types.model("CategorySet", { }), // user color assignments to categories in an attribute colors: types.map(types.string), + // user point-shape assignments to categories in an attribute + shapes: types.map(types.string), // user category re-orderings moves: types.array(types.frozen()) }) @@ -185,11 +188,6 @@ export const CategorySet = types.model("CategorySet", { get valuesArray(): string[] { return Array.from(self.values) }, - // list of actions that indicate deliberate action by the user - // used to determine when to move provisional category sets into the document - get userActionNames() { - return ["move", "setColorForCategory", "storeCurrentColorForCategory"] - }, get lastMove() { return self.moves.length > 0 ? self.moves[self.moves.length - 1] @@ -205,14 +203,39 @@ export const CategorySet = types.model("CategorySet", { // We intentionally create a new non-observable map here. // This way this map object can be observed and if it changes a user knows the // colors or categories have changed - const map: Record = {} - self.values.forEach((category, index) => map[category] = colorForCategory(category, index)) + // + // Null-prototyped and built from entries because category values come from the data and can be + // any string. Assigning `map["__proto__"]` sets the prototype rather than defining an own + // property, losing that category's color; and reading `map["constructor"]` off a normal object + // returns an inherited function rather than undefined for a category that has none. + const entries = self.values.map( + (category, index) => [category, colorForCategory(category, index)] as const + ) + const map: Record = Object.assign(Object.create(null), Object.fromEntries(entries)) return map } })) .views(self => ({ colorForCategory(category: string) { return self.colorMap[category] + }, + // Unlike colors, which cycle through a palette by category index, every category starts at the + // same default shape, so there is no index-derived fallback to compute. + shapeForCategory(category: string): PointShape { + return pointShapeOrDefault(self.shapes.get(category)) + }, + /* + * Only the categories carrying an explicit shape. Categories at the default are omitted, so + * exports stay empty until a user actually assigns one. + * + * Built with fromEntries: assignment would set the prototype for a category named `__proto__`. + */ + get shapeMap(): Record { + const entries: Array<[string, PointShape]> = [] + self.shapes.forEach((shape, category) => { + entries.push([String(category), pointShapeOrDefault(shape)]) + }) + return Object.fromEntries(entries) } })) .actions(self => ({ @@ -259,6 +282,15 @@ export const CategorySet = types.model("CategorySet", { self.colors.delete(value) } }, + // Storing the default is stored as absence, so a document only carries the shapes a user chose + // and a category reverted to circle round-trips as an unset entry. + setShapeForCategory(value: string, shape: PointShape) { + if (shape && shape !== kDefaultPointShape) { + self.shapes.set(value, shape) + } else { + self.shapes.delete(value) + } + }, storeCurrentColorForCategory(value: string) { const color = self.colorForCategory(value) if (color) { diff --git a/v3/src/models/data/v2-category-set-importer.test.ts b/v3/src/models/data/v2-category-set-importer.test.ts index 99ffe4c7b1..040721dc85 100644 --- a/v3/src/models/data/v2-category-set-importer.test.ts +++ b/v3/src/models/data/v2-category-set-importer.test.ts @@ -60,4 +60,68 @@ describe("importV2CategorySet", () => { { value: "e", fromIndex: 3, toIndex: 0, before: "d", length: 4 } ]) }) + + describe("point shapes", () => { + const makeAttribute = () => Attribute.create({ + id: "aId", name: "a", values: ["land", "water", "land"] + }) + + it("does not invent a color for a category named after an inherited member", () => { + // colorMap comes from a v2 document, so a bare `colorMap["constructor"]` returns the + // inherited function -- truthy, and not a color -- for a category the map has no entry for + const attribute = Attribute.create({ id: "aId", name: "a", values: ["constructor", "land"] }) + const result = importV2CategorySet(attribute, createCategoryMap(["constructor", "land"], {})) + + // own keys, not toHaveProperty: that would find the inherited constructor and fail whatever + // the code does -- the same hazard this test is about + expect(Object.keys(result?.colors ?? {})).not.toContain("constructor") + }) + + it("imports assigned shapes, which alone justify a category set", () => { + // no colors and no order here, so nothing but the shapes could have created the set + const result = importV2CategorySet(makeAttribute(), undefined, { land: "star", water: "diamond" }) + expect(result?.shapes).toEqual({ land: "star", water: "diamond" }) + }) + + it("returns nothing when there is no shape, color or move to restore", () => { + expect(importV2CategorySet(makeAttribute(), undefined, {})).toBeUndefined() + expect(importV2CategorySet(makeAttribute(), undefined, undefined)).toBeUndefined() + }) + + it("drops the default, which is stored as absence", () => { + const result = importV2CategorySet(makeAttribute(), undefined, { land: "circle", water: "star" }) + expect(result?.shapes).toEqual({ water: "star" }) + }) + + it("drops a shape this build does not recognize", () => { + // a document written by a newer build must not inject an unknown value into the model + const result = importV2CategorySet(makeAttribute(), undefined, { land: "hexagon", water: "star" }) + expect(result?.shapes).toEqual({ water: "star" }) + }) + + it("restores a category whose value is a reserved object key", () => { + // assignment into a plain object would set the prototype instead of an own property, so the + // shape for such a category would be dropped on import + const attribute = Attribute.create({ id: "aId", name: "a", values: ["__proto__", "land"] }) + // built by parsing, as it would be arriving from a v2 document: an object literal with a + // `__proto__` key sets the prototype instead, so it would not exercise the case at all + const categoryShapes = JSON.parse('{"__proto__":"star","land":"plus"}') + const result = importV2CategorySet(attribute, undefined, categoryShapes) + + expect(Object.keys(result?.shapes ?? {}).sort()).toEqual(["__proto__", "land"]) + expect(result?.shapes?.__proto__).toBe("star") + }) + + it("keeps a shape for a category not currently in the data", () => { + /* + * Deliberate, and deliberately unlike colors: every shape entry is a user assignment, so + * there is no auto-generated noise to age out. A category whose cases are deleted and later + * restored -- a sampler re-run, say -- gets the shape its user chose back. v2 keeps its own + * assignments for absent categories for the same reason. + */ + const result = importV2CategorySet(makeAttribute(), undefined, { land: "star", lava: "plus" }) + expect(result?.shapes).toEqual({ land: "star", lava: "plus" }) + }) + }) + }) diff --git a/v3/src/models/data/v2-category-set-importer.ts b/v3/src/models/data/v2-category-set-importer.ts index 005b39a3c5..4db0fae9b5 100644 --- a/v3/src/models/data/v2-category-set-importer.ts +++ b/v3/src/models/data/v2-category-set-importer.ts @@ -1,6 +1,7 @@ import { colord } from "colord" import { kellyColors } from "../../utilities/color-utils" import { compareValues } from "../../utilities/data-utils" +import { isPointShape, kDefaultPointShape } from "../../utilities/point-shape-utils" import { gLocale } from "../../utilities/translation/locale" import { CodapV2ColorMap, ICodapV2CategoryMap, isV2CategoryMap } from "../../v2/codap-v2-data-context-types" import { IAttribute } from "./attribute" @@ -9,10 +10,26 @@ import { MinimalMovesFinder } from "./minimal-moves-finder" export type V2CategorySetInput = CodapV2ColorMap | ICodapV2CategoryMap -export function importV2CategorySet(attribute: IAttribute, input: V2CategorySetInput): Maybe { +export function importV2CategorySet( + attribute: IAttribute, input: Maybe, categoryShapes?: Record +): Maybe { let moves: ICategoryMove[] = [] - // map from category string to hex color string - const colors: Record = {} + // category string to hex color string, collected as entries and built at the end: category + // values come from the data, and assigning `colors["__proto__"]` would set the prototype rather + // than define an own property, losing that category's color + const colorEntries: Array<[string, string]> = [] + + /* + * Taken as given rather than filtered against the categories currently in the data: every entry + * is a deliberate assignment, so a category whose cases are deleted and later restored keeps the + * shape the user chose for it. Nothing generates a shape, so there is no automatic value to age + * out the way the color loop below ages one out. + */ + // fromEntries: assignment would set the prototype for a category named `__proto__`. + const shapes: Record = Object.fromEntries( + Object.entries(categoryShapes ?? {}) + .filter(([, shape]) => isPointShape(shape) && shape !== kDefaultPointShape) + ) let colorMap: CodapV2ColorMap = {} @@ -44,7 +61,7 @@ export function importV2CategorySet(attribute: IAttribute, input: V2CategorySetI moves = minMovesFinder.minMoves() } else { - colorMap = input + colorMap = input ?? {} } // V2 assigns colors to categories in the order they appear in the data. @@ -52,23 +69,27 @@ export function importV2CategorySet(attribute: IAttribute, input: V2CategorySetI for (let i = 0; i < sortedOrder.length; ++i) { const category = sortedOrder[i] const defaultColor = kellyColors[i % kellyColors.length] - const importColor = colorMap[category] + // hasOwn rather than a bare lookup: for a category named `constructor` or `toString` with no + // entry, a plain object returns the inherited member, which is truthy and is not a color + const importColor = Object.prototype.hasOwnProperty.call(colorMap, category) ? colorMap[category] : undefined if (importColor) { const importColorStr = typeof importColor === "string" ? importColor : importColor.colorString const defaultColorD = colord(defaultColor) const importColorD = colord(importColorStr) // if the v2 color is different than the default color, store it as a color change if (defaultColorD.toHex() !== importColorD.toHex()) { - colors[category] = importColorD.toHex() + colorEntries.push([category, importColorD.toHex()]) } } } - if (moves.length > 0 || Object.keys(colors).length > 0) { + const colors: Record = Object.fromEntries(colorEntries) + if (moves.length > 0 || colorEntries.length > 0 || Object.keys(shapes).length > 0) { return { attribute: attribute.id, moves, - colors + colors, + shapes } } } diff --git a/v3/src/models/shared/data-set-metadata.test.ts b/v3/src/models/shared/data-set-metadata.test.ts index 4d464d14aa..fa4481681d 100644 --- a/v3/src/models/shared/data-set-metadata.test.ts +++ b/v3/src/models/shared/data-set-metadata.test.ts @@ -531,4 +531,43 @@ describe("DataSetMetadata", () => { expect(tree.metadata.caseCardTileId).toBe("foo-card") expect(tree.metadata.lastShownTableOrCardTileId).toBe("foo-table") }) + /* + * A category set starts provisional so that merely looking at an attribute does not dirty the + * document. It is promoted when the user changes something that has to persist. Choosing a + * point shape is such a change, and it can be the only one a user makes. + */ + it("promotes a provisional category set when only a shape is assigned", () => { + const categorySet = tree.metadata.getCategorySet("aId")! + expect(tree.metadata.attributes.size).toBe(0) + expect(tree.metadata.provisionalCategories.size).toBe(1) + + categorySet.setShapeForCategory("1", "star") + + expect(tree.metadata.attributes.size).toBe(1) + expect(tree.metadata.provisionalCategories.size).toBe(0) + // the promoted set carries the shape, so it survives into the saved document + expect(tree.metadata.getCategorySet("aId")?.shapeForCategory("1")).toBe("star") + }) + + it("promotes a provisional category set when categories are reordered", () => { + // moves are the other change that has to persist; kept alongside the shape and color cases + // so the set of changes that count as user modification is covered in one place + const categorySet = tree.metadata.getCategorySet("aId")! + expect(tree.metadata.provisionalCategories.size).toBe(1) + + categorySet.move("3", "1") + + expect(tree.metadata.attributes.size).toBe(1) + expect(tree.metadata.provisionalCategories.size).toBe(0) + }) + + it("does not promote for a shape set back to the default", () => { + const categorySet = tree.metadata.getCategorySet("aId")! + // the default is stored as absence, so this leaves nothing worth persisting + categorySet.setShapeForCategory("1", "circle") + + expect(tree.metadata.attributes.size).toBe(0) + expect(tree.metadata.provisionalCategories.size).toBe(1) + }) + }) diff --git a/v3/src/models/shared/data-set-metadata.ts b/v3/src/models/shared/data-set-metadata.ts index 91bf637cc4..688f1a6bc3 100644 --- a/v3/src/models/shared/data-set-metadata.ts +++ b/v3/src/models/shared/data-set-metadata.ts @@ -597,7 +597,8 @@ export const DataSetMetadata = SharedModel } })) .actions(self => ({ - // moves a category set from the provisional map to the official one + // Moves a category set from the provisional map to the official one, replacing the instance: + // the official set is built from a snapshot. promoteProvisionalCategorySet(categorySet: ICategorySet) { const attrId = categorySet.attribute.id self.setCategorySet(attrId, getSnapshot(categorySet)) @@ -606,7 +607,14 @@ export const DataSetMetadata = SharedModel } })) .views(self => ({ - // returns an existing category set (if available) or creates a new provisional one (for valid attributes) + /* + * Returns an existing category set (if available) or creates a new provisional one (for valid + * attributes). + * + * Call this again for each modification rather than holding the result across them: the first + * change promotes a provisional set, which replaces the instance, and writes through a stale + * reference are silently lost. + */ getCategorySet(attrId: string, createIfMissing = true): Maybe { let categorySet = self.attributes.get(attrId)?.categories ?? self.provisionalCategories.get(attrId) if (!categorySet && self.data?.attrFromID(attrId)) { @@ -619,7 +627,7 @@ export const DataSetMetadata = SharedModel }) // promote provisional category sets when they are modified by the user when( - () => !!categorySet?.moves.length || !!categorySet?.colors.size, + () => !!categorySet?.moves.length || !!categorySet?.colors.size || !!categorySet?.shapes.size, () => { if (categorySet && self.provisionalCategories.has(attrId)) { self.promoteProvisionalCategorySet(categorySet) diff --git a/v3/src/utilities/point-shape-utils.test.ts b/v3/src/utilities/point-shape-utils.test.ts new file mode 100644 index 0000000000..6457d33cb3 --- /dev/null +++ b/v3/src/utilities/point-shape-utils.test.ts @@ -0,0 +1,44 @@ +import { isPointShape, kDefaultPointShape, PointShapes, pointShapeOrDefault } from "./point-shape-utils" + +describe("point shape utils", () => { + it("defaults to circle, which is what existing documents render as", () => { + expect(kDefaultPointShape).toBe("circle") + expect(PointShapes).toContain(kDefaultPointShape) + }) + + it("lists the seven shapes with circle first", () => { + expect(PointShapes).toEqual(["circle", "square", "triangle", "diamond", "star", "plus", "x"]) + }) + + describe("isPointShape", () => { + it("accepts every listed shape", () => { + PointShapes.forEach(shape => expect(isPointShape(shape)).toBe(true)) + }) + + it("rejects anything else", () => { + expect(isPointShape("hexagon")).toBe(false) + expect(isPointShape("")).toBe(false) + expect(isPointShape(undefined)).toBe(false) + // case-sensitive: shapes are stored as written, not normalized + expect(isPointShape("Circle")).toBe(false) + }) + + it("is not fooled by inherited Array properties", () => { + expect(isPointShape("length")).toBe(false) + expect(isPointShape("constructor")).toBe(false) + }) + }) + + describe("pointShapeOrDefault", () => { + it("passes through a known shape", () => { + expect(pointShapeOrDefault("star")).toBe("star") + }) + + it("falls back for values a future build might not know", () => { + // A shape written by a newer version must not leave the point unrendered. + expect(pointShapeOrDefault("hexagon")).toBe(kDefaultPointShape) + expect(pointShapeOrDefault(undefined)).toBe(kDefaultPointShape) + expect(pointShapeOrDefault("")).toBe(kDefaultPointShape) + }) + }) +}) diff --git a/v3/src/utilities/point-shape-utils.ts b/v3/src/utilities/point-shape-utils.ts new file mode 100644 index 0000000000..6e70f69f92 --- /dev/null +++ b/v3/src/utilities/point-shape-utils.ts @@ -0,0 +1,26 @@ +/* + * The shapes a data point can take, in menu order. + * + * In utilities rather than under data-display so the models layer can reference it without + * importing from components, the way color-utils already serves both. + */ +export const PointShapes = ["circle", "square", "triangle", "diamond", "star", "plus", "x"] as const + +export type PointShape = typeof PointShapes[number] + +export const kDefaultPointShape: PointShape = "circle" + +const kPointShapeSet: ReadonlySet = new Set(PointShapes) + +export function isPointShape(value?: string): value is PointShape { + return kPointShapeSet.has(value as PointShape) +} + +/* + * Shapes arriving from saved documents, v2 imports and plugins are unvalidated strings. Anything + * unrecognized resolves to the default, so a value this build does not know about renders as a + * circle rather than leaving the point undrawn. + */ +export function pointShapeOrDefault(value?: string): PointShape { + return isPointShape(value) ? value : kDefaultPointShape +} diff --git a/v3/src/utilities/translation/lang/en-US.json5 b/v3/src/utilities/translation/lang/en-US.json5 index f3a6e7c06e..0294f94909 100644 --- a/v3/src/utilities/translation/lang/en-US.json5 +++ b/v3/src/utilities/translation/lang/en-US.json5 @@ -1389,6 +1389,8 @@ "V3.Redo.graph.uncheckLastParentOnly": "Redo unchecking \"Last\" checkbox", "V3.Undo.graph.changeAttributeBinningType": "Undo changing attribute binning type", "V3.Redo.graph.changeAttributeBinningType": "Redo changing attribute binning type", + "V3.Undo.graph.changePointShape": "Undo changing data shape", + "V3.Redo.graph.changePointShape": "Redo changing data shape", "V3.Undo.legend.setLegendMin": "Undo setting legend minimum", "V3.Redo.legend.setLegendMin": "Redo setting legend minimum", "V3.Undo.legend.setLegendMax": "Undo setting legend maximum", diff --git a/v3/src/v2/codap-v2-data-context-types.ts b/v3/src/v2/codap-v2-data-context-types.ts index 65cfc0dd8b..f3953acb1a 100644 --- a/v3/src/v2/codap-v2-data-context-types.ts +++ b/v3/src/v2/codap-v2-data-context-types.ts @@ -55,6 +55,20 @@ export interface ICodapV2Attribute { precision?: number | string | null unit?: string | null decimals?: string + /* + * v3-specific enhancements, for attribute state v2 has no field of its own for. + * + * This is deliberately not folded into _categoryMap. V2 treats every key of that map except + * `__order`, `stroke-color` and `stroke-transparency` as a category: it appends unknown keys to + * __order and writes the result back out, so an extra key there becomes a phantom category in + * the user's legend and is persisted. An unknown key here is merely mixed onto the SproutCore + * model and dropped by the attribute's toArchive allowlist, so v2 is unaffected and a v2 + * re-save loses the value rather than corrupting the document. + */ + v3?: { + // per-legend-category point shapes, keyed by category value; unset categories use the default + categoryShapes?: Record + } } export const v3TypeFromV2TypeIndex: Array = [ diff --git a/v3/src/v2/codap-v2-data-set-importer.ts b/v3/src/v2/codap-v2-data-set-importer.ts index 199e19d7f9..48901ebdc3 100644 --- a/v3/src/v2/codap-v2-data-set-importer.ts +++ b/v3/src/v2/codap-v2-data-set-importer.ts @@ -208,14 +208,15 @@ export class CodapV2DataSetImporter { importCategories(data: IDataSet, metadata: IDataSetMetadata, attributes: ICodapV2Attribute[]) { attributes.forEach(v2Attr => { const { - guid, colormap, _categoryMap + guid, colormap, _categoryMap, v3 } = v2Attr const attribute = data.getAttribute(toV3AttrId(guid)) if (attribute) { const categorySetInput: Maybe = _categoryMap || colormap - if (categorySetInput) { + const categoryShapes = v3?.categoryShapes + if (categorySetInput || categoryShapes) { // create CategorySet if necessary - const categorySetSnap = importV2CategorySet(attribute, categorySetInput) + const categorySetSnap = importV2CategorySet(attribute, categorySetInput, categoryShapes) if (categorySetSnap) { metadata.setCategorySet(attribute.id, categorySetSnap) } diff --git a/v3/src/v2/codap-v2-type-utils.ts b/v3/src/v2/codap-v2-type-utils.ts index 4c1b329277..a05de93c89 100644 --- a/v3/src/v2/codap-v2-type-utils.ts +++ b/v3/src/v2/codap-v2-type-utils.ts @@ -1,5 +1,6 @@ import { AxisModelType } from "../components/axis/models/axis-model" import { IFormula } from "../models/formula/formula" +import { kDefaultPointShape } from "../utilities/point-shape-utils" interface IBaseLegendQuantileProps { numberOfLegendQuantiles?: number @@ -24,6 +25,7 @@ export type V2PlaceToV3AxisTypeMap = Partial> interface IImportV3Properties extends IImportLegendQuantileProps { axisTypes?: V2PlaceToV3AxisTypeMap filterFormula?: string + pointShape?: string } function hasFilterFormula(props: IExportV3Properties): boolean { @@ -101,9 +103,10 @@ export function applyImportedLegendBinCount( interface IExportV3PropsOptions { axisTypes?: V2PlaceToV3AxisTypeMap includeLegendQuantiles?: boolean + pointShape?: string } export function exportV3Properties(props: IExportV3Properties, options?: IExportV3PropsOptions) { - const { axisTypes, includeLegendQuantiles } = options || {} + const { axisTypes, includeLegendQuantiles, pointShape } = options || {} const _hasFilter = hasFilterFormula(props) // Only write legend quantile props into the v3 namespace when explicitly requested (maps). Graphs // round-trip the bin count via the native top-level numberOfLegendQuantiles, so a v3 copy would be @@ -113,12 +116,15 @@ export function exportV3Properties(props: IExportV3Properties, options?: IExport : {} const _hasLegendQuantiles = Object.keys(legendStorage).length > 0 const _hasAxisTypes = axisTypes && (Object.keys(axisTypes).length > 0) - return _hasFilter || _hasLegendQuantiles || _hasAxisTypes + // Only a non-default shape is worth writing; a document that never used the feature gains nothing. + const _hasPointShape = pointShape != null && pointShape !== kDefaultPointShape + return _hasFilter || _hasLegendQuantiles || _hasAxisTypes || _hasPointShape ? { v3: { ...(_hasFilter ? { filterFormula: props.filterFormula?.display } : {}), ...legendStorage, - ...(axisTypes ? { axisTypes } : {}) + ...(axisTypes ? { axisTypes } : {}), + ...(_hasPointShape ? { pointShape } : {}) } } : {} diff --git a/v3/src/v2/codap-v2-types.ts b/v3/src/v2/codap-v2-types.ts index f9cf1a848f..2cd3b1749e 100644 --- a/v3/src/v2/codap-v2-types.ts +++ b/v3/src/v2/codap-v2-types.ts @@ -459,6 +459,9 @@ export interface ICodapV2GraphStorage extends ICodapV2BaseComponentStorage { // v3 extensions v3?: { filterFormula?: string + // The shape used when no legend attribute assigns one per category. Per-category shapes are + // attribute state and travel on the attribute's own v3 namespace instead. + pointShape?: string } & ICodapV2LegendQuantileV3Extensions } diff --git a/v3/src/v2/v2-document-round-trip.test.ts b/v3/src/v2/v2-document-round-trip.test.ts index 4205094e6b..6c35fb48f1 100644 --- a/v3/src/v2/v2-document-round-trip.test.ts +++ b/v3/src/v2/v2-document-round-trip.test.ts @@ -1,7 +1,10 @@ import { createCodapDocument } from "../models/codap/create-codap-document" +import { DataBroker } from "../models/data/data-broker" +import { kDefaultPointShape, PointShape } from "../utilities/point-shape-utils" import { createDataSet } from "../models/data/data-set-conversion" import { serializeCodapV2Document } from "../models/document/serialize-document" import { kSharedDataSetType, SharedDataSet } from "../models/shared/shared-data-set" +import { getMetadataFromDataSet } from "../models/shared/shared-data-utils" import { getSharedModelManager } from "../models/tiles/tile-environment" import { CodapV2Document } from "./codap-v2-document" import { importV2Document } from "./import-v2-document" @@ -51,3 +54,90 @@ describe("v2 document round-trip (CODAP-1348)", () => { expect(doubleAttr?.formula?.display).toBe("x * 2") }) }) + +/* + * Per-category point shapes travel in the attribute's v3 namespace, and the shape used when no + * legend is assigned travels in the graph component's. This exercises the whole chain -- model to + * v2 JSON and back -- rather than the export and import halves in isolation. + */ +describe("v2 document round-trip of point shapes", () => { + async function roundTrip(document: ReturnType) { + const v2Json = await serializeCodapV2Document(document) + const v3Document = importV2Document(new CodapV2Document(v2Json)) + const restoredData = getSharedModelManager(v3Document)! + .getSharedModelsByType(kSharedDataSetType)[0]?.dataSet + return { v2Json, v3Document, restoredData } + } + + function documentWithCategories() { + const document = createCodapDocument() + const sharedModelManager = getSharedModelManager(document)! + // DataBroker rather than a bare SharedDataSet, so the dataset gets its DataSetMetadata -- + // that is where category sets, and therefore shapes, live. + const dataBroker = new DataBroker({ sharedModelManager }) + const data = createDataSet({ attributes: [{ name: "habitat" }] }) + const attrId = data.attrFromName("habitat")!.id + data.addCases([ + { __id__: "c1", [attrId]: "land" }, + { __id__: "c2", [attrId]: "water" }, + { __id__: "c3", [attrId]: "both" } + ]) + data.validateCases() + dataBroker.addDataSet(data) + return { document, data, attrId } + } + + it("preserves per-category shapes", async () => { + const { document, data, attrId } = documentWithCategories() + const shapeFor = (cat: string, shape: PointShape) => + getMetadataFromDataSet(data)!.getCategorySet(attrId)!.setShapeForCategory(cat, shape) + shapeFor("land", "star") + shapeFor("water", "diamond") + + const { restoredData } = await roundTrip(document) + const restoredAttr = restoredData.attrFromName("habitat")! + const restoredSet = getMetadataFromDataSet(restoredData)!.getCategorySet(restoredAttr.id)! + + expect(restoredSet.shapeForCategory("land")).toBe("star") + expect(restoredSet.shapeForCategory("water")).toBe("diamond") + // a category left at the default comes back at the default + expect(restoredSet.shapeForCategory("both")).toBe(kDefaultPointShape) + }) + + it("preserves shapes alongside colors without either disturbing the other", async () => { + const { document, data, attrId } = documentWithCategories() + // re-read between mutations: the first one promotes the provisional set, replacing the instance + getMetadataFromDataSet(data)!.getCategorySet(attrId)!.setColorForCategory("land", "#123456") + getMetadataFromDataSet(data)!.getCategorySet(attrId)!.setShapeForCategory("land", "plus") + + const { restoredData } = await roundTrip(document) + const restoredAttr = restoredData.attrFromName("habitat")! + const restoredSet = getMetadataFromDataSet(restoredData)!.getCategorySet(restoredAttr.id)! + + expect(restoredSet.colorForCategory("land")).toBe("#123456") + expect(restoredSet.shapeForCategory("land")).toBe("plus") + }) + + it("adds nothing to the v2 JSON when no shape is assigned", async () => { + const { document, data, attrId } = documentWithCategories() + getMetadataFromDataSet(data)!.getCategorySet(attrId)!.setColorForCategory("land", "#123456") + + const { v2Json } = await roundTrip(document) + // documents that never used the feature are byte-for-byte unaffected by it + expect(JSON.stringify(v2Json)).not.toContain("categoryShapes") + }) + + it("keeps shapes out of _categoryMap, which v2 reads as categories", async () => { + const { document, data, attrId } = documentWithCategories() + getMetadataFromDataSet(data)!.getCategorySet(attrId)!.setColorForCategory("land", "#123456") + getMetadataFromDataSet(data)!.getCategorySet(attrId)!.setShapeForCategory("land", "star") + + const { v2Json } = await roundTrip(document) + const v2Attr = (v2Json.contexts?.[0] as any)?.collections?.[0]?.attrs?.[0] + expect(v2Attr).toBeDefined() + expect(v2Attr.v3?.categoryShapes).toEqual({ land: "star" }) + + expect(JSON.stringify(v2Attr._categoryMap)).not.toContain("star") + expect(v2Attr._categoryMap.__order).toEqual(["both", "land", "water"]) + }) +})