8323706: Remove SimpleSelector and CompoundSelector classes

[hjohn]

trafficstars

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

• [x] Change must not contain extraneous whitespace
• [ ] Change requires a CSR request matching fixVersion jfx24 to be approved (needs to be created)
• [x] Commit message must refer to an issue
• [x] Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

• JDK-8323706: Remove SimpleSelector and CompoundSelector classes (Enhancement - P4)

Reviewers

• Kevin Rushforth (@kevinrushforth - Reviewer)
• Andy Goryachev (@andy-goryachev-oracle - Reviewer)

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[bot]]

:wave: 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[bot]]

Webrevs

• 07: Full - Incremental (875c1a88)
• 06: Full - Incremental (39c94408)
• 05: Full (7d77e43e)
• 04: Full - Incremental (efe1beea)
• 03: Full - Incremental (4702972d)
• 02: Full (e177ef16)
• 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[bot]]

@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[bot]]

@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.

[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 #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 https://github.com/openjdk/jfx/pull/1340#issuecomment-1900843857 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[bot]]

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

[openjdk[bot]]

@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[bot]]

@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!

[hjohn]

@kevinrushforth I think this is ready for a closer inspection again; in a strict sense there are API additions in this PR, but because of how sealed works, they're not accessible except to permitted sub types (and none of those permitted types are public API).

[kevinrushforth]

I think this is ready for a closer inspection again

I'll put it on my review queue for after Thursday's fork.

in a strict sense there are API additions in this PR, but because of how sealed works, they're not accessible except to permitted sub types (and none of those permitted types are public API).

Right, the API additions are the package-scope constructor and the readBinary method, although as you say, they are only accessible to internal JavaFX classes. Since Selector will be a sealed class with all permitted classes being internal, no package-scope methods can be accessed by an application.

The main API change is the removal of the terminally-deprecated SimpleSelector and CompoundSelector classes. With that in mind, a better title for this bug would be:

"Remove SimpleSelector and CompoundSelector classes"

You could add the existing title as a summary:

"/summary Move SimpleSelector and CompoundSelector to internal packages"

[hjohn]

/summary Move SimpleSelector and CompoundSelector to internal packages

[openjdk[bot]]

@hjohn Setting summary to Move SimpleSelector and CompoundSelector to internal packages

[bridgekeeper[bot]]

@hjohn This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 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!

[hjohn]

this PR has been out for too long, could you please merge the latest master branch in?

I did yesterday, or did something go wrong?

[andy-goryachev-oracle]

oops, yes, you did, my mistake. the master progressed since then, got me confused.

I got a bunch of errors again after switching:

Description	Resource	Type	Path	Location
Cannot infer type arguments for ParsedValueImpl<>	CssParser.java	Java Problem	/graphics/src/main/java/javafx/css	line 3024
Cannot infer type arguments for ParsedValueImpl<>	CssParser.java	Java Problem	/graphics/src/main/java/javafx/css	line 3214
Cannot infer type arguments for ParsedValueImpl<>	CssParser.java	Java Problem	/graphics/src/main/java/javafx/css	line 3267
Cannot infer type arguments for ParsedValueImpl<>	CssParser.java	Java Problem	/graphics/src/main/java/javafx/css	line 3282
Cannot infer type arguments for ParsedValueImpl<>	CssParser.java	Java Problem	/graphics/src/main/java/javafx/css	line 3607
Cannot infer type arguments for ParsedValueImpl<>	CssParser.java	Java Problem	/graphics/src/main/java/javafx/css	line 3651

[nlisker]

I see a lot of aversion from for-each loops in favor of indexed loops in the classes this PR touches. While I don't suggest to change it here, any idea why?

[hjohn]

I see a lot of aversion from for-each loops in favor of indexed loops in the classes this PR touches. While I don't suggest to change it here, any idea why?

It's just the style the original code was written with. There is no reason to write the binary load/save code like this (as IO will be a far greater bottleneck).

There are/were other parts (in Match and the selector subtypes) however that are performance sensitive, so perhaps the entire classes were just written in a similar style "just in case".

[nlisker]

oops, yes, you did, my mistake. the master progressed since then, got me confused.

I got a bunch of errors again after switching:

Description	Resource	Type	Path	Location
Cannot infer type arguments for ParsedValueImpl<>	CssParser.java	Java Problem	/graphics/src/main/java/javafx/css	line 3024
Cannot infer type arguments for ParsedValueImpl<>	CssParser.java	Java Problem	/graphics/src/main/java/javafx/css	line 3214
Cannot infer type arguments for ParsedValueImpl<>	CssParser.java	Java Problem	/graphics/src/main/java/javafx/css	line 3267
Cannot infer type arguments for ParsedValueImpl<>	CssParser.java	Java Problem	/graphics/src/main/java/javafx/css	line 3282
Cannot infer type arguments for ParsedValueImpl<>	CssParser.java	Java Problem	/graphics/src/main/java/javafx/css	line 3607
Cannot infer type arguments for ParsedValueImpl<>	CssParser.java	Java Problem	/graphics/src/main/java/javafx/css	line 3651

I got these too, as well as many

The method createShader(String, InputStream, Map<String,Integer>, Map<String,Integer>, int, boolean, boolean) in the type ShaderFactory is not applicable for the arguments (InputStream, HashMap<String,Integer>, HashMap<String,Integer>, int, boolean, boolean)

I haven't worked on this repo for 3 weeks, what changed?

[hjohn]

oops, yes, you did, my mistake. the master progressed since then, got me confused. I got a bunch of errors again after switching:

Description	Resource	Type	Path	Location
Cannot infer type arguments for ParsedValueImpl<>	CssParser.java	Java Problem	/graphics/src/main/java/javafx/css	line 3024
Cannot infer type arguments for ParsedValueImpl<>	CssParser.java	Java Problem	/graphics/src/main/java/javafx/css	line 3214
Cannot infer type arguments for ParsedValueImpl<>	CssParser.java	Java Problem	/graphics/src/main/java/javafx/css	line 3267
Cannot infer type arguments for ParsedValueImpl<>	CssParser.java	Java Problem	/graphics/src/main/java/javafx/css	line 3282
Cannot infer type arguments for ParsedValueImpl<>	CssParser.java	Java Problem	/graphics/src/main/java/javafx/css	line 3607
Cannot infer type arguments for ParsedValueImpl<>	CssParser.java	Java Problem	/graphics/src/main/java/javafx/css	line 3651

I got these too, as well as many

The method createShader(String, InputStream, Map<String,Integer>, Map<String,Integer>, int, boolean, boolean) in the type ShaderFactory is not applicable for the arguments (InputStream, HashMap<String,Integer>, HashMap<String,Integer>, int, boolean, boolean)

I haven't worked on this repo for 3 weeks, what changed?

The ones mentioned by Andy I also see, and are just a difference in opinion between Javac and ECJ, or perhaps a bug in the incremental compilation that Eclipse does (as they usually only appear when it does an incremental compile, not a full one). When those occur, I just select them and remove them from the errors list.

The others I haven't seen before, and I'm not seeing them now. I did a search for createShader but didn't see anything unusual.

I'm not aware of any special changes in the last 3 weeks. I did a fresh import recently, and nothing strange popped up.