On Fri, 5 Sep 2025 15:55:37 GMT, Kevin Rushforth <[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...
>
> I recommend removing `(public api)` from the title of the JBS bug and this PR
> -- we don't typically do that and it seems superfluous here.
@kevinrushforth I think I managed a suitable unit test. It fails without the
fix, and passes with the fix.
-------------
PR Comment: https://git.openjdk.org/jfx/pull/1879#issuecomment-3355326869
