From 919f5db803fdb7dc15b8b2d41da969745765cc58 Mon Sep 17 00:00:00 2001 From: Marius Hanl Date: Mon, 15 Nov 2021 14:41:14 +0100 Subject: [PATCH 1/4] 8277122: SplitPane divider drag can hang the layout --- .../scene/control/skin/SplitPaneSkin.java | 21 ++++++--- .../javafx/scene/control/SplitPaneTest.java | 46 ++++++++++++++++++- 2 files changed, 60 insertions(+), 7 deletions(-) diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/SplitPaneSkin.java b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/SplitPaneSkin.java index 4f935e979a9..70e506f3592 100644 --- a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/SplitPaneSkin.java +++ b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/SplitPaneSkin.java @@ -36,7 +36,6 @@ import javafx.geometry.VPos; import javafx.scene.Cursor; import javafx.scene.Node; -import javafx.scene.control.Accordion; import javafx.scene.control.Control; import javafx.scene.control.SkinBase; import javafx.scene.control.SplitPane; @@ -65,6 +64,12 @@ public class SplitPaneSkin extends SkinBase { private ObservableList contentRegions; private ObservableList contentDividers; private boolean horizontal; + /** + * Flag which is set to true during {@link #layoutChildren(double, double, double, double)} + * and to false after. It is used to determine whether we need to request layout when a + * divider position changed or not. + */ + private boolean duringLayout; @@ -216,8 +221,8 @@ public SplitPaneSkin(final SplitPane control) { previousSize = horizontal ? sw : sh; } - // If the window is less than the min size we want to resize - // proportionally + // If the window is less than the min size we want to resize proportionally + duringLayout = true; double minSize = totalMinSize(); if (minSize > (horizontal ? w : h)) { double percentage = 0; @@ -235,6 +240,7 @@ public SplitPaneSkin(final SplitPane control) { setupContentAndDividerForLayout(); layoutDividersAndContent(w, h); resize = false; + duringLayout = false; return; } @@ -411,6 +417,7 @@ public SplitPaneSkin(final SplitPane control) { } layoutDividersAndContent(w, h); + duringLayout = false; resize = false; } @@ -617,7 +624,7 @@ private void addDivider(SplitPane.Divider d) { ChangeListener posPropertyListener = new PosPropertyListener(c); c.setPosPropertyListener(posPropertyListener); d.positionProperty().addListener(posPropertyListener); - initializeDivderEventHandlers(c); + initializeDividerEventHandlers(c); contentDividers.add(c); getChildren().add(c); } @@ -633,7 +640,7 @@ private void removeAllDividers() { lastDividerUpdate = 0; } - private void initializeDivderEventHandlers(final ContentDivider divider) { + private void initializeDividerEventHandlers(final ContentDivider divider) { // TODO: do we need to consume all mouse events? // they only bubble to the skin which consumes them by default divider.addEventHandler(MouseEvent.ANY, event -> { @@ -954,7 +961,9 @@ public PosPropertyListener(ContentDivider divider) { // When checking is enforced, we know that the position was set explicitly divider.posExplicit = true; } - getSkinnable().requestLayout(); + if (!duringLayout) { + getSkinnable().requestLayout(); + } } } diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/SplitPaneTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/SplitPaneTest.java index d47981b49ab..7fcbd31b5f4 100644 --- a/modules/javafx.controls/src/test/java/test/javafx/scene/control/SplitPaneTest.java +++ b/modules/javafx.controls/src/test/java/test/javafx/scene/control/SplitPaneTest.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2010, 2020, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2010, 2021, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -25,6 +25,10 @@ package test.javafx.scene.control; +import javafx.collections.FXCollections; +import javafx.scene.control.ComboBox; +import javafx.scene.control.Tab; +import javafx.scene.control.TabPane; import test.com.sun.javafx.scene.control.infrastructure.StageLoader; import javafx.css.CssMetaData; import static test.com.sun.javafx.scene.control.infrastructure.ControlTestUtils.*; @@ -53,6 +57,8 @@ import org.junit.Before; import org.junit.Test; +import java.util.concurrent.atomic.AtomicInteger; + /** * * @author srikalyc @@ -1329,4 +1335,42 @@ private double convertDividerPostionToAbsolutePostion(double pos, double edge) { sl.dispose(); } + + /** + * Verifies that a divider position change of the {@link SplitPane} does not hang the layout. + * Previously, this may happen when the divider position changed too a large number (>1), + * which can hang the layout when done during layout (in layoutChildren). + */ + @Test + public void testSplitPaneDividerChangeDoesNotHangLayout() { + AtomicInteger counter = new AtomicInteger(); + ComboBox cbx = new ComboBox<>(FXCollections.observableArrayList("1", "2", "3")) { + @Override + protected void layoutChildren() { + counter.incrementAndGet(); + super.layoutChildren(); + } + }; + SplitPane pane = new SplitPane(new Label("AAAAA"), new TabPane(new Tab("Test", cbx))); + StackPane root = new StackPane(pane); + + StageLoader stageLoader = new StageLoader(root); + + Toolkit.getToolkit().firePulse(); + + // Set a too big divider position. + pane.setDividerPosition(0, 12); + + Toolkit.getToolkit().firePulse(); + + // Reset counter + counter.set(0); + + cbx.getSelectionModel().select(0); + Toolkit.getToolkit().firePulse(); + + assertTrue(counter.get() > 0); + stageLoader.dispose(); + } + } From 9db28ff00194f49a628208301c7ae6c47d6b3f36 Mon Sep 17 00:00:00 2001 From: Marius Hanl Date: Mon, 22 Nov 2021 14:51:28 +0100 Subject: [PATCH 2/4] 8277122: Added test for setting a negative divider position + improved readability --- .../scene/control/skin/SplitPaneSkin.java | 6 ++-- .../javafx/scene/control/SplitPaneTest.java | 33 +++++++++++++------ 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/SplitPaneSkin.java b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/SplitPaneSkin.java index 70e506f3592..e6d9aa69279 100644 --- a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/SplitPaneSkin.java +++ b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/SplitPaneSkin.java @@ -65,9 +65,9 @@ public class SplitPaneSkin extends SkinBase { private ObservableList contentDividers; private boolean horizontal; /** - * Flag which is set to true during {@link #layoutChildren(double, double, double, double)} - * and to false after. It is used to determine whether we need to request layout when a - * divider position changed or not. + * Flag which is used to determine whether we need to request layout when a divider position changed or not. + * E.g. We don't want to request layout when we are changing the divider position in + * {@link #layoutChildren(double, double, double, double)} since we are currently doing the layout. */ private boolean duringLayout; diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/SplitPaneTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/SplitPaneTest.java index 7fcbd31b5f4..481a8c8d172 100644 --- a/modules/javafx.controls/src/test/java/test/javafx/scene/control/SplitPaneTest.java +++ b/modules/javafx.controls/src/test/java/test/javafx/scene/control/SplitPaneTest.java @@ -1338,16 +1338,30 @@ private double convertDividerPostionToAbsolutePostion(double pos, double edge) { /** * Verifies that a divider position change of the {@link SplitPane} does not hang the layout. - * Previously, this may happen when the divider position changed too a large number (>1), - * which can hang the layout when done during layout (in layoutChildren). + * Previously, this may happen when the divider position changed to a large number (>1), + * which can hang the layout as it resulted in multiple layout requests (through SplitPaneSkin.layoutChildren). */ @Test - public void testSplitPaneDividerChangeDoesNotHangLayout() { - AtomicInteger counter = new AtomicInteger(); + public void testDividerOverOneDoesNotHangLayout() { + testSetDividerPositionDoesNotHangLayout(10); + } + + /** + * Verifies that a divider position change of the {@link SplitPane} does not hang the layout. + * Previously, this may happen when the divider position changed to a negative number (<1), + * which can hang the layout as it resulted in multiple layout requests (through SplitPaneSkin.layoutChildren). + */ + @Test + public void testDividerUnderZeroDoesNotHangLayout() { + testSetDividerPositionDoesNotHangLayout(-1); + } + + private void testSetDividerPositionDoesNotHangLayout(double dividerPosition) { + AtomicInteger layoutCounter = new AtomicInteger(); ComboBox cbx = new ComboBox<>(FXCollections.observableArrayList("1", "2", "3")) { @Override protected void layoutChildren() { - counter.incrementAndGet(); + layoutCounter.incrementAndGet(); super.layoutChildren(); } }; @@ -1358,18 +1372,17 @@ protected void layoutChildren() { Toolkit.getToolkit().firePulse(); - // Set a too big divider position. - pane.setDividerPosition(0, 12); + pane.setDividerPosition(0, dividerPosition); Toolkit.getToolkit().firePulse(); - // Reset counter - counter.set(0); + // Reset layout counter + layoutCounter.set(0); cbx.getSelectionModel().select(0); Toolkit.getToolkit().firePulse(); - assertTrue(counter.get() > 0); + assertTrue(layoutCounter.get() > 0); stageLoader.dispose(); } From 345552d4c52cf4755c95d490e68478ca681582f1 Mon Sep 17 00:00:00 2001 From: Marius Hanl Date: Wed, 26 Jan 2022 21:01:51 +0100 Subject: [PATCH 3/4] 8277122: Added bug id to the fix and test for future reference --- .../src/main/java/javafx/scene/control/skin/SplitPaneSkin.java | 3 ++- .../src/test/java/test/javafx/scene/control/SplitPaneTest.java | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/SplitPaneSkin.java b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/SplitPaneSkin.java index e6d9aa69279..e49618270f5 100644 --- a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/SplitPaneSkin.java +++ b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/SplitPaneSkin.java @@ -68,6 +68,7 @@ public class SplitPaneSkin extends SkinBase { * Flag which is used to determine whether we need to request layout when a divider position changed or not. * E.g. We don't want to request layout when we are changing the divider position in * {@link #layoutChildren(double, double, double, double)} since we are currently doing the layout. + * See also: JDK-8277122 */ private boolean duringLayout; @@ -221,8 +222,8 @@ public SplitPaneSkin(final SplitPane control) { previousSize = horizontal ? sw : sh; } - // If the window is less than the min size we want to resize proportionally duringLayout = true; + // If the window is less than the min size we want to resize proportionally double minSize = totalMinSize(); if (minSize > (horizontal ? w : h)) { double percentage = 0; diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/SplitPaneTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/SplitPaneTest.java index 481a8c8d172..e4840e57a54 100644 --- a/modules/javafx.controls/src/test/java/test/javafx/scene/control/SplitPaneTest.java +++ b/modules/javafx.controls/src/test/java/test/javafx/scene/control/SplitPaneTest.java @@ -1340,6 +1340,7 @@ private double convertDividerPostionToAbsolutePostion(double pos, double edge) { * Verifies that a divider position change of the {@link SplitPane} does not hang the layout. * Previously, this may happen when the divider position changed to a large number (>1), * which can hang the layout as it resulted in multiple layout requests (through SplitPaneSkin.layoutChildren). + * See also: JDK-8277122 */ @Test public void testDividerOverOneDoesNotHangLayout() { @@ -1350,6 +1351,7 @@ public void testDividerOverOneDoesNotHangLayout() { * Verifies that a divider position change of the {@link SplitPane} does not hang the layout. * Previously, this may happen when the divider position changed to a negative number (<1), * which can hang the layout as it resulted in multiple layout requests (through SplitPaneSkin.layoutChildren). + * See also: JDK-8277122 */ @Test public void testDividerUnderZeroDoesNotHangLayout() { From a5f9f5002bbd022783bd5d360d40fcfa56e1f48a Mon Sep 17 00:00:00 2001 From: Marius Hanl Date: Thu, 27 Jan 2022 21:42:32 +0100 Subject: [PATCH 4/4] 8277122: Added and used global stageLoader + changed copyright year to 2022 --- .../javafx/scene/control/skin/SplitPaneSkin.java | 2 +- .../test/javafx/scene/control/SplitPaneTest.java | 12 +++++++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/SplitPaneSkin.java b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/SplitPaneSkin.java index e49618270f5..a5f6bbf0a5f 100644 --- a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/SplitPaneSkin.java +++ b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/SplitPaneSkin.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2010, 2021, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2010, 2022, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/SplitPaneTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/SplitPaneTest.java index e4840e57a54..28ed3e83530 100644 --- a/modules/javafx.controls/src/test/java/test/javafx/scene/control/SplitPaneTest.java +++ b/modules/javafx.controls/src/test/java/test/javafx/scene/control/SplitPaneTest.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2010, 2021, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2010, 2022, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -29,6 +29,7 @@ import javafx.scene.control.ComboBox; import javafx.scene.control.Tab; import javafx.scene.control.TabPane; +import org.junit.After; import test.com.sun.javafx.scene.control.infrastructure.StageLoader; import javafx.css.CssMetaData; import static test.com.sun.javafx.scene.control.infrastructure.ControlTestUtils.*; @@ -71,6 +72,7 @@ public class SplitPaneTest { private Scene scene; private Stage stage; private StackPane root; + private StageLoader stageLoader; @Before public void setup() { tk = (StubToolkit)Toolkit.getToolkit();//This step is not needed (Just to make sure StubToolkit is loaded into VM) @@ -85,6 +87,11 @@ public class SplitPaneTest { stage.setScene(scene); } + @After + public void cleanup() { + if (stageLoader != null) stageLoader.dispose(); + } + /********************************************************************* * Helper methods (NOTE TESTS) * ********************************************************************/ @@ -1370,7 +1377,7 @@ protected void layoutChildren() { SplitPane pane = new SplitPane(new Label("AAAAA"), new TabPane(new Tab("Test", cbx))); StackPane root = new StackPane(pane); - StageLoader stageLoader = new StageLoader(root); + stageLoader = new StageLoader(root); Toolkit.getToolkit().firePulse(); @@ -1385,7 +1392,6 @@ protected void layoutChildren() { Toolkit.getToolkit().firePulse(); assertTrue(layoutCounter.get() > 0); - stageLoader.dispose(); } }