On Tue, 12 Nov 2024 21:11:56 GMT, Marius Hanl <mh...@openjdk.org> wrote:
> This PR fixes a bug where the `TableCell` indices can be outdated (not > synchronized) with the `TableRow` index. > > What normally happens is: > - Index is changed: Cell update is requested when the row is empty (otherwise > noop) > - Item is changed: Cell update is requested, which will now update the > indices of the underlying `TableCells` > > Under some circumstances, when a `TableRow` is reused (e.g. index 60 -> 1) > the item can be the same > -> `oldIndex != newIndex && oldItem == newItem` > This can happen when the items of a `TableView` are changed, but some items > are the same in both item lists (Think about a filter, where we filter down > 60 to 2 items). > > -> In this scenario, the cell update is not triggered, so the underlying > `TableCell` indices will not be updated ever (e.g. they still have index 60 > set, but the row has 1 now). > > The fix is to always update the underlying `TableCell` indices when the > `TableRow` index changed. > While usually the item is different when the index changed, this is not > always the case (there is no guarantee that the item changed, as we can see > in the example, where the cell is reused). > > --- > > Also made sure that the issues linked in the code and ticket do not regress: > - https://bugs.openjdk.org/browse/JDK-8095357 > - https://bugs.openjdk.org/browse/JDK-8115269 tested with SCCE found in the ticket on macOS M1 14.7, also some limited testing with the monkey tester, no issues found. looks good; left some very minor formatting suggestions - will re-approve should you decide to fix. thanks for writing the tests! 1 reviewer should be enough. modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewRowTest.java line 222: > 220: row.updateIndex(0); > 221: > 222: List<TableCell<String, String>> cells = > row.getChildrenUnmodifiable().stream(). minor suggestion: maybe reformat to keep one statement per line row.getChildrenUnmodifiable().stream(). filter(TableCell.class::isInstance). map(e -> (TableCell<String, String>) e). toList(); modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewTest.java line 6335: > 6333: @Test > 6334: void testReSetItemsWithSameItemShouldUpdateCellIndices() { > 6335: table.setFixedCellSize(24); fixed size is not essential to the test, right? I've tested with the SCCE in the ticket, it has no effect. it's probably ok to leave it here though. ------------- Marked as reviewed by angorya (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1635#pullrequestreview-2434075916 PR Review Comment: https://git.openjdk.org/jfx/pull/1635#discussion_r1840949234 PR Review Comment: https://git.openjdk.org/jfx/pull/1635#discussion_r1840951836
