RFR: 8185887: TableRowSkinBase fails to correctly virtualize cells in horizontal direction [v4]

[Marius Hanl]

On Thu, 16 Jan 2025 07:18:07 GMT, Johan Vos <jvos at openjdk.org> wrote:

>> 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
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewVirtualizationTest.java line 177:
> 
>> 175:         tableView.getColumns().addFirst(tableColumn);
>> 176: 
>> 177:         // Needs a double pulse so that the viewport breadth is correctly calculated.
> 
> 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)?

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!)

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1644#discussion_r1917939496