RFR: 8313650: Add hasProperties method to MenuItem and Toggle [v3]

[John Hendrikx]

On Mon, 21 Aug 2023 15:56:59 GMT, Andy Goryachev <angorya at 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