RFR: JDK-8304959: Public API in javafx.css.Match should not return private API class PseudoClassState

[John Hendrikx]

On Thu, 30 Mar 2023 23:08:31 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> modules/javafx.graphics/src/main/java/javafx/css/Match.java line 79:
>> 
>>> 77:      * @return the pseudo class state
>>> 78:      */
>>> 79:     public Set<PseudoClass> getPseudoClasses() {
>> 
>> Should this be an `ObservableSet`? Changing the type to the `Set` superclass will mean that applications would need to do an `instanceof` check to know whether it was observable or not?
>
> 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.
> 
> However, this was not correctly spec'd, and you can change a `Match` now, and since it doesn't make a copy, you'd be changing the pseudo classes of whatever selector created it.  Note that `SimpleSelector` does make a copy, and goes to great pains to not expose anything mutable, but exposes it accidentally via `Match`.
> 
> 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.

The CSS API baffles me a bit.  It doesn't seem consistent.

Just now I took a look at the class `SimpleSelector` and `CompoundSelector`. These are public, yet cannot be constructed by users.  They're also not returned anywhere (the closest is `Selector#createSelector` which returns the interface).

Essentially this means that `SimpleSelector` and `CompoundSelector` should probably be package private.  Yet, I guess they were made public because `SelectorPartitioning` is doing instanceof checks and is casting to these classes.  But anybody can do that now, and that means that for example `SimpleSelector#getStyleClassSet` is exposed, which returns a mutable set...

Reading between the lines though it seems that `SimpleSelector` and `CompoundSelector` were intended to be fully immutable (which makes sense as they represent a style sheet).  Any changes would not be picked up as nothing is observing these properties.

I think these loopholes should be closed.

There are two options IMHO:

1. Move `SimpleSelector` and `CompoundSelector` to the com hierarchy.  They can't be publically constructed, and are never returned.  The only way to reach them is by casting.
2. If it's too late for that, then close all loopholes and ensure that these two classes are fully immutable. From what I can see now, only `getStyleClassSet` and the mentioned method in `Match` need closing.  `CompoundSelector` is already immutable.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1070#discussion_r1153894723