On Thu, 30 Mar 2023 23:59:50 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. >> >> 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. I'll take a closer look early next week. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1070#discussion_r1154473373
