Rework windows font collector #107

moi15moi · 2024-02-01T02:46:41Z

The main goal of this PR is to replace the "hack" in the Windows FontCollector which involves using the Windows registry to obtain the font filename. Now, we use DirectWrite which can interop with GDI, so we are 100% sure it use the same font has VSFilter.

I have also removed Uniscribe, which was used to determine if a font has a specific character.
The sole objective of this PR is to enhance the code quality in the Windows FontCollector.

Now, the user can request a font by a fullname or postscript name like said in libass/libass#203. Previously, on windows, the user could only request a family name.

Should I move the method normalizeFilePathCase to Aegisub/libaegisub/windows/fs.cpp?

Edit:
The commit de1d3e3 ensures that fonts installed via AddFontResource are detected by FontCollector. Previously, they were not recognized.

However, please note that this detection only works on Windows 10 or later. It's uncertain whether this behavior is a bug in the Windows API, but the fonts installed via AddFontResource aren't added to the IDWriteFontCollection. Consequently, we cannot convert IDWriteFontFace to IDWriteFont which impeding the ability to verify the presence of certain characters.

This replaces all the logic of using the Windows registry to obtain the font path by using DirectWrite. The goal is simply to improve the quality of the code. This doesn't change any functionality

Uniscribe was only used for the FontCollector. Since we now use DirectWrite, we don't need it anymore.

…raise On Windows, the initialization of the FontCollector can raise an exception

…can throw

… occur

moi15moi · 2024-02-04T02:38:11Z

The CI failure for MacOS seems unrelated to this PR.

/usr/bin/hdiutil detach /dev/disk2 -force
hdiutil: couldn't eject "disk2" - Resource busy
FAILED: meson-internal__osx-build-dmg 
env MESON_SOURCE_ROOT=/Users/runner/work/Aegisub/Aegisub MESON_BUILD_ROOT=/Users/runner/work/Aegisub/Aegisub/build MESON_SUBDIR=packages 'MESONINTROSPECT=/Users/runner/hostedtoolcache/Python/3.12.1/x64/bin/meson introspect' /Users/runner/work/Aegisub/Aegisub/tools/osx-dmg.sh /Users/runner/work/Aegisub/Aegisub /Users/runner/work/Aegisub/Aegisub/build 3.2.2

arch1t3cht

I finally had time to look through this - thanks a lot for the PR!

Looking through it, everything seems to be correct as long as I trust you on the DirectWrite/GDI wrangling (and barring the IDWriteLocalFontLoader question on Discord). I do have a few comments regarding general code style and conventions, though.

meson.build

src/dialog_fonts_collector.cpp

src/font_file_lister_gdi.cpp

…RITE_3

…se to std::wstring

moi15moi · 2024-03-03T03:29:32Z

Looking through it, everything seems to be correct as long as I trust you on the DirectWrite/GDI wrangling (and barring the IDWriteLocalFontLoader question on Discord).

I did a bunch of test with the user.
You can see my conclusion here.
Please, don't trust me too much. This PR should also be tested on Windows 7 or 8.
I only tested it on Windows 10.
Edit: Now, i also tested it on Windows 7.

src/font_file_lister_gdi.cpp

moi15moi · 2024-03-03T16:57:55Z

In some specific cases, the FontCollector can make a mistake in obtaining the font weight and italic style.
For example, install "AliviaRegular_Weight31961.ttf" and "Alivia.otf," which you can find here in the .zip file.

FontCollector will return the font path to "AliviaRegular_Weight31961.ttf" (which is correct), but it will select the weight and italic style from "Alivia.otf." Because of this problem, the fake_bold and fake_italic doesn't always represent the true information.

I tried to see if there is a way to avoid this problem, but I didn't find one.
Here is what I tried:

Always using the first logfont returned by EnumFontFamiliesEx. I thought the first logfont returned by the Enum is the one that would be selected in the HDC, but it isn't the case. A simple test with Arial shows that it won't necessarily use the first logfont.
GetTextMetricsW and GetOutlineTextMetricsW. They return the "simulated" font. For example, if you ask for a font with weight 700, but there is only a 400 font installed, both methods will return metrics with weight 700.
IDWriteFont::GetWeight and IDWriteFont::GetStyle. In some specific cases, DirectWrite is incompatible with GDI, and it won't return the same italic value. The same problem occurs with the weight. See this issue: Fonts with same name same weight and same italic moi15moi/FontCollector#8 (comment)

Conclusion:
There isn't any function that GDI offers to know the italic and weight of the selected font in the HDC.
The only "perfect" solution would be to parse the font like GDI. We could use IDWriteFontFace::TryGetFontTable and get the OS/2 table. If a font doesn't contain an OS/2 table, we would need to use the head table. I don't know if we really want to do that.
After some new tests, I realise that IDWriteFontFace::GetSimulations return compatible value with GDI. From what I understand, it is only the case because we created the IDWriteFontFace from IDWriteGdiInterop::CreateFontFaceFromHdc. If it wouldn't be the case, the value would not necessary be compatible with GDI.

…ect fake_italic/fake_bold See this comment: arch1t3cht#107 (comment)

…correctly verify if font has character(s) With the Visual Studio 2019 toolchain on Windows 7, it installs the Windows 10 SDK by default. Because of this, ``HAVE_DWRITE_3`` is true, so the ``QueryInterface`` always fails. Now, if the ``QueryInterface`` fails, we try to verify if the font has characters with a Windows Vista SP2 compatible code.

…ect fake_italic/fake_bold See this comment: arch1t3cht#107 (comment)

…pace AND truncated facename Problem 1: Previously, if a user wrote "\fn ", it would return the font Arial, which is not what we want. This is because when we request EnumFontFamiliesEx with whitespace or an empty lfFaceName, it will enumerate all the installed fonts. Solution 1: To resolve this issue, let's implement a solution similar to libass to determine if the selected facename exists: https://github.com/libass/libass/blob/649a7c2e1fc6f4188ea1a89968560715800b883d/libass/ass_directwrite.c#L737-L747 Problem 2: GDI truncates font names to 31 characters. See: libass/libass#459 However, since I changed the method to determine if a facename exists, I ensured that we still support this "feature". To test this, I used the font in: libass/libass#710

arch1t3cht

Thanks for the updates. Just a couple more nitpicks left. Once those are addressed, you can squash everything onto one commit and I'll merge it (or if you want to keep the commit history on your branch I'll squash it myself when merging).

arch1t3cht · 2024-03-17T16:33:00Z

src/font_file_lister_gdi.cpp

 	LOGFONTW lf{};
-	lf.lfCharSet = DEFAULT_CHARSET;
+	lf.lfCharSet = DEFAULT_CHARSET; // we should use the one specified in the ass file


Yes, it's probably better to treat encodings different from 1 as "unsupported". What I really meant here, though, is that currently this comment is a bit confusing. You can remove it or reword it to something like "note that this currently ignores the font encoding specified in the ass file".

src/font_file_lister_gdi.cpp

…ion of std::wstring over WCHAR

moi15moi · 2024-03-17T19:28:20Z

Thanks for the updates. Just a couple more nitpicks left. Once those are addressed, you can squash everything onto one commit and I'll merge it (or if you want to keep the commit history on your branch I'll squash it myself when merging).

I will let you squash them. I would like to conserve the commits. Some of them contain relevant information that may be useful if a problem is discovered in the future.

[src\meson.build] Add DirectWrite has dependency [src\font_file_lister_gdi] Rework GDI FontCollector to use DirectWrite This replaces all the logic of using the Windows registry to obtain the font path by using DirectWrite. The goal is simply to improve the quality of the code. This doesn't change any functionality [src\meson.build] Remove Uniscribe has dependency Uniscribe was only used for the FontCollector. Since we now use DirectWrite, we don't need it anymore. [src\dialog_fonts_collector] Catch exceptions that FontCollector may raise On Windows, the initialization of the FontCollector can raise an exception [src\font_file_lister] Document the exception that GdiFontFileLister can throw [src\font_file_lister_gdi] Correct possible memory leak when an error occur Fix error caused by AddFontResource on Windows 10 or higher [meson.build] Replace add_project_arguments with conf.set for HAVE_DWRITE_3 [src\dialog_fonts_collector] Update message error and optimisation [src\font_file_lister_gdi] Correct documentation typo [src\font_file_lister_gdi] Cosmetic nit - Initialize hfont in one line [src\font_file_lister_gdi] Cosmetic nit - Remove if statements brace [src\font_file_lister_gdi] Replace WCHAR param of normalizeFilePathCase to std::wstring [src\font_file_lister_gdi] Replace WCHAR by std::wstring [src\font_file_lister_gdi] Use IDWriteFontFace::GetSimulations to detect fake_italic/fake_bold See this comment: #107 (comment) [src\font_file_lister_gdi] If Win7/8 has Win 10 SDK on compile time, correctly verify if font has character(s) With the Visual Studio 2019 toolchain on Windows 7, it installs the Windows 10 SDK by default. Because of this, ``HAVE_DWRITE_3`` is true, so the ``QueryInterface`` always fails. Now, if the ``QueryInterface`` fails, we try to verify if the font has characters with a Windows Vista SP2 compatible code. [src\font_file_lister_gdi] Support facename that contains only whitespace AND truncated facename Problem 1: Previously, if a user wrote "\fn ", it would return the font Arial, which is not what we want. This is because when we request EnumFontFamiliesEx with whitespace or an empty lfFaceName, it will enumerate all the installed fonts. Solution 1: To resolve this issue, let's implement a solution similar to libass to determine if the selected facename exists: https://github.com/libass/libass/blob/649a7c2e1fc6f4188ea1a89968560715800b883d/libass/ass_directwrite.c#L737-L747 Problem 2: GDI truncates font names to 31 characters. See: libass/libass#459 However, since I changed the method to determine if a facename exists, I ensured that we still support this "feature". To test this, I used the font in: libass/libass#710 [src\font_file_lister_gdi] Add a FIXME comment regarding the utilization of std::wstring over WCHAR [src\font_file_lister_gdi] Add FIXME comment about charset

arch1t3cht · 2024-03-20T13:40:25Z

Thanks! Now squashed and merged in 7543060.

moi15moi added 6 commits January 31, 2024 20:48

[src\meson.build] Add DirectWrite has dependency

eea7006

[src\font_file_lister_gdi] Rework GDI FontCollector to use DirectWrite

042f0c5

This replaces all the logic of using the Windows registry to obtain the font path by using DirectWrite. The goal is simply to improve the quality of the code. This doesn't change any functionality

[src\meson.build] Remove Uniscribe has dependency

11cc16d

Uniscribe was only used for the FontCollector. Since we now use DirectWrite, we don't need it anymore.

[src\dialog_fonts_collector] Catch exceptions that FontCollector may …

e3107ac

…raise On Windows, the initialization of the FontCollector can raise an exception

[src\font_file_lister] Document the exception that GdiFontFileLister …

6a3643f

…can throw

[src\font_file_lister_gdi] Correct possible memory leak when an error…

ea6a339

… occur

Fix error caused by AddFontResource on Windows 10 or higher

de1d3e3

arch1t3cht force-pushed the feature branch from 0b8bc68 to dcca721 Compare February 20, 2024 14:04

arch1t3cht requested changes Feb 26, 2024

View reviewed changes

moi15moi added 6 commits March 2, 2024 12:25

[meson.build] Replace add_project_arguments with conf.set for HAVE_DW…

7d099da

…RITE_3

[src\dialog_fonts_collector] Update message error and optimisation

9697588

[src\font_file_lister_gdi] Correct documentation typo

5212d1c

[src\font_file_lister_gdi] Cosmetic nit - Initialize hfont in one line

9af7c22

[src\font_file_lister_gdi] Cosmetic nit - Remove if statements brace

863d427

[src\font_file_lister_gdi] Replace WCHAR param of normalizeFilePathCa…

38a5a99

…se to std::wstring

moi15moi requested a review from arch1t3cht March 3, 2024 03:31

moi15moi commented Mar 3, 2024

View reviewed changes

src/font_file_lister_gdi.cpp Outdated Show resolved Hide resolved

[src\font_file_lister_gdi] Replace WCHAR by std::wstring

62813a6

[src\font_file_lister_gdi] Use IDWriteFontFace::GetSimulations to det…

83f8c7f

…ect fake_italic/fake_bold See this comment: arch1t3cht#107 (comment)

moi15moi force-pushed the Rework-Windows-FontCollector branch from 336647f to 83f8c7f Compare March 5, 2024 22:49

0tkl pushed a commit to 0tkl/Aegisub that referenced this pull request Mar 8, 2024

[src\font_file_lister_gdi] Use IDWriteFontFace::GetSimulations to det…

6a28da9

…ect fake_italic/fake_bold See this comment: arch1t3cht#107 (comment)

arch1t3cht reviewed Mar 17, 2024

View reviewed changes

moi15moi added 2 commits March 17, 2024 15:20

[src\font_file_lister_gdi] Add a FIXME comment regarding the utilizat…

44b81fd

…ion of std::wstring over WCHAR

[src\font_file_lister_gdi] Add FIXME comment about charset

17a3f48

arch1t3cht closed this Mar 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework windows font collector #107

Rework windows font collector #107

moi15moi commented Feb 1, 2024 •

edited

moi15moi commented Feb 4, 2024

arch1t3cht left a comment

moi15moi commented Mar 3, 2024 •

edited

moi15moi commented Mar 3, 2024 •

edited

arch1t3cht left a comment

arch1t3cht Mar 17, 2024

moi15moi commented Mar 17, 2024 •

edited

arch1t3cht commented Mar 20, 2024

Rework windows font collector #107

Rework windows font collector #107

Conversation

moi15moi commented Feb 1, 2024 • edited

moi15moi commented Feb 4, 2024

arch1t3cht left a comment

Choose a reason for hiding this comment

moi15moi commented Mar 3, 2024 • edited

moi15moi commented Mar 3, 2024 • edited

arch1t3cht left a comment

Choose a reason for hiding this comment

arch1t3cht Mar 17, 2024

Choose a reason for hiding this comment

moi15moi commented Mar 17, 2024 • edited

arch1t3cht commented Mar 20, 2024

moi15moi commented Feb 1, 2024 •

edited

moi15moi commented Mar 3, 2024 •

edited

moi15moi commented Mar 3, 2024 •

edited

moi15moi commented Mar 17, 2024 •

edited