On Mon, 14 Apr 2025 09:56:43 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> improved implementation of NullCoalescingPropertyBase
>
> 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.
Platform preference change events always happen on the FX thread. The reason
for `synchronized` is as follows:
A `Scene` can be created on any thread as per spec. When the scene is created,
the `ScenePreferences` class and its associated properties are instantiated,
which in turn subscribe internally to the `Platform.Preferences` properties.
The synchronization is to protect the addition and removal of listeners.
This is not ideal, though. As soon as we're subscribed to
`Platform.Preferences`, the `Scene.Preferences` can receive change
notifications on the FX thread, which interferes with the mandated capability
to be created on any thread.
I think we have a few options here:
1. Don't subscribe scene preferences to platform preferences until the scene is
shown. The downside of this is that we don't know the actual values of the
preferences up until that point.
2. With limited synchronization, fetch the current preference values from the
platform when the scene is created, but don't subscribe to change notifications
until the scene is shown.
3. Specify that scene preference changes can happen on the FX thread, even when
the scene is created in a background 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.
I might just remove `equals()`/`hashCode()`, because the class is not used in a
way where equality would be relevant.
> 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?
The expression is entirely defined by `featureName`, `featureValue`, and
`value`. Since the associated function is usually a method reference, it won't
equal other method references (even though the referenced method is the same).
There's no point in equating different method references of the same method
because this just means that no two expressions are ever equal.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2042064946
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2042084919
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2042073727
