On Tue, 22 Aug 2023 15:34:17 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> In the Control hierarchy, all property accessor methods must be declared >> `final`. >> >> Added a test to check for missing `final` keyword and added the said keyword >> where required. > > Andy Goryachev has updated the pull request incrementally with one additional > commit since the last revision: > > review comments The fix looks good. So does the test with a couple comments. I also noted a couple potential follow-up items. modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlPropertiesTest.java line 95: > 93: * > 94: * Currently uses a list of classes, so any new Controls must be added > manually. > 95: * Perhaps the test should scan classpath and find all the Controls > automagically. Minor: "classpath" --> "modulepath" More interesting question: Should we do this in other modules (e.g., javafx.graphics), too? If so, that would be for a follow-up issue. modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlPropertiesTest.java line 103: > 101: // list all descendants of Control class. > 102: // or perhaps collect all classes in a package as described here: > 103: // > https://stackoverflow.com/questions/28678026/how-can-i-get-all-class-files-in-a-specific-package-in-java Please remove the reference to stackoverflow. modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlPropertiesTest.java line 212: > 210: private void checkModifiers(Method m) { > 211: int mod = m.getModifiers(); > 212: if (Modifier.isPublic(mod) && !Modifier.isFinal(mod)) { Should we also check `protected` methods? If so, that would be something for a follow-up issue. ------------- PR Review: https://git.openjdk.org/jfx/pull/1213#pullrequestreview-1616319317 PR Review Comment: https://git.openjdk.org/jfx/pull/1213#discussion_r1319172554 PR Review Comment: https://git.openjdk.org/jfx/pull/1213#discussion_r1319177546 PR Review Comment: https://git.openjdk.org/jfx/pull/1213#discussion_r1319181683
