eclipse-pde / eclipse.pde

Eclipse Public License 2.0
28 stars 81 forks source link

Slow resize of target location wizard #1497

Open ptziegler opened 2 days ago

ptziegler commented 2 days ago

I think a video speaks more than a thousand words:

https://github.com/user-attachments/assets/0aca13f2-771d-45de-8233-b90833b6c8a5

Looking at the profiler, I notice that almost all of the time is spent performing the layout on a tree widget, which is weird as this is the Maven wizard, which doesn't have a tree.

image

Using the debugger, I then checked the invocation chain and the layout is indeed done by the FilteredCheckboxTree, which is a PDE internal class and therefore unrelated to the Maven extension:

image

Am I correct to assume that this FilteredCheckboxTree is used for the "Target Content" tab? If so, then I don't believe it's intentional for a resize of the wizard to cause a relayout of the underlying editor?

ptziegler commented 2 days ago

Nevermind... M2E is extending the PDE internal BasePluginListPage and gets the FilteredCheckboxTree from there. So there is nothing to do on the PDE side.

ptziegler commented 2 days ago

it's this wizard page, to be precise: image

Even if it's an internal class, perhaps it would make sense to convert this to a lazy viewer? Because refreshing a tree with 1400 elements during every resize will probably also affect the editor.

vogella commented 2 days ago

perhaps it would make sense to convert this to a lazy viewer

Yes of course. I assume you are interested in providing a PR?

By the way, I would like to express my gratitude of your effort to improve the Eclipse platform and PDE, basically every issue and PR I see from you is helpful.

ptziegler commented 2 days ago

Yes of course. I assume you are interested in providing a PR?

Of course. It's already painful with my normal workspace. If I check out everything, I end up with 8000 plugins, which is even more dreadful to work with and just for fun, I've set up a work environment with the 2024-12 staging repository and the Babel update site and the IDE becomes completely unusable...

By the way, I would like to express my gratitude of your effort to improve the Eclipse platform and PDE, basically every issue and PR I see from you is helpful.

Thanks 😄

I found the places that need to be adapted for the virtual tree. However, it turns out that the VIRTUAL flag for the CheckboxTreeViewer is effectively meaningless because of this little gem here, which forces the creation of all tree items. So unless this is addressed, I don't think it's possible to avoid this performance problem...

private void applyState(CustomHashtable checked, CustomHashtable grayed,
        Widget widget) {
    Item[] items = getChildren(widget);
    for (Item item : items) {
        if (item instanceof TreeItem) {
            Object data = item.getData();
            if (data != null) {
                TreeItem ti = (TreeItem) item;
                ti.setChecked(checked.containsKey(data));
                ti.setGrayed(grayed.containsKey(data));
            }
        }
        applyState(checked, grayed, item);
    }
}
ptziegler commented 1 day ago

I've played around with the current implementation and as far as I can tell, the underlying problem is the usage of a ContainerCheckedTreeViewer instead of CheckboxTableViewer, to represent a one-dimensional list of elements. This is because the ContainerCheckedTreeViewer needs to create the entire tree to determine the checked/grayed state and therefore can't be used with virtual trees.

As seen in 4c1edad836207649d0dd9fbeee29de301c7d2a86, there used to be a TableViewer, but it was changed in order to add a filter to the viewer and to re-use the FilteredTree class provided by the platform.

My suggestion: Create a FilteredTable class and switch back to a table viewer. Edit: Or simply re-use the FilteredList...

vogella commented 1 day ago

We don't have a FilteredTable? Yes, we should add it. Would be great to have the option in this new class to enable/ disable the filter at runtime similar to the E4 FilteredTree

ptziegler commented 1 day ago

There's the FilteredList which works similar, I think? But I have to double-check.

vogella commented 1 day ago

IMHO every view which shows a table or a tree and does not allow to filter is bad UX.