RFR: JDK-8304959: Public API in javafx.css.Match should not return private API class PseudoClassState [v6]
Kevin Rushforth
kcr at openjdk.org
Fri May 12 23:09:56 UTC 2023
On Sat, 8 Apr 2023 19:43:51 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:
>>> 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.~~ Can't do that either, they're referred in private API.
OK. In that case, option 2 it is. And since there doesn't seem to be a good way to remove these classes from the public API, it makes no sense to deprecate them for removal.
We could deprecate them with a note that they are used by the CSS implementation, and not intended to be used by applications, but since applications aren't usefully using them today, we could defer that to a later time (or not do it at all).
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1070#discussion_r1190463585
More information about the openjfx-dev
mailing list