On Mon, 23 Mar 2026 20:39:38 GMT, Andy Goryachev <[email protected]> wrote:
>> Very similar to https://github.com/openjdk/jfx/pull/1829, this PR removes >> the questionable behavior that the `MouseEventFirer` may create a temporary >> `Stage` for your `Node`. >> >> Take the following test code: >> >> Button button = new Button("Button"); >> MouseEventFirer mouse = new MouseEventFirer(button); >> mouse.fireMousePressAndRelease(); >> mouse.fireMousePressAndRelease(); >> mouse.dispose() >> >> What it does is to create a `Stage` in the first method. The second method >> does not. This is not immediately clear. >> That is also the reason why the dispose method exists. To MAY clean it up. >> Or forget to call it. >> >> This does not test a realistic scenario and the chance is quite high that >> developers used that methods without even knowing that it contains such >> logic. >> >> So the idea is to remove the StageLoader code from MouseEventFirer and >> rather use it in the Test code before calling the Util methods. >> >> For the example above, this would result in: >> >> Button button = new Button("Button"); >> stageLoader = new StageLoader(button ); >> MouseEventFirer mouse = new MouseEventFirer(button); >> mouse.fireMousePressAndRelease(); >> mouse.fireMousePressAndRelease(); >> >> >> There were only two real tests that did not have a `Stage` yet. So most of >> the tests already had a good setup and this could never run. > > modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/infrastructure/MouseEventFirer.java > line 138: > >> 136: >> 137: if (node.getScene() == null || node.getScene().getWindow() == >> null) { >> 138: throw new IllegalArgumentException("Node: " + node + " must >> be in a Scene and Window " + > > maybe instruct to use StageLoader instead, addin an example in the comments? > something like this: > > // The target node must be in a Scene and Window, example: > // private StageLoader sl; > // .. > // sl = new StageLoader(node); > // ... > // sl.dispose() > if (node.getScene() == null || node.getScene().getWindow() == null) { > throw new IllegalArgumentException("Target node must be in a > Scene and Window (use StageLoader?)"); I thought about this as well, but my future plan (sneakpeak :)) is to rather have an abstract class that provides all needed methods, including the `stageLoader` cleanup and exception handler setup. And all controls test extend it. Saves us a lot of duplicate code, and if we need to change something there, we can do it just once. But the biggest benefit why I want to push this is to make it easier to write tests for new contributors, as there is essentially only one way to spin up a stage, fire mouse events, all in one class and used in every test. No ambiguity. And in this case, we would not really need this comment (I want to document everything on the abstract class then). If that seems good for you too, I would like to move on with that one time and rather do not clutter this class (or other test classes, effectively spreading all information around different test classes). ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/2124#discussion_r2977611490
