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