Don't create dummy item when removing invalid element from tree #3525

[ptziegler]

Due to the fix for Bug 210747, a dummy tree-item is added to the viewer if the same element is removed twice. The problem is that this fix doesn't distinguish between nodes that haven't been realized yet and nodes that are not part of the model.

This check has been refined to only call updatePlus(...) if the removed object is associated with such a dummy node.

To reproduce:

• Create a new project "Project A" and "Project B"
• Create a new text file "test" in "Project A"
• Move "test" to "Project B"

Closes #3525

[ptziegler]

@laeubi This fixes the issue with the project explorer. Though I'd still like to add a proper test case.

[laeubi]

@ptziegler good finding! I can guess its hard to come of with a simple testcase here given JDT is involved and we likely do not want to include it as a dependency.

[iloveeclipse]

Considering M3 is this week, I would prefer to postpone this change to 4.39 (assuming it works() in general and not only for specific use case).

This change (which affects org.eclipse.jface.viewers.AbstractTreeViewer.remove(Object...)` behavior must be tested for a longer time before release.

[ptziegler]

Aaah, now everything makes sense. The dummy tree item is created in updatePlus(...), in response to Bug [210747]

I can guess its hard to come of with a simple testcase here given JDT is involved and we likely do not want to include it as a dependency.

The remove() method might be called from anywhere. It just happens to be JDT for the Project Explorer. The difficult part is finding a good test case that can be adapted. 😄

[github-actions]

Test Results

 3 018 files  ± 0   3 018 suites  ±0   2h 18m 19s ⏱️ -14s
 8 246 tests +12   7 990 ✅ + 5  255 💤 + 6  1 ❌ +1 
23 658 runs  +36  22 845 ✅ +17  812 💤 +18  1 ❌ +1 

For more details on these failures, see this check.

Results for commit 562ad67. ± Comparison against base commit 6d2b614.

♻️ This comment has been updated with latest results.

[ptziegler]

I had a closer look at Bug 210747 and whatever was done with f1899ad doesn't work, regardless of my change.

Steps To Reproduce:

1. The content provider returns three elements (a), (b) and (c), while (b) is a child of (a) and (c) is a child of (b)

2. programatically expand item (a)
   observe: item (b) is rendered with a plus sign

3. Call AbstractTreeViewer.remove() to remove item (c)
   observe: the plus sign is not removed

As it is, item (b) continues to have a dummy tree-item as its child and would therefore show a plus sign. This is because the element is only removed from the UI and the call to isExpandable(...) queries the content provider (i.e. the model). So needsPlus returns true, even if its only child has just been removed.

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

Lines 3125 to 3127 in 39d028e

	boolean hasPlus = getItemCount(item) > 0;
	boolean needsPlus = isExpandable(item, null, element);
	boolean removeAll = false;

What a mess...

[ptziegler]

I've adapted the check so that for the use-case described in Bug 210747, where an element was removed from the tree that hasn't been realized yet, the updatePlus() method is still called. In my use-case, where an element is removed twice, nothing happens the second time.

I'm still not sure what the reasoning behind the initial change was, but I'm fairly confident that what was described in the bug was never fixed and probably can't be reasonbly fixed.

I've added a test case for this bug, which will obviously fail. Perhaps someone who is more familiar with the TreeViewer can have a look, otherwise I'll remove it again for the next M1.

Considering M3 is this week, I would prefer to postpone this change to 4.39 (assuming it works() in general and not only for specific use case).

This change (which affects org.eclipse.jface.viewers.AbstractTreeViewer.remove(Object...)` behavior must be tested for a longer time before release.

That's fine by me. This problem has existed for almost two decades, so three more months won't make much of a difference.

[ptziegler]

I'm a little bit concerned about this code block here, which is also responsible for the test failure and which was added with #810

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

Lines 2065 to 2084 in 601b0d3

	boolean continueOuter = false;
	if (getItemsLimit() > 0) {
	Widget[] itemsOfElement = internalFindItems(element);
	for (Widget item : itemsOfElement) {
	if (item instanceof TreeItem ti) {
	TreeItem parentItem = ti.getParentItem();
	if (parentItem == null) {
	internalRefreshStruct(ti.getParent(), getInput(), false);
	continueOuter = true;
	break;
	}
	// refresh parent item with the latest model.
	internalRefreshStruct(parentItem, parentItem.getData(), false);
	continueOuter = true;
	}
	}
	}
	if (continueOuter) {
	continue;
	}

In short, if setDisplayIncrementally() is called, you are no longer able to remove elements from the viewer without first removing them from the model.