From 53789860df5603d7d0ba191f3beeba9dbd3e6874 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Mon, 6 Nov 2023 22:30:03 +0100 Subject: [PATCH] Fix the fix for the fix of nearby font loading (#16196) I still don't know how to reproduce it properly, but I'm slowly wrapping my head around how and why it happens. The issue isn't that `FindFamilyName` fails with `exists=FALSE`, but rather that any of the followup calls like `GetDesignGlyphMetrics` fails, which results in an exception and subsequently in an orderly fallback to Consolas. I've always thought that the issue is that even with the nearby font collection we get an `exists=FALSE`... I'm not sure why I thought that. This changeset also drops the fallback iteration for Lucida Console and Courier New, because I felt like the code looks neater that way and I think it's a reasonable expectation that Consolas is always installed. Closes #16058 (cherry picked from commit 9e86c9811f2cf0ad53c9c543b886274b1079dd00) Service-Card-Id: 90885610 Service-Version: 1.18 --- src/renderer/atlas/AtlasEngine.api.cpp | 65 ++++++++++++-------------- src/renderer/atlas/common.h | 1 - src/renderer/dx/DxFontInfo.cpp | 18 +++---- 3 files changed, 40 insertions(+), 44 deletions(-) diff --git a/src/renderer/atlas/AtlasEngine.api.cpp b/src/renderer/atlas/AtlasEngine.api.cpp index c810dd115f62..0e2f7941688b 100644 --- a/src/renderer/atlas/AtlasEngine.api.cpp +++ b/src/renderer/atlas/AtlasEngine.api.cpp @@ -455,30 +455,34 @@ void AtlasEngine::SetWarningCallback(std::function pfn) noexcept [[nodiscard]] HRESULT AtlasEngine::UpdateFont(const FontInfoDesired& fontInfoDesired, FontInfo& fontInfo, const std::unordered_map& features, const std::unordered_map& axes) noexcept { - static constexpr std::array fallbackFaceNames{ static_cast(nullptr), L"Consolas", L"Lucida Console", L"Courier New" }; - auto it = fallbackFaceNames.begin(); - const auto end = fallbackFaceNames.end(); + try + { + _updateFont(fontInfoDesired.GetFaceName().c_str(), fontInfoDesired, fontInfo, features, axes); + return S_OK; + } + CATCH_LOG(); - for (;;) + if constexpr (Feature_NearbyFontLoading::IsEnabled()) { try { - _updateFont(*it, fontInfoDesired, fontInfo, features, axes); + // _resolveFontMetrics() checks `_api.s->font->fontCollection` for a pre-existing font collection, + // before falling back to using the system font collection. This way we can inject our custom one. See GH#9375. + // Doing it this way is a bit hacky, but it does have the benefit that we can cache a font collection + // instance across font changes, like when zooming the font size rapidly using the scroll wheel. + _api.s.write()->font.write()->fontCollection = FontCache::GetCached(); + _updateFont(fontInfoDesired.GetFaceName().c_str(), fontInfoDesired, fontInfo, features, axes); return S_OK; } - catch (...) - { - ++it; - if (it == end) - { - RETURN_CAUGHT_EXCEPTION(); - } - else - { - LOG_CAUGHT_EXCEPTION(); - } - } + CATCH_LOG(); } + + try + { + _updateFont(nullptr, fontInfoDesired, fontInfo, features, axes); + return S_OK; + } + CATCH_RETURN(); } void AtlasEngine::UpdateHyperlinkHoveredId(const uint16_t hoveredId) noexcept @@ -598,11 +602,7 @@ void AtlasEngine::_resolveFontMetrics(const wchar_t* requestedFaceName, const Fo if (!requestedFaceName) { - requestedFaceName = fontInfoDesired.GetFaceName().c_str(); - if (!requestedFaceName) - { - requestedFaceName = L"Consolas"; - } + requestedFaceName = L"Consolas"; } if (!requestedSize.height) { @@ -614,22 +614,19 @@ void AtlasEngine::_resolveFontMetrics(const wchar_t* requestedFaceName, const Fo requestedWeight = DWRITE_FONT_WEIGHT_NORMAL; } - wil::com_ptr fontCollection; - THROW_IF_FAILED(_p.dwriteFactory->GetSystemFontCollection(fontCollection.addressof(), FALSE)); + // UpdateFont() (and its NearbyFontLoading feature path specifically) sets `_api.s->font->fontCollection` + // to a custom font collection that includes .ttf files that are bundled with our app package. See GH#9375. + // Doing it this way is a bit hacky, but it does have the benefit that we can cache a font collection + // instance across font changes, like when zooming the font size rapidly using the scroll wheel. + auto fontCollection = _api.s->font->fontCollection; + if (!fontCollection) + { + THROW_IF_FAILED(_p.dwriteFactory->GetSystemFontCollection(fontCollection.addressof(), FALSE)); + } u32 index = 0; BOOL exists = false; THROW_IF_FAILED(fontCollection->FindFamilyName(requestedFaceName, &index, &exists)); - - if constexpr (Feature_NearbyFontLoading::IsEnabled()) - { - if (!exists) - { - fontCollection = FontCache::GetCached(); - THROW_IF_FAILED(fontCollection->FindFamilyName(requestedFaceName, &index, &exists)); - } - } - THROW_HR_IF(DWRITE_E_NOFONT, !exists); wil::com_ptr fontFamily; diff --git a/src/renderer/atlas/common.h b/src/renderer/atlas/common.h index 94434bffc9c2..b8fa3ac0d596 100644 --- a/src/renderer/atlas/common.h +++ b/src/renderer/atlas/common.h @@ -470,7 +470,6 @@ namespace Microsoft::Console::Render::Atlas wil::com_ptr systemFontFallback; wil::com_ptr systemFontFallback1; // optional, might be nullptr wil::com_ptr textAnalyzer; - wil::com_ptr renderingParams; std::function warningCallback; std::function swapChainChangedCallback; diff --git a/src/renderer/dx/DxFontInfo.cpp b/src/renderer/dx/DxFontInfo.cpp index 4945939fd868..b1d4844a6017 100644 --- a/src/renderer/dx/DxFontInfo.cpp +++ b/src/renderer/dx/DxFontInfo.cpp @@ -127,15 +127,6 @@ void DxFontInfo::SetFromEngine(const std::wstring_view familyName, { face = _FindFontFace(localeName); - if constexpr (Feature_NearbyFontLoading::IsEnabled()) - { - if (!face) - { - _fontCollection = FontCache::GetCached(); - face = _FindFontFace(localeName); - } - } - if (!face) { // If we missed, try looking a little more by trimming the last word off the requested family name a few times. @@ -167,6 +158,15 @@ void DxFontInfo::SetFromEngine(const std::wstring_view familyName, } CATCH_LOG(); + if constexpr (Feature_NearbyFontLoading::IsEnabled()) + { + if (!face) + { + _fontCollection = FontCache::GetCached(); + face = _FindFontFace(localeName); + } + } + // Alright, if our quick shot at trimming didn't work either... // move onto looking up a font from our hard-coded list of fonts // that should really always be available.