Overload method AbstractTreeViewer::internalConditionalExpandToLevel

[fedejeanne]

See @jukzi's comment: #2684 (comment)

Is required by

• Do not expand same class twice in (quick) type hierarchy eclipse-jdt/eclipse.jdt.ui#1932

[github-actions]

Test Results

 1 818 files  ± 0   1 818 suites  ±0   1h 34m 31s ⏱️ + 5m 35s
 7 745 tests + 6   7 517 ✅ + 7  228 💤 ±0  0 ❌  - 1 
24 396 runs  +18  23 647 ✅ +19  749 💤 ±0  0 ❌  - 1 

Results for commit 5600a40. ± Comparison against base commit 54028a5.

♻️ This comment has been updated with latest results.

[jukzi]

I think "internalConditionalExpandToLevel" is the wrong level of abstraction to make an API. Let alone the name "internal" ;-)
May better offer some
Function<Widget, Boolean> getShouldChildrenExpand()
or boolean shouldExpandDuplicate()
or setExpandDuplicate(boolean)
I don't know. but certainly not "Internal"Something.

[fedejeanne]

Added getShouldWigdetExpand()

[HeikoKlare]

Would it be possible to hold back this change until #1072 (review) is resolved?
The method to be modified has been introduced last year, but we may need to change it as there was something like a breaking API change caused by it which we may need to revise.

hold back

[fedejeanne]

Would it be possible to hold back this change until #1072 (review) is resolved? The method to be modified has been introduced last year, but we may need to change it as there was something like a breaking API change caused by it which we may need to revise.

Hm, I'd like to have this one and eclipse-jdt/eclipse.jdt.ui#1932 for M2.

Why not solve the other issue separately? The fix for #1072 (review) will have to expand widgets and its children regardless of how to do it i.e. it can make (and should) make use of getShouldWidgetExpand().

[HeikoKlare]

Hm, I'd like to have this one and eclipse-jdt/eclipse.jdt.ui#1932 for M2.

I want to have a solution for the other issue soon (before M2), so it should not prevent this from being in before M2. It may just make the resolution more complicated as it further a method whose extract may have been invalid and needs to be reverted.

Why not solve the other issue separately? The fix for #1072 (review) will have to expand widgets and its children regardless of how to do it i.e. it can make (and should) make use of getShouldWidgetExpand().

If you have a solution, it would be great if you can share. I did not think about the solution so far as I just got aware of the issue. To restore the original recursive behavior we may need to revert the method extraction, which will also affect this change.

[fedejeanne]

I still don't see why the need of postponing this change. If you need to revert the other change then you still can integrate this PR into the other one.

Why pass the buck and make this PR adapt to other one?

Also keep in mind that I don't only want to merge this PR, I also want the one in JDT so @jukzi would have to rush that one too which can lead to more errors.

[HeikoKlare]

As I said:

To restore the original recursive behavior we may need to revert the method extraction, which will also affect this change.

We potentially have to replace the extracted method with an inlined (recursively called one) again, so you cannot simply add new parameters like you are doing in tihs PR.

Why pass the buck and make this PR adapt to other one?

Because it might be possible that the other PR was invalid and this one would be as well (for the same reason, an API breakage).

I don't understand the problem with just waiting for some investigation. I will try to do that asap (hopefully today, maybe tomorrow).

[jukzi]

maybe you could agree on a setExpandDuplicate(boolean) which should be independent on how its implemented?

[HeikoKlare]

maybe you could agree on a setExpandDuplicate(boolean) which should be independent on how its implemented?

I currently have no strong opinion for or against any possible solution. This solution is also fine for me if will still be possible to apply it once the regression is fixed.

I have just created a PR with a potential fix of the regression:

• Restore recursive behavior of AbstractTreeViwer#internalExpandToLevel() #2707

[fedejeanne]

@HeikoKlare I just reviewed your PR and I think I can adapt this one to it. Let's process yours with a higher priority and then, once it's merged, I can adapt this one.

[fedejeanne]

maybe you could agree on a setExpandDuplicate(boolean) which should be independent on how its implemented?

The reasons I'm trying to avoid the usage of "duplicate" are :

1. The meaning of "duplication" is very specific. In your initial implementation (AbstractTreeViewer do not auto-expand same nodes multiple times #2684, which used a Set<Object>) duplication was detected by relying on what was returned by widget.getData(), but this is also an implementation detail.
2. The use case itself is also very specific. Maybe some consumers have no problem with expanding duplicated items but would like to keep the ones whose content starts with the letter "X" collapsed.

A solution that gives the chance to interrupt the unfolding and leaves the details to the consumer looks more flexible to me, that's why I tried to implement something more general by providing the Widget itself and letting the consumer make the choice.

[laeubi]

The meaning of "duplication" is very specific.

Because of this JFace has ViewerComparator to compare two items. Also trees are allowed to contain "duplicates" of course. One major drawback and performance problem of trees is that it does not maintain a parent<->child relation (TreeNodeContentProvider try to solve this but requires special consideration of the elements shown in a tree).

[fedejeanne]

I've adapted this PR according to the changes in #2707.

This PR is ready to be reviewed again.

[HeikoKlare]

If this is to be a field that has a meaning across method calls, it should, e.g., be filled during intialization. If this value is only relevant during a call of expandToLevel (or other) method (and thus reinitialized in every call of that method), it should not be a field, as it is specific to the execution time of that method.
You see the issues with this in the currently failing tests: there are other callers of internalCustomizedExpandToLevel, which accesses this field without it being initialized.

So I see two options:

• This predicate needs to be created within the method it is valid in and passed through the methods called from there, if necessary
• This precidate may be initialized at object initialization with a proper invariant holding for it during the object lifecycle

[fedejeanne]

I initialized it in the constructor too to avoid NPEs.

The tricky part is passing it all the way down through the method calls until it reaches internalCustomizedExpandToLevel without adding an extra parameter (or overloading) internalExpandToLevel(Widget widget, int level). Which is why I was only able to support the use cases where the call comes through expandToLevel(...).

Do you see any better way to do it? I couldn't come up with any.

[HeikoKlare]

This still means you are abusing a field of class to actually pass a method parameter. Fields of classes need to have a proper (or at least a reasonable) invariant they fulfill, but this one actually has not reasonable invariant (apart from "state x regarding last execution of y") as it is actually a method parameter.

I understand that changing the method signature is not easy/possible, but in my opinion solving this by abusing a field as a method parameter is an error prone design flaw. If you need to reinitialize the shouldItemExpand in some of the methods calling internalCustomizedExpandToLevel, why not in the others? Is there potentially some state in the predicate delivered by createShouldItemExpand? Then that one will usually be invalid while executing internalCustomizedExpandToLevel without having it properly initialized.

Wouldn't it be possible to achieve the same without any extension to AbstractTreeViewer by simply implementing the according subtype with an overwrittten expandToLevel, adding the predicate creation at the beginning of the method, and then consuming it in an overwritten setExpanded?

[fedejeanne]

Consuming it in setExpanded won't work because the execution needs to be stopped in its consumer (internalCustomizedExpandToLevel) which can only be achieved by changing its return value to boolean (API change, since it's a protected method) and doing an early return.

I've been looking at other subclasses of AbstractTreeViewer and I noticed that they end up hacking its way into the expansion mechanism by overriding methods. I think this may be a consequence of the little flexibility that the class offers. How about adding hook 2 methods that will be called only once right before and after the expansion runs? This would give the consumer/subclasses the chance to cleanly preserve some state outside AbstractTreeViewer and hopefully avoid future hacks.