On Mon, 22 Nov 2021 14:03:56 GMT, Marius Hanl <mh...@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
