On Fri, 2 Feb 2024 19:04:33 GMT, John Hendrikx <[email protected]> wrote:
>>> My conclusion is that we either need two rounds to get this right (to first
>>> free up our first choice getStyleClasses), or we can settle on one of:
>>> getStyleClassNames, getStyles, getStyleNames, getClasses, getClassNames.
>>
>> Let's settle on one of these names. I think `getClasses` is fine. My second
>> choice would be `getClassNames`.
>>
>> There is one other method used by SceneBuilder's `CssInternal` class,
>> `CompoundSelector::getSelectors`. I think you need to add a method to the
>> `Selector` base class that returns a `List<Selector>` instead of the
>> `List<SimpleSelector>` returned by the existing `CompoundSelector` method.
>> Maybe `getChildSelectors` or similar? That method would be overridden in the
>> subclasses with `CompoundSelector` wrapping the existing selectors list in
>> an unmodifiable list and `SimpleSelectors` returning an empty List.
>
>> There is one other method used by SceneBuilder's `CssInternal` class,
>> `CompoundSelector::getSelectors`. I think you need to add a method to the
>> `Selector` base class that returns a `List<Selector>` instead of the
>> `List<SimpleSelector>` returned by the existing `CompoundSelector` method.
>> Maybe `getChildSelectors` or similar? That method would be overridden in the
>> subclasses with `CompoundSelector` wrapping the existing selectors list in
>> an unmodifiable list and `SimpleSelectors` returning an empty List.
>
> @kevinrushforth if I'm correct, SceneBuilder won't need that anymore. Since
> it shouldn't need to do an `instanceof` check (which will break once they're
> moved to internal packages), the fragment that gets all the selectors should
> be changed to:
>
> for (Rule r : s.getRules()) {
> for (Selector ss : r.getSelectors()) {
> styleClasses.addAll(ss.getClasses()); // provisional name
> getClasses
> }
> }
>
> Old SceneBuilder code fragment:
>
> for (Rule r : s.getRules()) {
> for (Selector ss : r.getSelectors()) {
> if (ss instanceof SimpleSelector) {
> SimpleSelector simple = (SimpleSelector) ss;
> styleClasses.addAll(simple.getStyleClasses());
> } else {
> if (ss instanceof CompoundSelector) {
> CompoundSelector cs = (CompoundSelector) ss;
> for (Selector selector : cs.getSelectors()) {
> if (selector instanceof SimpleSelector) {
> SimpleSelector simple = (SimpleSelector)
> selector;
> styleClasses.addAll(simple.getStyleClasses());
> }
> }
> }
> }
> }
> }
>
> Note that in this old code, there is some unnecessary casting going on
> (`cs.getSelectors()` already returns `SimpleSelector`s, which makes it seem
> like `CompoundSelector` could contain something other than `SimpleSelector`s,
> which is not the case (they can't be nested or anything).
@hjohn since the implementation of `getClasses` in `CompoundSelector`
aggregates the style classes for all of its `SimpleSelectors`, then I think you
are right that a `getChildSelectors` is unnecessary.
@jperedadnr Will this work for you?
-------------
PR Comment: https://git.openjdk.org/jfx/pull/1340#issuecomment-1924543785
