Merge pull request #6960 from TerriaJS/array-merge-top

Add MergeStrategy to objectArrayTrait

CHANGES.md

@@ -2,6 +2,10 @@
 
 #### next release (8.5.3)
 
+- **Breaking changes:**
+  - Add `MergeStrategy` to `objectArrayTrait` - this includes a new `topStratum` strategy - similar to `Merge.All` (the default behaviour), but only elements that exist in the top-most strata will be merged with lower strata. Elements that only exist in lower strata will be removed.
+  - **Note** the only trait with `MergeStrategy` set to `topStratum` is `lines` in `TableChartStyleTraits`.
+- Fix `y-column` in `FeatureInfoPanelChart` (`<chart>`)
 - [The next improvement]
 
 #### 8.5.2 - 2024-03-07

lib/ReactViews/Custom/CsvChartCustomComponent.ts

@@ -169,7 +169,8 @@ export default class CsvChartCustomComponent extends ChartCustomComponent<CsvCat
       );
 
       (attrs.yColumns || []).forEach((y) => {
-        chartStyle.chart.addObject(CommonStrata.user, "lines", y)!;
+        const line = chartStyle.chart.addObject(CommonStrata.user, "lines", y)!;
+        line.setTrait(CommonStrata.user, "isSelectedInWorkbench", true);
       });
 
       item.setTrait(CommonStrata.user, "activeStyle", "chart");

lib/Traits/ArrayNestedStrataMap.ts

@@ -1,6 +1,7 @@
 import { computed, makeObservable } from "mobx";
-import createStratumInstance from "../Models/Definition/createStratumInstance";
 import StratumFromTraits from "../Models/Definition/StratumFromTraits";
+import createStratumInstance from "../Models/Definition/createStratumInstance";
+import { MergeStrategy } from "./Decorators/objectArrayTrait";
 import ModelTraits from "./ModelTraits";
 import Stratified from "./Stratified";
 import TraitsConstructor from "./TraitsConstructor";
@@ -18,7 +19,7 @@ export default class ArrayNestedStrataMap<T extends ModelTraits>
     readonly objectTraits: TraitsConstructorWithRemoval<T>,
     readonly objectIdProperty: string | number | symbol,
     readonly objectId: string,
-    readonly merge: boolean
+    readonly merge: MergeStrategy
   ) {
     makeObservable(this);
   }
@@ -157,8 +158,8 @@ export default class ArrayNestedStrataMap<T extends ModelTraits>
       // This stratum applies to this object.
       result.set(stratumId, thisObject);
 
-      // If merge is false, only return the top-most strata's object
-      if (!this.merge) return result;
+      // If merge strategy is None, only return the top-most strata's object
+      if (this.merge === MergeStrategy.None) return result;
     }
 
     return result;

lib/Traits/Decorators/objectArrayTrait.ts

@@ -18,15 +18,26 @@ import ModelTraits from "../ModelTraits";
 import Trait, { TraitOptions } from "../Trait";
 import traitsClassToModelClass from "../traitsClassToModelClass";
 
+export enum MergeStrategy {
+  /**
+   * Merge array elements across strata - this is the default.
+   */
+  All = "all",
+  /** Similar to Merge.All, but elements that exist in the top-most strata will be merged with lower strata. Elements that only exist in lower strata will be removed. */
+  TopStratum = "topStratum",
+  /** Don't merge array elements across strata. */
+  None = "none"
+}
+
 export interface ObjectArrayTraitOptions<T extends ModelTraits>
   extends TraitOptions {
   type: TraitsConstructorWithRemoval<T>;
   idProperty: keyof T | "index";
   modelClass?: ModelConstructor<Model<T>>;
   /**
-   * Merge array elements across strata (if merge is false, each element will only be the top-most strata's object). Default is `true`
+   * How to merge array elements across strata. By default all elements are merged.
    */
-  merge?: boolean;
+  merge?: MergeStrategy;
 }
 
 export default function objectArrayTrait<T extends ModelTraits>(
@@ -50,14 +61,14 @@ export class ObjectArrayTrait<T extends ModelTraits> extends Trait {
   readonly idProperty: keyof T | "index";
   readonly decoratorForFlattened = computed.struct;
   readonly modelClass: ModelConstructor<Model<T>>;
-  readonly merge: boolean;
+  readonly merge: MergeStrategy;
 
   constructor(id: string, options: ObjectArrayTraitOptions<T>, parent: any) {
     super(id, options, parent);
     this.type = options.type;
     this.idProperty = options.idProperty;
     this.modelClass = options.modelClass || traitsClassToModelClass(this.type);
-    this.merge = options.merge ?? true;
+    this.merge = options.merge ?? MergeStrategy.All;
   }
 
   private readonly createObject: (
@@ -116,7 +127,7 @@ export class ObjectArrayTrait<T extends ModelTraits> extends Trait {
     // Strata order is important here for two reasons:
 
     // Determining array order:
-    // By default, we assume bottom strata order is "more" correct than top
+    // By default, we assume bottom strata order is "more" correct than top - with exception to default stratum - definition stratum is more correct
     // For example:
     // - In some LoadableStratum we set the objectArray to: [{item:"one", value:"a"}, {item:"two", value:"b"}]
     // - Then in the user stratum we set [{item:"two", value:"c"}]
@@ -142,6 +153,19 @@ export class ObjectArrayTrait<T extends ModelTraits> extends Trait {
       StratumOrder.sortTopToBottom(model.strata)
     );
 
+    // If merge strategy is topStratum, then we only want to keep the ids that exist in the top stratum
+    if (this.merge === MergeStrategy.TopStratum) {
+      const topStratum = model.strataTopToBottom.values().next().value;
+
+      const topIds = this.getIdsAcrossStrata(new Map([["top", topStratum]]));
+      // Remove ids that don't exist in the top stratum
+      idsInCorrectOrder.forEach((id) => {
+        if (!topIds.has(id)) {
+          idsWithCorrectRemovals.delete(id);
+        }
+      });
+    }
+
     // Correct ids are:
     // - Ids ordered by strata bottom to top combined with
     // - Ids removed by strata top to bottom

lib/Traits/TraitsClasses/ExportWebCoverageServiceTraits.ts

@@ -1,4 +1,6 @@
-import objectArrayTrait from "../Decorators/objectArrayTrait";
+import objectArrayTrait, {
+  MergeStrategy
+} from "../Decorators/objectArrayTrait";
 import objectTrait from "../Decorators/objectTrait";
 import primitiveTrait from "../Decorators/primitiveTrait";
 import mixTraits from "../mixTraits";
@@ -57,7 +59,7 @@ export class WebCoverageServiceParameterTraits extends ModelTraits {
   @objectArrayTrait({
     type: KeyValueTraits,
     idProperty: "index",
-    merge: false,
+    merge: MergeStrategy.None,
     name: "Additional key-value parameters to add as URL query parameters",
     description:
       "Each key-value will be added to URL like so - `someurl.com?key=value`."

lib/Traits/TraitsClasses/LegendOwnerTraits.ts

@@ -1,4 +1,6 @@
-import objectArrayTrait from "../Decorators/objectArrayTrait";
+import objectArrayTrait, {
+  MergeStrategy
+} from "../Decorators/objectArrayTrait";
 import primitiveTrait from "../Decorators/primitiveTrait";
 import ModelTraits from "../ModelTraits";
 import LegendTraits from "./LegendTraits";
@@ -9,7 +11,7 @@ export default class LegendOwnerTraits extends ModelTraits {
     description: "The legends to display on the workbench.",
     type: LegendTraits,
     idProperty: "index",
-    merge: false
+    merge: MergeStrategy.None
   })
   legends?: LegendTraits[];
 

lib/Traits/TraitsClasses/Table/ChartStyleTraits.ts

@@ -1,6 +1,8 @@
-import ModelTraits from "../../ModelTraits";
+import objectArrayTrait, {
+  MergeStrategy
+} from "../../Decorators/objectArrayTrait";
 import primitiveTrait from "../../Decorators/primitiveTrait";
-import objectArrayTrait from "../../Decorators/objectArrayTrait";
+import ModelTraits from "../../ModelTraits";
 
 export class TableChartLineStyleTraits extends ModelTraits {
   @primitiveTrait({
@@ -60,7 +62,8 @@ export default class TableChartStyleTraits extends ModelTraits {
     description:
       "Lines on the chart, each of which is formed by plotting a column as the Y-axis.",
     type: TableChartLineStyleTraits,
-    idProperty: "yAxisColumn"
+    idProperty: "yAxisColumn",
+    merge: MergeStrategy.TopStratum
   })
   lines?: TableChartLineStyleTraits[];
 }

test/Traits/objectArrayTraitSpec.ts

@@ -2,9 +2,13 @@ import { configure } from "mobx";
 import CreateModel from "../../lib/Models/Definition/CreateModel";
 import createStratumInstance from "../../lib/Models/Definition/createStratumInstance";
 import Terria from "../../lib/Models/Terria";
-import objectArrayTrait from "../../lib/Traits/Decorators/objectArrayTrait";
+import objectArrayTrait, {
+  MergeStrategy
+} from "../../lib/Traits/Decorators/objectArrayTrait";
 import primitiveTrait from "../../lib/Traits/Decorators/primitiveTrait";
 import ModelTraits from "../../lib/Traits/ModelTraits";
+import updateModelFromJson from "../../lib/Models/Definition/updateModelFromJson";
+import CommonStrata from "../../lib/Models/Definition/CommonStrata";
 
 configure({
   enforceActions: "always",
@@ -69,7 +73,25 @@ class OuterTraitsNoMerge extends ModelTraits {
     name: "Inner",
     description: "Inner",
     idProperty: "foo",
-    merge: false
+    merge: MergeStrategy.None
+  })
+  inner?: InnerTraits[];
+
+  @primitiveTrait({
+    type: "string",
+    name: "Other",
+    description: "Other"
+  })
+  other?: string;
+}
+
+class OuterTraitsMergeTop extends ModelTraits {
+  @objectArrayTrait({
+    type: InnerTraits,
+    name: "Inner",
+    description: "Inner",
+    idProperty: "foo",
+    merge: MergeStrategy.TopStratum
   })
   inner?: InnerTraits[];
 
@@ -83,6 +105,7 @@ class OuterTraitsNoMerge extends ModelTraits {
 
 class TestModel extends CreateModel(OuterTraits) {}
 class TestModelNoMerge extends CreateModel(OuterTraitsNoMerge) {}
+class TestModelMergeTop extends CreateModel(OuterTraitsMergeTop) {}
 
 describe("objectArrayTrait", function () {
   it("returns an empty model if all strata are undefined", function () {
@@ -368,4 +391,43 @@ describe("objectArrayTrait", function () {
     expect(model.inner[0].bar).toBe(4);
     expect(model.inner[0].baz).toBe(true);
   });
+
+  it("updates to reflect new strata added after evaluation (with merge = top)", function () {
+    const terria = new Terria();
+    const model = new TestModelMergeTop("test", terria);
+
+    updateModelFromJson(model, CommonStrata.definition, {
+      inner: [
+        { foo: "this-exists-in-definition-and-user", bar: 1, baz: false },
+        { foo: "this-exists-in-definition", bar: 2, baz: true },
+        { foo: "this-also-exists-in-definition-and-user" }
+      ]
+    });
+
+    expect(model.inner.length).toBe(3);
+    expect(model.inner[0].foo).toBe("this-exists-in-definition-and-user");
+    expect(model.inner[0].bar).toBe(1);
+    expect(model.inner[0].baz).toBe(false);
+
+    expect(model.inner[1].foo).toBe("this-exists-in-definition");
+    expect(model.inner[1].bar).toBe(2);
+    expect(model.inner[1].baz).toBe(true);
+
+    expect(model.inner[2].foo).toBe("this-also-exists-in-definition-and-user");
+
+    updateModelFromJson(model, CommonStrata.user, {
+      inner: [
+        { foo: "this-exists-in-definition-and-user", baz: true },
+        { foo: "this-also-exists-in-definition-and-user", bar: 3, baz: true }
+      ]
+    });
+
+    expect(model.inner.length).toBe(2);
+    expect(model.inner[0].foo).toBe("this-exists-in-definition-and-user");
+    expect(model.inner[0].bar).toBe(1);
+    expect(model.inner[0].baz).toBe(true);
+    expect(model.inner[1].foo).toBe("this-also-exists-in-definition-and-user");
+    expect(model.inner[1].bar).toBe(3);
+    expect(model.inner[1].baz).toBe(true);
+  });
 });