8356770: TreeTableView not updated after removing a TreeItem with children and adding it to another parent #1971

Ziad-Mid · 2025-11-12T14:21:22Z

When a subtree is moved in a TreeTableView , the visuals don’t update until a resize or expand/collapse. The TreeTableViewSkin only rebuilds cells when the expanded row count changes.
This PR makes the skin to detect structural changes on childrenModificationEvent using new variable treeStructureDirty, and in updateItemCount() call requestRebuildCells() to refresh the visuals.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8356770: TreeTableView not updated after removing a TreeItem with children and adding it to another parent (Bug - P4)

Reviewers

Andy Goryachev (@andy-goryachev-oracle - Reviewer) 🔄 Re-review required (review applies to 58f9f4e7)
Marius Hanl (@Maran23 - Committer) 🔄 Re-review required (review applies to 58f9f4e7)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1971/head:pull/1971
$ git checkout pull/1971

Update a local copy of the PR:
$ git checkout pull/1971
$ git pull https://git.openjdk.org/jfx.git pull/1971/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1971

View PR using the GUI difftool:
$ git pr show -t 1971

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1971.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-11-12T14:23:59Z

👋 Welcome back zelmidaoui! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-11-12T14:25:11Z

@Ziad-Mid This change is no longer ready for integration - check the PR body for details.

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableViewSkin.java

mlbridge · 2025-11-12T15:52:53Z

Webrevs

andy-goryachev-oracle · 2025-11-12T20:13:41Z

/reviewers 2

openjdk · 2025-11-12T20:15:20Z

@andy-goryachev-oracle
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

andy-goryachev-oracle · 2025-11-12T20:23:04Z

@Ziad-Mid can we add a unit test which fails in master and passes with the fix?

andy-goryachev-oracle

I see no ill effects with the reproducer and the monkey tester (which was updated to add data manipulation context menus, see https://github.com/andy-goryachev-oracle/MonkeyTest ).

I suppose adding a flag in the skin (as opposed to similar flag in VirtualContainerBase) is ok since it deals with tree-table-specific functionality.

Ziad-Mid · 2025-11-13T18:49:14Z

can we add a unit test which fails in master and passes with the fix?

I have added test as suggested

andy-goryachev-oracle

Thank you for adding the test! Verified it fails in master and passes with the fix.

Maran23

Fix should be good, commented some improvements for the test

Maran23 · 2025-11-14T09:24:36Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java

    }

+    @Test
+    void test_jdk_8356770_reparentingItem() {


I would recommend a test name that reflects what exactly we want to test here. You can reference the JDK Ticket in a javadoc comment.
Example:

jfx/modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java

Lines 349 to 354 in 829d2be

/**

* The content display should also be taken into consideration when measuring the width.

* See also: <a href="https://bugs.openjdk.org/browse/JDK-8186188">JDK-8186188</a>

*/

@Test

public void testResizeColumnToFitContentWithGraphicAlignment() {

Maran23 · 2025-11-14T09:25:10Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java

+        table.setShowRoot(true);
+
+        stageLoader = new StageLoader(table);
+        Toolkit.getToolkit().firePulse();


Is this call really needed? Just asking because most of the time, it is not needed right after the StageLoader

Maran23 · 2025-11-14T09:27:39Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java

+        Toolkit.getToolkit().firePulse();
+
+        // Find "item B" row and record its disclosure node indent
+        TreeTableRow<String> rowBefore = findRow(table, "item B");


I would recommend using VirtualFlowTestUtils.getVirtualFlow(table).getVisibleCell(index);
The index should be easy to find out, since we always know where the TreeItem should be.

Yes exact, I usually prefer not to hardcode things, but sure in this case I will modify it.

Maran23 · 2025-11-14T09:29:01Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java

+    }
+
+    private static double disclosureX(TreeTableRow<String> row) {
+        Node disclosureNode = row.lookup(".tree-disclosure-node");


Can't you just call: row.getDisclosureNode() ?

Maran23 · 2025-11-14T09:30:29Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java

+        return null;
+    }
+
+    private static double disclosureX(TreeTableRow<String> row) {


I know IntelliJ makes methods static by default (when extracting), but I don't think it makes sense here

I think it's perfectly fine to have a static method here (and in tests in general).

Maran23 · 2025-11-14T09:31:52Z

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableViewSkin.java

                        control.requestLayout();
                        break;
                    }
+                    else if (eventType.equals(TreeItem.<T>childrenModificationEvent())) {


minor: else if should be in the previous line to follow the Java Code Convention: } else if (..)

Maran23 · 2025-11-14T09:37:29Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java

+        Toolkit.getToolkit().firePulse();
+
+        TreeTableRow<String> rowAfter = findRow(table, "item B");
+        assertNotNull(rowAfter, "item B row should not be null");


Null check is not needed, since we do test the disclosure node position instead

The null check is just for safety that the item exist, not that much important.

Ziad-Mid · 2025-11-14T15:52:39Z

I have updated the test, thanks for comments could you check.

Maran23

Two minor comments. Will reapprove once they are fixed.
Test looks good, and gives helpful insights to us in case it fails in the future. Good job!

modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java

Ziad-Mid · 2025-11-15T02:34:30Z

Changes added thank you for review, I will integrate by tomorrow if no more suggestions.

Added new treeStructureDirty

13d8b77

Ziad-Mid marked this pull request as ready for review November 12, 2025 15:47

openjdk bot added the rfr Ready for review label Nov 12, 2025

Maran23 reviewed Nov 12, 2025

View reviewed changes

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableViewSkin.java Show resolved Hide resolved

andy-goryachev-oracle approved these changes Nov 12, 2025

View reviewed changes

Add test

4a07d87

andy-goryachev-oracle approved these changes Nov 13, 2025

View reviewed changes

Ziad-Mid requested a review from Maran23 November 13, 2025 19:09

Maran23 reviewed Nov 14, 2025

View reviewed changes

Minor test changes

58f9f4e

andy-goryachev-oracle approved these changes Nov 14, 2025

View reviewed changes

Maran23 approved these changes Nov 14, 2025

View reviewed changes

modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java Outdated Show resolved Hide resolved

modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java Outdated Show resolved Hide resolved

openjdk bot added the ready Ready to be integrated label Nov 14, 2025

minor change

e160a15

openjdk bot removed the ready Ready to be integrated label Nov 15, 2025

	/**
	* The content display should also be taken into consideration when measuring the width.
	* See also: <a href="https://bugs.openjdk.org/browse/JDK-8186188">JDK-8186188</a>
	*/
	@Test
	public void testResizeColumnToFitContentWithGraphicAlignment() {

8356770: TreeTableView not updated after removing a TreeItem with children and adding it to another parent #1971

Are you sure you want to change the base?

8356770: TreeTableView not updated after removing a TreeItem with children and adding it to another parent #1971

Conversation

Ziad-Mid commented Nov 12, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Nov 12, 2025

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

andy-goryachev-oracle commented Nov 12, 2025

Uh oh!

openjdk bot commented Nov 12, 2025

Uh oh!

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

Uh oh!

Uh oh!

andy-goryachev-oracle left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ziad-Mid commented Nov 13, 2025

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

Maran23 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ziad-Mid commented Nov 14, 2025

Uh oh!

Maran23 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Ziad-Mid commented Nov 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Ziad-Mid commented Nov 12, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Nov 12, 2025 •

edited

Loading

mlbridge bot commented Nov 12, 2025 •

edited

Loading

andy-goryachev-oracle commented Nov 12, 2025 •

edited

Loading

andy-goryachev-oracle left a comment •

edited

Loading