RFR: JDK-8325402: TreeTableRow updateItem() does not check item with isItemChanged(..) unlike all other cell implementations
Andy Goryachev
angorya at openjdk.org
Wed Feb 14 19:48:07 UTC 2024
On Thu, 8 Feb 2024 18:55:54 GMT, Marius Hanl <mhanl at 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
More information about the openjfx-dev
mailing list