On Tue, 6 May 2025 07:37:35 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: > > improve synchronization in PreferenceProperties Thank you for making changes and adding tests! And sorry for all the nitpicking. Looks good, with only two sticky points remaining: - I would like to see an example of correct syntax for the operator (and/or how to write `and not` case) - still think synchronization should not be used, instead use `volatile` and check for fx thread in the mutators. modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PreferenceProperties.java line 228: > 226: public T get() { > 227: // We need to synchronized on 'mutex' to see > 'effectiveValue', because get() may be called > 228: // on a thread other than the FX application thread. use `volatile` instead of synchronization? modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PreferenceProperties.java line 258: > 256: } > 257: > 258: // This method must only be called on the FX application thread. add `Toolkit.getToolkit().checkFxUserThread();` then? modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PreferenceProperties.java line 312: > 310: > 311: // This method must only be called when synchronized on 'mutex'. > 312: public void updateEffectiveValue() { maybe make this method private then: the instance of this class is available via `Platform.getPreferences()` if I read this correctly modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PreferenceProperties.java line 324: > 322: > 323: // This method must only be called on the FX application thread. > 324: public void fireValueChangeIfNecessary() { same here: make the method `private`. modules/javafx.graphics/src/test/java/test/javafx/css/CssParser_mediaQuery_Test.java line 316: > 314: void invalidCombinationOfConjunctionAndNegationEvaluatesToFalse() { > 315: Stylesheet stylesheet = new CssParser().parse(""" > 316: @media (prefers-reduced-motion: reduce) and not > (prefers-color-scheme: dark) { for my education, how can one express this logic? i.e. `prefers A and not B` ? ------------- PR Review: https://git.openjdk.org/jfx/pull/1655#pullrequestreview-2818605375 PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2075664290 PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2075663501 PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2075688616 PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2075689325 PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2075698899
