8301302: Platform preferences API #1014
Conversation
|
👋 Welcome back mstrauss! A progress list of the required criteria for merging this PR into |
|
Just commenting on the convenience API part. I think in modern Java you'd use In this way you could get a certain key that may be platform specific like this (names made up): |
That's a good suggestion, I've updated the API accordingly. |
|
It is premature to review this, since we have not had a discussion of whether we want such an API in the core of JavaFX. I am moving it back to Draft. When/if it is ready to be reviewed it will need a CSR. /csr needed |
openjdk
bot
added
csr
|
@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request. @mstr2 please create a CSR request for issue JDK-8301302 with the correct fix version. This pull request cannot be integrated until the CSR request is approved. |
|
@kevinrushforth |
|
In the context of adding theme support in javafx, I think this PR is a step in the right direction. I also like a small set of platform-independent properties like fg and bg colors. I wonder if the same approach can be extended for other aspects of platform L&F, like fonts, spacing, and may be other aspects (like, hiding scrollbars setting on Mac?) I agree with @kevinrushforth - we'd need more discussion on the actual implementation. There are a few items that I feel are unnecessary, or might be done differently, and I'd like to learn what other people think. Specifically:
What do you think? |
That could indeed be a useful enhancement.
While dark/light mode can indeed be detected just by comparing foreground and background color, the reason for the By the way: the "auto" appearance of macOS is not actually a third appearance. It just swiches automatically from light to dark (and back) depending on the time of day. When a JavaFX application binds its stage apperance to the platform appearance (as described in the gist), this works as expected.
The use of |
|
On Tue, Jan 31, 2023 at 3:00 PM Andy Goryachev ***@***.***> wrote:
In the context of adding theme support in javafx, I think this PR is a
step in the right direction. I also like a small set of
platform-independent properties like fg and bg colors. I wonder if the same
approach can be extended for other aspects of platform L&F, like fonts,
spacing, and may be other aspects (like, hiding scrollbars setting on Mac?)
I agree with @kevinrushforth <https://github.com/kevinrushforth> - we'd
need more discussion on the actual implementation. There are a few items
that I feel are unnecessary, or might be done differently, and I'd like to
learn what other people think.
Specifically:
1. Appearance enum seems unnecessary - there might be more choices in
a specific platform (Mac Ventura has three: dark/light/auto).
Is it necessary for any application to know about "auto"? Presumably when
the mode automatically changes from one to the other there would be an
event that should be handled, just as if the user changed the setting while
the application was running.
Scott
… Message ID: ***@***.***>
|
|
the reason for the Appearance enumeration and the Preferences.appearance property is to support dark window frames.
I think that, by default, the FX frame decorations should pick up the platform theme (dark, light, accent color, etc.). It should be possible to override this behavior somehow - possibly by setting the base/accent color - but using an enum for this purpose feels unnecessary.
Saving the allocation of just a single map with 20-or-so key-value pairs is not a convincing reason to complicate the implementation
It's not just allocation, but also listening to changes at run time. For example, the user changing the theme from light to dark should, in my opinion, update any running FX application, unless otherwise is prescribed by the application requirements.
…-andy
From: mstr2 ***@***.***>
Date: Tuesday, January 31, 2023 at 12:18
To: openjdk/jfx ***@***.***>
Cc: Andy Goryachev ***@***.***>, Comment ***@***.***>
Subject: [External] : Re: [openjdk/jfx] 8301302: Platform preferences API (PR #1014)
In the context of adding theme support in javafx, I think this PR is a step in the right direction. I also like a small set of platform-independent properties like fg and bg colors. I wonder if the same approach can be extended for other aspects of platform L&F, like fonts, spacing, and may be other aspects (like, hiding scrollbars setting on Mac?)
That could indeed be a useful enhancement.
1. Appearance enum seems unnecessary - there might be more choices in a specific platform (Mac Ventura has three: dark/light/auto). Perhaps using fg/bg color intensity is sufficient to find out whether the overall theme is "dark" or "light".
While dark/light mode can indeed be detected just by comparing foreground and background color, the reason for the Appearance enumeration and the Preferences.appearance property is to support dark window frames. In this gist<https://urldefense.com/v3/__https:/gist.github.com/mstr2/9f46f92c98d3c86aa6a0b4224a9a6548__;!!ACWV5N9M2RV99hQ!IMBktwM52wz3rGVNfcYTrwhd0QKoYAK2MpRFZkhslCqpdNN8ca8ilF8yLnMfQBywAFpIZ-OirPQAk0US0ltRtKf2sbTJfA$> (see "Stage appearance"), I've described how the stage appearance and platform appearance APIs interact.
1. 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.
The use of ObservableMap is debatable.
I think that always initializing platform properties makes it easier to reason about the code. Saving the allocation of just a single map with 20-or-so key-value pairs is not a convincing reason to complicate the implementation, especially since most of the platform preferences implementation lives in the Glass toolkit, and not in the user-facing framework.
—
Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https:/github.com/openjdk/jfx/pull/1014*issuecomment-1411006332__;Iw!!ACWV5N9M2RV99hQ!IMBktwM52wz3rGVNfcYTrwhd0QKoYAK2MpRFZkhslCqpdNN8ca8ilF8yLnMfQBywAFpIZ-OirPQAk0US0ltRtKeGSZTOsg$>, or unsubscribe<https://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/AZQ34ZHDAK2M2JVQWMHVXNDWVFXQPANCNFSM6AAAAAAUJ6L3R4__;!!ACWV5N9M2RV99hQ!IMBktwM52wz3rGVNfcYTrwhd0QKoYAK2MpRFZkhslCqpdNN8ca8ilF8yLnMfQBywAFpIZ-OirPQAk0US0ltRtKf_uwcosw$>.
You are receiving this because you commented.Message ID: ***@***.***>
|
|
Is it necessary for any application to know about "auto"?
I actually don't know how the "auto" behaves. From the description it seems the colors might actually change gradually throughout the day, thus making an enum useless and necessitating the use of the actual colors.
***@***.***
-andy
From: Scott Palmer ***@***.***>
Date: Tuesday, January 31, 2023 at 15:05
To: openjdk/jfx ***@***.***>
Cc: Andy Goryachev ***@***.***>, Comment ***@***.***>
Subject: [External] : Re: [openjdk/jfx] 8301302: Platform preferences API (PR #1014)
On Tue, Jan 31, 2023 at 3:00 PM Andy Goryachev ***@***.***> wrote:
In the context of adding theme support in javafx, I think this PR is a
step in the right direction. I also like a small set of
platform-independent properties like fg and bg colors. I wonder if the same
approach can be extended for other aspects of platform L&F, like fonts,
spacing, and may be other aspects (like, hiding scrollbars setting on Mac?)
I agree with @kevinrushforth <https://github.com/kevinrushforth><https://urldefense.com/v3/__https:/github.com/kevinrushforth*3E__;JQ!!ACWV5N9M2RV99hQ!KHKaL2kyeeUaTInIrS8Uhsv6ERm_8WkoL8AnRSjL0wrZHd7sAwYhgS-04iFgvIaFFG8jBOn21R4qDKP5cQK_qehehGOxHA$> - we'd
need more discussion on the actual implementation. There are a few items
that I feel are unnecessary, or might be done differently, and I'd like to
learn what other people think.
Specifically:
1. Appearance enum seems unnecessary - there might be more choices in
a specific platform (Mac Ventura has three: dark/light/auto).
Is it necessary for any application to know about "auto"? Presumably when
the mode automatically changes from one to the other there would be an
event that should be handled, just as if the user changed the setting while
the application was running.
Scott
Message ID: ***@***.***>
—
Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https:/github.com/openjdk/jfx/pull/1014*issuecomment-1411193097__;Iw!!ACWV5N9M2RV99hQ!KHKaL2kyeeUaTInIrS8Uhsv6ERm_8WkoL8AnRSjL0wrZHd7sAwYhgS-04iFgvIaFFG8jBOn21R4qDKP5cQK_qejGo1dBdQ$>, or unsubscribe<https://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/AZQ34ZCIPVHZMVEXMC2WO7TWVGLB7ANCNFSM6AAAAAAUJ6L3R4__;!!ACWV5N9M2RV99hQ!KHKaL2kyeeUaTInIrS8Uhsv6ERm_8WkoL8AnRSjL0wrZHd7sAwYhgS-04iFgvIaFFG8jBOn21R4qDKP5cQK_qehZTOIwSA$>.
You are receiving this because you commented.Message ID: ***@***.***>
|
|
mailing list ate a screenshot from the last message. here it is:
|
I think it shouldn't do that automatically, because the default themes do not support a dark mode out of the box. No theme will support multiple appearances without being specifically designed to do so. If a theme supports multiple appearances, the "just use the platform appearance" behavior only requires a single line of code, which also takes care of macOS's "auto" appearance: var stage = new Stage();
stage.appearanceProperty().bind(Platform.getPreferences().appearanceProperty());By the way, On Windows, the dark appearance adds the |
|
Mailing list message from Scott Palmer on openjfx-dev: On Tue, Jan 31, 2023 at 6:15 PM Andy Goryachev <angorya at openjdk.org> wrote:
From https://support.apple.com/en-ca/guide/mac-help/mchl52e1c2d2/mac Auto just changes from light to dark based on the "Night Shift" setting, The point being that as far as making the application aware of the current Scott |
|
@mstr2 This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
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. |
I've pulled on that string a little more: 1.
|
|
In general, platform preferences correspond to OS-level settings and are updated dynamically. Third-party themes might integrate platform preferences into their look and feel, which is often what users expect to see. But consider a scenario where an application uses a third-party theme that adapts to the OS appearance, but the application author only wants to support a dark appearance (independent from the OS appearance). For this scenario, platform preferences should be overridable from application code. I've considered several potential approaches: 1. Provide two sets of preferences:class Platform {
Preferences getUserPreferences();
Preferences getPlatformPreferences();
}In this idea, 2. Add a separate, overridable property for each of the convenience API properties:interface Preferences {
...
ReadOnlyObjectProperty<Appearance> appearanceProperty();
ObjectProperty<Appearance> appearanceOverrideProperty();
...
}The value of the read-only 3. Add a special setter for each of the convenience API properties:interface Preferences {
...
ReadOnlyObjectProperty<Appearance> appearanceProperty();
void setAppearance(Appearance appearance);
...
}The I prefer the third option (the special setter), as it seems to be the cleanest approach that doesn't unnecessarily expand the API. |
This kind of functionality already exists, see for example
I was on the fence about this before as well, but I think |
Would an approach similar to how Stylesheets do it be useful here? A property of a control can be overridden programmatically as well as set by CSS. In either case, the getter returns the current value (overridden or set by CSS). When set to a non-null value, the value takes precedence over CSS, and when set to 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. |
That's how the current API works, with a little bit of added complexity: interface Preferences {
...
/**
* Overrides the value of the {@link #appearanceProperty() appearance} property.
* <p>
* Specifying {@code null} clears the override, which restores the value of the
* {@code appearance} property to the platform-provided value.
* <p>
* Calling this method does not update the {@code appearance} property instantaneously;
* instead, the property is only updated after calling {@link #commit()}, or after the
* occurrence of an operating system event that causes the {@code appearance} property
* to be recomputed.
*
* @param appearance the platform appearance override, or {@code null} to clear the override
*/
void setAppearance(Appearance appearance);
...
/**
* Overrides a key-value mapping.
* <p>
* If a platform-provided mapping for the key already exists, calling this method overrides
* the value that is mapped to the key. If a platform-provided mapping for the key doesn't
* exist, this method creates a new mapping.
* <p>
* Specifying a {@code null} value clears the override, which restores the value mapped to
* the key to the platform-provided value. If the platform does not provide a mapping for
* the specified key, the mapping is effectively removed.
* <p>
* Calling this method does not update the mapping instantaneously; instead, the mapping
* is only updated after calling {@link #commit()}, or after the occurrence of an operating
* system event that causes the mapped value to be recomputed.
*
* @param key the key
* @param value the new value, or {@code null} to clear the override
* @throws NullPointerException if {@code key} is null
* @throws IllegalArgumentException if a platform-provided mapping for the key exists, and
* the specified value is an instance of a different class
* than the platform-provided value
* @return the previous value associated with {@code key}
*/
<T> T override(String key, T value);
/**
* Commits outstanding overridden preferences, which also causes the values of derived
* properties to be recomputed.
*/
void commit();
}It is very likely the case that changing preferences can lead to very expensive operations in large real-world applications. For example, style themes or the entire user interface may be recreated, icons/images may be loaded, etc. The This works when the changes originate from the OS, but it doesn't work when an application overrides preference mappings manually. That's where the concept of uncommitted modifications comes into play: calling |
|
@swpalmer @hjohn @andy-goryachev-oracle @kevinrushforth The main reason why I think this should go into core JavaFX is that it is adding a useful feature that can only be reasonably provided by the native windowing toolkits, and not by third-party libraries. |
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. |
I think this could work, but it also means giving up on instant change notifications. A call to |
That's not quite what I meant. You can add listeners still and get instant change notifications. Just like when I listen to the I would think the same is possible with say the appearance property. When I change it from LIGHT to DARK, everyone interested gets this notification immediately. On the next pulse, the change is noticed and only then do we change the stylesheets or make other adjustments that are high impact. Basically, the computationally expensive stuff happens during a pulse; it could register invalidation listeners on properties of interest which just set a flag, that is checked and reset on the next pulse. I'm not 100% sure, but it seems you want to add listeners to these properties yourself to instantly trigger the Theme updating code -- I'm saying, only set a boolean that they've changed, check it on the next pulse using |
The javadoc for |
| public static WindowsHighContrastScheme fromThemeName(String themeName) { | ||
| if (themeName == null) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
This method is called only from PlatformImpl that already does the null check on the the string. In general, null checks should be done on the "outer most layer" and then all the inner layers can rely on the value being non-null.
Is this method expected to be called from other places as well? If not, the method can be made package visible.
There was a problem hiding this comment.
The method now returns NONE when another constant doesn't apply. I've removed the public modifier as you've suggested.
| private final List<InvalidationListener> invalidationListeners = new CopyOnWriteArrayList<>(); | ||
| private final List<MapChangeListener<? super String, Object>> mapChangeListeners = new CopyOnWriteArrayList<>(); |
There was a problem hiding this comment.
Can these be modified concurrently? What is the need for CopyOnWriteArrayList?
There was a problem hiding this comment.
It's to prevent ConcurrentModificationException if a listener implementation adds or removes itself (or another listener).
modules/javafx.graphics/src/main/java/javafx/application/Platform.java
Outdated
Show resolved
Hide resolved
| * | ||
| * @since 22 | ||
| */ | ||
| public interface Preferences extends ObservableMap<String, Object> { |
There was a problem hiding this comment.
Note that this will be the first case where a public API implementation of ObservableMap is not for a general use type, but for a specific use case. All previous implementations are:
MapBinding, MapExpression, MapProperty, MapPropertyBase, ReadOnlyMapProperty, ReadOnlyMapPropertyBase, ReadOnlyMapWrapper, SimpleMapProperty, and all are from the base module.
This might be fine, but consider composition here.
@kevinrushforth what do you think?
There was a problem hiding this comment.
I think this is OK. Given what it is being used for, I'm satisfied with the API. One thing we could consider is an @implNote indicating that applications are not expected to implement this. This could be done as a follow-up.
| * always available. | ||
| * <p> | ||
| * The following preferences are potentially available on the specified platforms: | ||
| * <table id="preferences-table"> |
There was a problem hiding this comment.
Long tables can benefit from alternating colored rows. This can be achieved with <table class="striped"> I think.
There was a problem hiding this comment.
That might be a good enhancement for other tables as well (GraphicsContext comes to mind). I would recommend this as a follow-up.
| Color getForegroundColor(); | ||
|
|
||
| /** | ||
| * The accent color. |
There was a problem hiding this comment.
I think that this needs to explanation on what the accent color is.
There was a problem hiding this comment.
I'll create a follow-up issue for that.
There was a problem hiding this comment.
Here it is: JDK-8321573
| /** | ||
| * The color used for background regions. | ||
| * <p> | ||
| * If the platform does not report a background color, this property defaults to {@code Color.WHITE}. |
There was a problem hiding this comment.
Is there value in writing this sentence on every property if they specify the @defaultValue?
There was a problem hiding this comment.
It might make it a bit more clear that this is the default value even on platforms that don't even have this preference.
| ColorScheme getColorScheme(); | ||
|
|
||
| /** | ||
| * The color used for background regions. |
There was a problem hiding this comment.
Maybe "The color used for background of regions"?
There was a problem hiding this comment.
But not all regions are backgrounds in this sense.
There was a problem hiding this comment.
I wonder if it's worth specifying the corresponding properties in JavaFX that match the ones in this class. For example, that the background property is used for Region#backgroundProperty(), and the foreground color is used for Shape#fillProperty() (or however TextField colors its text).
There was a problem hiding this comment.
With the exception of the high-contrast theme on Windows, JavaFX doesn't make any use of these platform-specific properties. It's up to the application to do that.
| * If the platform does not report a foreground color, this property defaults to {@code Color.BLACK}. | ||
| * | ||
| * @return the {@code foregroundColor} property | ||
| * @defaultValue {@code Color.BLACK} |
There was a problem hiding this comment.
Is BLACK a good default? From what I remember, because some devices have a difficulty with pure black, some dark gray color is used instead.
There was a problem hiding this comment.
I would expect this to be one of the properties that is set on all platforms anyway, but if not, I think this BLACK is a reasonable default.
| if (highContrastScheme == null) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Minor: You could eliminate the null check if you defined a new "NONE" or "UNKNOWN" enum and restored the (no-op) default: on line 837. It's fine the way you have it, if you prefer.
There was a problem hiding this comment.
I've added a NONE constant.
| for (WindowsHighContrastScheme item : values()) { | ||
| for (ResourceBundle resourceBundle : resourceBundles) { |
There was a problem hiding this comment.
Depending on how often this is called, might it be worth caching a Map<String,WindowsHighContrastScheme> whose keys are the OS theme names and values are the corresponding enum values? This could be a follow-up enhancement if it were deemed important enough, but maybe it doesn't matter.
There was a problem hiding this comment.
This class will probably change a bit and be moved around if and when we add style themes, so I think we can revisit it then.
| * | ||
| * @since 22 | ||
| */ | ||
| public interface Preferences extends ObservableMap<String, Object> { |
There was a problem hiding this comment.
I think this is OK. Given what it is being used for, I'm satisfied with the API. One thing we could consider is an @implNote indicating that applications are not expected to implement this. This could be done as a follow-up.
| * always available. | ||
| * <p> | ||
| * The following preferences are potentially available on the specified platforms: | ||
| * <table id="preferences-table"> |
There was a problem hiding this comment.
That might be a good enhancement for other tables as well (GraphicsContext comes to mind). I would recommend this as a follow-up.
| * @see javafx.application.Platform.Preferences#colorSchemeProperty() | ||
| * @since 22 | ||
| */ | ||
| public enum ColorScheme { |
There was a problem hiding this comment.
I suspect this is limited to the current choices, but it's a good question.
There was a problem hiding this comment.
With the exception of the high-contrast theme on Windows, JavaFX doesn't make any use of these platform-specific properties. It's up to the application to do that.
| * If the platform does not report a foreground color, this property defaults to {@code Color.BLACK}. | ||
| * | ||
| * @return the {@code foregroundColor} property | ||
| * @defaultValue {@code Color.BLACK} |
There was a problem hiding this comment.
I would expect this to be one of the properties that is set on all platforms anyway, but if not, I think this BLACK is a reasonable default.
|
Windows high contrast seems to work fine now. However, I'm building your PR (./gradlew sdk) and running the test on Mac, but I get: Note that files (classes, resources) from |
|
Good catch. I hadn't tested the latest update on Mac or Linux this time. |
I've changed this so that these resources are only queried on Windows. |
If we add user-modifiable preferences, these won't be singletons. |
| * <p> | ||
| * The high contrast feature may not be available on all platforms. | ||
| */ | ||
| enum WindowsHighContrastScheme { |
There was a problem hiding this comment.
I'd rather keep the old name: HighContrastScheme: It is in a non-platform specific package, and it applies to all platforms (even only with NONE).
You could add a comment about HIGH_CONTRAST_* enum constants being only available on Windows.
There was a problem hiding this comment.
But it never applied to all platforms, it reflects exactly the Windows implementation of high-contrast schemes. It will be moved once we add style themes.
|
@mstr2 This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 33 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
Thanks to all the people who were involved in developing this feature and helping to bring this across the finish line, especially @andy-goryachev-oracle @kevinrushforth @hjohn @nlisker and @jperedadnr 👍 |
|
/integrate |
|
Going to push as commit 170a4c1.
Your commit was automatically rebased without conflicts. |
|
On to phase 2 :) |
|
Thank you for your work @mstr2. It's nice to see something new and shiny added to JavaFX. I'm looking forward for your other PRs to get integrated as well, I really like your work. Thanks. |
Please read this document for an introduction to the Platform Preferences API, and how it interacts with the proposed style theme and stage appearance features.
Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1014/head:pull/1014$ git checkout pull/1014Update a local copy of the PR:
$ git checkout pull/1014$ git pull https://git.openjdk.org/jfx.git pull/1014/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1014View PR using the GUI difftool:
$ git pr show -t 1014Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1014.diff
Webrev
Link to Webrev Comment