RFR: JDK-8323706 Move SimpleSelector and CompoundSelector to internal packages

John Hendrikx jhendrikx at openjdk.org
Fri Jan 19 09:57:38 UTC 2024


On Thu, 18 Jan 2024 23:55:56 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> Moves `SimpleSelector` and `CompoundSelector` to internal packages.
>> 
>> This can be done with only a minor API break, as `SimpleSelector` and `CompoundSelector` were public before.  However, these classes could not be constructed by 3rd parties.  The only way to access them was by doing a cast (generally they're accessed via `Selector` not by their sub types).  The reason they were public at all was because the CSS engine needs to be able to access them from internal packages.
>> 
>> This change fixes a mistake (or possibly something that couldn't be modelled at the time) when the CSS API was first made public. The intention was always to have a `Selector` interface/abstract class, with private implementations (`SimpleSelector` and `CompoundSelector`).
>> 
>> This PR as said has a small API break.  The other changes are (AFAICS) source and binary compatible:
>> 
>> - Made `Selector` `sealed` only permitting `SimpleSelector` and `CompoundSelector` -- as `Selector` had a package private constructor, there are no concerns with pre-existing subclasses
>> - `Selector` has a few more methods that are now `protected` -- given that the class is now sealed, these modified methods are not accessible (they may still require rudimentary documentation I suppose)
>> - `Selector` now has a `public` default constructor -- as the class is sealed, it is inaccessible
>> - `SimpleSelector` and `CompoundSelector` have a few more `public` methods, but they're internal now, so it is irrelevant
>> - `createMatch` was implemented directly in `Selector` to avoid having to expose package private fields in `Match` for use by `CompoundSelector`
>> - No need anymore for the `SimpleSelectorShim`
>
> modules/javafx.graphics/src/main/java/javafx/css/Selector.java line 46:
> 
>> 44:  * @since 9
>> 45:  */
>> 46: public abstract sealed class Selector permits SimpleSelector, CompoundSelector {
> 
> This seems like a reasonable use of sealed. In order to move the only two implementations of this abstract class out of this package, the constructor must be made public (or protected, which for an abstract class has the same semantics). By making it sealed, you don't then open it up to extending by arbitrary classes.
> 
> However, it is an anti-pattern to have an implicitly-defined constructor -- see [JDK-8250558](https://bugs.openjdk.org/browse/JDK-8250558) -- so you will need to add an explicit constructor with an `@since 23`. I recommend making it protected to reinforce that this class is not to be instantiated directly.

Clear, I'll add it.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1458692961


More information about the openjfx-dev mailing list