8343398: Add reducedData preference #1656

mstr2 · 2024-12-03T15:17:19Z

The reducedData preference instructs applications to minimize internet traffic, as users might be on a metered network or a limited data plan.

This corresponds to the following OS settings:

Windows: Settings -> Network and Internet -> Ethernet/WiFi -> Metered connection
macOS: Settings -> Network -> Ethernet/WiFi -> Network Settings -> Low data mode
Ubuntu: Settings -> Network -> Wired/WiFi -> Metered connection

Change notifications work consistently on Windows and macOS. On my Ubuntu 24 system, the GIO network-changed signal is not sent when I only toggle the "metered connection" flag in network settings (and there's no signal specifically for low-data mode). The new value is only picked up when the connection changes by coming offline or going online.

/reviewers 2
/csr

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)
Change requires CSR request JDK-8345677 to be approved

Issues

JDK-8343398: Add reducedData preference (Enhancement - P4)
JDK-8345677: Add reducedData preference (CSR)

Reviewers

Andy Goryachev (@andy-goryachev-oracle - Reviewer)
Kevin Rushforth (@kevinrushforth - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1656

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-12-03T15:18:00Z

👋 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 · 2024-12-03T15:18:23Z

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

openjdk · 2024-12-03T15:18:57Z

@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).

openjdk · 2024-12-03T15:19:29Z

@mstr2 has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@mstr2 please create a CSR request for issue JDK-8343398 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

mlbridge · 2024-12-03T15:23:15Z

Webrevs

mstr2 · 2024-12-04T15:59:26Z

This PR also includes a small refactor of the GTK and macOS PlatformSupport classes.

The previous implementations had some parts of the change notification mechanism implemented outside of PlatformSupport in their respective GlassApplication class. The refactor moves this into PlatformSupport, which centralizes all aspects related to platform preference change detection.

andy-goryachev-oracle · 2024-12-11T16:02:27Z

Does not seem to work on macOS 15.1.1 M1:

andy-goryachev-oracle · 2024-12-11T16:05:02Z

mstr2 · 2024-12-11T16:15:05Z

Does not seem to work on macOS 15.1.1 M1:

It works on my 14.6 machine. Let me try upgrading and testing again...

andy-goryachev-oracle · 2024-12-11T16:17:04Z

I might suggest adding the instructions on how to control the platform settings as comments somewhere in the implementation.

Windows: Settings -> Network and Internet -> Ethernet/WiFi -> Metered connection
macOS: Settings -> Network -> Ethernet/WiFi -> Network Settings -> Low data mode
Ubuntu: Settings -> Network -> Wired/WiFi -> Metered connection

andy-goryachev-oracle · 2024-12-11T16:17:42Z

(I've merged the latest master prior to testing)

mstr2 · 2024-12-11T17:44:11Z

Does not seem to work on macOS 15.1.1 M1:

It also works on 15.1.1 on my machine. Just to check the obvious: did you close the WiFi Details window? The settings are only applied when you click OK.

andy-goryachev-oracle · 2024-12-11T17:58:01Z

Update: I was getting interference from the VPN application. Getting off the VPN helped - I see the property and macOS.*.currentPathConstrained key being toggled.

(macOS.*.currentPathExpensive remains false).

mstr2 · 2024-12-11T18:10:17Z

Update: I was getting interference from the VPN application. Getting off the VPN helped - I see the property and macOS.*.currentPathConstrained key being toggled.

(macOS.*.currentPathExpensive remains false).

That's the expected result.

mstr2 · 2024-12-11T18:14:40Z

I might suggest adding the instructions on how to control the platform settings as comments somewhere in the implementation.
Windows: Settings -> Network and Internet -> Ethernet/WiFi -> Metered connection
macOS: Settings -> Network -> Ethernet/WiFi -> Network Settings -> Low data mode
Ubuntu: Settings -> Network -> Wired/WiFi -> Metered connection

I feel this wouldn't age well, considering that the particular UI used by the different operating systems has often changed over the years.

andy-goryachev-oracle

It looks like the scope of this change goes far beyond adding a few properties. It looks like a non-trivial refactoring that might also impact other platform preferences.

I did check that switching between light/dark modes on macOS results in proper updates, but the native code needs to be re-reviewed.

andy-goryachev-oracle · 2024-12-11T18:33:10Z

modules/javafx.graphics/src/main/native-glass/mac/GlassApplication.m

@@ -204,6 +187,7 @@ - (void)applicationWillFinishLaunching:(NSNotification *)aNotification
    NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
    {
        // unblock main thread. Glass is started at this point.
+        self->platformSupport = [[PlatformSupport alloc] initWithEnv:env application:jApplication];


should we check for failure here? (alloc)

We never do that in obj-c native code, so this probably wouldn't be the place to start. In any case, a failure at this point wouldn't lead to a crash, as the only access of platformSupport checks for nil.

andy-goryachev-oracle · 2024-12-11T20:08:50Z

I feel this wouldn't age well, considering that the particular UI used by the different operating systems has often changed over the years.

I think it has merit. Now that we need to re-test the functionality, it would have helped. Even when something changes, it will be easier to look up a few missing pieces than dig through the PRs...

kevinrushforth · 2024-12-11T20:27:36Z

I feel this wouldn't age well, considering that the particular UI used by the different operating systems has often changed over the years.

I think it has merit. Now that we need to re-test the functionality, it would have helped. Even when something changes, it will be easier to look up a few missing pieces than dig through the PRs...

I think this sort of information is better suited for a Wiki.

Btw, I can review the native code changes. Just not for a couple days.

andy-goryachev-oracle · 2024-12-11T20:33:43Z

tested macOS behavior, all is well.

BTW, I've updated the Monkey Tester to highlight changes in Tools -> Platform Preferences Monitor.

https://github.com/andy-goryachev-oracle/MonkeyTest

andy-goryachev-oracle

tested on macOS. needs testing on linux and windows.

kevinrushforth

The changes to GlassApplication where you assume that [NSApp delegate] is a GlassApplication won't work in the case where the AWT toolkit is initialized first (e.g., a JFXPanel app).

kevinrushforth · 2024-12-18T19:29:39Z

modules/javafx.graphics/src/main/native-glass/mac/GlassApplication.m

+    GlassApplication* app = (GlassApplication*)[NSApp delegate];
+    return [app getPlatformPreferences];


I think this is the cause of the crash. You cannot assume that [NSApp delegate] is a GlassApplication. In the case where AWT initializes the toolkit first, it won't be.

kevinrushforth · 2024-12-18T20:42:25Z

I can reproduce this locally. Any app that initializes the AWT toolkit and then uses FX will hit this, including HelloJFXPanel (in apps/toys/Hello).

mstr2 · 2024-12-18T20:53:55Z

It seems that there's no reliable way to retreive the GlassApplication instance outside of the _runLoop() method where it is created. I've decided to split the method into _initDelegate() and _runLoop(), and store the app delegate reference in the MacApplication class on the Java side. Then the reference can be passed down into the native toolkit for methods that require it (like _getPlatformPreferences()).

kevinrushforth

I'll take a closer look later, but the changes to solve the problem with GlassApplication look good to me.

I'll also fire off another headful test run and report the results.

kevinrushforth · 2024-12-18T21:24:20Z

modules/javafx.graphics/src/main/native-glass/mac/GlassApplication.m

+    return (jlong)[[GlassApplication alloc] initWithEnv:env
+                                            application:japplication
+                                            launchable:jlaunchable
+                                            taskbarApplication:isTaskbarApplication
+                                            classLoader:classLoader];
+}

-        GlassApplication *glass = [[GlassApplication alloc] initWithEnv:env application:japplication launchable:jlaunchable taskbarApplication:isTaskbarApplication classLoader:classLoader];
+/*
+ * Class:     com_sun_glass_ui_mac_MacApplication
+ * Method:    _runLoop
+ * Signature: (J)V
+ */
+JNIEXPORT void JNICALL Java_com_sun_glass_ui_mac_MacApplication__1runLoop
+(JNIEnv *env, jobject japplication, jlong appDelegate)
+{
+    LOG("Java_com_sun_glass_ui_mac_MacApplication__1runLoop");
+
+    NSAutoreleasePool *glasspool = [[NSAutoreleasePool alloc] init];
+    {
+        GlassApplication* glass = (GlassApplication*)appDelegate;


This looks like a clean split to me.

kevinrushforth · 2024-12-18T21:24:30Z

modules/javafx.graphics/src/main/native-glass/mac/GlassApplication.m

 {
    LOG("Java_com_sun_glass_ui_mac_MacApplication__1finishTerminating");

+    if (appDelegate) {
+        [(GlassApplication*)appDelegate release];


This is needed because you moved the allocation of GlassApplication outside (before) the auto-release pool in runLoop, so looks good.

andy-goryachev-oracle

HelloJFXPanel does not fail anymore
for completeness sake, ran swing/fx interop tools in the monkey tester - no problems before and after the latest changes (can't really reproduce the HelloJFXPanel use case with the MT)
see the property events for light/dark mode and reduced data on macOS 15.1.1

kevinrushforth · 2024-12-18T22:48:26Z

Our CI headful test build passed on macOS and Linux (I'll need to run Windows manually later), so the problem is fixed (the other Robot color mismatch test failures were unrelated, and fixed by rebooting the macOS systems in question).

I don't have access to Windows or Linux system where I can turn on the metering option, so I can't test if the notification works. Maybe someone else can?

I'll review the code tomorrow.

kevinrushforth · 2024-12-19T18:07:45Z

The headful test run passed on Windows. While doing manual testing of other settings (since the notification logic has changed), I discovered a reproducible crash on Windows 11:

Run the manual PlatformPreferencesChangedTest app (or any other JavaFX app)
Toggle Settings --> Accessibility --> Visual effects --> Animation effects

JavaFX will crash, likely in response to the notification of a change in animation effect.

mstr2 · 2024-12-19T21:07:56Z

The headful test run passed on Windows. While doing manual testing of other settings (since the notification logic has changed), I discovered a reproducible crash on Windows 11:

Run the manual PlatformPreferencesChangedTest app (or any other JavaFX app)

Toggle Settings --> Accessibility --> Visual effects --> Animation effects

JavaFX will crash, likely in response to the notification of a change in animation effect.

This is a weird one. I've narrowed it down to INetworkInformation::GetInternetConnectionProfile returning null, and then crashing the app. Now, this method is specified to return null if there's no connection profile, so a null-check is obviously in order.

What is strange is that at least on my machine, GetInternetConnectionProfile always returns a non-null value except when the method is called in response to receiving WM_SETCLIENTAREAANIMATION. Note that the method doesn't fail with an error, it simply returns null. I've checked all "obvious" things that might go wrong, such as calling the method on a wrong thread (not the case, it's always the same thread, regardless of whether we react to a window message or a WinRT callback).

The quick fix in this case is simply not calling the API in question in response to the unrelated WM_SETCLIENTAREAANIMATION message. I've added a parameter to collectPreferences() that indicates what type of preferences we're going to collect.

kevinrushforth

The changes all look good now.

I did notice one thing in macOS while reviewing the changes in PlatformSupport.m. The PlatformSupport object stays around after the GlassApplication is terminated via finishTerminating. It will continue to process platform events, including making Java upcalls even after the FX toolkit is stopped. You would only see this with a Swing interop app that initializes FX and then later terminates it while keeping AWT alive. I think it is preexisting, and doesn't seem to be causing any problems. It might be worth a P4 follow-up issue to investigate.

kevinrushforth · 2024-12-20T15:01:00Z

I did notice one thing in macOS while reviewing the changes in PlatformSupport.m. The PlatformSupport object stays around after the GlassApplication is terminated via finishTerminating. It will continue to process platform events, including making Java upcalls even after the FX toolkit is stopped.

Correction: the listeners no longer get notified after the FX toolkit is stopped, perhaps due to the fix forJDK-8335630. Needs more investigation.

kevinrushforth · 2024-12-20T16:53:13Z

Correction: the listeners no longer get notified after the FX toolkit is stopped, perhaps due to the fix forJDK-8335630. Needs more investigation.

This is not related to JDK-8335630. The notification does get delivered, but ends up going through the "else" block in the following code in PlatformImpl::updatePreferences:

        if (isFxApplicationThread()) {
            checkHighContrastThemeChanged(preferences);
            platformPreferences.update(preferences);
        } else {
            // Make a defensive copy in case the caller of this method decides to re-use or
            // modify its preferences map after the method returns. Don't use Map.copyOf
            // because the preferences map may contain null values.
            Map<String, Object> preferencesCopy = new HashMap<>(preferences);
            runLater(() -> updatePreferences(preferencesCopy));
        }

This happens because isFxApplicationThread() returns false after the toolkit is stopped (see Application::finishTerminating). The runLater at the end of the else block never runs and the runnable is discarded, so the else block ends up being a no-op.

So I think my recommendation of "maybe file a P4 bug to look at later" is the most that is needed here.

mstr2 · 2024-12-20T18:02:50Z

The changes all look good now.

I did notice one thing in macOS while reviewing the changes in PlatformSupport.m. The PlatformSupport object stays around after the GlassApplication is terminated via finishTerminating. It will continue to process platform events, including making Java upcalls even after the FX toolkit is stopped. You would only see this with a Swing interop app that initializes FX and then later terminates it while keeping AWT alive. I think it is preexisting, and doesn't seem to be causing any problems. It might be worth a P4 follow-up issue to investigate.

I've added some code that ensures that the PlatformSupport instance stops event processing and is released when GlassApplication shuts down. I've verified this by logging whether PlatformSupport::dealloc is called, which it is.

andy-goryachev-oracle · 2024-12-20T18:19:20Z

I highly recommend syncing up with the latest master once the PR branch is out for a prolonged period. This will ensure that the new changes do not interfere with the latest updates, and it also saves the reviewer's time as they need to merge in order to do a good review (times N reviewers).

andy-goryachev-oracle

Looks good (macOS).

An observation: when running HelloJFXPanel, noticed that the FX animation restarts each time the appearance preference gets changed between light and dark modes. This might be an existing issue since it can also be observed in the master branch.

I am not entirely sure this is expected, since neither the color of the animated rectangle nor the background color changes. It's unrelated to the changes in this PR (but might be related to the way the platform notifications are being processed).

Another observation related to the same animation, and also can be observed with the master branch: when one runs the HelloJFXPanel app, the initial animation is smooth and is not affected by mouse over. Once the platform light/dark mode preference gets changed, however, the movement becomes jerky on mouse over.

kevinrushforth

Looks good.

kevinrushforth · 2024-12-20T19:15:46Z

An observation: when running HelloJFXPanel, noticed that the FX animation restarts each time the appearance preference gets changed between light and dark modes. This might be an existing issue since it can also be observed in the master branch.
I am not entirely sure this is expected, since neither the color of the animated rectangle nor the background color changes. It's unrelated to the changes in this PR (but might be related to the way the platform notifications are being processed).

When I run it, it doesn't reset, it seems to jump -- often, but not always, to the beginning or end. In any case, it's completely unrelated to platform notifications: I just checked and it behaves the same way on JDK 8, which has no platform preference support.

Another observation related to the same animation, and also can be observed with the master branch: when one runs the HelloJFXPanel app, the initial animation is smooth and is not affected by mouse over. Once the platform light/dark mode preference gets changed, however, the movement becomes jerky on mouse over.

I can't reproduce this on my system. If you can characterize this, please file a new bug.

andy-goryachev-oracle · 2024-12-20T19:43:39Z

More on jerky animation: it's unrelated to the platform preferences change (which is good, for this PR anyway).

It seems the jerky movement appears after HelloJFXPanel loses focus to some other window. Switch back, mouse over and the jerkiness starts. Happens even with the second external monitor disconnected.

mstr2 added 4 commits December 3, 2024 01:59

Implementation of reducedData

b473f3e

add linux implementation

9d22732

add macOS implementation

fe22bd5

error message tweak

0245c2e

openjdk bot added the rfr Ready for review label Dec 3, 2024

openjdk bot added the csr Need approved CSR to integrate pull request label Dec 3, 2024

mstr2 added 2 commits December 4, 2024 01:39

javadoc change

5c2e255

refactor macOS PlatformSupport

8b8b1bd

Merge branch 'master' into feature/reduced-data

455613f

andy-goryachev-oracle reviewed Dec 11, 2024

View reviewed changes

kevinrushforth self-requested a review December 11, 2024 20:25

andy-goryachev-oracle approved these changes Dec 11, 2024

View reviewed changes

kevinrushforth requested changes Dec 18, 2024

View reviewed changes

mstr2 added 2 commits December 18, 2024 21:47

store the app delegate reference in MacApplication

43254d3

typo

fd4846a

kevinrushforth self-requested a review December 18, 2024 21:10

kevinrushforth reviewed Dec 18, 2024

View reviewed changes

andy-goryachev-oracle approved these changes Dec 18, 2024

View reviewed changes

mstr2 added 2 commits December 19, 2024 05:13

smaller diff

a590b31

method name

7feba0b

mstr2 added 2 commits December 19, 2024 20:31

only update preferences that may have changed

f838492

change enum constant names

b7840bd

kevinrushforth approved these changes Dec 20, 2024

View reviewed changes

kevinrushforth requested a review from andy-goryachev-oracle December 20, 2024 14:48

andy-goryachev-oracle approved these changes Dec 20, 2024

View reviewed changes

release macOS PlatformSupport instance

6f015a4

mstr2 requested review from kevinrushforth and andy-goryachev-oracle December 20, 2024 18:06

Merge branch 'master' into feature/reduced-data

b163571

andy-goryachev-oracle approved these changes Dec 20, 2024

View reviewed changes

kevinrushforth approved these changes Dec 20, 2024

View reviewed changes

		GlassApplication* app = (GlassApplication*)[NSApp delegate];
		return [app getPlatformPreferences];

8343398: Add reducedData preference #1656

Are you sure you want to change the base?

8343398: Add reducedData preference #1656

Conversation

mstr2 commented Dec 3, 2024 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Dec 3, 2024

openjdk bot commented Dec 3, 2024

openjdk bot commented Dec 3, 2024

openjdk bot commented Dec 3, 2024

mlbridge bot commented Dec 3, 2024 • edited Loading

Webrevs

mstr2 commented Dec 4, 2024

andy-goryachev-oracle commented Dec 11, 2024

andy-goryachev-oracle commented Dec 11, 2024

mstr2 commented Dec 11, 2024

andy-goryachev-oracle commented Dec 11, 2024

andy-goryachev-oracle commented Dec 11, 2024

mstr2 commented Dec 11, 2024

andy-goryachev-oracle commented Dec 11, 2024

mstr2 commented Dec 11, 2024

mstr2 commented Dec 11, 2024

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

andy-goryachev-oracle Dec 11, 2024

Choose a reason for hiding this comment

mstr2 Dec 11, 2024

Choose a reason for hiding this comment

andy-goryachev-oracle commented Dec 11, 2024

kevinrushforth commented Dec 11, 2024

andy-goryachev-oracle commented Dec 11, 2024

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth Dec 18, 2024

Choose a reason for hiding this comment

kevinrushforth commented Dec 18, 2024

mstr2 commented Dec 18, 2024

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth Dec 18, 2024

Choose a reason for hiding this comment

kevinrushforth Dec 18, 2024

Choose a reason for hiding this comment

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

kevinrushforth commented Dec 18, 2024

kevinrushforth commented Dec 19, 2024

mstr2 commented Dec 19, 2024

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth commented Dec 20, 2024

kevinrushforth commented Dec 20, 2024

mstr2 commented Dec 20, 2024 • edited Loading

andy-goryachev-oracle commented Dec 20, 2024

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth commented Dec 20, 2024

andy-goryachev-oracle commented Dec 20, 2024

mstr2 commented Dec 3, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 3, 2024 •

edited

Loading

mstr2 commented Dec 20, 2024 •

edited

Loading