Skip to content

Commit

Permalink
Fix the fix for the fix of nearby font loading (#16196)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
lhecker authored Nov 6, 2023
1 parent 0289cb0 commit 9e86c98
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 44 deletions.
65 changes: 31 additions & 34 deletions src/renderer/atlas/AtlasEngine.api.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -455,30 +455,34 @@ void AtlasEngine::SetWarningCallback(std::function<void(HRESULT)> pfn) noexcept

[[nodiscard]] HRESULT AtlasEngine::UpdateFont(const FontInfoDesired& fontInfoDesired, FontInfo& fontInfo, const std::unordered_map<std::wstring_view, uint32_t>& features, const std::unordered_map<std::wstring_view, float>& axes) noexcept
{
static constexpr std::array fallbackFaceNames{ static_cast<const wchar_t*>(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
Expand Down Expand Up @@ -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)
{
Expand All @@ -614,22 +614,19 @@ void AtlasEngine::_resolveFontMetrics(const wchar_t* requestedFaceName, const Fo
requestedWeight = DWRITE_FONT_WEIGHT_NORMAL;
}

wil::com_ptr<IDWriteFontCollection> 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<IDWriteFontFamily> fontFamily;
Expand Down
1 change: 0 additions & 1 deletion src/renderer/atlas/common.h
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,6 @@ namespace Microsoft::Console::Render::Atlas
wil::com_ptr<IDWriteFontFallback> systemFontFallback;
wil::com_ptr<IDWriteFontFallback1> systemFontFallback1; // optional, might be nullptr
wil::com_ptr<IDWriteTextAnalyzer1> textAnalyzer;
wil::com_ptr<IDWriteRenderingParams1> renderingParams;
std::function<void(HRESULT)> warningCallback;
std::function<void(HANDLE)> swapChainChangedCallback;

Expand Down
18 changes: 9 additions & 9 deletions src/renderer/dx/DxFontInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down

0 comments on commit 9e86c98

Please sign in to comment.