On Thu, 8 Feb 2024 18:55:54 GMT, Marius Hanl <mh...@openjdk.org> wrote:
> `TreeTableRow` does not check the item with `isItemChanged(..)`, unlike all > other implementations of the cell. > > This also means that the `TreeTableRow` always updates the item, although it > should not, resulting in a performance loss as a `TreeTableRow` will always > call `updateItem(..)`. > > It looks like that this was forgotten in > [JDK-8092593](https://bugs.openjdk.org/browse/JDK-8092593). > > Checking the whole history, it looks like the following was happening: > 1. There was a check if the item is the same in all cell implementations > (with `.equals(..)`) > 2. Check was removed as it caused bugs > 3. Check was readded, but instead we first check the index (same index) and > then if we also have the same item (this time with `.isItemChanged(..)`, to > allow developers to subclass it) > 4. Forgotten for `TreeTableRow` inbetween this chaos. > > Added many tests for the case where an item should be changed and where it > should not. > Improved existing tests as well. Using `stageLoader`, as before the tests > only created a stage when doing the assert. > > NOTE: The update logic is now the same as for all other 5 cell > implementations. Especially `TreeCell` (since it is for a tree structure as > well). Tried the path with the TreeTableView page in the MonkeyTester on macOS 14.3, see no ill effects - though the page is somewhat limited to what it can exercise. The SCCE generates the following output: updateItem updateItem updateItem updateItem updateItem updateItem updateItem updateItem updateItem updateItem updateItem updateItem updateItem updateItem updateItem updateItem updateItem updateItem updateItem updateItem updateItem updateItem updateItem isItemChanged <clicked to expand> isItemChanged isItemChanged updateItem isItemChanged isItemChanged <clicked to collapse> isItemChanged updateItem isItemChanged is this expected output? modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableRow.java line 421: > 419: if (oldIndex == newIndex) { > 420: if (!isItemChanged(oldValue, newValue)) { > 421: // RT-37054: we break out of the if/else code here > and could we also add (or replace?) the new reference for RT-37054: JDK-8096969 modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableRow.java line 424: > 422: // proceed with the code following this, so that we > may > 423: // still update references, listeners, etc as > required. > 424: break outer; I understand neither the comment nor this `break outer;`: there is no code that follows `outer: if`, so we might as well `return`, right? modules/javafx.controls/src/test/java/test/javafx/scene/control/ListCellTest.java line 1150: > 1148: super.updateItem(item, empty); > 1149: > 1150: counter.incrementAndGet(); very, very minor, and probably not important: shouldn't 'incrementAndGet()` be called before `super.updateItem()`? modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java line 943: > 941: super.updateItem(item, empty); > 942: > 943: counter.incrementAndGet(); same here: move before super.updateItem? modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java line 961: > 959: * is called when the index is 'changed' to the same number as the > old one, to evaluate if we need to call > 960: * updateItem(..). > 961: */ thank you so much for providing clear explanations of what all these tests are trying to accomplish! modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewRowTest.java line 176: > 174: super.updateItem(item, empty); > 175: > 176: counter.incrementAndGet(); and here. the reason is that if super.updateItem() throws an exception which gets eaten (it shouldn't but we should not be assuming that) we still get the counter incremented. modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeCellTest.java line 1078: > 1076: super.updateItem(item, empty); > 1077: > 1078: counter.incrementAndGet(); ... modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableCellTest.java line 1255: > 1253: super.updateItem(item, empty); > 1254: > 1255: counter.incrementAndGet(); ... modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableRowTest.java line 960: > 958: super.updateItem(item, empty); > 959: > 960: counter.incrementAndGet(); ... modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java line 1777: > 1775: } > 1776: > 1777: @Test public void testSetChildrenShouldUpdateTheCells() { we probably should have `@Test` on its own line modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java line 2639: > 2637: StageLoader sl = new StageLoader(treeTableView); > 2638: > 2639: assertEquals(18, rt_31200_count); magic number... should we explain why exactly 18 ? ------------- PR Review: https://git.openjdk.org/jfx/pull/1360#pullrequestreview-1881075752 PR Review Comment: https://git.openjdk.org/jfx/pull/1360#discussion_r1489954389 PR Review Comment: https://git.openjdk.org/jfx/pull/1360#discussion_r1489958225 PR Review Comment: https://git.openjdk.org/jfx/pull/1360#discussion_r1489961310 PR Review Comment: https://git.openjdk.org/jfx/pull/1360#discussion_r1489965524 PR Review Comment: https://git.openjdk.org/jfx/pull/1360#discussion_r1489966660 PR Review Comment: https://git.openjdk.org/jfx/pull/1360#discussion_r1489968231 PR Review Comment: https://git.openjdk.org/jfx/pull/1360#discussion_r1489968818 PR Review Comment: https://git.openjdk.org/jfx/pull/1360#discussion_r1489969213 PR Review Comment: https://git.openjdk.org/jfx/pull/1360#discussion_r1489969686 PR Review Comment: https://git.openjdk.org/jfx/pull/1360#discussion_r1489970617 PR Review Comment: https://git.openjdk.org/jfx/pull/1360#discussion_r1489972931
