On Wed, 14 Feb 2024 19:21:39 GMT, Andy Goryachev <ango...@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). > > 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 same as the other comment, I would prefer to do that for all cell implementations in one go. If that is okay for you as well. :) > 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()`? Hm, I see no reason for either one or the other. So not strong preference from my side here. > 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 ? It is the count, how often update item was called. That it got smaller shows, that we call it less often, as the item was not changed (which is a good thing). That also matches with your observation, which is the expected output here. I saw that this was changed for some tests in https://github.com/openjdk/jfx/pull/863, we also can do something similar here as well. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1360#discussion_r1490054700 PR Review Comment: https://git.openjdk.org/jfx/pull/1360#discussion_r1490056222 PR Review Comment: https://git.openjdk.org/jfx/pull/1360#discussion_r1490053457
