On Thu, 18 Jan 2024 23:58:17 GMT, Kevin Rushforth <[email protected]> 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 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.
Yes, correct, `CompoundSelector` accesses the package private fields `idCount`
and `styleClassCount` of `Match` directly, which it can't do anymore after
being moved to a different package; these lines:
idCount += match.idCount;
styleClassCount += match.styleClassCount;
I'm aware of the Helper classes, but I feel that they are a much more invasive
concept (and also harder to follow) to achieve this than doing a pattern match
with `instanceof` (which can be replaced with pattern matches for switch once
we can use Java 21). However, if you think this is a requirement, I'm happy to
change it -- that said, we're not locked in either choice as far as I can see.
Alternatively, with everything needed in `Selector` being publicly accessible,
I'm not sure if the match creation really needed to be in `Selector` or its
subtypes at all. It feels like more a job for an external type to handle (like
how you don't write serialization logic for JSON or XML in each subtype). If
it were up to me, I'd probably create a static method in `Match` which given a
`Selector` creates the match. That way, no `Match` internals need exposing at
all. I could still do this, as the method needed could be package private, and
then all the match fields can be made fully private.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1458660294
