On Sun, 14 Jan 2024 04:58:35 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> If this method is not used anywhere, why do we need to expose
>> `getStyleClassNames()` as new API to replace this one? I'm a bit puzzled by
>> that, especially since you're saying that the API shouldn't be used. Why
>> create something that shouldn't be used?
>>
>> I'd rather just document that you shouldn't expect great performance from
>> this method, and be done with it. Changing API in a performance optimization
>> PR seems out of scope.
>
> You were talking about `List<String> getStyleClasses()`. This is not used by
> FX, and unlikely to be used anywhere by 3rd parties (see reasoning in other
> comment).
>
> Then there is a 2nd method: `Set<StyleClass> getStyleClassSet()`. This
> is/was called by `SelectorPartitioning`, an internal FX class. However, this
> method makes use of the `StyleClass` wrapper that is hurting performance.
>
> To ensure `SelectorPartitioning` has a fast way to access the styles, I
> needed a new method: `Set<String> getStyleClassNames`. As it is called from
> outside the `javafx.css` package, it has to be `public`.
>
> **Now perhaps you were eluding to something else:**
>
> Would `SelectorPartitioning` mind if the styles were provided as `List` or
> `Collection`? Looking at that code, I think it doesn't really care. It just
> uses the set as a key, and calls `containsAll` on it. So perhaps we don't
> need to deprecate `List<String> getStyleClasses()` then, and we don't need a
> new API. Instead I'll have `SelectorPartitioning` be happy with a
> `Collection` so I also don't have to make make a set implement a `List`
> (which you can't do without violating the contract of either `List` or `Set`
> as `hashCode` is defined differently).
>
> Edit: it would still need to be a `List` as I can't return a `Set<String>`
> from a method that returns `List<String>`... that means either change that
> method to return `Collection`, so still need an API change, or store
> everything as `List`...
>
> The only thing that may land us into trouble here (reusing `List<String>
> getStyleClasses()`) is that there may be an expectation that it still
> contains duplicates, and retains the original order with which
> `SimpleSelector` was created. This is already not the case (as it is based
> on the old `BitSet` that was created) -- it is deduplicated, and its order is
> changed (whether that was the intention is unclear, but that's what it does
> right now). Then again, the method is currently not in use...
I took another look, and I don't see a better option here than introducing a
new method. Things I looked at:
1. Reuse `List<String> getStyles()` by having the sets implement `List` as well
as `Set`
- this is impossible as the two specify different `equals` and `hashCode`
2. Reuse `List<String> getStyles()` by having the sets implement only `List`
but still eliminate duplicates
- this would violate `List` contract (can't eliminate duplicates)
- still need fast `containsAll` performance in the rare case we may have a
large number of styles in the set...
3. Moving `SimpleSelector` and `CompoundSelector` out of `javafx.css`:
- arguably, they never should have been there (we ran into this before) but
moving them is still an API break (albeit a minor one as they're only
accessible by casting) -- we could start this process though, so any public
methods we add now will be of no consequence once moved
4. Moving a ton of internal CSS classes to `javafx.css` (but keep most package
private) and introduce a package private `getStyleClassNames`:
- this runs into an issue that **something** eventually needs to be
`public` to make use of the CSS engine, and that would be new API then
Of the above options **3** is by far the best. I've looked further into how
this can be achieved, and it requires the following:
**Minor API break**
- Move `SimpleSelector` and `CompoundSelector` out of `javafx.css` and into a
non-public package -- this is a minor API break as these classes were
accessible before if you cast `Selector` to either of these. You could not
construct them however.
This options was discussed before when it was determined these classes should
probably never have been made public. They can't be easily moved though because
they use package private methods of the abstract base class `Selector`.
However... nowadays we can **seal** classes, see below:
**Fully compatible changes**
- Make `Selector` `sealed`, only allowing `CompoundSelector` and
`SimpleSelector` -- this is compatible because `Selector` does not allow
arbitrary subclasses (it has a package private constructor)
- Change `Selector`s constructor to be `protected` and also one of its static
methods needs to be `protected`. This does not create new public API as
`protected` methods are only accessible by subclasses, and it is now sealed.
- Implement `createMatch` directly in `Selector` (it is `abstract` now) and
make it `final` for good measure. All the information required is already
available via `public` accessible methods of its sealed sub types. This avoids
having to access package private fields in `Match` by `CompoundSelector` which
will no longer be in the same package. This is fully compatible.
- Change some internal classes imports to the new location of
`CompoundSelector` and `SimpleSelector`.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1316#discussion_r1451738916
