On Fri, 7 Apr 2023 21:44:12 GMT, Kevin Rushforth <[email protected]> wrote:
>> I'll take a closer look early next week.
>
>> I think that Match is supposed to be immutable, given the non-public
>> constructor. Match itself will never change the set (and nothing else will)
>> so making it observable seems unnecessary.
>
> Agreed.
>
>> In other words, simpleSelector.createMatch().getPseudoClasses().clear()
>> would break the Selectors encapsulation.
>>
>> I think it's best to close that loophole. If you agree, I can document this
>> method that it returns an immutable set, which is also what I assumed would
>> be the case in my other PR where I made many of these immutable.
>
> Yes, this seems like the best solution to me.
>
>> Essentially this means that SimpleSelector and CompoundSelector should
>> probably be package private.
>
> I agree that these two classes should not have been made public when the
> javafx.css package was created as public API back in JDK 9.
>
> The usual process for removing API is to deprecate it for removal in one
> release and then remove it in a future release, but in this case, since those
> classes cannot be constructed, and are never returned by any public API, any
> use of them would be relying on an implementation detail (via an `instanceof`
> and a cast, to no good purpose).
>
> Since there is no useful way an application could be using these classes, I
> recommend option 1, as long as those two classes can be cleanly moved to
> com.sun.javafx.css. Otherwise, we could go with option 2 along with
> deprecating those two classes for removal.
>
> The Specification section of the CSR would simply propose to remove those two
> classes. You could describe what is happening (moving them to a non-exported
> package) in the Solution section.
@kevinrushforth It looks like it will have to be option 2; the reason is that
both `Selector` and `Match` are abstract classes (with a package private
constructor). The `CompoundSelector` and `SimpleSelector` extend this abstract
class (as there is some small overlap). If `Selector` and `Match` had been
interfaces, it would have allowed this.
I couldn't find anything about binary compatibility when changing an abstract
class with a package private constructor to an interface, so I tested it and I
got an `IncompatibleClassChangeError` when calling `Selector
Selector.createSelector(...)` method. Apparently, the compiled method call
does encode the difference between an interface and a class:
invokestatic #7 // Method
Selector.createSelector:(I)LSelector;
vs:
invokestatic #7 // InterfaceMethod
Selector.createSelector:(I)LSelector;
At the most, I could make `CompoundSelector` and `SimpleSelector` package
private, or nested classes in `Selector`; they would at least be hidden then.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1070#discussion_r1161150369
