RFR: 8277122: SplitPane divider drag can hang the layout [v2]

[Jeanette Winzenburg]

On Mon, 22 Nov 2021 14:03:56 GMT, Marius Hanl <mhanl at openjdk.org> wrote:

>> When a divider is moved via drag or code it will call **requestLayout()** for the **SplitPane**.
>> While this is fine, it is also called when the **SplitPaneSkin#layoutChildren(..)** method is repositioning the divider.
>> 
>> This makes no sense since we are currently layouting everything, so we don't need to request it again. (The divider positioning is the first part of **layoutChildren(..)**. In the second part the SplitPane content is layouted based off those positions)
>> 
>> -> With this behaviour the layout may hang under some conditions (check attached source). The problem is that the **requestLayout()** will mark the **SplitPane** dirty but won't propagate to the parent since the **SplitPane** is currently doing a layout.
>> 
>> This PR fixes this by not requesting layout when we are currently doing it (and thus repositioning the dividers as part of it).
>> 
>> Note: I also fixed a simple typo of a private method in SplitPaneSkin:
>> initializeDivderEventHandlers -> initializeDiv**i**derEventHandlers
>
> Marius Hanl has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8277122: Added test for setting a negative divider position + improved readability

fix looks good (though it _feels_ a bit upside down that it is needed ;) - verified example/tests failing/passing before/after fixing

left minor inline comments (just to make it easier to understand :)

modules/javafx.controls/src/main/java/javafx/scene/control/skin/SplitPaneSkin.java line 72:

> 70:      * {@link #layoutChildren(double, double, double, double)} since we are currently doing the layout.
> 71:      */
> 72:     private boolean duringLayout;

would like a reference to the bug this fixes

modules/javafx.controls/src/main/java/javafx/scene/control/skin/SplitPaneSkin.java line 226:

> 224:         // If the window is less than the min size we want to resize proportionally
> 225:         duringLayout = true;
> 226:         double minSize = totalMinSize();

- setting the flag belongs above the code comment to not disrupt explanation and its target (== minsize)
- I think we don't do formatting (here: change the code comment to a single line)

modules/javafx.controls/src/test/java/test/javafx/scene/control/SplitPaneTest.java line 1344:

> 1342:      * which can hang the layout as it resulted in multiple layout requests (through SplitPaneSkin.layoutChildren).
> 1343:      */
> 1344:     @Test

My preference would be to add the bug id to the tests as well ..

modules/javafx.controls/src/test/java/test/javafx/scene/control/SplitPaneTest.java line 1387:

> 1385:         assertTrue(layoutCounter.get() > 0);
> 1386:         stageLoader.dispose();
> 1387:     }

the stageLoader is not disposed if the test fails - a (recently introduced :) general pattern is to use an instance field that's disposed in the test cleanup

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

Changes requested by fastegal (Reviewer).

PR: https://git.openjdk.java.net/jfx/pull/669