On Wed, 1 Oct 2025 08:44:14 GMT, John Hendrikx <[email protected]> wrote:
>> Ensures proper propagation of layout flags when using `forceParentLayout =
>> true`.
>>
>> This was the root cause of issue #1874
>>
>> ### Note
>>
>> Apparently it is still quite easy to mess up the layout flags. Basically,
>> the layout flag tracked by Parent should always either be `CLEAN` for any
>> scene graph branch, or `!CLEAN` + a layout pulse is scheduled on the
>> corresponding `Scene`.
>>
>> However, with careful use of the public API `requestLayout` one can get
>> these flags in a bad state still:
>>
>> Let's say I have a branch `A (root node under Scene) -> B -> C`, **and** a
>> layout is in progress, **and** we're currently in the `layoutChildren`
>> method of `C`. The flag `performingLayout` will be `true` for all nodes in
>> the branch `A` -> `B` -> `C`. The `layout` method will set the layout flag
>> to `CLEAN` as its first action, so when we're at `C::layoutChildren`, all
>> flags have been reset to `CLEAN` already. See the `Parent::layout` method
>> for how all this works.
>>
>> Now, to mess up the flags, all you need to do is call `requestLayout` on `B`
>> or `C` from the `layoutChildren` of `C` (or indirectly by changing something
>> and something is listening to this and schedules a layout on something
>> somewhere in this branch); note that `requestLayout` is not documented to be
>> illegal to call during layout, and some classes in FX will do so
>> (ScrollPaneSkin, NumberAxis, etc..) risking the flags getting in a bad
>> state... -- usually you get away with this, as there are many ways that
>> layout is triggered, and eventually the flags will get overwritten and reset
>> to a consistent state.
>>
>> The bad state occurs because this code path is followed (all code from
>> `Parent`):
>>
>> public void requestLayout() {
>> clearSizeCache();
>> markDirtyLayout(false, forceParentLayout);
>> }
>>
>> Calls to `markDirtyLayout(false, false)`:
>>
>> private void markDirtyLayout(boolean local, boolean forceParentLayout) {
>> setLayoutFlag(LayoutFlags.NEEDS_LAYOUT);
>> if (local || layoutRoot) {
>> if (sceneRoot) {
>> Toolkit.getToolkit().requestNextPulse();
>> if (getSubScene() != null) {
>> getSubScene().setDirtyLayout(this);
>> }
>> } else {
>> markDirtyLayoutBranch();
>> }
>> } else {
>> requestParentLayout(forceParentLayout);
>> }
>> }
>>
>> Before going into the `else` (as none of the nodes is a layout root, and
>> `lo...
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add test case
Marked as reviewed by angorya (Reviewer).
modules/javafx.graphics/src/main/java/javafx/scene/Parent.java line 1077:
> 1075: */
> 1076:
> 1077: p.requestLayout(forceParentLayout);
very, very minor suggestion to makes it more compact (it's ok to leave things
as is):
// The forceParentLayout flag must be propagated to mark all
ancestors
// as needing layout. Failure to do so while performingLayout
is true
// would stop the propagation mid-tree. This leaves some nodes
as needing
// layout, while its ancestors are clean, which is an
inconsistent state.
p.requestLayout(forceParentLayout);
-------------
PR Review: https://git.openjdk.org/jfx/pull/1879#pullrequestreview-3290055086
PR Review Comment: https://git.openjdk.org/jfx/pull/1879#discussion_r2395189222
