[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8313650: Add hasProperties method to MenuItem and Toggle [v3]
From:       Andy Goryachev <angorya () openjdk ! org>
Date:       2023-08-22 15:56:48
Message-ID: lIprIS17T1x6lbbjeT9WR7XCRLFuZCzroocNRCPUQsI=.7011e2a1-bc29-45ed-8d8d-62c5be226a43 () github ! com
[Download RAW message or body]

On Tue, 22 Aug 2023 06:03:01 GMT, John Hendrikx <jhendrikx@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


[prev in list] [next in list] [prev in thread] [next in thread]