On Sun, 13 Apr 2025 17:34:15 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> Implementation of [CSS media >> queries](https://gist.github.com/mstr2/cbb93bff03e073ec0c32aac317b22de7). > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > improved implementation of NullCoalescingPropertyBase A partial review, I didn't look at the tests. I noticed some non-intuitive equals/hashCode overrides, but found no evidence they're needed to make the non-test code work; if they're only there to make testing more convenient, I really think the overrides should be removed. In general, IMHO, any equals/hashCode override that doesn't fully test equality (apart from `transient` or derived fields) should be avoided as it will be surprising if such classes are ever used with `equals` or any hash based collection classes. If there's need for partial equality, then externalize this. modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PreferenceProperties.java line 246: > 244: } > 245: > 246: public synchronized void fireValueChangedIfNecessary() { I noticed that you made many methods `synchronized` in this class, but I have trouble seeing why this is. A short explanation may be in order. So, if I had to guess: it's possible to change the value override and/or the updating of all properties from a different thread, which is not the FX thread. However, what guarantees are now given to listeners (if any)? The value changed events can now also be triggered from any thread, unless wrapped in a `Platform.runLater`. What I mean here is, let's say I listen on `accentColorProperty` of `Platform.Preferences`, on what thread can the change event happen? The `Platform.Preferences` class does not mention anything special, so I think it is fair to assume it should be on the FX thread, allowing me to make modifications to an active scene graph in my change handler; however that will fail if the change notification was not triggered from the FX thread. modules/javafx.graphics/src/main/java/com/sun/javafx/css/media/MediaRule.java line 129: > 127: public int hashCode() { > 128: return hash; > 129: } You're basically saying here that a media rule with or without parent but the same queries is the same. However, parent is taken into account while writing/reading and evaluating. I think this deserves a justification. modules/javafx.graphics/src/main/java/com/sun/javafx/css/media/expression/FunctionExpression.java line 72: > 70: public String toString() { > 71: return "(" + (featureValue != null ? featureName + ": " + > featureValue : featureName) + ")"; > 72: } It's really unusual to override these methods in a record. What reason do you have for excluding the function from equals/hashCode ? What does it mean when all fields match but a different function is used? Where is equality used? modules/javafx.graphics/src/main/java/com/sun/javafx/css/parser/TokenStream.java line 31: > 29: import java.util.function.Predicate; > 30: > 31: public final class TokenStream { I think the naming of the methods in this class leaves something to be desired. - `Token consume()` **OK** - `Token consume(Predicate)` -> `consumeIf` or `consumeIfMatches` Makes it clearer, as `consume` seems to imply something is always consumed (ie. it still skips one token if the predicate didn't match returning `null`). - `Token peek()` **OK** - `reconsume` -> `unconsume`, `undoConsume`, `undo`, `previous`, `resetToPrevious`, `decrementIndex` Nothing is consumed which reconsume seems to imply, instead it moves the index back one place so the next call to `consume` may indeed reconsume a token; reconsume as-is would do nothing (and if it returned anything it would be same the as `current`. - `boolean peekMany(Predicate...)` -> `matches` It doesn't work like `peek`. `peekMany` would imply it returns a `List<Token>`; it also doesn't convey that it returns a `boolean`. - `reset(int)` -> `setIndex` This seems to be similar to what say `InputStream` provides, but `InputStream` hides the `index` parameter so you can't set it to some arbitrary value (like skipping ahead). If you want to mimic this pattern, I'd suggest removing the parameter and providing a `mark` method (or make it non-public). modules/javafx.graphics/src/main/java/javafx/css/Rule.java line 64: > 62: private final MediaRule mediaRule; > 63: > 64: private List<Selector> selectors = null; I dislike the accessor pattern somewhat; I wonder, would it be possible to: - Make `Rule` sealed: `public abstract sealed class Rule permits InternalRule` (*) similar to how this was done for `Selector`: `public abstract sealed class Selector permits SimpleSelector, CompoundSelector` - `InternalRule` would not be public API, and can therefore introduce a public method to get `MediaRule` - `Rule` does not have a public constructor, so IMHO there's no risk in there ever being an instance of `Rule` directly (similar reasoning was used to when the `SimpleSelector` / `CompountSelector` refactoring was done). This way getting access to the `getMediaRule` method is just a simple `instanceof` check away: MediaRule rule = rule instanceof InternalRule ir ? ir.getMediaRule() : null; (*) `InternalRule` is just a place holder name ------------- PR Review: https://git.openjdk.org/jfx/pull/1655#pullrequestreview-2763805568 PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2041819506 PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2041903462 PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2041884462 PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2041936819 PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2041960550
