8357067: Platform preference change can emit multiple notifications #1810

mstr2 · 2025-05-15T17:50:50Z

Some platform preference changes can trigger the emission of multiple notifications. For example, when switching from a high-contrast theme on Windows to the regular theme, the following notifications are emitted (log can be viewed in PlatformPreferencesChangedTest):

changed:
	Windows.UIColor.Accent=0x0078d4ff
	Windows.SysColor.COLOR_HIGHLIGHTTEXT=0xffffffff
	Windows.SysColor.COLOR_WINDOW=0xffffffff
	Windows.UIColor.AccentLight1=0x0091f8ff
	Windows.SysColor.COLOR_3DFACE=0xf0f0f0ff
	Windows.SysColor.COLOR_HOTLIGHT=0x0066ccff
	Windows.SysColor.COLOR_WINDOWTEXT=0x000000ff
	Windows.SysColor.COLOR_BTNTEXT=0x000000ff
	Windows.UIColor.Foreground=0x000000ff
	Windows.UIColor.AccentDark1=0x0067c0ff
	Windows.UIColor.Background=0xffffffff
	Windows.UIColor.AccentLight3=0x99ebffff
	Windows.UIColor.AccentLight2=0x4cc2ffff
	Windows.SysColor.COLOR_GRAYTEXT=0x6d6d6dff
	Windows.SysColor.COLOR_HIGHLIGHT=0x0078d7ff
	Windows.UIColor.AccentDark2=0x003e92ff
	Windows.UIColor.AccentDark3=0x001a68ff

changed:
	-Windows.SPI.HighContrastColorScheme
	Windows.SPI.HighContrast=false

changed:
	Windows.UIColor.Foreground=0xffffffff
	Windows.UIColor.Background=0x000000ff

Notably, the values for Windows.UIColor.Foreground/Background are inconsistent between the notifications (although they are eventually correct). In general, only a single notification should be emitted that includes all of the changed preferences.

This artifact is only visible on Windows. The reason is that changing some system settings (like high-contrast theme) causes a number of different window messages to be sent to the application. We should wait for all window messages to come in, and then aggregate all of the changed preferences into a single notification.

In order to minimize the delay between changing a system setting and sending out the notification in JavaFX, the implementation only waits when instructed to do so by the native toolkit. This allows us to instantly send out notifications for most changes, but selectively wait a bit for changes where the native toolkit knows that more changes might be coming in.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8357067: Platform preference change can emit multiple notifications (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1810/head:pull/1810
$ git checkout pull/1810

Update a local copy of the PR:
$ git checkout pull/1810
$ git pull https://git.openjdk.org/jfx.git pull/1810/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1810

View PR using the GUI difftool:
$ git pr show -t 1810

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1810.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-05-15T17:51:20Z

👋 Welcome back mstrauss! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-05-15T17:52:02Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

mlbridge · 2025-05-15T17:55:12Z

Webrevs

mstr2 · 2025-05-15T17:56:11Z

/reviewers 2

openjdk · 2025-05-15T17:56:27Z

@mstr2
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

andy-goryachev-oracle · 2025-05-15T18:11:17Z

modules/javafx.graphics/src/main/java/com/sun/glass/ui/Application.java

@@ -84,7 +84,7 @@ public void handleOpenFilesAction(Application app, long time, String files[]) {
        // currently used only on Mac OS X
        public void handleQuitAction(Application app, long time) {
        }
-        public void handlePreferencesChanged(Map<String, Object> preferences) {
+        public void handlePreferencesChanged(Map<String, Object> preferences, int suggestedDelayMillis) {


I wonder if we should always debounce the changes with a short delay?
A delay of 100-150 ms will be acceptable from the user perspective.
What do you think?

Most changed settings are not correlated with other settings, so no waiting is necessary (for example, we wouldn't wait after a reducedMotion change). We only need to wait a bit when a single user-facing setting can affect several correlated preferences. This only seems to be the case on Windows, and only when changing high-contrast themes.

andy-goryachev-oracle · 2025-05-15T18:13:35Z

modules/javafx.graphics/src/main/native-glass/win/PlatformSupport.h

+    /**
+     * Suggested aggregation delay for changes that come in over a period of time.
+     */
+    static constexpr int SUGGESTED_DELAY_MILLIS = 1000;


1 second seems too long.
What is the typical range for the high contrast train of changes?

Quite a lot, Windows takes around 1-2 seconds to change to and from a high-contrast theme (there's even a full-frame wait screen that pops up). The suggested delay of 1 second works reliably on my machine.

If it takes that long to update all the colors, maybe we should not add complexity to this subsystem and just send the events as they come?

What exactly is the problem the user experiences right now?

A single user-facing setting change should only result in a single change notification in JavaFX, as that's what developers would intuitively expect. I'm planning to add support for CSS system colors soon, which basically exposes platform colors to stylesheet authors. When the color palette changes, I want all changes to be applied at the same time and not piece by piece.

Are you sure a delay is a good way to solve this? It won't guarantee that no inconsistent states will be observed (what if Windows is slower, or a change is reverted within the delay period -- you may still see "half" changes).

I don't think that there's a perfect way to solve this. Yes, this can theoretically happen, and then you'll see a visual glitch. I can imagine that this might also be the reason why Windows shows a full-screen popup when you switch to and from a high-contrast theme: to hide glitchy applications during the transition period.

Alright, just making sure, as you mentioned in another thread seeing inconsistent transient states isn't an acceptable trade-off. They can still occur, and so users can't rely on them. In that respect the situation is similar to before this change, other than it being less likely to occur (and perhaps never or rarely in practice).

Yes, I think having inconsistent transient states is not an acceptable trade-off, that's why I think reducing them by 99,9% (from guaranteed to happen, to almost hever happening) is sensible. If it does happen anyway, it shouldn't crash your application but show up as a visual glitch (I mean, we're talking about colors). In that sense, you can "rely" on it.

andy-goryachev-oracle · 2025-05-15T19:00:01Z

I am not comfortable with this change, as it falls under application requirements.

The function of the JavaFX platform is to communicate the OS events as faithfully as it can, in my opinion. If the application requirements call for some debouncing, especially that with a one second delay, then the application is free to implement that.

If, however, the application requirements call for no delay, this change makes it impossible.

mstr2 · 2025-05-15T19:16:03Z

I am not comfortable with this change, as it falls under application requirements.

The function of the JavaFX platform is to communicate the OS events as faithfully as it can, in my opinion. If the application requirements call for some debouncing, especially that with a one second delay, then the application is free to implement that.

The platform preferences API is an abstraction, and nowhere do we specify that it replicates the exact stream of OS events. What we do (implicitly) specify, however, is that the observable state of platform preferences accurately reflects the OS settings. Do you really think that it is okay for an application to be able to observe inconsistent transient states? In this example, applications can observe inconsistent foregroundColor and backgroundColor values.

If, however, the application requirements call for no delay, this change makes it impossible.

An application can require all it wants, but "inconsistent transient states are okay" is not an acceptable trade-off. There's no delay for uncorrelated changes, which is as good as it gets.

drmarmac · 2025-05-15T19:32:56Z

I'm in favor of abstracting this away as good as possible, i.e. trying to aggregate such events if some sufficiently universal timing parameters (or something better than time-based logic) can be found. As an application developer I often choose javafx especially because it keeps OS-implementation dependent behavior to a minimum and reduces the likelihood of me "using things the wrong way", like having inconsistent states that might need to be handled. And who knows what the next version of windows will do...

hjohn · 2025-05-15T20:34:44Z

I am not comfortable with this change, as it falls under application requirements.

The function of the JavaFX platform is to communicate the OS events as faithfully as it can, in my opinion

The function of JavaFX is to abstract the platform it runs on, preferably in such a way that one can't distinguish between platforms at all, just like about every other API the JDK offers (ie. NIO, Threads, Swing, etc). This is one of the reasons we don't expose window handles or DirectX/GL API's to users.

add delayed change aggregation

a512dc4

openjdk bot added the rfr Ready for review label May 15, 2025

andy-goryachev-oracle reviewed May 15, 2025

View reviewed changes

simplified DelayedChangedAggregator

8fce572

use try-finally

78139e5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8357067: Platform preference change can emit multiple notifications #1810

8357067: Platform preference change can emit multiple notifications #1810

mstr2 commented May 15, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented May 15, 2025

openjdk bot commented May 15, 2025

mlbridge bot commented May 15, 2025 •

edited

Loading

mstr2 commented May 15, 2025

openjdk bot commented May 15, 2025

andy-goryachev-oracle May 15, 2025

mstr2 May 15, 2025

andy-goryachev-oracle May 15, 2025

mstr2 May 15, 2025

andy-goryachev-oracle May 15, 2025

mstr2 May 15, 2025

hjohn May 15, 2025

mstr2 May 15, 2025

hjohn May 15, 2025

mstr2 May 15, 2025

andy-goryachev-oracle commented May 15, 2025

mstr2 commented May 15, 2025

drmarmac commented May 15, 2025

hjohn commented May 15, 2025

8357067: Platform preference change can emit multiple notifications #1810

Are you sure you want to change the base?

8357067: Platform preference change can emit multiple notifications #1810

Conversation

mstr2 commented May 15, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewing

bridgekeeper bot commented May 15, 2025

openjdk bot commented May 15, 2025

mlbridge bot commented May 15, 2025 • edited Loading

Webrevs

mstr2 commented May 15, 2025

openjdk bot commented May 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-goryachev-oracle commented May 15, 2025

mstr2 commented May 15, 2025

drmarmac commented May 15, 2025

hjohn commented May 15, 2025

mstr2 commented May 15, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented May 15, 2025 •

edited

Loading