On Wed, 19 Feb 2025 20:39:19 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
> - enforced fx application thread > - added a headful test `TestThreadingRestrictions` > > ## Note to the Reviewers > > To avoid merge conflicts, the preferred order of integrations: > > #1697 > #1713 > #1717 The API doc changes look good, although I noted a couple additional changes that are needed. The API docs for `Dialog::hide` should also be documented to throw ISE (since it calls `close()` directly the implementation is fine) The API docs for `Stage::close` should also be documented to throw ISE (since it calls `hide()` directly the implementation is fine) For `ComboBoxBase`, were you going to get rid of the word "aspect" in the opening sentence "... display the popup aspect of the user interface."? I think that's a little awkward even with that word removed. Maybe it could just be: "... display the popup associated with this control." As for the implementation, There are a few places where the skin will call hide(), and I can't tell by just looking at the code that they are all guaranteed to be on the FX app thread. For example, I recommend checking the following: * `MenuBarSkin` line 799 * `ComboBoxListViewSkin` line 199 There may be others. tests/system/src/test/java/test/robot/javafx/scene/TestThreadingRestrictions.java line 63: > 61: * This test ensures that the threading restrictions are in place where > required. > 62: */ > 63: @TestMethodOrder(MethodOrderer.MethodName.class) This is unnecessary (and not a usual practice). tests/system/src/test/java/test/robot/javafx/scene/TestThreadingRestrictions.java line 239: > 237: test(TPopupWindow::new, (p) -> p.show(stage)); > 238: test(TPopupWindow::new, (p) -> p.show(stage, 0, 0)); > 239: test(TPopupWindow::new, (p) -> p.show(contentPane, 0, 0)); It's probably worth adding a test for `hide` when it isn't showing (like you've done for other objects). tests/system/src/test/java/test/robot/javafx/scene/TestThreadingRestrictions.java line 262: > 260: p.show(); > 261: }); > 262: p.hide(); // do we need to fail early here, or only when > showing? We should fail early (although this comment seems misplaced, since it is the case where it _is_ showing). tests/system/src/test/java/test/robot/javafx/scene/TestThreadingRestrictions.java line 268: > 266: > 267: @Test > 268: @Timeout(value = 1, unit = TimeUnit.DAYS) Um, 1 DAY??? Seriously, though: Remove the timeout (it isn't needed or wanted). ------------- PR Review: https://git.openjdk.org/jfx/pull/1717#pullrequestreview-2642150796 PR Review Comment: https://git.openjdk.org/jfx/pull/1717#discussion_r1970380200 PR Review Comment: https://git.openjdk.org/jfx/pull/1717#discussion_r1970398687 PR Review Comment: https://git.openjdk.org/jfx/pull/1717#discussion_r1970400883 PR Review Comment: https://git.openjdk.org/jfx/pull/1717#discussion_r1970401521
