On Fri, 19 Jan 2024 09:33:15 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> modules/javafx.graphics/src/main/java/javafx/css/Selector.java line 102: >> >>> 100: >>> 101: return new Match(this, s.getPseudoClassStates(), idCount, >>> styleClassCount); >>> 102: } >> >> I presume you are moving the implementations to this base class because some >> of the types, constructors (e.g., Match), or methods only have package >> visibility? Using the accessor pattern via a Helper class is usually how we >> deal with this. Have you considered that? It would allow the implementation >> to remain in the subclasses. > > Yes, correct, `CompoundSelector` accesses the package private fields > `idCount` and `styleClassCount` of `Match` directly, which it can't do > anymore after being moved to a different package; these lines: > > idCount += match.idCount; > styleClassCount += match.styleClassCount; > > I'm aware of the Helper classes, but I feel that they are a much more > invasive concept (and also harder to follow) to achieve this than doing a > pattern match with `instanceof` (which can be replaced with pattern matches > for switch once we can use Java 21). However, if you think this is a > requirement, I'm happy to change it -- that said, we're not locked in either > choice as far as I can see. > > Alternatively, with everything needed in `Selector` being publicly > accessible, I'm not sure if the match creation really needed to be in > `Selector` or its subtypes at all. It feels like more a job for an external > type to handle (like how you don't write serialization logic for JSON or XML > in each subtype). If it were up to me, I'd probably create a static method > in `Match` which given a `Selector` creates the match. That way, no `Match` > internals need exposing at all. I could still do this, as the method needed > could be package private, and then all the match fields can be made fully > private. what do you think of Match MatchHelper.create(SimpleSelector) Match MatchHelper.create(CompoundSelector) ? ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1670912593
