RFR: JDK-8324182 Deprecate for removal SimpleSelector and CompoundSelector classes [v2]

[Kevin Rushforth]

On Fri, 2 Feb 2024 19:04:33 GMT, John Hendrikx <jhendrikx at openjdk.org> 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