RFR: 8301302: Platform preferences API [v3]

Mon Jul 24 17:45:34 UTC 2023

On Thu, 2 Feb 2023 19:54:33 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> Please read [this document](https://gist.github.com/mstr2/9f46f92c98d3c86aa6a0b4224a9a6548) for an introduction to the Platform Preferences API, and how it interacts with the proposed style theme and stage appearance features.
>
> Michael Strauß has updated the pull request with a new target base due to a merge or a rebase.

I think this would still be good to add to JavaFX as it will allow better integration with the platforms it runs on, without requiring the user to query native libraries to good integration.

> > 2. ObservableMap.  Similarly to Node.getProperties(), I wonder if there might be a better way to observe the changes.  May be a different metaphor (subscription?), like adding a value change listener to a specific key.  We do need a set of keys (perhaps that can be an ObservableSet).  Having said that, ObservableMap is good enough solution, and forgive me for stating the obvious, it should not initialize anything if the platform properties have not been requested by the application code.
> 
> I've pulled on that string a little more:
> 
> #### 1. `Optional` API + listeners
> This would remove the `ObservableMap` implementation, and add the following methods instead:
> 
> ```java
> interface Preferences {
>     ...
>     void addListener(String key, InvalidationListener listener);
>     void removeListener(String key, InvalidationListener listener);
> 
>     void addListener(String key, ChangeListener<?> listener);
>     void removeListener(String key, ChangeListener<?> listener);
> 
>     Optional<Integer> getInteger(String key);
>     Optional<Double> getDouble(String key);
>     ...
> }
> ```
> 
> I don't quite like this idea though, since it would allow developers to either add listeners to non-existent keys, or require developers to probe whether a key exists before adding a listener for it (maybe by also adding a `boolean keyExists(String key)` method. 

This kind of functionality already exists, see for example `Bindings#stringValueAt`; you can bind a key of a map (even if it doesn't exist, in which case it will be `null`). It's provided as separate from `ObservableMap`, although it could have been integrated as well.

> It also doesn't allow developers to enumerate the keys that are available on a given platform. If we added a set of keys (maybe as an `ObservableSet`), the result is basically a map. We're better off implementing `ObservableMap` in this case.
>  
> This API combines all aspects into a single method call. We also can't enumerate the platform preferences.
> 
> I still think that implementing `ObservableMap` is preferable to all of these alternatives.

I was on the fence about this before as well, but I think `ObservableMap` may be the way to go now.  The thing that mainly irked me is all the mutation methods that will also be available, but I suppose that's inherent in the design of collections in Java.

> > With the preferences Map, this could work similar perhaps; set a key to whatever you want with put, and restore it to its original value by setting it to null.
> 
> This works when the changes originate from the OS, but it doesn't work when an application overrides preference mappings manually. `ObservableMap` doesn't support bulk changes, so repeatedly calling `override(...)` would end up firing multiple change notifications, and subscribers would have no way of knowing when the bulk change transaction has ended.
> 
> That's where the concept of _uncommitted modifications_ comes into play: calling `override(...)` or any of the property setters like `setAppearance(...)` doesn't apply the changes immediately, it delays those changes until `commit()` is called or until an OS event causes the preference to be recomputed. When that happens, a single invalidation notification is fired, the same as it would have if the change originated from the OS.

I'm not convinced that a delayed change + commit system is the correct way to do this. Properties should behave the same everywhere in JavaFX and this seems to change how they work quite a bit.

Instead, I propose to look at how layout in JavaFX is handling this problem. Layout looks at thousands of properties, yet changing one or many of the involved properties does not involve an expensive layout recalculation per change. Instead, changes are tracked by marking certain aspects of the involved controls dirty. On the next pulse, the layout code notices that something that would influence layout and CSS decisions has changed, and performs the required changes. The properties involved are all normal properties, that can be changed quickly, reflect their current value immediately and that can be overridden by the user or reset back to defaults. There is no override or commit system needed.

Have you considered allowing users to change preference values directly, but not acting on those changes until the next pulse occurs? Users can still listen for keys/properties, just like they can for layout properties, but the major changes that involve recomputing CSS is only done once per pulse.

This would make it possible to change several preference values without penalty (which happens on the FX thread anyway, so pulses are on hold during that time), and they're automatically "committed" once the user is done on the FX thread and the next pulse fires.  I think it would be a very good fit.

> However, I'm thinking that the Preferences API might not be the right place to solve this problem. Maybe that should be built into the style theme API instead, for example with another interface:
> 
> ```java
> public interface PlatformStyleTheme extends StyleTheme {
>     void onPreferencesChanged(Iterable<MapChangeListener.Change<String, Object>> changes);
> }
> ```
> 
> The Preferences _implementation_ would then aggregate the changes that have accumulated since the last pulse, and inform the current style theme by invoking its `onPreferencesChanged` method. This means that a style theme doesn't need to register listeners for different kinds of preferences, and the Preferences API can behave just as any normal property would behave.

This would be an improvement, and since it is synced to a pulse it would do the calculation only once and only when necessary.
I don't think you need to bother with aggregating the changes though, the theme can query whatever it needs easily enough and determine if a re-style is needed.

I'm still not sold on the `override` method.  Would it not be better to make the map writable (with `put`) and if there is need to reset properties back to default offer a `reset` method?  I'm saying this because adding an override method makes the map effectively writable (although delayed, if that's not going to be changed), and in that case I'd prefer using existing map methods for this purpose.

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

PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1498951848
PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1501205183
PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1519428999
PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1521494003
PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1521504250