On Fri, 28 Apr 2023 16:47:24 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> Monkey Tester - a JavaFX application designed to support manual ad-hoc >> testing of individual JavaFX controls. Unlike Ensemble, the goal of this >> application is to facilitate manual testing rather than demonstrate the >> capabilities of JavaFX. >> >> Feedback and suggestions are always welcome. >> >>  > > Andy Goryachev has updated the pull request incrementally with one additional > commit since the last revision: > > review comments tests/manual/monkey/README.md line 10: > 8: ## Prerequisites > 9: > 10: JavaFX SDK is required to build the tool. The latest SDK can be found > here: Since this is largely a developer tool, I think it might be worth adding something like "You can use a JavaFX SDK that you build or you can download the JavaFX SDK. The latest SDK ..." tests/manual/monkey/README.md line 38: > 36: Applications stores the user preferences (window position, currently > selected page, etc.) in `~/.MonkeyTester` directory. > 37: > 38: To use a different directory, redefine the `user.home` system property, > `-Duser.home=<DIR>`. I presume this is mainly so you can run multiple MonkeyTester instances without having them fight over the preferences, right? Maybe add a sentence to that effect? Possible future RFE: This might have other implications (e.g., it will change the location of the default WebView user data directory). Do you think it would it be better to have a custom property that, when set, will be used instead of asking users to change `user.home`? This could be done later if you think it's worth doing. tests/manual/monkey/README.md line 40: > 38: To use a different directory, redefine the `user.home` system property, > `-Duser.home=<DIR>`. > 39: > 40: To disable saving, specify `-Ddisable.settings=true` VM agrument. This also disables loading of settings (as expected), so maybe say "disable loading and saving"? tests/manual/monkey/build.xml line 97: > 95: > 96: <!-- build all --> > 97: <target name="build-all" depends="compile, copy-resources, make-jar, > sha-jar, copy-jar" /> Suggestion: Add a `jar` target that depends on `build-all` (so it can be used as an alias, since most of our apps use that). tests/manual/monkey/src/com/oracle/tools/fx/monkey/pages/AccordionPage.java line 35: > 33: > 34: /** > 35: * Maybe add something here? tests/manual/monkey/src/com/oracle/tools/fx/monkey/pages/ChartPage.java line 49: > 47: */ > 48: public class ChartPage extends TestPaneBase { > 49: public enum Mode { Possible future RFE: Add Pie chart? tests/manual/monkey/src/com/oracle/tools/fx/monkey/pages/DemoPage.java line 31: > 29: > 30: /** > 31: * Maybe add a comment here? tests/manual/monkey/src/com/oracle/tools/fx/monkey/pages/DualFocusPage.java line 1: > 1: // Copyright © 2018-2019 Andy Goryachev <a...@goryachev.com> You need to replace this with a standard copyright header. tests/manual/monkey/src/com/oracle/tools/fx/monkey/settings/FxSettings.java line 43: > 41: /** > 42: * This facility coordinates saving UI settings to and from persistent > media. > 43: * All the calls, excepr useProvider(), are expected to happen in an FX > application thread. Minor typo: "excepr" --> "except" tests/manual/monkey/src/com/oracle/tools/fx/monkey/settings/FxSettingsFileProvider.java line 67: > 65: } finally { > 66: rd.close(); > 67: } Suggestion: Consider try-with-resources instead of try...finally tests/manual/monkey/src/com/oracle/tools/fx/monkey/settings/FxSettingsFileProvider.java line 84: > 82: } finally { > 83: wr.close(); > 84: } Same suggestion here (try-with-resources) tests/manual/monkey/src/com/oracle/tools/fx/monkey/util/FX.java line 46: > 44: > 45: /** > 46: * Shortcuts and convenience methods that should be a part of JavaFX. It's a debatable point that they "should be" part of JavaFX. I recommend changing it to: "perhaps could be added to JavaFX" tests/manual/monkey/src/com/oracle/tools/fx/monkey/util/FX.java line 155: > 153: public static Background background(Color c) { > 154: return new Background(new BackgroundFill(c, null, null)); > 155: } Btw, we already have a utility method that does this. See [Background::fill](https://github.com/openjdk/jfx/blob/jfx20/modules/javafx.graphics/src/main/java/javafx/scene/layout/Background.java#L359). tests/manual/monkey/src/com/oracle/tools/fx/monkey/util/OptionPane.java line 43: > 41: public OptionPane() { > 42: // no such thing > 43: // > https://stackoverflow.com/questions/20454021/how-to-set-padding-between-columns-of-a-javafx-gridpane I'd prefer we didn't reference stackoverflow. tests/manual/monkey/src/module-info.java line 1: > 1: module monkey_tester { This file needs a standard copyright header. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180788354 PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180787264 PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180788771 PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180789785 PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180792641 PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180802943 PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180811048 PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180811546 PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180813483 PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180817616 PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180817807 PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180821129 PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180824373 PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180826474 PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180819728
