RFR: JDK-8304959: Public API in javafx.css.Match should not return private API class PseudoClassState [v6]

[Kevin Rushforth]

On Sun, 9 Apr 2023 09:22:54 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> The class `PseudoClassState` is private API, but was exposed erroneously in the CSS API. Instead, `Set<PseudoClass>` should have been used. This PR corrects this.
>
> John Hendrikx has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix PseudoClassTest

The changes look good with a few inline comments.

modules/javafx.graphics/src/main/java/com/sun/javafx/css/BitSet.java line 519:

> 517: 
> 518:     @Override
> 519:     public boolean equals(Object obj) {

Since you override `equals`, you should also override `hashCode`. Presuming that `super.hashCode` honors the contract of `equals` / `hashCode`, I'd prefer that to be explicit along with a comment as to why it's OK (which you sort of do in a comment in the equals method).

modules/javafx.graphics/src/main/java/javafx/css/Match.java line 74:

> 72:      * Gets the {@code Selector}.
> 73:      *
> 74:      * @return the {@code Selector}, never {@code null}

Can you add this to the CSR?

modules/javafx.graphics/src/main/java/javafx/css/Selector.java line 97:

> 95:      * Creates a {@code Match}.
> 96:      *
> 97:      * @return a match, never {@code null}

Can you add this to the CSR?

modules/javafx.graphics/src/main/java/javafx/css/SimpleSelector.java line 76:

> 74:      * @return an immutable list of style-classes of the {@code Selector}
> 75:      */
> 76:     public List<String> getStyleClasses() {

This is a public method, so removing it would be an incompatible API change. Given the decision to live with this public class, I think you should restore this method.

modules/javafx.graphics/src/main/java/javafx/css/SimpleSelector.java line 136:

> 134: 
> 135:         if (styleClasses != null) {
> 136:             for(int n = 0; n < styleClasses.size(); n++) {

Minor: space after `for`

modules/javafx.graphics/src/main/java/javafx/css/SimpleSelector.java line 152:

> 150: 
> 151:         if (pseudoClasses != null) {
> 152:             for(int n = 0; n < pseudoClasses.size(); n++) {

Minor: space after `for`

-------------

PR Review: https://git.openjdk.org/jfx/pull/1070#pullrequestreview-1421475805
PR Review Comment: https://git.openjdk.org/jfx/pull/1070#discussion_r1192844466
PR Review Comment: https://git.openjdk.org/jfx/pull/1070#discussion_r1192847207
PR Review Comment: https://git.openjdk.org/jfx/pull/1070#discussion_r1190464417
PR Review Comment: https://git.openjdk.org/jfx/pull/1070#discussion_r1192854967
PR Review Comment: https://git.openjdk.org/jfx/pull/1070#discussion_r1192856315
PR Review Comment: https://git.openjdk.org/jfx/pull/1070#discussion_r1192856335