On Thu, 19 Dec 2024 16:03:02 GMT, Marius Hanl <mh...@openjdk.org> wrote:
>> This PR fixes the horizontal virtualization performed in the >> `TableRowSkinBase`, which significantly improves performance with many >> columns (and thus many cells). Scrolling up and down as well as scrolling >> left and right feels much more snappy. >> >> In order to do that, there are multiple things needed: >> - the `isColumnPartiallyOrFullyVisible` needs to be fixed to take the >> `VirtualFlow` into account, not the `TableView`. Since rows are inside the >> `VirtualFlow`, we need to use the width from there >> - The wrong implementation right now leads to >> [JDK-8276326](https://bugs.openjdk.org/browse/JDK-8276326), where if you >> scroll to the right with many columns, cells on the left start to be empty >> (since they are removed when they shouldn't be) >> - It also does not help performance when scrolled to the left, as the >> right cells are not removed (only when scrolling to the right, cells on the >> left are removed) >> - To improve performance, `isColumnPartiallyOrFullyVisible` was refactored >> to take in everything that is needed directly, without reiterating through >> all columns >> - As before, the `TableRow` adds or removes cells that are visible or not. >> Note that this is only done when a fixed cell size is set. >> The reason for that is that we always know the row height. If not set, we >> need all cells so we can iterate over them to get the max height. I'm not >> sure if this can be improved, but this is not the goal of this PR and can be >> checked later >> - The other issue mentioned in >> [JDK-8276326](https://bugs.openjdk.org/browse/JDK-8276326) happens only when >> a fixed cell size is set (empty rows). This is related and also fixed: >> - Cells start to be empty when scolling around a lot. This is because >> cells that are in the pile (ready to be reused) will not receive any layout >> requests (`requestLayout`) while all cells in the viewport will receive >> them. As soon as they are reused, they are visually outdated and not >> updated, leading to empty cells >> Fix is to request layout to those cells as well, and as soon as they are >> reused, they will layout themself >> - This has revealed another error: Cells that are not used anymore (added >> to the pile and invisible) are STILL inside the `VirtualFlow` sheet (as >> children). This will cause them to actually do the layout when requested, >> and especially when the height of the `TableView` changed drastically (e.g. >> from 50 visible cells to just 10), we have 40 cells laying around, receiving >> the layout request I added to fix [JDK-82763... > > Marius Hanl has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains four commits: > > - Merge branch 'master' of https://github.com/openjdk/jfx into > 8185887-virtualization > > # Conflicts: > # > modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java > - Merge branch 'master' of https://github.com/openjdk/jfx into > 8185887-virtualization > > # Conflicts: > # > modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java > # > modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java > # > modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java > - 8185887: Reset disclosureNodeDirty in updateDisclosureNodeAndGraphic() > - 8185887: TableRowSkinBase fails to correctly virtualize cells in > horizontal direction @henrykdz : could you please attach the video clip to https://bugs.openjdk.org/browse/JDK-8347464 ? ------------- PR Comment: https://git.openjdk.org/jfx/pull/1644#issuecomment-2591129445
