On Sat, 24 Feb 2024 17:38:11 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). > > 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
