RFR: 8356770: TreeTableView not updated after removing a TreeItem with children and adding it to another parent [v2]

[Marius Hanl]

On Thu, 13 Nov 2025 18:52:46 GMT, Ziad El Midaoui <zelmidaoui at openjdk.org> wrote:

>> 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.
>
> Ziad El Midaoui has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add test

Fix should be good, commented some improvements for the test

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

> 148:                         break;
> 149:                     }
> 150:                     else if (eventType.equals(TreeItem.<T>childrenModificationEvent())) {

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

modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java line 6443:

> 6441: 
> 6442:     @Test
> 6443:     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:
https://github.com/openjdk/jfx/blob/829d2be46edeedd08f8413d02fd349047ed9f21b/modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java#L349-L354

modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java line 6468:

> 6466: 
> 6467:         stageLoader = new StageLoader(table);
> 6468:         Toolkit.getToolkit().firePulse();

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

modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java line 6471:

> 6469: 
> 6470:         // Find "item B" row and record its disclosure node indent
> 6471:         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.

modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java line 6481:

> 6479: 
> 6480:         TreeTableRow<String> rowAfter = findRow(table, "item B");
> 6481:         assertNotNull(rowAfter, "item B row should not be null");

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

modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java line 6501:

> 6499:     }
> 6500: 
> 6501:     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

modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java line 6502:

> 6500: 
> 6501:     private static double disclosureX(TreeTableRow<String> row) {
> 6502:         Node disclosureNode = row.lookup(".tree-disclosure-node");

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

-------------

PR Review: https://git.openjdk.org/jfx/pull/1971#pullrequestreview-3463891709
PR Review Comment: https://git.openjdk.org/jfx/pull/1971#discussion_r2526700182
PR Review Comment: https://git.openjdk.org/jfx/pull/1971#discussion_r2526666566
PR Review Comment: https://git.openjdk.org/jfx/pull/1971#discussion_r2526669167
PR Review Comment: https://git.openjdk.org/jfx/pull/1971#discussion_r2526681643
PR Review Comment: https://git.openjdk.org/jfx/pull/1971#discussion_r2526723454
PR Review Comment: https://git.openjdk.org/jfx/pull/1971#discussion_r2526694506
PR Review Comment: https://git.openjdk.org/jfx/pull/1971#discussion_r2526688347