[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: JDK-8304959: Public API in javafx.css.Match should not return private API class PseudoClass
From:       Kevin Rushforth <kcr () openjdk ! org>
Date:       2023-05-31 22:07:19
Message-ID: cR89Xw6G8mC7k99sNEdeKjK8PaQrw4j5zS_En9hXpp0=.bbffc0c2-01d1-4f6f-9109-df61cc2ae67e () github ! com
[Download RAW message or body]

On Sat, 20 May 2023 00:57:08 GMT, John Hendrikx <jhendrikx@openjdk.org> 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


[prev in list] [next in list] [prev in thread] [next in thread]