arch1t3cht / Aegisub

Cross-platform advanced subtitle editor, with new feature branches. Read the README on the feature branch.
http://www.aegisub.org
Other
695 stars 32 forks source link

Rework windows font collector #107

Closed moi15moi closed 4 months ago

moi15moi commented 5 months ago

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 https://github.com/libass/libass/pull/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 de1d3e38d19b1fa3a762450bb3c59bc4e13f7e23 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.

moi15moi commented 5 months ago

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
moi15moi commented 4 months ago

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.

moi15moi commented 4 months ago

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:

  1. 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.
  2. 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.
  3. 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: https://github.com/moi15moi/FontCollector/issues/8#issuecomment-1755885113

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.

moi15moi commented 4 months ago

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.

arch1t3cht commented 4 months ago

Thanks! Now squashed and merged in 7543060.