On Sat, 20 May 2023 00:57:08 GMT, John Hendrikx <[email protected]> wrote:
>> I took a closer look at the compatibility issues Joe raised in the CSR, and
>> I can confirm that yes, this change will break binary compatibility for any
>> application that calls the existing method. It would also can break source
>> compatibility for the subset of those applications that use the return value
>> as an `ObservableSet` (which is the lowest public supertype of
>> `PseudoClassState`), although this latter concern seems _very_ unlikely, and
>> not all that impactful.
>>
>> So it's the binary compatibility we need to consider. The question is: is
>> the `Match::getPseudoClasses` method being called in a useful manner by
>> applications today?
>>
>> If not, then the change seems OK.
>>
>> If it is, then we might need to consider adding a replacement method and
>> deprecating the existing method for removal. The implementation of the
>> replacement method would be exactly what your PR currently has for the
>> existing method, and the existing method, would create a `PseudoClassState`
>> object to hold a copy of the objects in the set.
>
>> So it's the binary compatibility we need to consider. The question is: is
>> the `Match::getPseudoClasses` method being called in a useful manner by
>> applications today?
>>
>> If not, then the change seems OK.
>>
>> If it is, then we might need to consider adding a replacement method and
>> deprecating the existing method for removal. The implementation of the
>> replacement method would be exactly what your PR currently has for the
>> existing method, and the existing method, would create a `PseudoClassState`
>> object to hold a copy of the objects in the set.
>
> I've done some Googling with `"getPseudoClasses" import javafx`, and also
> looked at several code search sites, and if it's used, then I can't find any
> examples at all. I also searched for `createMatch`, with similar results, no
> uses outside the JavaFX code base.
>
> To call this method, one would to do something like this at a minimum:
>
> StyleSheet styleSheet = StyleSheet.loadBinary( <input stream> );
>
> for (Rule rule : styleSheet.getRules()) {
> for (Selector selector : rule.getSelectors()) {
> Match match = selector.createMatch();
>
> match.getPseudoClassStates(); // here...
> }
> }
>
> I did find a few references to `loadBinary`, but nothing for `getRules` which
> would be needed next.
>
> There's not a lot of reason to call `getPseudoClassStates` -- the method is
> mainly public so internal code outside its the public package can reach these
> to do the actual matching logic. This is why I initially tried to make
> `Selector`'s subclasses private, but couldn't.
@hjohn In case you missed seeing it, the CSR was approved, so Skara marked this
PR as `ready`.
-------------
PR Comment: https://git.openjdk.org/jfx/pull/1070#issuecomment-1571025701
