On Sat, 8 Apr 2023 19:43:51 GMT, John Hendrikx <jhendr...@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
