On Mon, 21 Aug 2023 15:56:59 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> 1. Creating a new `javafx.scene.ContainsProperties` interface that declares >> two methods: >> - public ObservableMap<Object, Object> getProperties(); >> - public boolean hasProperties(); >> >> 2. Node, MenuItem, and Toggle now extend ContainsProperties interface. >> >> 3. Making implemented methods in Node, MenuItem, and Toggle final. >> >> While this change is an improvement from a design point of view, it >> introduces a larger compatibility risk (by adding 'final' keyword similar to >> [JDK-8313651](https://bugs.openjdk.org/browse/JDK-8313651)) >> >> This change requires CSR. > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains six additional > commits since the last revision: > > - javadoc > - contains properties interface > - Merge remote-tracking branch 'origin/master' into 8313650.properties > - review comments > - test > - has properties modules/javafx.graphics/src/main/java/javafx/scene/ContainsProperties.java line 36: > 34: * @since 22 > 35: */ > 36: public interface ContainsProperties { This name fails the "is-a" test "is a contains properties", like for example "is an `EventTarget`" or "is a `Styleable`". Suggestions: `ApplicationPropertyProvider` `PropertyProvider` `ApplicationPropertyAccessor` `PropertyAccessor` I'd personally go for the most descriptive one (the first one). It's long, but it's unlikely to be ever encountered as a type for a variable. Suggestion for the docs: /* * Provides access to properties for use primarily by application developers. */ modules/javafx.graphics/src/main/java/javafx/scene/ContainsProperties.java line 42: > 40: * @return an observable map of properties on this node for use > primarily by application developers > 41: */ > 42: public ObservableMap<Object, Object> getProperties(); Interface methods are always public, the keyword is just noise. Suggestion: ObservableMap<Object, Object> getProperties(); modules/javafx.graphics/src/main/java/javafx/scene/ContainsProperties.java line 46: > 44: /** > 45: * Tests if this object has properties. > 46: * @return true if this object has properties. Suggestion: * @return {@code true} if this object has properties modules/javafx.graphics/src/main/java/javafx/scene/ContainsProperties.java line 48: > 46: * @return true if this object has properties. > 47: */ > 48: public boolean hasProperties(); Suggestion: boolean hasProperties(); ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1215#discussion_r1301003272 PR Review Comment: https://git.openjdk.org/jfx/pull/1215#discussion_r1301006329 PR Review Comment: https://git.openjdk.org/jfx/pull/1215#discussion_r1301004008 PR Review Comment: https://git.openjdk.org/jfx/pull/1215#discussion_r1301005555
