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

Unsynchronized "expand" in TreeGrid #1627

Closed
ghost opened this issue Oct 25, 2019 · 14 comments
Closed

Unsynchronized "expand" in TreeGrid #1627

ghost opened this issue Oct 25, 2019 · 14 comments

Comments

@ghost
Copy link

ghost commented Oct 25, 2019

The expansion of TreeGrids seems to be an asynchr. event sent to the client. When UI elements in the TreeGrid are updated (e.g. in a ComponentColumn that has icons) while the TreeGrid is still expanding on client-side, there are exceptions.

How to reproduce:

  • have a "large" TreeGrid (~50 nodes, 4-5 levels deep)
  • show the TreeGrid in a client-view, expand entire TreeGrid on server side
  • update the entries in the TreeGrid, so that there are changes in the UI (while the client still expands the TreeGrid)

Results:

  • it takes several seconds until the TreeGrid expands
  • Exceptions on client side, sometimes an empty TreeGrid is shown, sometimes the ComponentValues are not rendered any more, sometimes the server-side receives an exception that "node IDs are unknown on client side".

Expected Result:

  • the TreeGrid expands within a really short time (well below a second)
  • server-side and client-side are synchronized in a way, that the client side already knows about IDs when it receives instructions from the server-side to update them.

Version used: Vaadin 14.0.5 (via vaadin-spring-boot-starter)

@TatuLund
Copy link
Contributor

TatuLund commented Mar 9, 2020

See also vaadin/vaadin-grid-flow#476

@tulioag
Copy link
Contributor

tulioag commented Mar 31, 2020

This example reproduces the performance issues.
treegriddemo.zip

@pekam pekam self-assigned this May 25, 2020
@pekam
Copy link
Contributor

pekam commented May 25, 2020

I'm not able to reproduce the issue.

I'm using vaadin-grid-flow 4.0.6, which belongs to Vaadin 14.0.5.

With the following code, I click the first button to expand all items. While the grid is expanding, I click the second button which refreshes both the first item (already rendered), and the last item (still waiting to be loaded). Everything works as expected; there are no exceptions, the first item is updated immediately and the last one renders the updated value when loaded. Using refreshAll instead of refreshItem works as well.

@Route("treegrid-deep-tree")
public class TreeGridDeepTreePage extends Div {

    private static final int DEPTH = 50;

    private TreeGrid<Person> treeGrid;
    private List<Person> items;
    private TreeDataProvider<Person> dataProvider;

    public TreeGridDeepTreePage() {
        initializeDataProvider();
        treeGrid = new TreeGrid<>();
        treeGrid.setDataProvider(dataProvider);
        treeGrid.setWidth("100%");
        treeGrid.addHierarchyColumn(Person::getFirstName);
        treeGrid.addComponentColumn(
                person -> new NativeButton(person.getFirstName()));

        NativeButton expandAll = new NativeButton("expand all",
                e -> treeGrid.expandRecursively(items.subList(0, 1), DEPTH));

        NativeButton refresh = new NativeButton("refresh", e -> {
            Person first = items.get(0);
            first.setFirstName("updated");
            dataProvider.refreshItem(first);

            Person last = items.get(items.size() - 1);
            last.setFirstName("updated");
            dataProvider.refreshItem(last);
        });

        add(treeGrid, expandAll, refresh);
    }

    private void initializeDataProvider() {
        items = new PeopleGenerator().generatePeople(DEPTH);
        TreeData<Person> data = new TreeData<>();
        data.addRootItems(items.get(0));
        IntStream.range(1, items.size()).forEach(i -> {
            Person parent = items.get(i - 1);
            Person item = items.get(i);
            data.addItem(parent, item);
        });
        dataProvider = new TreeDataProvider<>(data);
    }
}

(Person and PeopleGenerator are classes in vaadin-grid-flow-integration-tests module)

More info is needed to work on this issue, preferably a copy-pasteable code example which reproduces the bug.

@pekam
Copy link
Contributor

pekam commented May 25, 2020

it takes several seconds until the TreeGrid expands

For the slowness, please refer to another issue: vaadin/vaadin-grid-flow#476 (reopened)

Let's keep this issue just for these described bugs:

Exceptions on client side, sometimes an empty TreeGrid is shown, sometimes the ComponentValues are not rendered any more, sometimes the server-side receives an exception that "node IDs are unknown on client side".

(although waiting for more information to reproduce the issue)

@tomivirkki
Copy link
Member

Closing the issue as it's lacking instructions for reproduction. Will reopen if an example is provided.

@edler-san
Copy link

example802.zip
Customer has provided an example. Run via mvn:spring-boot:run, click on "toggle first item recursively" and then scroll down

@edler-san edler-san reopened this Jul 15, 2020
@tomivirkki tomivirkki assigned tomivirkki and unassigned pekam Aug 12, 2020
@TatuLund
Copy link
Contributor

My 5cents. I would add an API to define whether TreeGrid is EAGER or LAZY mode. Currently it is LAZY, and we should retain it as default. There are drawbacks from the lazyness, e.g. the recursive expansion, if you want the TreeGrid preopened. On the other hand current implementation is good when TreeGrid is not preopened and fully operated by user, not programmatically. EAGER mode would be better in applications where we want to preopen it fully or partially, provided that it is not super huge. On the other hand EAGER can make first render a bit slower. What is better is highly application specific, hence I would add the API to control this, so that developer who knows the application can decide what is good for him.

@tomivirkki tomivirkki removed their assignment Sep 3, 2020
@tomivirkki
Copy link
Member

Just to mention, the performance of the example app provided by @edler-san above was significantly improved by vaadin/vaadin-grid-flow#1084 even without this change

@vaadin-bot vaadin-bot transferred this issue from vaadin/vaadin-grid-flow Oct 6, 2020
@pleku
Copy link
Contributor

pleku commented Oct 30, 2020

The point that @TatuLund made above is IMO valid and I think it could be fixed in HierarchicalDataCommunicator based on the range of items that the client requests initially. It might mean thought that the semantics of the requested range would change a bit. So basically the server will be able to count (or even make a good guess) on how many items it needs to get eagerly - not fetching all expanded items & levels eagerly, but just enough to fill the viewport.

I would fix this at the same time when we make the item count query optional for hierarchical data (TreeGrid). I think this should be the default behavior, I can't see what added value the ping-pong between the client and server can bring for expanding the nodes. Even though it would be a behavior change.

@tomivirkki
Copy link
Member

tomivirkki commented Jan 4, 2022

Did a quick PoC of the TreeGrid eager fetch mode

Lazy loading mode (slow network throttling on):

treegrid-lazy.mp4

Eager loading mode (slow network throttling on):

treegrid-eager.mp4

@sirbris
Copy link

sirbris commented Jan 11, 2022

@tomivirkki that's already a huge improvement... any chance, this will be implemented in Vaadin 14?

@tomivirkki
Copy link
Member

@sirbris Yes, I don't see any blockers to backporting it to Vaadin 14 also.

@TatuLund
Copy link
Contributor

Why this ticket is still open? I am mainly pointing out that #2648 has been closed.

@tomivirkki
Copy link
Member

Good point. No reason to keep this open anymore, closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants