8185887: TableRowSkinBase fails to correctly virtualize cells in horizontal direction #1644
Conversation
|
👋 Welcome back mhanl! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@Maran23 |
Maran23
force-pushed
the
8185887-virtualization
branch
from
62f46c6 to
e811572
Compare
|
@Maran23 Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
|
/reviewers 2 |
|
@andy-goryachev-oracle |
|
We will need to check that this doesn't impact accessibility, since there are some "interesting" cases where cells are requested and a layout pass is done even when they aren't visible. @arapte As I recall, you were looking into VirtualFlow in connection with an a11y at one point. Can you test this? |
|
@johanvos You may want to take a look since this touches VirtualFlow. |
|
/issue add JDK-8252566 same issue as https://bugs.openjdk.org/browse/JDK-8276326, but for the |
|
@Maran23 |
Good point! Checking |
I tested the PR changes with a few a11y scenarios, and did not observe any issues. |
|
there seems to be a merge conflict, please sync up with the latest |
There was a problem hiding this comment.
Testing on macOS 14.7.1 with the latest Monkey Tester (where I've added 200 columns case for Tree/TableView,
https://github.com/andy-goryachev-oracle/MonkeyTest )
looks good.
It feels like the horizontal scrolling has improved a bit, the vertical exhibits the same slight hiccups as with the master branch.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java
Show resolved
Hide resolved
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java
Outdated
Show resolved
Hide resolved
|
I've got an exception on the TreeTableView page in the monkey tester after clicking a cell and then pressing control+command+RIGHT_ARROW with the VoiceOver on: settings: UPD: seems unrelated, the issue can be reproduced with the master branch. UPD2: the problem seems to be with the MenuBar. In the monkey tester, press |
Is this reproducible? Does it happen without this fix? If this is a regression then it needs to be fixed. UPDATE: nm, I just read Andy's updated comment. |
|
Created JDK-8235989 for the a11y exception issue. |
Yes, especially if you try with many columns (> 100), scrolling up and down is much faster (especially when scrolled completely to the left). When you scroll completely to the right, it is just a little bit faster than master. I have also some ideas how to improve vertical scrolling with many items, but for another day. :) |
Thank you for testing! |
|
@Maran23 this pull request can not be integrated into git checkout 8185887-virtualization
git fetch https://git.openjdk.org/jfx.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
openjdk
bot
added
the
merge-conflict
|
By the way, a Tree/TableView with 500 columns is barely usable - locks up the UI giving me a macOS spinning beach ball quite often. Not that it's a reasonable use case. 200 is ok. May be the 500 case can be used for profiling? Speaking of the vertical scrolling - I could not use the idea implemented by the VirtualFlow in the RichTextArea as it won't work in the case of very large models. So instead it uses approximation, see |
…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
openjdk
bot
removed
the
merge-conflict
With fixed cell size, it seems okay to me. How is your table configured?
There is a lot of stuff that can improved in the You can for example check AG-Grid, which is a pretty nicely implemented table that is very fast and has a nice API design: We only have the fixed cell size implemented, but it is not the default (and too late to change). This helps a lot since the math is very easy then. |
|
I checked the changes in VirtualFlow and those are ok. In absence of those tests, I believe this PR is a good step in the good direction. |
@johanvos would you approve this PR then? |
…virtualization # Conflicts: # modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java
I only looked into the changes in VirtualFlow, so I can't say anything about the other changes -- and approving a PR means the whole PR has been reviewed. |
|
Thank you for the partial review nevertheless! Every little bit helps... |
|
@jperedadnr can you review? |
|
/issue add JDK-8346824 |
|
@Maran23 |
|
@henrykdz :
|
|
@henrykdz : I've created https://bugs.openjdk.org/browse/JDK-8347464 to track the new issue. The new issue is not related to this PR, so sorry @Maran23 for adding unrelated comments. In the future, it's probably better to file the bug via normal channel described in https://bugs.java.com/bugdatabase/faq or by sending a message to [email protected] |
|
@henrykdz : |
| tableColumn.setPrefWidth(50); | ||
| tableView.getColumns().addFirst(tableColumn); | ||
|
|
||
| // Needs a double pulse so that the viewport breadth is correctly calculated. |
There was a problem hiding this comment.
This worries me a bit (the double pulse happens on a few places in the new tests). I would expect the platform to deal with is, so when a test requires 2 explicit pulse requests, it sounds like an issue.
Why can't this be done with a single request? (and then waiting until the pulse and pending runnables on the FX Thread have finished)?
There was a problem hiding this comment.
Unfortunately I don't think there is any other way. The VirtualFlow needs two pulses (in real life applications) as the first time, the layout is not yet correct for some cases (e.g. for No ScrollBar -> ScrollBar). I even used some watchpoints to confirm this behavior.
You can see the same thing in the VirtualFlowTest.setUp method, which initializes the flow and does two pulse calls after. So I think this is a premature problem, which probably can be fixed, but more refactorings/optimzations are needed (some of which I want to file a PR when I have more time).
Maybe at some point we can completely eliminate this problem, but I don't think I can do that here in this PR (unless you have an idea, which is very much welcome!)
There was a problem hiding this comment.
Maybe the name pulse in the test you point out is misleading, since it actually calls flow.layout(), and two layout passes is not the same as two pulses.
There was a problem hiding this comment.
That is right, but the pulse call will call layout, which will call layoutChildren (when needed). I could also just call layout directly, but want to keep it the same way as the actual Application would behave.
There was a problem hiding this comment.
Relying on two pulses may cause flicker (I had a similar issue with the RichTextArea).
The solution is to re-compute the layout in layoutChildren() when needed (i.e. when scroll bar status changes).
There was a problem hiding this comment.
Good to know, we should document your solution in more detail (and with the concrete fix -> code snippet from the RTA) together with the HSB bug as a new ticket probably? Then we may already have a potential solution with code, and of course a ticket that tracks the bug, if we do not have already (I think I saw a similar bug already, but not 100% sure).
There was a problem hiding this comment.
VFlow LL1529-1555
// scroll bars
boolean vsbVisible = useContentHeight ?
false :
(topCellIndex() > 0) ?
true :
(arrangementHeight + contentPaddingTop + contentPaddingBottom) > viewPortHeight;
if (vsbVisible != vscroll.isVisible()) {
vscroll.setVisible(vsbVisible);
// do another layout pass with the scrollbar updated
layoutCells();
return;
}
if (vsbVisible) {
width -= vsbWidth;
}
boolean hsbVisible = (wrap || useContentWidth) ?
false :
(unwrappedWidth + contentPaddingLeft + contentPaddingRight) > viewPortWidth;
if (hscroll.isVisible() != hsbVisible) {
hscroll.setVisible(hsbVisible);
// do another layout pass with the scrollbar updated
layoutCells();
return;
}
There was a problem hiding this comment.
Thanks. That makes sense IMO. Can probably also be applied to the VirtualFlow.
Just need to be careful that we never can run in a stack overflow ofc. :)
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkin.java
Show resolved
Hide resolved
| if (disclosureNodeDirty) { | ||
| updateDisclosureNodeAndGraphic(); | ||
| disclosureNodeDirty = false; | ||
| Node disclosureNode = getDisclosureNode(); |
There was a problem hiding this comment.
I take these changes (related to disclosure node) are not really related to the initial issue of this PR, and I wonder if JDK-8346824 should not be part of this PR. Do the added tests verify these changes and if the issue is solved with them?
There was a problem hiding this comment.
This is more likely an 'accidential' fix. I changed this code because the code in the super class changed (e.g. updateCells and updateChildren). So while I see your point, if possible I would not try to cherrypick this out (if even possible) and keep it in here.
Good point, I will check if I can write a test for it.
There was a problem hiding this comment.
EDIT: The issue is about the TreeView specifically, so this code is not related to the issue (it is in TreeTableRowSkin).
The fix is in the VirtualFlow, which I did for the table virtualization (issues), but that also fixes the issue for the TreeView as well.
There was a problem hiding this comment.
EDIT2: I wrote a test, that fails before and succeeds after for that specific problem!
There was a problem hiding this comment.
I second @jperedadnr concern about combining separate issues into one PR. Small focused PRs will get tested, reviewed, and integrated almost always faster than the big complicated ones.
There was a problem hiding this comment.
Completely agree, but in this case, the fix for JDK-8276326 is also the same for JDK-8346824 - that is the VirtualFlow changes.
It is not really possible to separate those issues.
What I could try is to cherrypick the changes from the VirtualFlow, leaving out the virtualization optimizations. Although this was not the original point here.
The reason those are together is that they are very closely related. Fixing the virtualization without the VirtualFlow will still break applications and tests because you can somewhat randomly get empty cells.
Fixing the VirtualFlow first might work, but if you scroll enough you will get empty cells because of broken virtualization.
So this PR got bigger in order to have a working virtualization and also scrolling behavior with no empty cells.
If it really helps, I can split them up, as mentioned above.
There was a problem hiding this comment.
If it really helps, I can split them up
probably not worth it at this point, I think. We just need to be extra careful during the review to consider all the scenarios involved.
| tableColumn.setPrefWidth(50); | ||
| tableView.getColumns().addFirst(tableColumn); | ||
|
|
||
| // Needs a double pulse so that the viewport breadth is correctly calculated. |
There was a problem hiding this comment.
Maybe the name pulse in the test you point out is misleading, since it actually calls flow.layout(), and two layout passes is not the same as two pulses.
|
Can attest to the fact that two pulses are indeed needed when testing with the monkey tester (unconstrained resize policy, columns with "all constraints", 10 rows; resize to the point where both scrollbars are visible but very close to where the SB are not needed, then increase the height slowly). The good news is that I could not get it to flicker (at least on macOS, with 1:1 scale). I see SB disappears, then another ("two pulses"). Visually, it's the same behavior as with the master branch. The not-so-good news is that there is a configuration when the HSB remains, but once you click on it it disappears (might be hard to reproduce). In both this PR and with the master branch, the HSB remains even though it's not needed:
Nevertheless, I think the behavior is acceptable as the user can easily resize and most importantly there is no continuous flicker. |
When I debugged the double pulse, I found out that the viewport breadth changed a little bit between the first and the second call. Which might also change the HSB. |
I've deleted it from here, not knowing it would cause this, sorry. I guess I can't connect to the old URL again? |
If the video is small enough it can be added to the ticket. Alternatively, you could attach two frame grabs - one with and one without the problem. Otherwise no one can see what you were talking about once the link is gone. |
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
isColumnPartiallyOrFullyVisibleneeds to be fixed to take theVirtualFlowinto account, not theTableView. Since rows are inside theVirtualFlow, we need to use the width from thereTo improve performance,
isColumnPartiallyOrFullyVisiblewas refactored to take in everything that is needed directly, without reiterating through all columnsAs before, the
TableRowadds 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 happens only when a fixed cell size is set (empty rows). This is related and also fixed:
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 cellsFix is to request layout to those cells as well, and as soon as they are reused, they will layout themself
VirtualFlowsheet (as children). This will cause them to actually do the layout when requested, and especially when the height of theTableViewchanged 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, as mentioned aboveThe fix is to remove those cells from the viewport when not needed anymore.
NOTE: The
VirtualFlowdoes 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
VirtualFlowdoes not receive any Mouse or KeyEvents. This can be cleaned up in a follow up PRConsiderations:
removeAll), as the scenario that mostly happens is:Testing:
TreeTableRowcode, which before addedLabeledTextsometimes (fromLabeledSkinBase), although it was not needed, leading to code that need to count that particular node in, in some testsVirtualFlowTestUtils.BLOCK_STAGE_LOADER_DISPOSEwhich was hacky and replaced it with the normalStageLoadermechanism (as we use pretty much everywhere else)/issue add JDK-8276326
Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1644/head:pull/1644$ git checkout pull/1644Update a local copy of the PR:
$ git checkout pull/1644$ git pull https://git.openjdk.org/jfx.git pull/1644/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1644View PR using the GUI difftool:
$ git pr show -t 1644Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1644.diff
Using Webrev
Link to Webrev Comment