Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8243255: Font size is large in JavaFX app with enabled Monocle on Raspberry Pi #193

Conversation

@AlexanderScherbatiy
Copy link

@AlexanderScherbatiy AlexanderScherbatiy commented Apr 21, 2020

See the detailed issue description on: http://mail.openjdk.java.net/pipermail/openjfx-dev/2020-April/025975.html

The fix 8236448 #75 changes MonocleApplication.staticScreen_getScreens() method code from

ns.getDPI(), ns.getDPI(), ns.getScale(), ns.getScale(), ns.getScale(), ns.getScale()

to

ns.getWidth(), ns.getHeight(), 1.f, 1.f, ns.getScale(), ns.getScale()"

so the font size is not properly calculated based on the given DPI value.

I left the platformScaleX and platformScaleY as 1.f because I do not know how it affects Android/Dalvik platform. On Raspberry Pi where I run JavaFX code with Monocle the DispmanScreen is used which have fixed scale 1.0 value so the app works in the same way.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8243255: Font size is large in JavaFX app with enabled Monocle on Raspberry Pi

Reviewers

  • jgneff (no known github.com user name / role)
  • Kevin Rushforth (kcr - Reviewer)
  • Johan Vos (jvos - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/193/head:pull/193
$ git checkout pull/193

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 21, 2020

👋 Welcome back alexsch! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@openjdk openjdk bot added the rfr label Apr 21, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 21, 2020

Webrevs

@kevinrushforth kevinrushforth requested a review from johanvos Apr 21, 2020
@jgneff
Copy link
Contributor

@jgneff jgneff commented Apr 23, 2020

I couldn't understand why I wasn't hitting this error on the embedded Monocle EPD platform. Stepping through the code with the debugger, I found that JavaFX is looking for the javafx.platform.properties file in the wrong place. I moved the file to its parent directory, the location JavaFX expects, and saw the problem right away.

The first image below shows the button and text without the properties file, and the second image shows the problem once the properties file is loaded:

normal toobig

So there might be a secondary problem. After unzipping the SDK archive under $HOME/lib, the properties file is located at the following location on my system:

/home/ubuntu/lib/armv6hf-sdk/lib/javafx.platform.properties

Yet com.sun.javafx.PlatformUtil.loadProperties looks in the following three locations, in order:

/home/ubuntu/lib/armv6hf-sdk/javafx.platform.properties
/home/ubuntu/opt/jdk-14.0.1+7/lib/javafx.platform.properties
/javafx.platform.properties

The first is based on the "runtime directory," the second is based on the value of java.home, and the third is based on the value of javafx.runtime.path, which is null on my system.

@AlexanderScherbatiy Did you move the javafx.platform.properties file to its parent directory manually, as I just did?

@jgneff
Copy link
Contributor

@jgneff jgneff commented Apr 23, 2020

Wow, this is going to take some getting used to. 😃 Below is a photograph of the button and text with two changes:

  1. the code from this pull request, and
  2. the javafx.platform.properties file moved to its parent directory.

withfix

The native screen on this platform returns 167 for its DPI. In general, the devices I'm testing have pixel densities of either 167 or 300 pixels per inch.

@jgneff
Copy link
Contributor

@jgneff jgneff commented Apr 24, 2020

I got out my trusty C-Thru Ruler (GA-96), and the type now measures 12 points, just as intended.

PrismFontFactory.getSystemFontSize

  } else if (isEmbedded) {
      try {
          int screenDPI = Screen.getMainScreen().getResolutionY();
          systemFontSize = ((float) screenDPI) / 6f; // 12 points
      } catch (NullPointerException npe) {
          // if no screen is defined
          systemFontSize = 13f; // same as desktop Linux
      }
  } else {
      systemFontSize = 13f; // Gnome uses 13.
  }

Without the platform properties file, I've been running (for years!) with a default font size of 13 pixels, which on this device is very small: (13 px ÷ 167 px/in) × 72 points/in = 5.60 points.

Now JavaFX is setting isEmbedded to true, picking up the correct screen DPI, and I'm finally getting a good default font size: ((167 px/in ÷ 6 per in) ÷ 167 px/in) × 72 points/in = 12.0 points. Amazing.

@AlexanderScherbatiy
Copy link
Author

@AlexanderScherbatiy AlexanderScherbatiy commented Apr 24, 2020

@AlexanderScherbatiy Did you move the javafx.platform.properties file to its parent directory manually, as I just did?

I used the full version of jdk 14.0.1 on Raspberry Pi where JavaFX is included in jdk so the file javafx.platform.properties is already in jdk-14.0.1-full/lib directory.

To debug the JavaFX on Raspberry Pi I built armv6hf-sdk and just copied the file javafx.platform.properties from the full jdk version with JavaFX to jdk-14.0.1/lib directory of jdk without JavaFX which I used to run my java application with JavaFX modules from armv6hf-sdk/lib.

@jgneff
Copy link
Contributor

@jgneff jgneff commented Apr 24, 2020

To debug the JavaFX on Raspberry Pi I built armv6hf-sdk and just copied the file javafx.platform.properties from the full jdk version with JavaFX to jdk-14.0.1/lib directory of jdk without JavaFX which I used to run my java application with JavaFX modules from armv6hf-sdk/lib.

May I then suggest one additional change to this pull request? It's a two-line fix:

--- a/modules/javafx.base/src/main/java/com/sun/javafx/PlatformUtil.java
+++ b/modules/javafx.base/src/main/java/com/sun/javafx/PlatformUtil.java
@@ -238,8 +238,7 @@ public class PlatformUtil {
             // Strip everything after the last "/" or "\" to get rid of the jar filename
             int lastIndexOfSlash = Math.max(
                     s.lastIndexOf('/'), s.lastIndexOf('\\'));
-            return new File(new URL(s.substring(0, lastIndexOfSlash + 1)).getPath())
-                    .getParentFile();
+            return new File(new URL(s.substring(0, lastIndexOfSlash + 1)).getPath());
         } catch (MalformedURLException e) {
             return null;
         }

That change corrects the code to look for the javafx.platform.properties file in the same directory as the javafx.base.jar file instead of looking for it in the parent directory of the JAR file. I just stepped through the code with this change, and the method PlatformUtil.loadProperties now finds the properties file in the location where it is packaged by the build in the SDK.

@jgneff
Copy link
Contributor

@jgneff jgneff commented Apr 24, 2020

Related issues, including the original request for enhancement:

  • JDK-8100775: Means of bundling platform specific settings for glass/runtime
  • JDK-8115678: JavaFX preview on Raspberry Pi requires -Djavafx.platform=eglfb to be set
  • JDK-8117705: Embedded JavaFX can't find javafx.platform.properties

The last two issues listed above were fixed by the following changeset:

Fix for RT-28035 (Can't find embedded platform properties) and RT-27194 (Must set javafx.platform)

In JDK 8, all of the Java property files, including javafx.platform.properties, are located under the jre/lib directory of the JDK, but the JavaFX 8 JAR file jfxrt.jar is found in the subdirectory jre/lib/ext. The changeset above fixed the JavaFX 8 code to look in the parent directory for its properties.

Now, though, the JavaFX SDK puts the JAR file javafx.base.jar in the same lib directory as the property files, so we need to remove the method call that changes the path to its parent.

@AlexanderScherbatiy
Copy link
Author

@AlexanderScherbatiy AlexanderScherbatiy commented Apr 27, 2020

May I then suggest one additional change to this pull request? It's a two-line fix.
That change corrects the code to look for the javafx.platform.properties file in the same directory as the javafx.base.jar file instead of looking for it in the parent directory of the JAR file.

I have added the fix for PlatformUtil.getRTDir() method to the current pull request.

@jgneff
jgneff approved these changes Apr 27, 2020
Copy link
Contributor

@jgneff jgneff left a comment

Thanks, Alexander! I just tested your pull request again, and JavaFX is now picking up the properties file along with the correct native screen DPI on my Monocle ARM platform.

@johanvos
Copy link
Collaborator

@johanvos johanvos commented Apr 27, 2020

@AlexanderScherbatiy Did you move the javafx.platform.properties file to its parent directory manually, as I just did?

I used the full version of jdk 14.0.1 on Raspberry Pi where JavaFX is included in jdk so the file javafx.platform.properties is already in jdk-14.0.1-full/lib directory.

To debug the JavaFX on Raspberry Pi I built armv6hf-sdk and just copied the file javafx.platform.properties from the full jdk version with JavaFX to jdk-14.0.1/lib directory of jdk without JavaFX which I used to run my java application with JavaFX modules from armv6hf-sdk/lib.

I'm confused by this, what is the full version of JDK 14.0.1? JavaFX is not part of the JDK anymore, therefore I don't expect a javafx.platform.properties in the JDK. Iif this is the case, it seems a serious bug to me.

@AlexanderScherbatiy
Copy link
Author

@AlexanderScherbatiy AlexanderScherbatiy commented Apr 27, 2020

I'm confused by this, what is the full version of JDK 14.0.1? JavaFX is not part of the JDK anymore, therefore I don't expect a javafx.platform.properties in the JDK. Iif this is the case, it seems a serious bug to me.

I used non Oracle jdk 14.0.1 build which full version includes JavaFX.

@@ -238,8 +238,7 @@ private static File getRTDir() {
// Strip everything after the last "/" or "\" to get rid of the jar filename
int lastIndexOfSlash = Math.max(
s.lastIndexOf('/'), s.lastIndexOf('\\'));
return new File(new URL(s.substring(0, lastIndexOfSlash + 1)).getPath())
.getParentFile();
return new File(new URL(s.substring(0, lastIndexOfSlash + 1)).getPath());

This comment has been minimized.

@kevinrushforth

kevinrushforth Apr 27, 2020
Member

The previous code looks like a hold-over from JDK 8, where jfxrt.jar was in lib/ext. It also assumes that the JavaFX runtime is being loaded from a jar URL, which isn't necessarily the case. Probably the only reason it hasn't caused problems before now is that it is only used to locate javafx.platform.properties. Worth noting is that we won't get here in the case JavaFX is jlinked into the Java runtime, although in that case, the fallback method of locating it relative to the JDK should be used.

I'll take a closer look this specific change, but I think it should be OK.

I'll defer the review as a whole to Johan.

This comment has been minimized.

@kevinrushforth

kevinrushforth Apr 28, 2020
Member

I can confirm that this part of the fix is correct. The change to DPI looks correct to me, too.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 28, 2020

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Apr 28, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 28, 2020

Pending review by @johanvos

@openjdk
Copy link

@openjdk openjdk bot commented May 1, 2020

@AlexanderScherbatiy This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8243255: Font size is large in JavaFX app with enabled Monocle on Raspberry Pi

Reviewed-by: kcr, jvos
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there have been 14 commits pushed to the master branch:

  • 2e90076: 8242507: Add support for Visual Studio 2019
  • 39d9c3b: 8244110: NPE in MenuButtonSkinBase change listener
  • 99f7747: 8241999: ChoiceBox: incorrect toggle selected for uncontained
  • 1cae6a8: 8176499: Dependence on java.util.Timer freezes screen when OS time resets backwards
  • 2b9eb52: 8237504: Update copyright header for files modified in 2020
  • 3130fc8: 8198402: ToggleButton.setToggleGroup causes memory leak when button is removed via ToggleGroup.getToggles()
  • 8ad5805: 8191758: Match WebKit's font weight rendering with JavaFX
  • 66c3b38: 8227425: Add support for e-paper displays on i.MX6 devices
  • e30049f: 8242077: Add information about HTTP/2 and HttpClient usage in WebEngine
  • e0ffca3: 8242505: Some WebKit tests might fail because Microsoft libraries are not loaded
  • ... and 4 more: https://git.openjdk.java.net/jfx/compare/dedf7cb73a012e74b098cc49330bc00c20b97bac...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 2e900764e6dc80d21f50146fc7dcf1771ba1400b.

As you do not have Committer status in this project, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth, @johanvos) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label May 1, 2020
@AlexanderScherbatiy
Copy link
Author

@AlexanderScherbatiy AlexanderScherbatiy commented May 6, 2020

/integrate

@openjdk openjdk bot added the sponsor label May 6, 2020
@openjdk
Copy link

@openjdk openjdk bot commented May 6, 2020

@AlexanderScherbatiy
Your change (at version 058e760) is now ready to be sponsored by a Committer.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented May 6, 2020

/sponsor

@openjdk openjdk bot closed this May 6, 2020
@openjdk openjdk bot added the integrated label May 6, 2020
@openjdk openjdk bot removed sponsor ready labels May 6, 2020
@openjdk
Copy link

@openjdk openjdk bot commented May 6, 2020

@kevinrushforth @AlexanderScherbatiy The following commits have been pushed to master since your change was applied:

  • 2e90076: 8242507: Add support for Visual Studio 2019
  • 39d9c3b: 8244110: NPE in MenuButtonSkinBase change listener
  • 99f7747: 8241999: ChoiceBox: incorrect toggle selected for uncontained
  • 1cae6a8: 8176499: Dependence on java.util.Timer freezes screen when OS time resets backwards
  • 2b9eb52: 8237504: Update copyright header for files modified in 2020
  • 3130fc8: 8198402: ToggleButton.setToggleGroup causes memory leak when button is removed via ToggleGroup.getToggles()
  • 8ad5805: 8191758: Match WebKit's font weight rendering with JavaFX
  • 66c3b38: 8227425: Add support for e-paper displays on i.MX6 devices
  • e30049f: 8242077: Add information about HTTP/2 and HttpClient usage in WebEngine
  • e0ffca3: 8242505: Some WebKit tests might fail because Microsoft libraries are not loaded
  • ceb3fce: 8087555: [ChoiceBox] uncontained value not shown
  • 818ac00: 8175358: Memory leak when moving MenuButton into another Scene
  • 91d4c8b: 8241737: TabPaneSkin memory leak on replacing selectionModel
  • 48476eb: 8241582: Infinite animation does not start from the end when started with a negative rate

Your commit was automatically rebased without conflicts.

Pushed as commit 0385563.

@openjdk openjdk bot removed the rfr label May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants