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

ptziegler · 2025-11-10T13:09:15Z

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 · 2025-11-10T13:10:32Z

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

laeubi · 2025-11-10T13:12:19Z

@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 · 2025-11-10T13:26:54Z

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 · 2025-11-10T13:27:04Z

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 · 2025-11-10T13:49:54Z

Test Results

3 018 files ± 0 3 018 suites ±0 2h 21m 6s ⏱️ + 5m 41s
8 242 tests + 6 7 993 ✅ + 7 249 💤 ±0 0 ❌ - 1
23 646 runs +18 22 852 ✅ +19 794 💤 ±0 0 ❌ - 1

Results for commit 210df04. ± Comparison against base commit 2d4334f.

♻️ This comment has been updated with latest results.

ptziegler · 2025-11-10T14:53:55Z

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

Steps To Reproduce:

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

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

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 · 2025-11-11T09:49:32Z

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 · 2025-11-12T09:16:41Z

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.

eclipse-platform-bot · 2025-12-01T07:59:13Z

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

bundles/org.eclipse.jface/META-INF/MANIFEST.MF
tests/org.eclipse.jface.tests/META-INF/MANIFEST.MF

An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch.

Git patch

From 1090c5e35f44f281885be256d76bcc8b9d4d541c Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <[email protected]>
Date: Tue, 2 Dec 2025 08:36:06 +0000
Subject: [PATCH] Version bump(s) for 4.39 stream


diff --git a/bundles/org.eclipse.jface/META-INF/MANIFEST.MF b/bundles/org.eclipse.jface/META-INF/MANIFEST.MF
index b2c427bf1a..81de89277b 100644
--- a/bundles/org.eclipse.jface/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.jface/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %pluginName
 Bundle-SymbolicName: org.eclipse.jface;singleton:=true
-Bundle-Version: 3.38.100.qualifier
+Bundle-Version: 3.38.200.qualifier
 Bundle-Vendor: %providerName
 Bundle-Localization: plugin
 Export-Package: org.eclipse.jface,
diff --git a/tests/org.eclipse.jface.tests/META-INF/MANIFEST.MF b/tests/org.eclipse.jface.tests/META-INF/MANIFEST.MF
index b8dc172d3e..8cadbfa2c6 100644
--- a/tests/org.eclipse.jface.tests/META-INF/MANIFEST.MF
+++ b/tests/org.eclipse.jface.tests/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %Bundle-Name
 Bundle-SymbolicName: org.eclipse.jface.tests
-Bundle-Version: 1.4.1100.qualifier
+Bundle-Version: 1.4.1200.qualifier
 Automatic-Module-Name: org.eclipse.jface.tests
 Bundle-RequiredExecutionEnvironment: JavaSE-17
 Require-Bundle: org.junit;bundle-version="4.12.0",
-- 
2.51.2

Further information are available in Common Build Issues - Missing version increments.

…se-platform#3525 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 eclipse-platform#3525

ptziegler · 2025-12-01T09:06:12Z

If there are no objections (and if the remaining checks succeed), I'll plan on merging this within the next few days.

iloveeclipse · 2025-12-01T09:22:22Z

@ptziegler : not sure if the commit title & message, PR title & message are reflecting the actual fix?
It would be good to update everything to the latest change state.

iloveeclipse · 2025-12-01T09:32:14Z

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"

I can't reproduce on master, tried with Package and Project Explorers. Anything missing, like projects should be expanded / collapsed etc? Or is this only Windows specific (I'm on RHEL Linux/X11).

iloveeclipse · 2025-12-01T09:48:53Z

I can't reproduce on master, tried with Package and Project Explorers. Anything missing, like projects should be expanded / collapsed etc?

OK, following is needed:

Create a new project "X" and "Y".
Make sure in view preferences there is no other files or directories (like .settings, .project etc) are shown, so both projects are shown as "empty".
Create a new text file "hello.txt" in "X". It should be the only visible file in the project.
Move "hello.txt" to "Y".
kaboom: the Package Explorer doesn't show "hello.txt", and Project Explorer shows strange empty node

iloveeclipse · 2025-12-01T09:51:08Z

This patch doesn't fix for me the problem with the steps above.

ptziegler · 2025-12-01T11:19:49Z

Make sure in view preferences there is no other files or directories (like .settings, .project etc) are shown, so both projects are shown as "empty".

Yes, the resource filter for files starting with a '.' needs to be enabled. Apologies for the inaccuracy.

This patch doesn't fix for me the problem with the #3527 (comment).

On Linux, this block is entered, which then causes the creation of an additional tree item. But not on Windows. Commenting this block out makes the viewer behave as expected, but that shouldn't be the solution.

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

Lines 279 to 299 in 8b402ec

    
           if (!getExpanded(ti)) { 
        
           	boolean needDummy = isExpandable(ti, path, parent); 
        
           	boolean haveDummy = false; 
        
           	// remove all children 
        
           	Item[] items = getItems(ti); 
        
           	for (Item item : items) { 
        
           		if (item.getData() != null) { 
        
           			disassociate(item); 
        
           			item.dispose(); 
        
           		} else if (needDummy && !haveDummy) { 
        
           			haveDummy = true; 
        
           		} else { 
        
           			item.dispose(); 
        
           		} 
        
           	} 
        
           	// append a dummy if necessary 
        
           	if (needDummy && !haveDummy) { 
        
           		newItem(ti, SWT.NULL, -1); 
        
           	} 
        
           	return; 
        
           }

The call to getExpanded() is simply delegated to the TreeItem widget. This looks like in inconsistency in SWT? Both on Linux and Windows, this method should return true.

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

Lines 176 to 179 in 8b402ec

    
           @Override 
        
           protected boolean getExpanded(Item item) { 
        
           	return ((TreeItem) item).getExpanded(); 
        
           }

iloveeclipse · 2025-12-01T11:24:53Z

This looks like in inconsistency in SWT?

So it looks like the patch works on Windows only? Have you Mac access, any idea how it looks there?

Could you please investigate that Windows/Linux inconsistency before merging the patch?

ptziegler · 2025-12-01T11:24:56Z

The TreeItem has a local variable that keeps track of the "expanded" state. However, this variable is not used inside getExpanded(), which instead calls the GTK API directly. The result is two conflicting values: true from the local variable and false from GTK. This doesn't look right...

ptziegler · 2025-12-01T11:27:51Z

So it looks like the patch works on Windows only?

That seems to be the cases, yes.

Have you Mac access, any idea how it looks there?

No, only Windows and Linux.

Could you please investigate that Windows/Linux inconsistency before merging the patch?

That's my current plan. If this really is an SWT issue, it should be possible to derive a small snippet with the same, inconsistent "expanded" state.

ptziegler force-pushed the issue3525 branch from f2cba2b to 299345e Compare November 10, 2025 13:09

ptziegler marked this pull request as draft November 10, 2025 13:09

ptziegler force-pushed the issue3525 branch 2 times, most recently from 652520a to 657b1b7 Compare November 11, 2025 09:40

ptziegler marked this pull request as ready for review November 11, 2025 09:43

ptziegler force-pushed the issue3525 branch from 657b1b7 to d87f7ec Compare November 11, 2025 10:13

ptziegler changed the title ~~Don't try to remove elements that are not in the tree #3525~~ Don't create dummy item when removing invalid element from tree #3525 Nov 11, 2025

ptziegler force-pushed the issue3525 branch 3 times, most recently from ad4c5c7 to 562ad67 Compare November 12, 2025 13:22

ptziegler force-pushed the issue3525 branch from 562ad67 to 9d559b2 Compare December 1, 2025 07:50

ptziegler force-pushed the issue3525 branch from 8ebac2b to f9e236d Compare December 1, 2025 08:10

ptziegler mentioned this pull request Dec 1, 2025

Inconsistency in TreeItem.getExpanded() between Linux and Windows eclipse-platform/eclipse.platform.swt#2834

Open

ptziegler force-pushed the issue3525 branch from ba6597c to f9e236d Compare December 2, 2025 08:30

Version bump(s) for 4.39 stream

210df04

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

Are you sure you want to change the base?

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

Conversation

ptziegler commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ptziegler commented Nov 10, 2025

Uh oh!

laeubi commented Nov 10, 2025

Uh oh!

iloveeclipse commented Nov 10, 2025

Uh oh!

ptziegler commented Nov 10, 2025

Uh oh!

github-actions bot commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

ptziegler commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ptziegler commented Nov 11, 2025

Uh oh!

ptziegler commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eclipse-platform-bot commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ptziegler commented Dec 1, 2025

Uh oh!

iloveeclipse commented Dec 1, 2025

Uh oh!

iloveeclipse commented Dec 1, 2025

Uh oh!

iloveeclipse commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iloveeclipse commented Dec 1, 2025

Uh oh!

ptziegler commented Dec 1, 2025

Uh oh!

iloveeclipse commented Dec 1, 2025

Uh oh!

ptziegler commented Dec 1, 2025

Uh oh!

ptziegler commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ptziegler commented Nov 10, 2025 •

edited

Loading

github-actions bot commented Nov 10, 2025 •

edited

Loading

ptziegler commented Nov 10, 2025 •

edited

Loading

ptziegler commented Nov 12, 2025 •

edited

Loading

eclipse-platform-bot commented Dec 1, 2025 •

edited

Loading

iloveeclipse commented Dec 1, 2025 •

edited

Loading

ptziegler commented Dec 1, 2025 •

edited

Loading