On Sun, 14 Jan 2024 04:20:56 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> This is possible, although `List#get` would not perform too well when it is >> implemented for `FixedCapacitySet.OpenAddressed` as the array used as hash >> table in this class can have gaps (so we'd need to iterate to find the >> index). >> >> However, I am very sure this method is not used anywhere (not even in 3rd >> party code as it requires casting to access), and I wouldn't encourage its >> use, so I'd be more inclined to remove it completely. > > 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... ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1316#discussion_r1451656563
