On Mon, 13 Apr 2026 00:09:53 GMT, Charlie Schlosser <[email protected]> wrote:
>> Fixes https://bugs.openjdk.org/browse/JDK-8181411 and >> https://bugs.openjdk.org/browse/JDK-8380308 >> >> `TreeUtil.getItem` is used pervasively throughout `TreeView` and >> `TreeTableView`, specifically `getTreeItem`. This performs a >> depth-first-search (DFS) of the tree, and caches the target item. This is ok >> for random access, but is slow for sequential access with a cold cache, as >> each subsequent call to `getTreeItem` starts a new DFS from the root of the >> tree. For `selectAll`, this has O(N^2) time complexity. >> >> This PR adds a stateful iterator that resumes the DFS from the previously >> retrieved item, and caches each item during the traversal. This reduces the >> time complexity `selectAll` case to O(N). >> >> This change revealed an otherwise harmless bug in the order of event >> handling in the `invalidated()` method of the root property. >> >> Current behavior: >> 1. `expandedItemCountChangeEvent` fires >> 2. Focus model listener fires >> 3. Focus model calls `getTreeItem`, `expandedItemCountDirty` is false, cache >> miss, correct item returned >> 5. `rootEvent` fires, sets `expandedItemCountDirty = true` >> >> The correct behavior would be to set `expandedItemCountDirty = true` before >> the focus model calls getTreeItem. It works anyway because any item that is >> not in the cache triggers a full DFS from the root, which is always valid. >> >> This PR: >> 1. `expandedItemCountChangeEvent` fires >> 2. `rootEvent` fires, sets `expandedItemCountDirty = true` >> 3. Focus model listener fires >> 4. Focus model calls `getTreeItem`, `expandedItemCountDirty` is true, >> iterator resets, correct item returned >> >> This is fixed by using `expandedItemCountChangeEvent` instead of the super >> type `treeNotificationEvent`. The subclass event types are processed before >> their super counterparts, regardless of registration order. Now that they >> are the same type, the order is correct. >> >> This behavior is already covered (and is motivated) by `TreeTableViewTest > >> test_rt17522_focusShouldMoveWhenItemRemovedBeforeFocusIndex_1()` and >> `TreeTableViewTest > >> test_rt17522_focusShouldMoveWhenItemAddedAtFocusIndex_1()`. >> >> >> >> ┌─────────┬───────────────────┬──────────────────┬────────────────────────┬───────────────────────┐ >> │ Items │ TreeView (before) │ TreeView (after) │ TreeTableView (before) >> │ TreeTableView (after) │ >> >> ├─────────┼───────────────────┼──────────────────┼────────────────────────┼───────────────────────┤ >> │ 64 │ 1ms │ 1ms │ ... > > Charlie Schlosser has updated the pull request incrementally with one > additional commit since the last revision: > > return null of iterator is exhausted modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java line 32: > 30: import static > test.com.sun.javafx.scene.control.infrastructure.ControlTestUtils.assertStyleClassContains; > 31: > 32: import java.beans.Transient; This and some other imports are unused modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java line 7939: > 7937: @Test > 7938: public void testGetTreeItem() { > 7939: Unneeded new line here and the other test modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java line 7978: > 7976: treeItems.get(16).setExpanded(true); > 7977: > 7978: TreeTableView<String> ttv = new > TreeTableView<>(treeItems.get(0)); Very minor: You can use `treeItems.getFirst()` here modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java line 8036: > 8034: > 8035: assertEquals(17, ttv.getExpandedItemCount()); > 8036: expectedValues = List.of(0, 1, 2, 3, 7, 8, 9, 10, 11, 12, 13, > 14, 15, 16, 17, 18, 19).stream().map(String::valueOf).toList(); Minor: This can be: `expectedValues = Stream.of(0, 1, 2, 3, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19).map(String::valueOf).toList();` modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java line 8065: > 8063: > 8064: assertEquals(12, ttv.getExpandedItemCount()); > 8065: expectedValues = List.of(0, 1, 2, 11, 12, 13, 14, 15, 16, 17, > 18, 19).stream().map(String::valueOf).toList(); Minor: This can be: `expectedValues = Stream.of(0, 1, 2, 11, 12, 13, 14, 15, 16, 17, 18, 19).map(String::valueOf).toList();` Same for the other test ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/2142#discussion_r3072887793 PR Review Comment: https://git.openjdk.org/jfx/pull/2142#discussion_r3072891836 PR Review Comment: https://git.openjdk.org/jfx/pull/2142#discussion_r3072896537 PR Review Comment: https://git.openjdk.org/jfx/pull/2142#discussion_r3072903041 PR Review Comment: https://git.openjdk.org/jfx/pull/2142#discussion_r3072906015
