RFR: JDK-8325402: TreeTableRow updateItem() does not check item with isItemChanged(..) unlike all other cell implementations

[Marius Hanl]

On Wed, 14 Feb 2024 19:21:39 GMT, Andy Goryachev <angorya 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).
>
> 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