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

[Kevin Rushforth]

On Sun, 14 Jan 2024 14:54:36 GMT, John Hendrikx <jhendrikx 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`

This seems like a reasonable thing to do. As I mentioned above, the first step is to file a new enhancement to deprecate these two classes for removal, with the idea being to get them into jfx22 via a late enhancement request. This will need to be done quickly, since we have only 2 weeks before RDP2.

In parallel with that, discussion on openjfx-dev with a new subject indicating that you propose to remove these two classes from the public API by deprecating them for removal in JavaFX 22 and removing them in 23.

I left a few inline comments, but nothing that would call the overall proposal into question.

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.

modules/javafx.graphics/src/main/java/javafx/css/Selector.java line 96:

> 94:      * @return a match, never {@code null}
> 95:      */
> 96:     public final Match createMatch() {

This is a compatible change only because this class is sealed. I do have a question, though, about whether it should remain abstract.

modules/javafx.graphics/src/main/java/javafx/css/Selector.java line 102:

> 100: 
> 101:             return new Match(this, s.getPseudoClassStates(), idCount, styleClassCount);
> 102:         }

I presume you are moving the implementations to this base class because some of the types, constructors (e.g., Match), or methods only have package visibility? Using the accessor pattern via a Helper class is usually how we deal with this. Have you considered that? It would allow the implementation to remain in the subclasses.

modules/javafx.graphics/src/main/java/javafx/css/Selector.java line 179:

> 177:      * @throws IOException if reading from {@code DataInputStream} fails
> 178:      */
> 179:     protected static Selector readBinary(int bssVersion, DataInputStream is, String[] strings)

This is an API addition, so will need an `@since 23`. Since you are adding new API here, should it be an instance method rather than a static method? Or is is called without having an instance of Selector in some cases?

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

PR Review: https://git.openjdk.org/jfx/pull/1333#pullrequestreview-1830758962
PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1458108202
PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1458108802
PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1458109403
PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1458110482