TreeViewer: auto expansion for elements with single children

[Wittmaxi]

As implementation of #1063, This PR addresses the need to automatically expand paths which consist of chained singular nodes, eg:

com.wittmaxi.plugin
 └─src
    └─org.foo.com
       └─Bar.java

It introduces two new API-Methods into TreeViewer:

-setAutoExpandTrivialPathsLevel
-getAutoExpandTrivialPathsLevel

The PR also uses the new features in the Project Explorer, thus resolving #1063

[Wittmaxi]

I still need to address a few points as part of this PR

• When double-clicking on an item, the TreeListener-hook is not called
• There are no Unit-Tests yet
• The behaviour should be off by default
• What should be the default value for expandTrivialPathsLevel?

[merks]

For auto expand there are only getAutoExpandLevel/setAutoExpandLevel with 0 and -1 having special meaning so it's seems a little strange/confusing four methods for this new feature...

[github-actions]

Test Results

   917 files   -     1     917 suites   - 1   36m 44s ⏱️ - 12m 44s
 7 507 tests +   55   7 355 ✅ +   57  151 💤 ±  0  1 ❌  - 2 
22 097 runs   - 1 408  21 710 ✅  - 1 285  386 💤  - 119  1 ❌  - 4 

For more details on these failures, see this check.

Results for commit 8e0452b. ± Comparison against base commit 7572bd4.

This pull request removes 7 and adds 62 tests. Note that renamed tests count towards both.

org.eclipse.jface.tests.viewers.CheckboxTreeViewerTest ‑ testFilterExpanded
org.eclipse.jface.tests.viewers.MultipleEqualElementsTreeViewerTest ‑ testFilterExpanded
org.eclipse.jface.tests.viewers.TreeViewerColumnTest ‑ testFilterExpanded
org.eclipse.jface.tests.viewers.TreeViewerTest ‑ testFilterExpanded
org.eclipse.jface.tests.viewers.TreeViewerWithLimitCompatibilityTest ‑ testFilterExpanded
org.eclipse.jface.tests.viewers.VirtualLazyTreeViewerTest ‑ testFilterExpanded
org.eclipse.jface.tests.viewers.VirtualTreeViewerTest ‑ testFilterExpanded

org.eclipse.jface.tests.viewers.CheckboxTreeViewerTest ‑ testAutoExpandOnSingleChild
org.eclipse.jface.tests.viewers.CheckboxTreeViewerTest ‑ testAutoExpandOnSingleChildDeeperDownPath
org.eclipse.jface.tests.viewers.CheckboxTreeViewerTest ‑ testAutoExpandOnSingleChildFromRoot
org.eclipse.jface.tests.viewers.CheckboxTreeViewerTest ‑ testAutoExpandOnSingleChildFromRootWithSensibleTrivialPath
org.eclipse.jface.tests.viewers.CheckboxTreeViewerTest ‑ testAutoExpandOnSingleChildInfiniteExpand
org.eclipse.jface.tests.viewers.CheckboxTreeViewerTest ‑ testAutoExpandOnSingleChildManualDisable
org.eclipse.jface.tests.viewers.CheckboxTreeViewerTest ‑ testAutoExpandOnSingleChildSmallerThanAutoExpandDepth
org.eclipse.jface.tests.viewers.CheckboxTreeViewerTest ‑ testAutoExpandOnSingleChildThroughEvent
org.eclipse.jface.tests.viewers.CheckboxTreeViewerTest ‑ testSetAutoExpandOnSingleChildLevels
org.eclipse.jface.tests.viewers.MultipleEqualElementsTreeViewerTest ‑ testAutoExpandOnSingleChild
…

♻️ This comment has been updated with latest results.

[Wittmaxi]

Good catch, I changed my proposal to be more consistent with the existing behaviour @merks

[merks]

I think this will be a cool feature!

[HeikoKlare]

I like the proposed change in general, still I have two major remarks (and some fine-grained ones made as code comments):

Functionality:
The changes to AbstractTreeViewer are currently API breaking: the new methods for setting the auto expansion (such as setAutoExpandTrivialPathsLevel) claim that when setting the expansion level, the instance will perform auto-expansion. The current implementation does, however, not guarantee that but relies on implementations of that class (such as TreeViewer) to provide the according behavior. I would propose to change the implementation such that AbstractTreeViewer already provides the claimed functionality by changing the according methods handling changes of the expansion state of a node and adding a listener that reacts to expansion state changes (like now done in the TreeViewer.

Naming:
I am not in favor of the current naming, as I don't think that there is a common understanding of what "trivial paths" are. This requires every method related to the functionality to explain what is meant. I would suggest to change the naming to something more self-explaning. This could be something like autoExpandOnSingleChild or the like (better proposals are welcome).

[HeikoKlare]

This seems to reimplement the essential expansion functionality as already implemented in setExpandedState. Is there a necessity for this additional method?

[HeikoKlare]

I would expect the additional extension to happen after the "ordinary" extension has happended completely, i.e., after creating childen and setting the expand state has happened. First performing the recusive extension and then setting the expand state does not look to be the right order to me.

[Wittmaxi]

I have changed the order in which both operations happen, I suspect that this does not correctly reflect what you actually intended by your comment. If that is the case, can you please specify what exactly you have in mind?

[HeikoKlare]

Recursive expansion should happen by completely expanding an items before starting the expansion of a child. The state change of an auto expansion should be equal to manually extending levels one-by-one, i.e., before expanding a child, the parent expansion should be finished in terms of creating all its children, settings it expanded state (and maybe further state changes I am not aware of). Currently, the logic is quite convoluted, as this method sets the expanded state and then calls expandAndCreateChildren, which sets the state again.

[Wittmaxi]

if (item instanceof Item) {
	setExpanded((Item) item, expanded);
	if (expanded) {
		if (expandAndCreateChildren(item).length == 1) {
			expandPathOfSingleChildren(getChildren(item)[0], getAutoExpandOnSingleChildLevel());
		}
	}
}

What currently happens:

1. set the expanded state (calls the winapi, doesn't add the children)
   if the expanded state is expanded:
2. set the expanded state again (doesn't do anything new)
3. create the children
4. return the amount of children
5. check: does my parent only have one child? If so, start the recursive routine that expands trivial paths and keeps track of how many levels it has recursed

Steps 1-4 expand the node and do not yet add any behaviour. Step 5 is the actual expansion of paths of singular children - just as you had described in your comment, I first perform the existing behaviour and then simulatie "clicking" through the children to consecutively open them.
From what I understood in your comment, your issue boils down to 1) and 2) being duplicates of eachother. A rather obvious solution would be to check for the expanded state so that I only set it once

if (!expanded) {
  setExpanded((Item) item, false);
} else {
  if (expandAndCreateChildren(item).length == 1) {
    expandPathOfSingleChildren(getChildren(item)[0], getAutoExpandOnSingleChildLevel());
  }
}

I am a bit hesitant about making this change as it introduces additional control flow and complexity. However, setExpanded performs a call to winapi (... and probably to cocoa and gtk too), hence I can understand why we would want to limit it's use.

[HeikoKlare]

I like that you were able to move functionality completely into AbstractTreeViewer. That makes my previous concerns with respect to API compatibility obsolete.

Please consider my unanswered comment from last review.

[HeikoKlare]

Recursive expansion should happen by completely expanding an items before starting the expansion of a child. The state change of an auto expansion should be equal to manually extending levels one-by-one, i.e., before expanding a child, the parent expansion should be finished in terms of creating all its children, settings it expanded state (and maybe further state changes I am not aware of). Currently, the logic is quite convoluted, as this method sets the expanded state and then calls expandAndCreateChildren, which sets the state again.

[HeikoKlare]

I have taken a deeper look into the overall design of the expand mechanism in the AbstractTreeViewer. I would like to make some proposals to improve this PR by reusing existing functionality and the insights behind.

1. internalExpandToLevel already has a sophisticated implementation for expanding a certain amount of levels, including some performance improvements. I would propose to let the recursive expansion when only having a single child use this functionality by generalizing it to some kind of internalConditionalExpandToLevel that additionally accepts a shouldExpand function (Widget -> boolean) that is used to calculate whether the next recursion of expansion shall be performed. Then the existing internalExpandToLevel method can use this method with a function always returning true, while the expand on single child functionality will pass a condition based on the number of childs of the widget.
2. The expandToLevel(...) methods disable the redraw of the tree viewer while expanding (to avoid flickering). That may not be a severe problem with recursive expansion when only having one child, as this should be quite fast, still the functionality is already there and I would thus propose to reuse it. You could, for example, encapsulate the transaction that ensures proper disablement and re-enablement of redraw into a functional that can be used by both the existing expandToLevel as well as the expandOnSingleChild. It could look something like this:

private void excuteWithRedrawDisabled(Runnable toExecute) {
	Control control = getControl();
	try {
		control.setRedraw(false);
		toExecute.run();
	} finally {
		control.setRedraw(true);
	}
}

[HeikoKlare]

I thought that the refactoring of the temporary redraw disablement would be rather local and thus proposed to do it in this PR or extract it to another that is merged before. Since a reasonable refactoring will probably be a bit larger (see #1191) and should not prevent this PR from being merged, So feel free to change the code back to the original sequence of manual setRedraw(false) and setRedraw(true), which will then be refactored together with all other analogous places in the code in an according refactoring. Sorry for the back and forth, but I was not aware that the issue is a more general one when proposing the refactoring.

[HeikoKlare]

I have two major remarks (and requests for change) and some minor detailed comments:

1. The current changes break public API (setExpandedState). This needs to be adapted.
2. The tests should cover all different usage scenarios of the different methods. In particular, they should cover the different ways in which auto-expansion can happen, i.e., not only via setExpandedState but also via SWT events. I'd propose to relate the tests to the different added or changed methods. For example, there should also be trivial tests, such as get...Level returning what has been set by set...Level to easy debugging in case only the setter is broken, as currently the tests for the expand behavior would break although it's state may simply not be represented as expected after calling the setter.

[HeikoKlare]

Changes work and look good now.

I propose to target the merge for 2024-06 M1. The change affects the TreeViewer as a rather central component, so I would propose to not risk a potentially undetected regression now that we are already quite late in 2024-03 development.

[HeikoKlare]

Failing test is pre-existing and already documented: #1005.