UI: Make tree node modicifation detection more robust (bp #1023)

common/changes/@bentley/ui-components/ui-components-collect-model-changes_2021-03-24-13-34.json

@@ -0,0 +1,11 @@
+{
+  "changes": [
+    {
+      "packageName": "@bentley/ui-components",
+      "comment": "`TreeModelSource`: Fix `onModelChanged` event sometimes listing same node id multiple times.",
+      "type": "none"
+    }
+  ],
+  "packageName": "@bentley/ui-components",
+  "email": "70327485+roluk@users.noreply.github.com"
+}

common/changes/@bentley/ui-framework/ui-components-collect-model-changes_2021-03-24-13-34.json

@@ -0,0 +1,11 @@
+{
+  "changes": [
+    {
+      "packageName": "@bentley/ui-framework",
+      "comment": "",
+      "type": "none"
+    }
+  ],
+  "packageName": "@bentley/ui-framework",
+  "email": "70327485+roluk@users.noreply.github.com"
+}

ui/components/src/test/tree/controlled/TreeModelSource.test.ts

@@ -3,24 +3,27 @@
 * See LICENSE.md in the project root for license terms and full copyright notice.
 *--------------------------------------------------------------------------------------------*/
 import { expect } from "chai";
-import * as faker from "faker";
 import sinon from "sinon";
 import * as moq from "typemoq";
 import { BeEvent } from "@bentley/bentleyjs-core";
 import { PropertyRecord } from "@bentley/ui-abstract";
 import { MutableTreeModel, TreeModelNodeInput, VisibleTreeNodes } from "../../../ui-components/tree/controlled/TreeModel";
-import { TreeModelSource } from "../../../ui-components/tree/controlled/TreeModelSource";
+import { TreeModelChanges, TreeModelSource } from "../../../ui-components/tree/controlled/TreeModelSource";
 import { ITreeDataProvider, TreeDataChangesListener } from "../../../ui-components/tree/TreeDataProvider";
+import TestUtils from "../../TestUtils";
 
 describe("TreeModelSource", () => {
-
   let modelSource: TreeModelSource;
   const dataProviderMock = moq.Mock.ofType<ITreeDataProvider>();
   const mutableTreeModelMock = moq.Mock.ofType<MutableTreeModel>();
   const visibleNodesMock = moq.Mock.ofType<VisibleTreeNodes>();
 
   let onTreeNodeChanged: BeEvent<TreeDataChangesListener>;
 
+  before(async () => {
+    await TestUtils.initializeUiComponents();
+  });
+
   beforeEach(() => {
     dataProviderMock.reset();
     mutableTreeModelMock.reset();
@@ -32,7 +35,6 @@ describe("TreeModelSource", () => {
   });
 
   describe("constructor", () => {
-
     it("listens for onModelChanged events", () => {
       (modelSource as any)._model = mutableTreeModelMock.object;
       mutableTreeModelMock.setup((x) => x.computeVisibleNodes()).returns(() => visibleNodesMock.object).verifiable(moq.Times.exactly(2));
@@ -41,23 +43,23 @@ describe("TreeModelSource", () => {
       modelSource.getVisibleNodes();
       mutableTreeModelMock.verifyAll();
     });
-
   });
 
   describe("modifyModel", () => {
-    let inputNode: TreeModelNodeInput;
-
-    beforeEach(() => {
-      inputNode = {
-        id: faker.random.uuid(),
-        isExpanded: faker.random.boolean(),
-        item: { id: faker.random.uuid(), label: PropertyRecord.fromString(faker.random.word(), "label") },
-        label: PropertyRecord.fromString(faker.random.word(), "label"),
-        isLoading: faker.random.boolean(),
-        isSelected: faker.random.boolean(),
+    function createNodeInput(id: string): TreeModelNodeInput {
+      const label = PropertyRecord.fromString(id, id);
+      return {
+        id,
+        item: { id, label },
+        label,
+        isExpanded: false,
+        isLoading: false,
+        isSelected: false,
       };
+    }
 
-      modelSource.modifyModel((model) => { model.setChildren(undefined, [inputNode], 0); });
+    beforeEach(() => {
+      modelSource.modifyModel((model) => { model.setChildren(undefined, [createNodeInput("root1")], 0); });
     });
 
     it("does not emit onModelChanged event if model did not change", () => {
@@ -67,20 +69,12 @@ describe("TreeModelSource", () => {
     });
 
     it("emits onModelChanged event with added node id", () => {
-      const newInput: TreeModelNodeInput = {
-        id: faker.random.uuid(),
-        isExpanded: faker.random.boolean(),
-        item: { id: faker.random.uuid(), label: PropertyRecord.fromString(faker.random.word(), "label") },
-        label: PropertyRecord.fromString(faker.random.word(), "label"),
-        isLoading: faker.random.boolean(),
-        isSelected: faker.random.boolean(),
-      };
       const spy = sinon.spy(modelSource.onModelChanged, "emit");
-      modelSource.modifyModel((model) => { model.setChildren(undefined, [newInput], 1); });
+      modelSource.modifyModel((model) => { model.setChildren(undefined, [createNodeInput("root2")], 1); });
       expect(spy).to.be.called;
       const changes = spy.args[0][0][1];
       expect(changes.addedNodeIds.length).to.be.eq(1);
-      expect(changes.addedNodeIds[0]).to.be.eq(newInput.id);
+      expect(changes.addedNodeIds[0]).to.be.eq("root2");
     });
 
     it("emits onModelChanged event with removed node id", () => {
@@ -89,73 +83,180 @@ describe("TreeModelSource", () => {
       expect(spy).to.be.called;
       const changes = spy.args[0][0][1];
       expect(changes.removedNodeIds.length).to.be.eq(1);
-      expect(changes.removedNodeIds[0]).to.be.eq(inputNode.id);
+      expect(changes.removedNodeIds[0]).to.be.eq("root1");
     });
 
     it("emits onModelChanged event with modified node id", () => {
       const spy = sinon.spy(modelSource.onModelChanged, "emit");
       modelSource.modifyModel((model) => {
-        const node = model.getNode(inputNode.id);
+        const node = model.getNode("root1");
         node!.isSelected = !node!.isSelected;
       });
       expect(spy).to.be.called;
       const changes = spy.args[0][0][1];
       expect(changes.modifiedNodeIds.length).to.be.eq(1);
-      expect(changes.modifiedNodeIds[0]).to.be.eq(inputNode.id);
+      expect(changes.modifiedNodeIds[0]).to.be.eq("root1");
     });
 
     it("clears model and adds new nodes", () => {
       modelSource.modifyModel((model) => {
         model.clearChildren(undefined);
       });
       expect(modelSource.getVisibleNodes().getNumNodes()).to.be.eq(0);
-      const newInput: TreeModelNodeInput = {
-        id: faker.random.uuid(),
-        isExpanded: faker.random.boolean(),
-        item: { id: faker.random.uuid(), label: PropertyRecord.fromString(faker.random.word(), "label") },
-        label: PropertyRecord.fromString(faker.random.word(), "label"),
-        isLoading: faker.random.boolean(),
-        isSelected: faker.random.boolean(),
-      };
       modelSource.modifyModel((model) => {
-        model.setChildren(undefined, [newInput], 0);
+        model.setChildren(undefined, [createNodeInput("new_root1")], 0);
       });
       expect(modelSource.getVisibleNodes().getNumNodes()).to.be.eq(1);
     });
 
     it("overrides existing children multiple times", () => {
-      const newInput: TreeModelNodeInput = {
-        id: faker.random.uuid(),
-        isExpanded: faker.random.boolean(),
-        item: { id: faker.random.uuid(), label: PropertyRecord.fromString(faker.random.word(), "label") },
-        label: PropertyRecord.fromString(faker.random.word(), "label"),
-        isLoading: faker.random.boolean(),
-        isSelected: faker.random.boolean(),
-      };
       modelSource.modifyModel((model) => {
         model.setNumChildren(undefined, 1);
-        model.setChildren(undefined, [newInput], 0);
+        model.setChildren(undefined, [createNodeInput("new_root1")], 0);
       });
       modelSource.modifyModel((model) => {
         model.setNumChildren(undefined, 1);
-        model.setChildren(undefined, [inputNode], 0);
+        model.setChildren(undefined, [createNodeInput("root1")], 0);
       });
     });
 
+    describe("node change detection", () => {
+      describe("node addition", () => {
+        it("does not duplicate added nodes", () => {
+          const spy = sinon.spy(modelSource.onModelChanged, "emit");
+          modelSource.modifyModel((model) => {
+            // Add added_node twice
+            expect(model.changeNodeId("root1", "added_node")).to.be.true;
+            expect(model.changeNodeId("added_node", "temp")).to.be.true;
+            expect(model.changeNodeId("temp", "added_node")).to.be.true;
+          });
+
+          const expectedChanges: TreeModelChanges = {
+            addedNodeIds: ["added_node"],
+            modifiedNodeIds: [],
+            removedNodeIds: ["root1"],
+          };
+          expect(spy).to.have.been.calledOnceWithExactly([modelSource.getModel(), expectedChanges]);
+        });
+
+        it("does not mistake attribute addition as node addtion", () => {
+          const spy = sinon.spy(modelSource.onModelChanged, "emit");
+          modelSource.modifyModel((model) => {
+            const node = model.getNode("root1")!;
+            node.checkbox.tooltip = "test tooltip";
+          });
+
+          const expectedChanges: TreeModelChanges = {
+            addedNodeIds: [],
+            modifiedNodeIds: ["root1"],
+            removedNodeIds: [],
+          };
+          expect(spy).to.have.been.calledOnceWithExactly([modelSource.getModel(), expectedChanges]);
+        });
+
+        it("only notifies of node addition even when the added node is also modified", () => {
+          const spy = sinon.spy(modelSource.onModelChanged, "emit");
+          modelSource.modifyModel((model) => {
+            model.insertChild(undefined, createNodeInput("root2"), 1);
+            const node = model.getNode("root2")!;
+            node.isSelected = true;
+          });
+
+          const expectedChanges: TreeModelChanges = {
+            addedNodeIds: ["root2"],
+            modifiedNodeIds: [],
+            removedNodeIds: [],
+          };
+          expect(spy).to.have.been.calledOnceWithExactly([modelSource.getModel(), expectedChanges]);
+        });
+      });
+
+      describe("node removal", () => {
+        it("does not duplicate removed nodes", () => {
+          const spy = sinon.spy(modelSource.onModelChanged, "emit");
+          modelSource.modifyModel((model) => {
+            // Remove root1 node twice
+            expect(model.changeNodeId("root1", "another_id")).to.be.true;
+            expect(model.changeNodeId("another_id", "root1")).to.be.true;
+            expect(model.changeNodeId("root1", "another_id")).to.be.true;
+          });
+
+          const expectedChanges: TreeModelChanges = {
+            addedNodeIds: ["another_id"],
+            modifiedNodeIds: [],
+            removedNodeIds: ["root1"],
+          };
+          expect(spy).to.have.been.calledOnceWithExactly([modelSource.getModel(), expectedChanges]);
+        });
+
+        it("does not mistake attribute removal as node removal", () => {
+          modelSource.modifyModel((model) => {
+            const node = model.getNode("root1")!;
+            node.checkbox.tooltip = "test tooltip";
+            node.editingInfo = { onCommit: () => { }, onCancel: () => { } };
+          });
+
+          const spy = sinon.spy(modelSource.onModelChanged, "emit");
+          modelSource.modifyModel((model) => {
+            const node = model.getNode("root1")!;
+            delete node.checkbox.tooltip;
+            delete node.editingInfo;
+          });
+
+          const expectedChanges: TreeModelChanges = {
+            addedNodeIds: [],
+            modifiedNodeIds: ["root1"],
+            removedNodeIds: [],
+          };
+          expect(spy).to.have.been.calledOnceWithExactly([modelSource.getModel(), expectedChanges]);
+        });
+      });
+
+      describe("node modification", () => {
+        it("does not duplicate modified nodes when overwriting nodes", () => {
+          const spy = sinon.spy(modelSource.onModelChanged, "emit");
+          modelSource.modifyModel((model) => {
+            // Reassign root1 node twice
+            model.insertChild(undefined, createNodeInput("root1"), 0);
+            model.insertChild(undefined, createNodeInput("root1"), 0);
+          });
+
+          const expectedChanges: TreeModelChanges = {
+            addedNodeIds: [],
+            modifiedNodeIds: ["root1"],
+            removedNodeIds: [],
+          };
+          expect(spy).to.have.been.calledOnceWithExactly([modelSource.getModel(), expectedChanges]);
+        });
+
+        it("does not duplicate modified nodes when adding new properties", () => {
+          const spy = sinon.spy(modelSource.onModelChanged, "emit");
+          modelSource.modifyModel((model) => {
+            const node = model.getNode("root1")!;
+            node.checkbox.tooltip = "test";
+            node.editingInfo = { onCommit: () => { }, onCancel: () => { } };
+          });
+
+          const expectedChanges: TreeModelChanges = {
+            addedNodeIds: [],
+            modifiedNodeIds: ["root1"],
+            removedNodeIds: [],
+          };
+          expect(spy).to.have.been.calledOnceWithExactly([modelSource.getModel(), expectedChanges]);
+        });
+      });
+    });
   });
 
   describe("getModel", () => {
-
     it("returns model", () => {
       (modelSource as any)._model = mutableTreeModelMock.object;
       const model = modelSource.getModel();
       expect(model).to.be.eq(mutableTreeModelMock.object);
     });
-
   });
 
   describe("getVisibleNodes", () => {
-
     beforeEach(() => {
       (modelSource as any)._model = mutableTreeModelMock.object;
     });
@@ -172,7 +273,5 @@ describe("TreeModelSource", () => {
       modelSource.getVisibleNodes();
       mutableTreeModelMock.verifyAll();
     });
-
   });
-
 });

ui/components/src/ui-components/tree/controlled/TreeModelSource.ts

@@ -62,22 +62,35 @@ export class TreeModelSource {
 
   private collectModelChanges(modelPatches: Patch[]): TreeModelChanges {
     const addedNodeIds: string[] = [];
-    const modifiedNodeIds: string[] = [];
+    const modifiedNodeIds = new Set<string>();
     const removedNodeIds: string[] = [];
     for (const patch of modelPatches) {
-      if (patch.path[0] === "_tree" && patch.path[1] === "_idToNode") {
+      if (patch.path.length >= 3 && patch.path[0] === "_tree" && patch.path[1] === "_idToNode") {
         const nodeId = patch.path[2] as string;
+
+        if (patch.path.length > 3) {
+          // Modification occured somewhere inside a node
+          modifiedNodeIds.add(nodeId);
+          continue;
+        }
+
+        // Modification occured directly on _idToNode object
         switch (patch.op) {
-          case "add": addedNodeIds.push(nodeId); break;
-          case "remove": removedNodeIds.push(nodeId); break;
-          case "replace": {
-            if (modifiedNodeIds.indexOf(nodeId) === -1)
-              modifiedNodeIds.push(nodeId);
+          case "add":
+            addedNodeIds.push(nodeId);
+            break;
+
+          case "remove":
+            removedNodeIds.push(nodeId);
+            break;
+
+          case "replace":
+            modifiedNodeIds.add(nodeId);
             break;
-          }
         }
       }
     }
-    return { addedNodeIds, modifiedNodeIds, removedNodeIds };
+
+    return { addedNodeIds, modifiedNodeIds: [...modifiedNodeIds], removedNodeIds };
   }
 }

ui/framework/src/test/imodel-components/models-tree/ModelsTree.test.tsx

@@ -149,7 +149,7 @@ describe("ModelsTree", () => {
         const node = createModelNode();
         setupDataProvider([node]);
 
-        visibilityHandlerMock.setup((x) => x.getVisibilityStatus(moq.It.isAny(), moq.It.isAny())).returns(() => ({ state: "hidden", isDisabled: false })).verifiable(moq.Times.exactly(3));
+        visibilityHandlerMock.setup((x) => x.getVisibilityStatus(moq.It.isAny(), moq.It.isAny())).returns(() => ({ state: "hidden", isDisabled: false })).verifiable(moq.Times.exactly(2));
         const result = render(<ModelsTree iModel={imodelMock.object} modelsVisibilityHandler={visibilityHandlerMock.object} dataProvider={dataProvider} />);
         await waitForElement(() => {
           const renderedNode = result.getByTestId("tree-node");