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
modules/javafx.graphics/src/test/java/test/javafx/scene/ParentTest.java line
646:
> 644: assertEquals(LayoutFlags.CLEAN,
> ParentShim.getLayoutFlag(level2));
> 645: assertEquals(LayoutFlags.CLEAN, ParentShim.getLayoutFlag(leaf));
> 646: assertEquals(LayoutFlags.CLEAN,
> ParentShim.getLayoutFlag(sibling));
Excellent test, thank you!
Question:
When a situation like this happens in reality, we might see a momentary flicker
due to the layout spanning several pulses, correct?
The platform itself can't really do anything, except for the application code
to call the layout() explicitly to avoid flicker, right?
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1879#discussion_r2395101793
