On Fri, 22 Nov 2024 16:32:49 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-8276326](https://bugs.openjdk.org... This pull request has now been integrated. Changeset: 1b12c8a4 Author: Marius Hanl <mh...@openjdk.org> URL: https://git.openjdk.org/jfx/commit/1b12c8a490a87691e7e815a7e768854354cb54aa Stats: 1425 lines in 15 files changed: 1245 ins; 142 del; 38 mod 8185887: TableRowSkinBase fails to correctly virtualize cells in horizontal direction 8276326: Empty Cells in TableView supposedly after using setFixedCellSize() 8252566: TreeTableView: broken row layout for fixedCellSize 8346824: Collapsing tree view causes rendering issues Reviewed-by: angorya, mstrauss ------------- PR: https://git.openjdk.org/jfx/pull/1644
