Add child elements category to models tree #1206
Conversation
...es/itwin/tree-widget/src/test/trees/models-tree/internal/ModelsTreeVisibilityHandler.test.ts
Outdated
Show resolved
Hide resolved
…/ModelsTreeVisibilityHandler.test.ts Co-authored-by: Grigas <[email protected]>
Added one test, and modified existing test a little bit. |
There was a problem hiding this comment.
There seem to be some issues with display states.
I understand you're going to update display states handling in a separate PR, and that's fine. But the screenshots show that display states of nodes above the new category nodes have changed. I don't understand why.
There was a problem hiding this comment.
I think we should move the mouse pointer away from nodes to avoid the hover effect.
There was a problem hiding this comment.
Do you know why "Tag-Category" is in partial display state?
There was a problem hiding this comment.
Do you know why display state of "Tag-Category" is "off"?
There was a problem hiding this comment.
Do you know why display state of "E-101 [4-1F4]" is "partial"? Also, shouldn't its class grouping node also be in partial display state?
Display states of above elements are different, because I've adjusted idsCache (previously it returned only categories of elements that have no parents, now it returns categories of elements that have and don't have parents). I've decided to add display states handling in this PR, since after adding categories, display states became more confusing. |
There was a problem hiding this comment.
I think this needs to be reviewed from the perspective of the executed queries.
E.g. when I open http://localhost:3000/?iTwinId=892aa2c9-5be8-4865-9f37-7d4c7e75ebbf&iModelId=77d710e9-83aa-4864-9248-23dfe4bc0047, I see 73 queryRows requests. Quite a few of them take around a second. One of them takes almost 2 second and returns ids of 82k elements.
| const onVisibilityChange = new BeEvent<() => void>(); | ||
| let pendingVisibilityChange: undefined | ReturnType<typeof setTimeout>; | ||
| let suppressChangeEvents: number = 0; | ||
| const handleVisibilityChange = () => { | ||
| if (pendingVisibilityChange || suppressChangeEvents > 0) { | ||
| return; | ||
| } | ||
| pendingVisibilityChange = setTimeout(() => { | ||
| pendingVisibilityChange = setTimeout(async () => { | ||
| await childrenVisibilityCache?.clearChangedValues(); |
There was a problem hiding this comment.
I don't think it should be listener's responsibility to clear cache. Can this be moved to whoever creates the listener and owns the cache?
| ctes: [ | ||
| ` | ||
| ElementInfo(elementId, modelId, categoryId, parentId) AS ( | ||
| ecsql: ` |
There was a problem hiding this comment.
why don't we need to select parents of these elements anymore?
| (() => { | ||
| using _ = new AlwaysAndNeverDrawnElementInfo(vp); | ||
| expect(event.numberOfListeners).to.eq(1); | ||
| }); | ||
| await sinon.clock.runAllAsync(); | ||
| })(); |
There was a problem hiding this comment.
| (() => { | |
| using _ = new AlwaysAndNeverDrawnElementInfo(vp); | |
| expect(event.numberOfListeners).to.eq(1); | |
| }); | |
| await sinon.clock.runAllAsync(); | |
| })(); | |
| { | |
| using _ = new AlwaysAndNeverDrawnElementInfo(vp); | |
| expect(event.numberOfListeners).to.eq(1); | |
| } |
| CategoryElements(id, modelId, categoryId) AS ( | ||
| SELECT ECInstanceId, Model.Id, Category.Id | ||
| ` | ||
| ElementsChildrenInfo(Id, CategoryId, ParentId, RootElementCategoryId, ParentHasParent) AS ( |
There was a problem hiding this comment.
This CTE selects all elements of given model - that could be millions of elements. Did you check how this query performs in large datasets?
| SELECT 1 | ||
| FROM ( | ||
| SELECT ParentId FROM ElementsChildrenInfo | ||
| ) | ||
| WHERE ParentId = this.Id | ||
| LIMIT 1 |
There was a problem hiding this comment.
why not just SELECT 1 FROM ${this._hierarchyConfig.elementClassSpecification} WHERE Parent.Id = this.Id?
| * There can be two different cases: | ||
| * 1. If category is directly under model, then it returns only those elements that have children. | ||
| * 2. If category is under element, then it returns all child elements. |
There was a problem hiding this comment.
why not have two different functions for this?
| @@ -90,7 +89,7 @@ interface ChangeGeometricElementsDisplayStateProps { | |||
| // (undocumented) | |||
| categoryId: Id64String; | |||
| // (undocumented) | |||
| elementIds: Id64Set; | |||
| elementIds: Map<Id64String, boolean>; | |||
| @@ -179,6 +178,8 @@ interface GetCategoryVisibilityStatusProps { | |||
| categoryId: Id64String; | |||
| // (undocumented) | |||
| modelId: Id64String; | |||
| // (undocumented) | |||
| parentElementIds: Id64Array; | |||
closes #1100
This change, allows models tree to show category nodes of child elements, when their category differs from parent node's category.
PS. Don't review yet, visibility changes are still needed