On Sat, 15 Feb 2025 00:08:22 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
> ## Added Missing Controls > > ButtonBar > ProgressIndicator > Separator > Slider, > > ## Added Node Classes > > AnchorPane > AmbientLight > Arc > BorderPane > Box > Circle > CubicCurve > Cylinder > DialogPane > DirectionalLight > Ellipse > FlowPane > GridPane > Group > HBox > ImageView > Line > MediaView > MeshView > Pane > ParallelCamera > Path > PerspectiveCamera > PointLight > Polygon > Polyline > QuadCurve > Rectangle > Region > Sphere > StackPane > SVGPath > TilePane > VBox > > > ## Miscellaneous > > - minor improvements > - remove tests that execute show() in non-fx threads per > [JDK-8350048](https://bugs.openjdk.org/browse/JDK-8350048) > > > > ## Not Included Due to Threading Limitations > > HTMLEditor > MenuBar > SwingNode > WebView > > ## Note to the Reviewers > > To avoid merge conflicts, the preferred order of integrations: > > #1697 > #1713 > #1717 The newly added tests look good, as do the modified tests to eliminate calling show / hide from a background thread. One more test that would be useful is one that verifies that Scene can be constructed and modified (root node changed and descendants of the root node modified / added / removed) on a background thread. I left a few questions / comments inline as well. tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 224: > 222: * Also, the visible node gets accessed periodically in the FX > application thread just to shake things up. > 223: */ > 224: @TestMethodOrder(MethodOrderer.MethodName.class) Why is this needed? Our tests use the JUnit5 default (which is random) without a compelling reason to do otherwise. I recommend removing it. tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 227: > 225: public class NodeInitializationStressTest extends RobotTestBase { > 226: /* debugging aid: set this flag to true and comment out > assumeFalse(SKIP_TEST) to run specific test(s). */ > 227: private static final boolean SKIP_TEST = false; Maybe it's time to remove this flag? Does Eclipse not provide a way to run a single test? tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 328: > 326: c.setCenter(n); > 327: BorderPane.setAlignment(n, nextEnum(Pos.class)); > 328: accessNode(c); Should this call `accessRegion` instead? tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 656: > 654: > 655: @Test > 656: public void imageView() { You mentioned that there was a problem with modifying a writable image on a background thread. Similarly, there might be a problem loading animated GIF images on a background thread. Is there a bug filed? tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 737: > 735: c.setFitHeight(nextDouble(200)); > 736: c.setFitWidth(nextDouble(200)); > 737: //c.setMediaPlayer(new MediaPlayer(new > Media("no-data-url-support"))); Would it be useful to load a media file? If so, you could use the default media file that Ensemble uses. tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 1124: > 1122: } > 1123: > 1124: //@Test disabled because it times out Please file a bug and add an `@Disabled` annotation using that bug ID. tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 1388: > 1386: > 1387: private static void accessPane(Pane p, Consumer<Node> onChild) { > 1388: ObservableList<Node> children = p.getChildren(); It might be useful for this method to call `accessRegion` (as is done from `accessControl`). tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 1423: > 1421: c.setDrawMode(nextEnum(DrawMode.class)); > 1422: PhongMaterial m = new PhongMaterial(); > 1423: //m.setSelfIlluminationMap(Image); // what is expected? There is a good description of the material properties and how they work, complete with illustrations, in the `PhongMaterial` class docs. tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 1426: > 1424: m.setSpecularColor(nextColor()); > 1425: //m.setSpecularMap(Image); // what is expected? > 1426: m.setSpecularPower(nextDouble(100)); // what is the acceptable > range? There is technically no upper limit, but 1.0 - 200.0 is a good range (so maybe add 1.0 to the value returned by `nextDouble`). tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 1671: > 1669: > 1670: private static PathElement createPathElement() { > 1671: switch (nextInt(7)) { Minor: fix Indentation tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 1755: > 1753: > 1754: private static String createSvgPath() { > 1755: switch(nextInt(4)) { Minor: space after switch ------------- PR Review: https://git.openjdk.org/jfx/pull/1713#pullrequestreview-2641843482 PR Review Comment: https://git.openjdk.org/jfx/pull/1713#discussion_r1970196814 PR Review Comment: https://git.openjdk.org/jfx/pull/1713#discussion_r1970198168 PR Review Comment: https://git.openjdk.org/jfx/pull/1713#discussion_r1970258183 PR Review Comment: https://git.openjdk.org/jfx/pull/1713#discussion_r1970231968 PR Review Comment: https://git.openjdk.org/jfx/pull/1713#discussion_r1970236555 PR Review Comment: https://git.openjdk.org/jfx/pull/1713#discussion_r1970267812 PR Review Comment: https://git.openjdk.org/jfx/pull/1713#discussion_r1970271244 PR Review Comment: https://git.openjdk.org/jfx/pull/1713#discussion_r1970276883 PR Review Comment: https://git.openjdk.org/jfx/pull/1713#discussion_r1970277986 PR Review Comment: https://git.openjdk.org/jfx/pull/1713#discussion_r1970288827 PR Review Comment: https://git.openjdk.org/jfx/pull/1713#discussion_r1970292234
