Skip to content

[Tree-widget]: Add option to get a smaller part of models tree - #1375

Merged
JonasDov merged 18 commits into
masterfrom
JonasD/subset-tree
Jul 30, 2025
Merged

[Tree-widget]: Add option to get a smaller part of models tree#1375
JonasDov merged 18 commits into
masterfrom
JonasD/subset-tree

Conversation

@JonasDov

@JonasDov JonasDov commented Jul 14, 2025

Copy link
Copy Markdown
Contributor

closes #1319
Added a way for consumers to get only a part of models tree.
This can be achieved by providing getSubTreePaths. It works similarly to getFilteredPaths except it does not interfere with default models tree filtering. Previously, if consumers wanted to achieve this, they had to define a custom getFilteredPaths function and there was a problem with this:
By defining the getFilteredPaths function, consumers override the default filtering behaviour. So if they want filtering to work as before they have to: use createInstanceKeyPaths with filter, then use it with targetItems and combine the results.

getSubTreePaths solves this issue.

@JonasDov
JonasDov requested review from a team as code owners July 14, 2025 08:35

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tree-Widget benchmark

Benchmark suite Current: b863dd4 Previous: 6e71184 Deviation Status
categories tree creates initial filtered view for 50k items 2528.63 ms 2458.25 ms 2.86% 〰️
categories tree creates initial filtered view for 50k items (P95 of main thread blocks) 1672 ms 1697 ms -1.47% 〰️
categories tree changing category visibility changes visibility for 50k subCategories 9012.68 ms 10188.05 ms -11.54%
categories tree changing category visibility changes visibility for 50k subCategories (P95 of main thread blocks) 2591 ms 3437 ms -24.61%
categories tree changing definition container visibility changes visibility for 50k categories 7217.31 ms 6316.99 ms 14.25% 🚨
categories tree changing definition container visibility changes visibility for 50k categories (P95 of main thread blocks) 4465 ms 3072 ms 45.35% 🚨
models tree creates initial filtered view for 50k target items 2256.02 ms 2350.41 ms -4.02% 〰️
models tree creates initial filtered view for 50k target items (P95 of main thread blocks) 203 ms 314 ms -35.35%
models tree validates categories visibility for imodel with 50k categories 15608.61 ms 15935.16 ms -2.05% 〰️
models tree validates categories visibility for imodel with 50k categories (P95 of main thread blocks) 4291 ms 4544 ms -5.57% 〰️
models tree changing model visibility changes visibility for 50k elements 49628.68 ms 53510.4 ms -7.25% 〰️
models tree changing model visibility changes visibility for 50k elements (P95 of main thread blocks) 269 ms 307 ms -12.38% 〰️
models tree changing category visibility changes visibility for 50k elements 49127.2 ms 52809.22 ms -6.97% 〰️
models tree changing category visibility changes visibility for 50k elements (P95 of main thread blocks) 267 ms 283 ms -5.65% 〰️
models tree changing per-model-category override changes visibility for 50k elements 48799.6 ms 54634.7 ms -10.68%
models tree changing per-model-category override changes visibility for 50k elements (P95 of main thread blocks) 248 ms 302 ms -17.88%
models tree changing element visibility changes only parent nodes visibility with 50k elements 51144.03 ms 55387.59 ms -7.66% 〰️
models tree changing element visibility changes only parent nodes visibility with 50k elements (P95 of main thread blocks) 486 ms 543 ms -10.50%

This comment was automatically generated by workflow using github-action-benchmark.

@JonasDov JonasDov changed the title Add subsetTree [Tree-widget]: Add option to show a portion of models tree Jul 21, 2025
@JonasDov JonasDov changed the title [Tree-widget]: Add option to show a portion of models tree [Tree-widget]: Add option to get a portion of models tree Jul 21, 2025
@JonasDov JonasDov changed the title [Tree-widget]: Add option to get a portion of models tree [Tree-widget]: Add option to get a smaller part of models tree Jul 21, 2025
@grigasp

grigasp commented Jul 22, 2025

Copy link
Copy Markdown
Member

Can you explain the reasoning behind exposing this feature as subsetTreeConfig?: { targetItems: Array<InstanceKey> } prop?

  • What other options do your foresee being added to subsetTreeConfig object?
  • Is there a reason to use a different API than what we have to getFilteredPaths?

@JonasDov

Copy link
Copy Markdown
Contributor Author

Can you explain the reasoning behind exposing this feature as subsetTreeConfig?: { targetItems: Array<InstanceKey> } prop?

  • What other options do your foresee being added to subsetTreeConfig object?
  • Is there a reason to use a different API than what we have to getFilteredPaths?
  • Currently I see two more options that consumers might want to be added later on:
    1. autoExpand option: consumers might want the hierarchy to be expanded up to target items.
    2. hideChildren: at the moment children of targetItems are in the hierarchy, having an option to not show children could be needed in the future.
  • Three main reasons:
    1. This API is simpler to use than getFilteredPaths: consumers only need to provide target items and not filter paths.
    2. At the moment internal implementation works almost like getFilteredPaths, but in the future we might want to modify the implementation. This API allows adding new features without breaking the API.
    3. We might want to add some options that can be used in subsetTree and not filter paths e.g. hideChildren (not saying that hideChildren should not be an option in filter paths, just saying that there might be in the future).

@grigasp

grigasp commented Jul 23, 2025

Copy link
Copy Markdown
Member

Currently I see two more options that consumers might want to be added later on:

  1. autoExpand option: consumers might want the hierarchy to be expanded up to target items.
  2. hideChildren: at the moment children of targetItems are in the hierarchy, having an option to not show children could be needed in the future.

Wouldn't we want to implement that the same way getFilteredPaths is implemented? autoExpand is handled per-path. Hiding of children would probably be handled the same way.

  1. This API is simpler to use than getFilteredPaths: consumers only need to provide target items and not filter paths.

I agree it's simpler. On the other hand, I see it this way - we already have the complex getFilteredPaths API, so adding a new different API increases the complexity at least a little bit. However, if the new API is completely the same as the already existing one, then we don't increase the total complexity at all. And we keep APIs of similar features consistent.

  1. At the moment internal implementation works almost like getFilteredPaths, but in the future we might want to modify the implementation. This API allows adding new features without breaking the API.
  2. We might want to add some options that can be used in subsetTree and not filter paths e.g. hideChildren (not saying that hideChildren should not be an option in filter paths, just saying that there might be in the future).

I think these two are a bit of a stretch - we can't foresee all the possible changes we may need to make to the API in the future, and I don't see why subsetTreeConfig is better than getFilteredPaths for that matter.

IMO, consistency is more important in this case.

@JonasDov

Copy link
Copy Markdown
Contributor Author

Currently I see two more options that consumers might want to be added later on:

  1. autoExpand option: consumers might want the hierarchy to be expanded up to target items.
  2. hideChildren: at the moment children of targetItems are in the hierarchy, having an option to not show children could be needed in the future.

Wouldn't we want to implement that the same way getFilteredPaths is implemented? autoExpand is handled per-path. Hiding of children would probably be handled the same way.

  1. This API is simpler to use than getFilteredPaths: consumers only need to provide target items and not filter paths.

I agree it's simpler. On the other hand, I see it this way - we already have the complex getFilteredPaths API, so adding a new different API increases the complexity at least a little bit. However, if the new API is completely the same as the already existing one, then we don't increase the total complexity at all. And we keep APIs of similar features consistent.

  1. At the moment internal implementation works almost like getFilteredPaths, but in the future we might want to modify the implementation. This API allows adding new features without breaking the API.
  2. We might want to add some options that can be used in subsetTree and not filter paths e.g. hideChildren (not saying that hideChildren should not be an option in filter paths, just saying that there might be in the future).

I think these two are a bit of a stretch - we can't foresee all the possible changes we may need to make to the API in the future, and I don't see why subsetTreeConfig is better than getFilteredPaths for that matter.

IMO, consistency is more important in this case.

Changed subsetTreeConfig to getSubsetTreePaths

@grigasp

grigasp commented Jul 23, 2025

Copy link
Copy Markdown
Member

Hate to ask for another rename, but what do you think about calling it SubTree instead of SubsetTree?

@JonasDov

Copy link
Copy Markdown
Contributor Author

Hate to ask for another rename, but what do you think about calling it SubTree instead of SubsetTree?

Renamed to getSubTreePaths

Comment thread change/@itwin-tree-widget-react-8f8e08b3-854d-476d-a87b-7e0e2e51f245.json Outdated
Comment thread packages/itwin/tree-widget/src/tree-widget-react/components/trees/common/Utils.ts Outdated
Comment thread packages/itwin/tree-widget/src/tree-widget-react/components/trees/common/Utils.ts Outdated
Comment thread packages/itwin/tree-widget/src/tree-widget-react/components/trees/common/Utils.ts Outdated
Comment thread packages/itwin/tree-widget/src/test/trees/common/Utils.test.ts Outdated
Comment thread packages/itwin/tree-widget/public/locales/en/TreeWidget.json Outdated
Comment thread change/@itwin-tree-widget-react-8f8e08b3-854d-476d-a87b-7e0e2e51f245.json Outdated

@grigasp grigasp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add an integration test that combines sub-tree functionality with filtering? Ideally, we'd have tests for all 3 types of filtering that we support (filter text, instance focus, getFilteredPaths).

@JonasDov

Copy link
Copy Markdown
Contributor Author

Could you add an integration test that combines sub-tree functionality with filtering? Ideally, we'd have tests for all 3 types of filtering that we support (filter text, instance focus, getFilteredPaths).

Added 5 UseModelsTree tests: one where getSubTreePaths is not defined, one where only it is defined, and one for each filtering case (filter text, instance focus and getFilteredPaths) when combined with getSubTreePaths

Comment thread packages/itwin/tree-widget/src/test/trees/models-tree/UseModelsTree.test.tsx Outdated
Comment thread packages/itwin/tree-widget/src/test/trees/models-tree/UseModelsTree.test.tsx Outdated
@JonasDov
JonasDov enabled auto-merge (squash) July 28, 2025 16:12
@JonasDov
JonasDov merged commit 4bec3e0 into master Jul 30, 2025
14 checks passed
@JonasDov
JonasDov deleted the JonasD/subset-tree branch July 30, 2025 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tree widget: filtering is broken when getFilteredPaths is used

3 participants