8281327: JavaFX does not support fonts installed per-user on Windows 10/11 #1017

jperedadnr · 2023-01-31T10:30:22Z

This PR simply applies the patch from JDK-8218914 that solved the same issue for the JDK.

I've tested it on Windows 11 (Version 22H2 Build 22621.1105).

I have the Roboto font installed under C:\Users%user\AppData\Local\Microsoft\Windows\Fonts
and with this PR, -Dprism.debugfonts shows:

...
font=segoe ui historic file=seguihis.ttf
font=roboto file=C:\Users\%user\AppData\Local\Microsoft\Windows\Fonts\Roboto-Regular.ttf
font=yu mincho file=yumin.ttf
...

Also, I can see that the font is picked in a simple JavaFX application.
Without this PR, the font is not used, and defaults to a system one.

I don't think I can add this as a system/manual test to the PR, though.

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-8281327: JavaFX does not support fonts installed per-user on Windows 10/11

Reviewers

Phil Race (@prrace - Reviewer)
Ambarish Rapte (@arapte - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1017

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

Using diff file

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

bridgekeeper · 2023-01-31T10:31:25Z

👋 Welcome back jpereda! 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.

mlbridge · 2023-01-31T10:37:01Z

Webrevs

00: Full (352cfa7b)

Glavo · 2023-01-31T11:18:42Z

That is great! We have been looking forward to this patch for two years.

kevinrushforth · 2023-01-31T12:26:45Z

/reviewers 2

openjdk · 2023-01-31T12:26:51Z

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

mstr2 · 2023-01-31T16:47:01Z

modules/javafx.graphics/src/main/native-font/fontpath.c

+
+    if (ret != ERROR_SUCCESS ||
+        dwMaxValueNameLen >= MAX_BUFFER ||
+        dwMaxValueDataLen >= MAX_BUFFER) {


This implementation instantly fails if any value or data exceeds MAX_BUFFER. Wouldn't it be better to only skip the values that are too large, instead of ignoring all values entirely?

Instead of using RegQueryInfoKeyW, the documentation suggests the following approach:

To enumerate values, an application should initially call the RegEnumValue function with the dwIndex parameter set to zero. The application should then increment dwIndex and call the RegEnumValue function until there are no more values (until the function returns ERROR_NO_MORE_ITEMS).

Please note that this is just a refactoring of the existing code, extracting a method so that it can be called twice, and it is also what the JDK is doing.
If the affected code needs changes, maybe that is for a follow up.

prrace

Code looks good and I applied the patch and verified it works as advertised.

openjdk · 2023-02-16T09:14:52Z

@jperedadnr 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:

8281327: JavaFX does not support fonts installed per-user on Windows 10/11

Reviewed-by: prr, arapte

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 21 new commits pushed to the master branch:

dbab36b: 8300893: Wrong state after deselecting two or more cells of a TableView selection
97ae4b8: 8302294: Cherry-pick WebKit 615.1 stabilization fixes
d1572f4: 8298528: Clean up raw type warnings in base in bindings and collections packages
de8339d: 8088998: XYChart: duplicate child added exception when remove & add a series in several charts
55eb83d: 8299986: Wrong sublist used in ListChangeListener
d3654e3: 8230833: LabeledSkinBase computes wrong height with ContentDisplay.GRAPHIC_ONLY
e7d3191: Merge
abe8058: 8301832: InputMethodEvents are not enabled for text input controls
192ba4f: 8301797: Pagination control has the wrong size
13d8df3: 8301712: [linux] Crash on exit from WebKit 615.1
... and 11 more: https://git.openjdk.org/jfx/compare/e234c89a253ece4962803a05d6ef77f0a7a80a82...master

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 master branch, type /integrate in a new comment.

jperedadnr · 2023-02-16T10:27:12Z

/integrate

openjdk · 2023-02-16T10:28:22Z

Going to push as commit f4ca00d.
Since your change was applied there have been 21 commits pushed to the master branch:

dbab36b: 8300893: Wrong state after deselecting two or more cells of a TableView selection
97ae4b8: 8302294: Cherry-pick WebKit 615.1 stabilization fixes
d1572f4: 8298528: Clean up raw type warnings in base in bindings and collections packages
de8339d: 8088998: XYChart: duplicate child added exception when remove & add a series in several charts
55eb83d: 8299986: Wrong sublist used in ListChangeListener
d3654e3: 8230833: LabeledSkinBase computes wrong height with ContentDisplay.GRAPHIC_ONLY
e7d3191: Merge
abe8058: 8301832: InputMethodEvents are not enabled for text input controls
192ba4f: 8301797: Pagination control has the wrong size
13d8df3: 8301712: [linux] Crash on exit from WebKit 615.1
... and 11 more: https://git.openjdk.org/jfx/compare/e234c89a253ece4962803a05d6ef77f0a7a80a82...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-02-16T10:28:38Z

@jperedadnr Pushed as commit f4ca00d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Apply patch from JDK-8218914

352cfa7

openjdk bot added the rfr Ready for review label Jan 31, 2023

Glavo mentioned this pull request Jan 31, 2023

自定义字体功能只能选择“为所有用户安装”的字体，无法选择当前用户已安装的字体 HMCL-dev/HMCL#1058

Closed

kevinrushforth requested review from prrace and arapte January 31, 2023 12:25

mstr2 reviewed Jan 31, 2023

View reviewed changes

prrace approved these changes Feb 14, 2023

View reviewed changes

arapte approved these changes Feb 16, 2023

View reviewed changes

openjdk bot added the ready Ready to be integrated label Feb 16, 2023

openjdk bot added the integrated Pull request has been integrated label Feb 16, 2023

openjdk bot closed this Feb 16, 2023

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Feb 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8281327: JavaFX does not support fonts installed per-user on Windows 10/11 #1017

8281327: JavaFX does not support fonts installed per-user on Windows 10/11 #1017

jperedadnr commented Jan 31, 2023 •

edited by openjdk bot

bridgekeeper bot commented Jan 31, 2023

mlbridge bot commented Jan 31, 2023

Glavo commented Jan 31, 2023

kevinrushforth commented Jan 31, 2023

openjdk bot commented Jan 31, 2023

mstr2 Jan 31, 2023

jperedadnr Jan 31, 2023

prrace left a comment

openjdk bot commented Feb 16, 2023

jperedadnr commented Feb 16, 2023

openjdk bot commented Feb 16, 2023

openjdk bot commented Feb 16, 2023

8281327: JavaFX does not support fonts installed per-user on Windows 10/11 #1017

8281327: JavaFX does not support fonts installed per-user on Windows 10/11 #1017

Conversation

jperedadnr commented Jan 31, 2023 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 31, 2023

mlbridge bot commented Jan 31, 2023

Webrevs

Glavo commented Jan 31, 2023

kevinrushforth commented Jan 31, 2023

openjdk bot commented Jan 31, 2023

mstr2 Jan 31, 2023

Choose a reason for hiding this comment

jperedadnr Jan 31, 2023

Choose a reason for hiding this comment

prrace left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 16, 2023

jperedadnr commented Feb 16, 2023

openjdk bot commented Feb 16, 2023

openjdk bot commented Feb 16, 2023

jperedadnr commented Jan 31, 2023 •

edited by openjdk bot