On Fri, 19 Jan 2024 00:00:32 GMT, Kevin Rushforth <k...@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 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?
I'll add the `@since 23`, however it can't be called by anyone except FX itself.
Some background: the readBinary/writeBinary methods are ultimately called via
the publicly accessible methods `loadBinary` and `saveBinary` in
`javafx.css.Stylesheet`.
The reason it needs to be `protected` now is because `CompoundSelector` is
using this (but IMHO, it shouldn't have). Why I say it shouldn't? That's
because it already knows what it will be reading will all be `SimpleSelector`s,
as it stores a counter (a `short`) and then loads that many `SimpleSelector`s.
However, by not taking the direct route of using `SimpleSelector#readBinary`
(and the corresponding `SimpleSelector#writeBinary`) there is an additional
`byte` prefix indicating the type of selector -- this isn't needed and wastes
some space in the binary format.
Changing that now however would make the binary format incompatible with
earlier versions, so I think making this `protected` is a reasonable solution.
It also mirrors the `writeBinary` method then, which was already `protected`.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1458688819
