On Fri, 19 Jan 2024 10:21:05 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> 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` > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Add since tags > 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](https://github.com/openjdk/jfx/pull/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. ------------- PR Comment: https://git.openjdk.org/jfx/pull/1333#issuecomment-1900949541
