diff --git a/common/changes/@bentley/ui-components/ui-components-collect-model-changes_2021-03-24-13-34.json b/common/changes/@bentley/ui-components/ui-components-collect-model-changes_2021-03-24-13-34.json new file mode 100644 index 000000000000..e7cd1282340a --- /dev/null +++ b/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" +} \ No newline at end of file diff --git a/common/changes/@bentley/ui-framework/ui-components-collect-model-changes_2021-03-24-13-34.json b/common/changes/@bentley/ui-framework/ui-components-collect-model-changes_2021-03-24-13-34.json new file mode 100644 index 000000000000..61faabf7bcce --- /dev/null +++ b/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" +} \ No newline at end of file diff --git a/ui/components/src/test/tree/controlled/TreeModelSource.test.ts b/ui/components/src/test/tree/controlled/TreeModelSource.test.ts index c0286646601e..052ead80993f 100644 --- a/ui/components/src/test/tree/controlled/TreeModelSource.test.ts +++ b/ui/components/src/test/tree/controlled/TreeModelSource.test.ts @@ -3,17 +3,16 @@ * 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(); const mutableTreeModelMock = moq.Mock.ofType(); @@ -21,6 +20,10 @@ describe("TreeModelSource", () => { let onTreeNodeChanged: BeEvent; + 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,19 +83,19 @@ 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", () => { @@ -109,53 +103,160 @@ describe("TreeModelSource", () => { 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(); }); - }); - }); diff --git a/ui/components/src/ui-components/tree/controlled/TreeModelSource.ts b/ui/components/src/ui-components/tree/controlled/TreeModelSource.ts index 26e415a78e71..e112d52778ad 100644 --- a/ui/components/src/ui-components/tree/controlled/TreeModelSource.ts +++ b/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(); 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 }; } } diff --git a/ui/framework/src/test/imodel-components/models-tree/ModelsTree.test.tsx b/ui/framework/src/test/imodel-components/models-tree/ModelsTree.test.tsx index dbb67b05c014..ec30f68082b8 100644 --- a/ui/framework/src/test/imodel-components/models-tree/ModelsTree.test.tsx +++ b/ui/framework/src/test/imodel-components/models-tree/ModelsTree.test.tsx @@ -142,7 +142,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(); await waitForElement(() => { const renderedNode = result.getByTestId("tree-node");