-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Use nearby fonts for font fallback #11764
Conversation
With this PR finally open I'll go back into I have a branch that fixes this issue much more thoroughly, by getting rid of the entire DxFontInfo class as well as most of the font features and axes code and some other bits altogether (removes about 10-15% of DxEngine). I realize I didn't test it with WT under Windows Server 2022, but I had to uninstall Hyper-V in order to test Windows 7 rendering stuff, so... Yeah let me know if you think my testing methodology is enough. Otherwise I'll reinstall Hyper-V soon and test it again. |
This does address the underlying problem on Server 2022. However, it doesn't make any changes to the settings UI. As I noted in #11032 the SUI uses the system font collection, so the font isn't displayed in the dropdown. This leads to a bit of a weird UI - the dropdown looks like it's empty. If you click on it, you can select a font, but not Cascadia. If you select e.g. Courier New, then decide you want to switch back, you need to use the reset button. Under the debugger clicking it causes an exception to be raised, but without the debugger attached, the button just disappears. Clicking save does revert the font successfully. Demo: Windows2022.mp4 |
@ianjoneill I pushed the code for the settings UI just now. |
This comment has been minimized.
This comment has been minimized.
@@ -3,13 +3,62 @@ | |||
|
|||
#include "pch.h" | |||
#include "Profiles.h" | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah this is just me following the most common C++ style guide for imports... It breaks them up into the following order:
- "pch.h"
- "class.h"
- blank line
- <system headers>
- blank line
- <stdlib headers>
- blank line
- <other global headers>
- blank line
- "local headers.h"
(Google has a good explanation here.)
This ordering helps with clarity as you know immediately where some file resides at. This is especially helpful with MSVC, which for whatever reason has decided to not actually implement C++ and treats ¹ You'll find that most projects follow this style and in fact even in this project most code follows this style already.#include "foo"
and #include <foo>
synonymous. With the above order such imports cannot be confused anymore under msvc, which is nice.
But in short: Considering that our project already does this for most files, I feel like it's alright to modify files that don't do this yet.
¹ Turns out every compiler does that. The only thing msvc does differently is that it searches in the parent directories of all previously included files in a unit as well.``
// The convenience interfaces for loading fonts from files | ||
// are only available on Windows 10+. | ||
wil::com_ptr<IDWriteFactory6> factory6; | ||
// wil's query() facilities don't work inside WinRT land at the moment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this filed against microsoft/wil and/or are we just behind on updating our copy of wil?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got a TODO item to open a PR for that when I'm back at work. Since it's an easy fix that seemed better to me than opening an issue. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC that's an ordering issue - something about including wil before winrt, or vice-versa, or something like that. My memory's hazy on this...
@@ -107,8 +156,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation | |||
reinterpret_cast<::IUnknown**>(factory.put()))); | |||
|
|||
// get the font collection; subscribe to updates | |||
com_ptr<IDWriteFontCollection> fontCollection; | |||
THROW_IF_FAILED(factory->GetSystemFontCollection(fontCollection.put(), TRUE)); | |||
const auto fontCollection = NearbyCollection(factory.get()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this will recreate it every time the page reloads the view model? Not a perf issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I could tell this only loads once when the settings page is opened. Navigating back and forth between views didn't call this function again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm only worried about the Win10 comment there. Otherwise this is a ✅ from me
// The convenience interfaces for loading fonts from files | ||
// are only available on Windows 10+. | ||
wil::com_ptr<IDWriteFactory6> factory6; | ||
// wil's query() facilities don't work inside WinRT land at the moment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC that's an ordering issue - something about including wil before winrt, or vice-versa, or something like that. My memory's hazy on this...
|
||
// The convenience interfaces for loading fonts from files | ||
// are only available on Windows 10+. | ||
// Don't try to look up if below that OS version. | ||
static const bool s_isWindows10OrGreater = IsWindows10OrGreater(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happened to this check? Are these available on Win7 now? (If so, we should get rid of the comment, or update to say that it doesn't matter)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I had to guess, it's because we switched to using QueryInterface
to determine whether the factory supported the requisite interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is always better to do live detection of features (think backports!) instead of version number checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, version checks (including the entirety of IsWindowsXOrGreater()
functions) are basically deprecated. We're supposed to do feature checks nowadays instead.
This is especially so since the current code has a bug. Can you spot it? 😄
It's this line:
// Builder2 has a convenience to just feed in paths to font files.
::Microsoft::WRL::ComPtr<IDWriteFontSetBuilder2> fontSetBuilder2;
THROW_IF_FAILED(fontSetBuilder.As(&fontSetBuilder2));
This will throw if you don't have Windows 10 Build 20348 or newer installed, which lead to #10211 and got solved somewhat "by accident" in a way, since the problem wasn't just corrupted fonts per se.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh btw: Let's not store results from VersionHelpers.h
into statics. The data it uses comes from the PEB (process environment block) and while the version check isn't trival (since you can set a ton of fields and flags when calling VerifyVersionInfoW
), it's not a syscall and can read the version straight from process-local memory (the PEB is local to each process).
The overhead of using a static is likely greater than just calling the function. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know who "wins" in this font collection? Ideally, if a user installs a newer version of Cascadia on their system the nearby version would be ignored.
I suspect that the font set builder is an ordered list. @miniksa mentioned that it always prefers the system font collection if the code is written this way. |
Hello @zadjii-msft! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
This commit is a minimal fix in order to pass the `IDWriteFontCollection` we create out of .ttf files residing next to our binaries to the `IDWriteFontFallback::MapCharacters` call. The `IDWriteTextFormat` is used in order to carry the font collection over into `CustomTextLayout`. ## Validation * Put `JetBrainsMono-Regular.ttf` into the binary output directory * Modify `HKCU:\Console\*\FaceName` to `JetBrains Mono` * Launch OpenConsole.exe * OpenConsole uses JetBrains Mono ✔️ Closes #11032 Closes #11648 (cherry picked from commit 131f5d2)
This commit is a minimal fix in order to pass the `IDWriteFontCollection` we create out of .ttf files residing next to our binaries to the `IDWriteFontFallback::MapCharacters` call. The `IDWriteTextFormat` is used in order to carry the font collection over into `CustomTextLayout`. ## Validation * Put `JetBrainsMono-Regular.ttf` into the binary output directory * Modify `HKCU:\Console\*\FaceName` to `JetBrains Mono` * Launch OpenConsole.exe * OpenConsole uses JetBrains Mono ✔️ Closes #11032 Closes #11648 (cherry picked from commit 131f5d2)
🎉 Handy links: |
🎉 Handy links: |
This commit is a minimal fix in order to pass the
IDWriteFontCollection
we create out of .ttf files residing next to ourbinaries to the
IDWriteFontFallback::MapCharacters
call. TheIDWriteTextFormat
is used in order to carry the font collection overinto
CustomTextLayout
.Validation
JetBrainsMono-Regular.ttf
into the binary output directoryHKCU:\Console\*\FaceName
toJetBrains Mono
Closes #11032
Closes #11648