RFR: JDK-8325402: TreeTableRow updateItem() does not check item with isItemChanged(..) unlike all other cell implementations [v2]
Andy Goryachev
angorya at openjdk.org
Thu Feb 29 19:34:53 UTC 2024
On Sat, 24 Feb 2024 17:38:11 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).
>
> Marius Hanl has updated the pull request incrementally with two additional commits since the last revision:
>
> - JDK-8325402: remove labeled if
> - JDK-8325402: Unit test improvements
Tested in MT on macOS 14.3.1
Looks good, thank you for making the changes!
-------------
Marked as reviewed by angorya (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/1360#pullrequestreview-1909680339
More information about the openjfx-dev
mailing list