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 should'nt 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-8276326](https://bugs.openjdk.org/browse/JDK-8276326), as mentioned above The fix is to remove those cells from the viewport when not needed anymore. NOTE: The `VirtualFlow` does a lot of 'clean up everything and decide which cells are visible', so I chose a performant fix (instead of removing all cells and readding them again) NOTE2: This makes the logic to make cells invisible and visible again obsolete. This was there so those unused cells laying around in the `VirtualFlow` does not receive any Mouse or KeyEvents. This can be cleaned up in a follow up PR Considerations: - - I decided to not do more logic like batching all cells that should be removed (`removeAll`), as the scenario that mostly happens is: - You start scrolling to the right, we add and remove cells one by one - Therefore, we mostly do not remove or add multiple cells at once anyway - Other bigger refactoring for performance can be done in a follow up (if we find things, I do not want to blow up this PR) Testing: - - I added many tests to cover all possible scenarios, including: - Reordering a column - Decrease/Increase column width - Adding and removing columns - Scrolling around and checking, that no empty cells are around ([JDK-8276326](https://bugs.openjdk.org/browse/JDK-8276326) - I did lots of testing on my own and also on JavaFX applications. I found no regression - Some tests need to be adjusted since the fix also improves `TreeTableRow` code, which before added `LabeledText` sometimes (from `LabeledSkinBase`), although it was not needed, leading to code that need to count that particular node in, in some tests - Remove ` VirtualFlowTestUtils.BLOCK_STAGE_LOADER_DISPOSE` which was hacky and replaced it with the normal `StageLoader` mechanism (as we use pretty much everywhere else) ------------- Commit messages: - 8185887: TableRowSkinBase fails to correctly virtualize cells in horizontal direction Changes: https://git.openjdk.org/jfx/pull/1644/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1644&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8185887 Stats: 1340 lines in 14 files changed: 1170 ins; 135 del; 35 mod Patch: https://git.openjdk.org/jfx/pull/1644.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1644/head:pull/1644 PR: https://git.openjdk.org/jfx/pull/1644
