On Thu, 30 Mar 2023 23:08:31 GMT, John Hendrikx <jhendr...@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
