Add definitionContainers to categories tree #1172

JonasDov · 2025-01-31T18:01:09Z

closes #1126

grigasp

Impressive set of tests - great work!

...et/src/tree-widget-react/components/trees/categories-tree/internal/CategoriesTreeIdsCache.ts

...twin/tree-widget/src/test/trees/categories-tree/internal/CategoriesVisibilityHandler.test.ts

grigasp · 2025-02-03T09:26:13Z

packages/itwin/tree-widget/src/test/trees/categories-tree/internal/VisibilityValidation.ts

+      mergeMap(async (node) => validateNodeVisibility({ ...props, node })),
+    ),
+  );
+  expect(props.nodesToExpect.every((nodeId) => nodesFound.includes(nodeId))).to.be.true;


do we need this check? we're testing visibility here, not the hierarchy

I think we do. I think we need to confirm that visibility of expected nodes is checked. Theoretically, without this check, validateHierarchyVisibility could not check anything and it wouldn't fail

without this check, validateHierarchyVisibility could not check anything and it wouldn't fail

Yes, but that should be tested in our hierarchy tests, not visibility ones. This check unnecessarily complicates the tests, e.g.:

// this is only needed for checking what we got in the hierarchy const expectedIds = [ keys.definitionContainerRoot.id, keys.definitionContainerChild.id, keys.directCategory.id, keys.indirectCategory.id, keys.indirectSubCategory.id, ]; using visibilityTestData = await createVisibilityTestData({ imodel, isVisibleOnInitialize }); const { handler, provider, viewport } = visibilityTestData; await handler.changeVisibility(createDefinitionContainerHierarchyNode(keys.definitionContainerRoot.id), true); await validateHierarchyVisibility({ provider, handler, viewport, visibilityExpectations: {}, expectedIds, all: "visible", });

...twin/tree-widget/src/test/trees/categories-tree/internal/CategoriesVisibilityHandler.test.ts

…tainers

…ees/categories-tree/internal/CategoriesTreeIdsCache.ts Co-authored-by: Grigas <[email protected]>

…ees/categories-tree/internal/CategoriesTreeNode.ts Co-authored-by: Grigas <[email protected]>

…rnal/CategoriesTreeIdsCache.test.ts Co-authored-by: Grigas <[email protected]>

…ttps://github.com/iTwin/viewer-components-react into JonasD/categories-tree-with-definition-containers

…ees/categories-tree/CategoriesTreeDefinition.ts Co-authored-by: Grigas <[email protected]>

…ttps://github.com/iTwin/viewer-components-react into JonasD/categories-tree-with-definition-containers

…tainers

saskliutas · 2025-02-05T08:01:12Z

...win/tree-widget/src/tree-widget-react/components/trees/categories-tree/UseCategoriesTree.tsx

+  const getCategoriesTreeIdsCache = useCallback(() => {
+    if (!idsCacheRef.current) {
+      idsCacheRef.current = new CategoriesTreeIdsCache(createECSqlQueryExecutor(iModel), viewType);
+    }
+    return idsCacheRef.current;
+  }, [viewType, iModel]);


I don't see where idsCacheRef.current is reset when view type or iModel changes so it will always return same cache.

Yeah, missed that, removed the condition, changed to useMemo

grigasp · 2025-02-05T10:40:42Z

...c/tree-widget-react/components/trees/categories-tree/internal/CategoriesVisibilityHandler.ts

-  private static getInstanceIdFromHierarchyNode(node: HierarchyNode) {
-    return HierarchyNode.isInstancesNode(node) && node.key.instanceKeys.length > 0 ? node.key.instanceKeys[0].id : /* istanbul ignore next */ "";
+  private static getInstanceIdsFromHierarchyNode(node: HierarchyNode) {
+    return HierarchyNode.isInstancesNode(node) && node.key.instanceKeys.length > 0


I don't think we need && node.key.instanceKeys.length > 0

Yes, removed

grigasp · 2025-02-05T10:48:06Z

...win/tree-widget/src/tree-widget-react/components/trees/categories-tree/UseCategoriesTree.tsx

+        !HierarchyNodeIdentifier.isInstanceNodeIdentifier(secondToLastNode) ||
+        secondToLastNode.className === DEFINITION_CONTAINER_CLASS
+      ) {
+        continue;


We shouldn't hide those bugs. I suggest changing this to an assert.

grigasp · 2025-02-05T11:03:28Z

packages/itwin/tree-widget/src/test/trees/categories-tree/internal/Utils.ts

-export class ViewportMock {
-  private _idsCache: CategoriesTreeIdsCache;
-  private _subCategories: Map<Id64String, boolean>;
+export async function createViewportStub(idsCache: CategoriesTreeIdsCache, isVisibleOnInitialize = false): Promise<Viewport & ViewportStubValidation> {


I suggest changing args to props - easier to maintain

grigasp · 2025-02-05T11:28:39Z

packages/itwin/tree-widget/src/test/trees/categories-tree/internal/VisibilityValidation.ts

+}: Omit<ValidateNodeProps, "visibilityExpectations"> & {
+  visibilityExpectations: VisibilityExpectations;
+  provider: HierarchyProvider;
+}) {


What's the purpose of this?

Omit<ValidateNodeProps, "visibilityExpectations"> & { visibilityExpectations: VisibilityExpectations }

No purpose, removed

grigasp · 2025-02-05T12:13:28Z

...ee-widget/src/tree-widget-react/components/trees/categories-tree/CategoriesTreeDefinition.ts

+  private _implWithoutDefinitionContainers: HierarchyDefinition;
+  private _implWithDefinitionContainers: HierarchyDefinition;


do we really need both of these? maybe one would be enough, with one of the childNodes specs being added conditionally?

We don't, removed

grigasp · 2025-02-05T12:24:00Z

...et/src/tree-widget-react/components/trees/categories-tree/internal/CategoriesTreeIdsCache.ts

+    const CATEGORIES_CTE = "AllVisibleCategories";
+    const isDefinitionContainerSupported = await this.getIsDefinitionContainerSupported();
+    const ctes = [
+      `${CATEGORIES_CTE}(ECInstanceId, ChildCount, ModelId ${isDefinitionContainerSupported ? ", ParentDefinitionContainerExists" : ""}) AS (


maybe the ParentDefinitionContainerExists property could always be there, but it's value would always be false if the class doesn't exist?

grigasp · 2025-02-05T12:30:36Z

packages/itwin/tree-widget/src/test/trees/categories-tree/internal/VisibilityValidation.ts

+      mergeMap(async (node) => validateNodeVisibility({ ...props, node })),
+    ),
+  );
+  expect(props.nodesToExpect.every((nodeId) => nodesFound.includes(nodeId))).to.be.true;


without this check, validateHierarchyVisibility could not check anything and it wouldn't fail

Yes, but that should be tested in our hierarchy tests, not visibility ones. This check unnecessarily complicates the tests, e.g.:

// this is only needed for checking what we got in the hierarchy const expectedIds = [ keys.definitionContainerRoot.id, keys.definitionContainerChild.id, keys.directCategory.id, keys.indirectCategory.id, keys.indirectSubCategory.id, ]; using visibilityTestData = await createVisibilityTestData({ imodel, isVisibleOnInitialize }); const { handler, provider, viewport } = visibilityTestData; await handler.changeVisibility(createDefinitionContainerHierarchyNode(keys.definitionContainerRoot.id), true); await validateHierarchyVisibility({ provider, handler, viewport, visibilityExpectations: {}, expectedIds, all: "visible", });

grigasp · 2025-02-05T12:32:13Z

packages/itwin/tree-widget/src/test/trees/categories-tree/internal/VisibilityValidation.ts

+  visibilityExpectations: VisibilityExpectations;
+  expectedIds: Id64Array;
+  all?: "visible" | "hidden";


what do you think about changing { visibilityExpectations: VisibilityExpectations; all?: "visible" | "hidden"; } to { expect: "all-visible" | "all-hidden" | VisibilityExpectations }?

I think it is better, changed to your suggestion, just instead of naming expect named to expectations since we are importing expect from chai

…tainers

Add definitionContainers to categories tree

7cf395c

JonasDov requested review from a team as code owners January 31, 2025 18:01

JonasDov added 2 commits January 31, 2025 21:15

Update tests

20f6c13

Fix tests

27ff768

grigasp reviewed Feb 3, 2025

View reviewed changes

JonasDov and others added 24 commits February 3, 2025 15:31

Merge branch 'master' into JonasD/categories-tree-with-definition-con…

ba1904a

…tainers

Fix typo: childCound -> childCount

2f5ffa3

Rename cte constants to Upper case for readability

0c35d45

change getAllContainedCategories to accept Id64Array

9ab2b2d

Update packages/itwin/tree-widget/src/tree-widget-react/components/tr…

7542a02

…ees/categories-tree/internal/CategoriesTreeIdsCache.ts Co-authored-by: Grigas <[email protected]>

Update packages/itwin/tree-widget/src/tree-widget-react/components/tr…

3a396eb

…ees/categories-tree/internal/CategoriesTreeIdsCache.ts Co-authored-by: Grigas <[email protected]>

Update packages/itwin/tree-widget/src/tree-widget-react/components/tr…

8b08331

…ees/categories-tree/internal/CategoriesTreeIdsCache.ts Co-authored-by: Grigas <[email protected]>

Update packages/itwin/tree-widget/src/tree-widget-react/components/tr…

502a66e

…ees/categories-tree/internal/CategoriesTreeNode.ts Co-authored-by: Grigas <[email protected]>

Update packages/itwin/tree-widget/src/tree-widget-react/components/tr…

58504d7

…ees/categories-tree/internal/CategoriesTreeNode.ts Co-authored-by: Grigas <[email protected]>

Update packages/itwin/tree-widget/src/test/trees/categories-tree/inte…

4195e85

…rnal/CategoriesTreeIdsCache.test.ts Co-authored-by: Grigas <[email protected]>

Merge branch 'JonasD/categories-tree-with-definition-containers' of h…

6a40ff4

…ttps://github.com/iTwin/viewer-components-react into JonasD/categories-tree-with-definition-containers

Update packages/itwin/tree-widget/src/tree-widget-react/components/tr…

c40294e

…ees/categories-tree/CategoriesTreeDefinition.ts Co-authored-by: Grigas <[email protected]>

Use Promise.all in getRootDefinitionContainersAndCategories

2a34afe

Merge branch 'JonasD/categories-tree-with-definition-containers' of h…

fb73449

…ttps://github.com/iTwin/viewer-components-react into JonasD/categories-tree-with-definition-containers

Make as upper case

7e04bc6

Move await in propper position

9033116

change collect -> mergeMap + toArray

3c30e77

Remove validateViewsCalls

c5a6120

Adjust catgory visibility

b5a53ca

Create a separate file for class name definitions

1679c5c

Remove activeView dependency

0865c2c

Use HierarchyFilteringPath.normalize(path)

589760d

Adjust getCategoriesFromPaths

18d904f

Rename definitionContainer to definition container in test names

82b585a

JonasDov added 13 commits February 4, 2025 10:34

Rename tests

704539a

Adjust expect calls

47418fa

Dispose provider

05988d0

Adjust viewport mock creation

c226c0e

Adjust visibilityExpectations API

afc99b1

Merge branch 'master' into JonasD/categories-tree-with-definition-con…

1913e12

…tainers

Update queryCategories implementation

c21bd26

Fix e2e tests

05cdb52

Add changeset

e3001ce

Use function for DefinitionContainers in parentInstancesNodePredicate

971d6c7

Adjust categoriesTreeDefinition

ef53d44

Update screenshots

80788d9

Merge branch 'master' into JonasD/categories-tree-with-definition-con…

ffdc0de

…tainers

saskliutas reviewed Feb 5, 2025

View reviewed changes

JonasDov added 2 commits February 5, 2025 11:05

Create new CategoriesTreeIdsCache when imodel or viewport change

2070956

Use useMemo for creating idsCache

96a150f

grigasp reviewed Feb 5, 2025

View reviewed changes

JonasDov added 2 commits February 7, 2025 14:46

Merge branch 'master' into JonasD/categories-tree-with-definition-con…

199b4dd

…tainers

Adress comments

e9ee67f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add definitionContainers to categories tree #1172

Add definitionContainers to categories tree #1172

JonasDov commented Jan 31, 2025

grigasp left a comment

grigasp Feb 3, 2025

JonasDov Feb 4, 2025

grigasp Feb 5, 2025

saskliutas Feb 5, 2025

JonasDov Feb 5, 2025 •

edited

Loading

grigasp Feb 5, 2025

JonasDov Feb 7, 2025

grigasp Feb 5, 2025

grigasp Feb 5, 2025

JonasDov Feb 7, 2025

grigasp Feb 5, 2025

JonasDov Feb 7, 2025

grigasp Feb 5, 2025

JonasDov Feb 7, 2025

grigasp Feb 5, 2025

JonasDov Feb 7, 2025

grigasp Feb 5, 2025

grigasp Feb 5, 2025

JonasDov Feb 7, 2025

		private _implWithoutDefinitionContainers: HierarchyDefinition;
		private _implWithDefinitionContainers: HierarchyDefinition;

Add definitionContainers to categories tree #1172

Are you sure you want to change the base?

Add definitionContainers to categories tree #1172

Conversation

JonasDov commented Jan 31, 2025

grigasp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonasDov Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonasDov Feb 5, 2025 •

edited

Loading