Proper way to restore the expand/collapse state of virtual viewers

[ptziegler]

This is an issue I've been scratching my head at for the past few days... I have a virtual tree viewer where I fetch the currently expanded elements via getExpandedElements(), update the input and then restore it via setExpandedElements().

The problem: This doesn't work, as updating the input collapses the tree and therefore all tree items are virtual and without data object. Restoring the expanded state ist herefore ignored (line 2175).

eclipse.platform.ui/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/AbstractTreeViewer.java

Lines 2170 to 2190 in 9474c95

	private void internalSetExpanded(CustomHashtable expandedElements,
	Widget widget) {
	Item[] items = getChildren(widget);
	for (Item item : items) {
	Object data = item.getData();
	if (data != null) {
	// remove the element to avoid an infinite loop
	// if the same element appears on a child item
	boolean expanded = expandedElements.remove(data) != null;
	if (expanded != getExpanded(item)) {
	if (expanded) {
	createChildren(item);
	}
	setExpanded(item, expanded);
	}
	}
	if (expandedElements.size() > 0) {
	internalSetExpanded(expandedElements, item);
	}
	}
	}

For my next attempt, I tried to use getTreePathFromItem(), but this has a similar issue except that it simply throws an exception because segment == null (line 3189).

eclipse.platform.ui/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/AbstractTreeViewer.java

Lines 3185 to 3194 in 9474c95

	protected TreePath getTreePathFromItem(Item item) {
	LinkedList<Object> segments = new LinkedList<>();
	while (item != null) {
	Object segment = item.getData();
	Assert.isNotNull(segment);
	segments.addFirst(segment);
	item = getParentItem(item);
	}
	return new TreePath(segments.toArray());
	}

After overwriting this method and simply returning null instead of throwing an exception, the next error was thrown because the method getExpandedTreePaths() skips such null elements but doesn't consider this when calculating the size of the returned array (line 3181).

eclipse.platform.ui/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/AbstractTreeViewer.java

Lines 3271 to 3282 in 9474c95

	public TreePath[] getExpandedTreePaths() {
	ArrayList<Item> items = new ArrayList<>();
	internalCollectExpandedItems(items, getControl());
	ArrayList<TreePath> result = new ArrayList<>(items.size());
	for (Item item : items) {
	TreePath treePath = getTreePathFromItem(item);
	if (treePath != null) {
	result.add(treePath);
	}
	}
	return result.toArray(new TreePath[items.size()]);
	}

My question: Some of those checks seem intentional, so is this functionality something that's generally unsupported for virtual trees and something where I have to switch back to a "normal" tree?

[laeubi]

Virtual Trees are a bit tricky what I do in one of my applications with a virtual tree is to start with a TreePath containing the root object (that is collapsed), then I expand that one then use path.createChildPath(nextSegment) expand that and so on until I have expanded all items.

As expanding one level of the tree then loads the children, this iteratively expands everything up to the desired state.

[ptziegler]

what I do in one of my applications with a virtual tree is to start with a TreePath containing the root object (that is collapsed), then I expand that one then use path.createChildPath(nextSegment) expand that and so on until I have expanded all items.

That sounds similar to my latest approach, except that I need to rely on getTreePathFromItem() because I want to restore the previous selection and therefore don't know which segments to expand.

As a side node: I've made some smaller changes to the problematic methods and then setExpandedTreePaths() works as I would expect. Would this be worth being push back upstream?

[laeubi]

I know... but as I said: It's not that simple.

As said lazy models are powerful but require some good design, and I suspect something is strange with your data-model as you obviously know that something has changed so why not carry the what changed together in your notification?

Anyways as mentioned this will result in devastating performance compared to a true data-model backed approach where a datamodel is the place where modifications happens and then properly updates the UI. So if we talk about "proper ways" I would not find it proper to refresh the whole viewer of a lazy model as it a bit defeats its purpose.

[ptziegler]

As said lazy models are powerful but require some good design, and I suspect something is strange with your data-model as you obviously know that something has changed so why not carry the what changed together in your notification?

That viewer and data model was designed back in the Eclipse 2 era and for such a small scope where it didn't really matter if one refreshes the entire tree. So a proper design has sadly not been the highest priority. If it were designed today, obviously one could use more modern design patterns.

But the harsh truth is that the world runs on legacy code and at some point I have to make compromises regarding which parts can be cleaned up and which can't.

[laeubi]

But the harsh truth is that the world runs on legacy code and at some point I have to make compromises regarding which parts can be cleaned up and which can't.

The good part is that it somehow forces you to improve the LazyTree support :-D

it didn't really matter if one refreshes the entire tree

Maybe then a LazyTree is not even required?!

[ptziegler]

The good part is that it somehow forces you to improve the LazyTree support :-D

I at least hope so... While I was able to reproduce the problem reliably, I still have a hard time understanding why it happens and this whole virtual/non-virtual distinction in the viewer doesn't help in making this any less confusing. :/

Maybe then a LazyTree is not even required?!

Probably... I think performance-wise, I think they break pretty much even. Though I still have the slim chance that at some point, I get more time to have this viewer actually utilize some of the lazy-loading mechanisms.

Regardless, I still have a more recent viewer to work on where I actually do know what changes. So your suggestions are very much appreciated. :-)

[laeubi]

I think I somewhere started some work to make using lazy viewers more convenient but can't find it right now, at least one really needs to rethink some common usages, sadly JFace lacks a real model (even though everywhere "model" is mentioned) compared to e.g. Swing.

[vogella]

Would this be worth being push back upstream?

Yes.