Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RichTreeViewPro] Add virtualization #13520

Draft
wants to merge 60 commits into
base: master
Choose a base branch
from

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Jun 17, 2024

Closes #9685

Extracted PRs

Benchmarks

TODO

Very incomplete list

  • Make sure it reacts correctly to collapsing / expanding
  • Make sure it react correctly when items change
  • Fix the width of the content to never have a horizontal scrollbar
  • Implement the smart buffer of the grid (pre-load more items in the direction being scrolled)
  • Do not unmount any "active" item (the item being editing, the focus item, the item being dragged, ...)

@flaviendelangle flaviendelangle self-assigned this Jun 17, 2024
@flaviendelangle flaviendelangle added the component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! label Jun 17, 2024
@mui-bot
Copy link

mui-bot commented Jun 17, 2024

@JCQuintas
Copy link
Member

I spent some time looking at the PR and I see what you meant now. 😆

I don't know if it is possible to do full virtualization on a nested DOM arch. And if possible it would still be much more complex than just having flat DOM... 🙃

With that said.

If we can assume all items have the same height, and we know the list of items beforehand, however nested it is, we can try to flatten the list keeping references, eg:

const items = [
  {
    id: '1',
    children: [{ id: 'a' }, { id: 'b', children: [{ id: 'I' }] }],
  },
  {
    id: '2',
    children: [{ id: 'a', children: [{ id: 'I' }, { id: 'II' }] }, { id: 'b' }],
  },
];

// Maybe use the array index if we don't have ids? (e.g. `'0.0.0, '0.1.0'`)?
const flatIds = ['1', '1.a', '1.b', '1.b.I', '2', '2.a', '2.a.I', '2.a.II', '2.b'];

We could then have a custom render that knows this context, so if say, we are inside 2.a.II, it keeps 2 and 2.a rendered, albeit slightly offscreen, and scrolls the elements inside 2.a

| 2.         |
|| a.       ||
||| ...n-m ||| <- "fake" scroll happens in `a.` content
---visible----
||| ...n   |||
---visible----
||| ...n+m |||

Though I'm not sure it would work, and orchestrating all these containers might be a real pain, because we will have depth+1 containers on the screen at all times 😅

@flaviendelangle
Copy link
Member Author

@JCQuintas I think we have the same idea 👌

If we can assume all items have the same height, and we know the list of items beforehand, however nested it is, we can try to flatten the list keeping references, eg:

At some point I have to work on a flat list yes, even if the final DOM structure end up being nested.
Right now, I have the flat list of elements to render, with some work I could turn it back into a tree and make sure to keep any ancestor that have some children rendered.

But then the main challenge is to orchestrate all those offsets correctly 😬 , hopefully it's doable without to much headache...


Keeping the depth+1 contains on the screen at all time is not a big issue IMHO (except on very specific edge cases), we could even not render the content of those ancestors but just the groupTransition if it improves the performances (and adapt the offsets of course).

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 1, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 1, 2024
Copy link

github-actions bot commented Aug 1, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 1, 2024
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 13, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 14, 2024
@flaviendelangle
Copy link
Member Author

Performance-wise, it looks like trying to virtualize with a Collapse is not the best idea, a lot of the computing time is spent in this component (and right now the transition doesn't even work).

I'll explore a bit to see if we can correctly support Collapse + virtualization or if we should remove the transition when virtualization is enabled.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 9, 2024
Copy link

github-actions bot commented Sep 9, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[tree view] Add virtualization support
5 participants