On Tue, 22 Aug 2023 06:03:01 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> 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.
> */
Shouldn't it be `ApplicationPropertiesProvider` then, since multiple properties
are involved?
> 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();
personal preference: prefer not to think when I see it in the "Declaration"
window in Eclipse.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1215#discussion_r1301825295
PR Review Comment: https://git.openjdk.org/jfx/pull/1215#discussion_r1301838535
