8323706: Move SimpleSelector and CompoundSelector to internal packages

[hjohn]

Moves SimpleSelector and CompoundSelector to internal packages.

This can be done with only a minor API break, as SimpleSelector and CompoundSelector were public before. However, these classes could not be constructed by 3rd parties. The only way to access them was by doing a cast (generally they're accessed via Selector not by their sub types). The reason they were public at all was because the CSS engine needs to be able to access them from internal packages.

This change fixes a mistake (or possibly something that couldn't be modelled at the time) when the CSS API was first made public. The intention was always to have a Selector interface/abstract class, with private implementations (SimpleSelector and CompoundSelector).

This PR as said has a small API break. The other changes are (AFAICS) source and binary compatible:

• Made Selector sealed only permitting SimpleSelector and CompoundSelector -- as Selector had a package private constructor, there are no concerns with pre-existing subclasses
• Selector has a few more methods that are now protected -- given that the class is now sealed, these modified methods are not accessible (they may still require rudimentary documentation I suppose)
• Selector now has a public default constructor -- as the class is sealed, it is inaccessible
• SimpleSelector and CompoundSelector have a few more public methods, but they're internal now, so it is irrelevant
• createMatch was implemented directly in Selector to avoid having to expose package private fields in Match for use by CompoundSelector
• No need anymore for the SimpleSelectorShim

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (3 reviews required, with at least 1 Reviewer, 2 Authors)
• Change requires a CSR request matching fixVersion jfx23 to be approved (needs to be created)

Issue

• JDK-8323706: Move SimpleSelector and CompoundSelector to internal packages (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1333/head:pull/1333
$ git checkout pull/1333

Update a local copy of the PR:
$ git checkout pull/1333
$ git pull https://git.openjdk.org/jfx.git pull/1333/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1333

View PR using the GUI difftool:
$ git pr show -t 1333

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1333.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jhendrikx! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[mlbridge]

Webrevs

• 01: Full - Incremental (e02d84c7)
• 00: Full (2909af46)

[kevinrushforth]

I'll take a closer look tomorrow, but this will very likely need to be done in two steps. Step 1 would be to terminally deprecate the existing public classes in an exported package. Step 2 would then be to remove them from the public API by moving them to a non-exported package. Given that we are still in RDP1 for JavaFX 22, we could do the first step (terminal deprecation) in 22 paving the way for removal in 23, so the net result would be the same, but it would give a clear warning to anyone using JavaFX 22 that this removal is pending.

/reviewers 3
/csr

[openjdk]

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 3 (with at least 1 Reviewer, 2 Authors).

[openjdk]

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@hjohn please create a CSR request for issue JDK-8323706 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

[kevinrushforth]

This seems like a reasonable thing to do. As I mentioned above, the first step is to file a new enhancement to deprecate these two classes for removal, with the idea being to get them into jfx22 via a late enhancement request. This will need to be done quickly, since we have only 2 weeks before RDP2.

In parallel with that, discussion on openjfx-dev with a new subject indicating that you propose to remove these two classes from the public API by deprecating them for removal in JavaFX 22 and removing them in 23.

I left a few inline comments, but nothing that would call the overall proposal into question.

[kevinrushforth]

This is a compatible change only because this class is sealed. I do have a question, though, about whether it should remain abstract.

[kevinrushforth]

I presume you are moving the implementations to this base class because some of the types, constructors (e.g., Match), or methods only have package visibility? Using the accessor pattern via a Helper class is usually how we deal with this. Have you considered that? It would allow the implementation to remain in the subclasses.

[hjohn]

Yes, correct, CompoundSelector accesses the package private fields idCount and styleClassCount of Match directly, which it can't do anymore after being moved to a different package; these lines:

            idCount += match.idCount;
            styleClassCount += match.styleClassCount;

I'm aware of the Helper classes, but I feel that they are a much more invasive concept (and also harder to follow) to achieve this than doing a pattern match with instanceof (which can be replaced with pattern matches for switch once we can use Java 21). However, if you think this is a requirement, I'm happy to change it -- that said, we're not locked in either choice as far as I can see.

Alternatively, with everything needed in Selector being publicly accessible, I'm not sure if the match creation really needed to be in Selector or its subtypes at all. It feels like more a job for an external type to handle (like how you don't write serialization logic for JSON or XML in each subtype). If it were up to me, I'd probably create a static method in Match which given a Selector creates the match. That way, no Match internals need exposing at all. I could still do this, as the method needed could be package private, and then all the match fields can be made fully private.

[kevinrushforth]

This is an API addition, so will need an @since 23. Since you are adding new API here, should it be an instance method rather than a static method? Or is is called without having an instance of Selector in some cases?

[hjohn]

I'll add the @since 23, however it can't be called by anyone except FX itself.

Some background: the readBinary/writeBinary methods are ultimately called via the publicly accessible methods loadBinary and saveBinary in javafx.css.Stylesheet.

The reason it needs to be protected now is because CompoundSelector is using this (but IMHO, it shouldn't have). Why I say it shouldn't? That's because it already knows what it will be reading will all be SimpleSelectors, as it stores a counter (a short) and then loads that many SimpleSelectors. However, by not taking the direct route of using SimpleSelector#readBinary (and the corresponding SimpleSelector#writeBinary) there is an additional byte prefix indicating the type of selector -- this isn't needed and wastes some space in the binary format.

Changing that now however would make the binary format incompatible with earlier versions, so I think making this protected is a reasonable solution. It also mirrors the writeBinary method then, which was already protected.

[hjohn]

I missed something in my above evaluation. The counterpart method writeBinary is not static. Although that makes sense as in that case you do have an instance you wish to write, it does make the read/write API asymmetric.

It's not possible to make readBinary an instance method though as it is the method that is creating those instances.

The other way around is possible (make writeBinary static), but it would then again need a pattern match to determine the correct static writeBinary to call when writing an arbitrary Selector. However, this would have allowed CompoundSelector to call a static version of SimpleSelector#writeBinary and readBinary, avoiding the extra identifying byte in the binary format, and avoiding the cast when reading it back.

The read/write loops below:

•   final int nSelectors = is.readShort();
    final List<SimpleSelector> selectors = new ArrayList<>();
    for (int n=0; n<nSelectors; n++) {
        selectors.add((SimpleSelector)Selector.readBinary(bssVersion, is,strings));
    }
  

•   os.writeShort(selectors.size());   // writes the number of SimpleSelectors to the binary stream
    for (int n=0; n< selectors.size(); n++) selectors.get(n).writeBinary(os,stringStore);  // writes each individually
  

Would then have become:

•   final int nSelectors = is.readShort();
    final List<SimpleSelector> selectors = new ArrayList<>();
    for (int n=0; n<nSelectors; n++) {
        selectors.add(SimpleSelector.readBinary(bssVersion, is,strings));  // cast is gone
    }
  

•   os.writeShort(selectors.size());   // writes the number of SimpleSelectors to the binary stream
    for (int n=0; n< selectors.size(); n++) SimpleSelector.writeBinaryStatic(selectors.get(n), os, stringStore);  // writes each individually
  

[andy-goryachev-oracle]

Maybe @jperedadnr could check whether the ScenicView tools uses either SimpleSelector or CompoundSelector?

[jperedadnr]

@andy-goryachev-oracle Scenic View source code doesn't use neither of them.

However, Scene Builder does: https://github.com/gluonhq/scenebuilder/blob/master/kit/src/main/java/com/oracle/javafx/scenebuilder/kit/util/CssInternal.java#L251

I guess it can be used a quick test of this PR?

[andy-goryachev-oracle]

However, Scene Builder does:

Thank you, @jperedadnr !
Does this mean that we ought to add a public method to Selector?

[hjohn]

@andy-goryachev-oracle Scenic View source code doesn't use neither of them.

However, Scene Builder does: https://github.com/gluonhq/scenebuilder/blob/master/kit/src/main/java/com/oracle/javafx/scenebuilder/kit/util/CssInternal.java#L251

I guess it can be used a quick test of this PR?

Well, because those classes are moved, SceneBuilder would no longer work (probably something like a class not found exception will occur).

Is SceneBuilder tied to JavaFX releases? Otherwise we may need to take more action.

I see that SceneBuilder is using these classes to obtain a list of all style classes that are part of a style sheet. Such functionality could be offered in several places, either on Stylesheet, which gathers all the style classes, a public method on Rule which gets all style classes that the rule uses, or a public method on Selector which returns all style classes that the selector uses.

[andy-goryachev-oracle]

In my opinion, Selector might be the most logical place to add the new methods.

This is a breaking change, so I think we should talk about adding these methods at the same time as deprecating the classes, to give the tools developers time to switch to the new implementation and possibly re-packaging their app in a multi-release jar (it's always fun to make changes to the build scripts!).

Will there be enough time for a complete review?

[kevinrushforth]

Hmm. This suggests taking a step back. I think there are only two choices that make sense (there are a couple others, but I don't think they are useful).

1. Abandon the notion of making an incompatible change to SimpleSelector and CompoundSelector. This means that PR 8322964: Optimize performance of CSS selector matching #1316 will need to preserve the existing SimpleSelector method List<String> getStyleClasses() and Set<StyleClass> getStyleClassSet(), deprecating them with simple deprecation (not for removal).
2. Make an incompatible change, meaning that at some point, the existing SceneBuilder binaries will break. We could make the transition a little less painful by providing replacement API in a release that still has the existing API (in fact, I think if we go with this option, then we must do that), but the fact remains that there will then be no way to have a single binary that runs on JavaFX 21 and JavaFX 23 short of a multi-release jar file, which really isn't a good solution for this sort of problem.

Preserving the existing SimpleSelector methods is fairly easy and already done. So, is there any real drawback to doing this? I can't think of any.

[kevinrushforth]

I note that we could still consider deprecating the SimpleSelector and CompoundSelector classes for eventual removal, but it wouldn't have to be done in such a rush.

[andy-goryachev-oracle]

I note that we could still consider deprecating the SimpleSelector and CompoundSelector classes for eventual removal

I like the idea of deprecating with the following removal, coupled with adding the "missing" APIs to Selector. I think the lack of this method in the base class was an unfortunate oversight, and we should fix that.

[hjohn]

Hmm. This suggests taking a step back. I think there are only two choices that make sense (there are a couple others, but I don't think they are useful).

1. Abandon the notion of making an incompatible change to SimpleSelector and CompoundSelector. This means that PR JDK-8322964 Optimize performance of CSS selector matching #1316 will need to preserve the existing SimpleSelector method List<String> getStyleClasses() and Set<StyleClass> getStyleClassSet(), deprecating them with simple deprecation (not for removal).
2. Make an incompatible change, meaning that at some point, the existing SceneBuilder binaries will break. We could make the transition a little less painful by providing replacement API in a release that still has the existing API (in fact, I think if we go with this option, then we must do that), but the fact remains that there will then be no way to have a single binary that runs on JavaFX 21 and JavaFX 23 short of a multi-release jar file, which really isn't a good solution for this sort of problem.

Preserving the existing SimpleSelector methods is fairly easy and already done. So, is there any real drawback to doing this? I can't think of any.

Does the comment from José Pereda here #1340 (comment) make any difference in that regard? If scene builder is always tied to a specific FX release, then I would think we could still go ahead.

The drawback might be that more code may start relying on these classes, or that it will be harder to evolve the CSS API if more than just Simple/CompoundSelector are needed at some point. The API still seems to have been designed with the idea that Selector is the only public class.

I think regardless adding a public API to get the style classes is a good idea, it seems to round out the API a bit, and it would make Scene Builder less dependent on what we consider internals in the future.

[hjohn]

I note that we could still consider deprecating the SimpleSelector and CompoundSelector classes for eventual removal, but it wouldn't have to be done in such a rush.

That's certainly true, the only reason we wanted to do a quick deprecation is to avoid adding a new public method to SimpleSelector for the performance improvement PR. However, I think everyone will be able to live with adding such a method for now if that method will eventually become part of a non-exported package. Still would have almost 6 months to make up our minds about that if we'd really want to move those classes in FX 23 or not.

[kevinrushforth]

This PR should probably be moved to draft. Now that the deprecation for removal is targeted for 23, this won't happen until JavaFX 24.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@hjohn this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout feature/selectors-to-private-api-standalone
git fetch https://git.openjdk.org/jfx.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[bridgekeeper]

@hjohn This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!