-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,6 +32,7 @@ hyperlink | |
hyperlinking | ||
hyperlinks | ||
img | ||
inlined | ||
It'd | ||
kje | ||
liga | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,13 +3,62 @@ | |
|
||
#include "pch.h" | ||
#include "Profiles.h" | ||
|
||
#include "PreviewConnection.h" | ||
#include "Profiles.g.cpp" | ||
#include "EnumEntry.h" | ||
|
||
#include <LibraryResources.h> | ||
#include "..\WinRTUtils\inc\Utils.h" | ||
|
||
// This function is a copy of DxFontInfo::_NearbyCollection() with | ||
// * the call to DxFontInfo::s_GetNearbyFonts() inlined | ||
// * checkForUpdates for GetSystemFontCollection() set to true | ||
static wil::com_ptr<IDWriteFontCollection1> NearbyCollection(IDWriteFactory* dwriteFactory) | ||
{ | ||
// 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 commentThe 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 commentThe 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 commentThe 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... |
||
// They produce a compilation error due to IUnknown and winrt::Windows::Foundation::IUnknown being ambiguous. | ||
if (!SUCCEEDED(dwriteFactory->QueryInterface(__uuidof(IDWriteFactory6), factory6.put_void()))) | ||
{ | ||
return nullptr; | ||
} | ||
|
||
wil::com_ptr<IDWriteFontCollection1> systemFontCollection; | ||
THROW_IF_FAILED(factory6->GetSystemFontCollection(false, systemFontCollection.addressof(), true)); | ||
|
||
wil::com_ptr<IDWriteFontSet> systemFontSet; | ||
THROW_IF_FAILED(systemFontCollection->GetFontSet(systemFontSet.addressof())); | ||
|
||
wil::com_ptr<IDWriteFontSetBuilder2> fontSetBuilder2; | ||
THROW_IF_FAILED(factory6->CreateFontSetBuilder(fontSetBuilder2.addressof())); | ||
|
||
THROW_IF_FAILED(fontSetBuilder2->AddFontSet(systemFontSet.get())); | ||
|
||
{ | ||
const std::filesystem::path module{ wil::GetModuleFileNameW<std::wstring>(nullptr) }; | ||
const auto folder{ module.parent_path() }; | ||
|
||
for (const auto& p : std::filesystem::directory_iterator(folder)) | ||
{ | ||
if (til::ends_with(p.path().native(), L".ttf")) | ||
{ | ||
fontSetBuilder2->AddFontFile(p.path().c_str()); | ||
} | ||
} | ||
} | ||
|
||
wil::com_ptr<IDWriteFontSet> fontSet; | ||
THROW_IF_FAILED(fontSetBuilder2->CreateFontSet(fontSet.addressof())); | ||
|
||
wil::com_ptr<IDWriteFontCollection1> fontCollection; | ||
THROW_IF_FAILED(factory6->CreateFontCollectionFromFontSet(fontSet.get(), &fontCollection)); | ||
|
||
return fontCollection; | ||
} | ||
|
||
using namespace winrt::Windows::UI::Text; | ||
using namespace winrt::Windows::UI::Xaml; | ||
using namespace winrt::Windows::UI::Xaml::Controls; | ||
|
@@ -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 commentThe 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 commentThe 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. |
||
|
||
for (UINT32 i = 0; i < fontCollection->GetFontFamilyCount(); ++i) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -96,6 +96,11 @@ bool DxFontInfo::GetFallback() const noexcept | |
return _didFallback; | ||
} | ||
|
||
IDWriteFontCollection* DxFontInfo::GetNearbyCollection() const noexcept | ||
{ | ||
return _nearbyCollection.Get(); | ||
} | ||
|
||
void DxFontInfo::SetFromEngine(const std::wstring_view familyName, | ||
const DWRITE_FONT_WEIGHT weight, | ||
const DWRITE_FONT_STYLE style, | ||
|
@@ -223,13 +228,11 @@ void DxFontInfo::SetFromEngine(const std::wstring_view familyName, | |
// If the system collection missed, try the files sitting next to our binary. | ||
if (withNearbyLookup && !familyExists) | ||
{ | ||
auto&& nearbyCollection = _NearbyCollection(dwriteFactory); | ||
|
||
// May be null on OS below Windows 10. If null, just skip the attempt. | ||
if (nearbyCollection) | ||
if (const auto nearbyCollection = _NearbyCollection(dwriteFactory)) | ||
{ | ||
nearbyCollection.As(&fontCollection); | ||
THROW_IF_FAILED(fontCollection->FindFamilyName(_familyName.data(), &familyIndex, &familyExists)); | ||
THROW_IF_FAILED(nearbyCollection->FindFamilyName(_familyName.data(), &familyIndex, &familyExists)); | ||
fontCollection = nearbyCollection; | ||
} | ||
} | ||
|
||
|
@@ -332,42 +335,48 @@ void DxFontInfo::SetFromEngine(const std::wstring_view familyName, | |
// - dwriteFactory - The DWrite factory to use | ||
// Return Value: | ||
// - DirectWrite font collection. May be null if one cannot be created. | ||
[[nodiscard]] const Microsoft::WRL::ComPtr<IDWriteFontCollection1>& DxFontInfo::_NearbyCollection(gsl::not_null<IDWriteFactory1*> dwriteFactory) const | ||
[[nodiscard]] IDWriteFontCollection* DxFontInfo::_NearbyCollection(gsl::not_null<IDWriteFactory1*> dwriteFactory) | ||
{ | ||
// Magic static so we only attempt to grovel the hard disk once no matter how many instances | ||
// of the font collection itself we require. | ||
static const auto knownPaths = s_GetNearbyFonts(); | ||
if (_nearbyCollection) | ||
{ | ||
return _nearbyCollection.Get(); | ||
} | ||
|
||
// 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. If I had to guess, it's because we switched to using There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, version checks (including the entirety of This is especially so since the current code has a bug. Can you spot it? 😄 // 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 commentThe reason will be displayed to describe this comment to others. Learn more. Oh btw: Let's not store results from |
||
|
||
if (s_isWindows10OrGreater && !_nearbyCollection) | ||
::Microsoft::WRL::ComPtr<IDWriteFactory6> factory6; | ||
if (FAILED(dwriteFactory->QueryInterface<IDWriteFactory6>(&factory6))) | ||
{ | ||
// Factory3 has a convenience to get us a font set builder. | ||
::Microsoft::WRL::ComPtr<IDWriteFactory3> factory3; | ||
THROW_IF_FAILED(dwriteFactory->QueryInterface<IDWriteFactory3>(&factory3)); | ||
return nullptr; | ||
} | ||
|
||
::Microsoft::WRL::ComPtr<IDWriteFontSetBuilder> fontSetBuilder; | ||
THROW_IF_FAILED(factory3->CreateFontSetBuilder(&fontSetBuilder)); | ||
::Microsoft::WRL::ComPtr<IDWriteFontCollection1> systemFontCollection; | ||
THROW_IF_FAILED(factory6->GetSystemFontCollection(false, &systemFontCollection, 0)); | ||
|
||
// Builder2 has a convenience to just feed in paths to font files. | ||
::Microsoft::WRL::ComPtr<IDWriteFontSetBuilder2> fontSetBuilder2; | ||
THROW_IF_FAILED(fontSetBuilder.As(&fontSetBuilder2)); | ||
::Microsoft::WRL::ComPtr<IDWriteFontSet> systemFontSet; | ||
THROW_IF_FAILED(systemFontCollection->GetFontSet(&systemFontSet)); | ||
|
||
for (auto& p : knownPaths) | ||
{ | ||
fontSetBuilder2->AddFontFile(p.c_str()); | ||
} | ||
::Microsoft::WRL::ComPtr<IDWriteFontSetBuilder2> fontSetBuilder2; | ||
THROW_IF_FAILED(factory6->CreateFontSetBuilder(&fontSetBuilder2)); | ||
|
||
::Microsoft::WRL::ComPtr<IDWriteFontSet> fontSet; | ||
THROW_IF_FAILED(fontSetBuilder2->CreateFontSet(&fontSet)); | ||
THROW_IF_FAILED(fontSetBuilder2->AddFontSet(systemFontSet.Get())); | ||
|
||
THROW_IF_FAILED(factory3->CreateFontCollectionFromFontSet(fontSet.Get(), &_nearbyCollection)); | ||
// Magic static so we only attempt to grovel the hard disk once no matter how many instances | ||
// of the font collection itself we require. | ||
static const auto knownPaths = s_GetNearbyFonts(); | ||
for (auto& p : knownPaths) | ||
{ | ||
fontSetBuilder2->AddFontFile(p.c_str()); | ||
} | ||
|
||
return _nearbyCollection; | ||
::Microsoft::WRL::ComPtr<IDWriteFontSet> fontSet; | ||
THROW_IF_FAILED(fontSetBuilder2->CreateFontSet(&fontSet)); | ||
|
||
::Microsoft::WRL::ComPtr<IDWriteFontCollection1> fontCollection; | ||
THROW_IF_FAILED(factory6->CreateFontCollectionFromFontSet(fontSet.Get(), &fontCollection)); | ||
|
||
_nearbyCollection = fontCollection; | ||
return _nearbyCollection.Get(); | ||
} | ||
|
||
// Routine Description: | ||
|
@@ -386,18 +395,11 @@ void DxFontInfo::SetFromEngine(const std::wstring_view familyName, | |
const std::filesystem::path module{ wil::GetModuleFileNameW<std::wstring>(nullptr) }; | ||
const auto folder{ module.parent_path() }; | ||
|
||
for (auto& p : std::filesystem::directory_iterator(folder)) | ||
for (const auto& p : std::filesystem::directory_iterator(folder)) | ||
{ | ||
if (p.is_regular_file()) | ||
if (til::ends_with(p.path().native(), L".ttf")) | ||
{ | ||
auto extension = p.path().extension().wstring(); | ||
std::transform(extension.begin(), extension.end(), extension.begin(), std::towlower); | ||
|
||
static constexpr std::wstring_view ttfExtension{ L".ttf" }; | ||
if (ttfExtension == extension) | ||
{ | ||
paths.push_back(p); | ||
} | ||
paths.push_back(p.path()); | ||
} | ||
} | ||
|
||
|
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:
(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.``