RFR: 8323706: Move SimpleSelector and CompoundSelector to internal packages [v3]
Kevin Rushforth
kcr at openjdk.org
Tue Jul 9 15:41:43 UTC 2024
On Tue, 9 Jul 2024 12:25:07 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`
>
> John Hendrikx has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>
> - Merge branch 'master' of https://git.openjdk.org/jfx into feature/selectors-to-private-api-standalone
> - Add since tags
> - Move SimpleSelector and CompoundSelector to private classes
I left a couple quick comments on the API changes. I'll review the rest after the fork.
modules/javafx.graphics/src/main/java/javafx/css/Selector.java line 51:
> 49: * Explicit constructor for subtype use.
> 50: *
> 51: * @since 23
The pattern we use elsewhere is:
"Constructor for subclasses to call."
Also, that should be `@since 24`.
modules/javafx.graphics/src/main/java/javafx/css/Selector.java line 197:
> 195: * @return a selector, never {@code null}
> 196: * @throws IOException if reading from {@code DataInputStream} fails
> 197: * @since 23
That should be `@since 24`.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1333#pullrequestreview-2166691276
PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1670750269
PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1670751417
More information about the openjfx-dev
mailing list