Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix bugs introduced in #16821 (custom font fallback) #16993

Merged
merged 3 commits into from
Apr 1, 2024
Merged

Fix bugs introduced in #16821 (custom font fallback) #16993

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a74d6b5
Fix bugs introduced in #16821 (custom font fallback)
lhecker Apr 1, 2024
a54e935
Better comments
lhecker Apr 1, 2024
51fc550
Fix AuditMode
lhecker Apr 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/cascadia/TerminalSettingsEditor/Appearances.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,11 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
BOOL hasPowerlineCharacters = FALSE;

til::iterate_font_families(fontFace, [&](wil::zwstring_view name) {
if (primaryFontName.empty())
{
primaryFontName = name;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A temporary hack.


std::wstring* accumulator = nullptr;

UINT32 index = 0;
Expand All @@ -434,11 +439,6 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
break;
}

if (primaryFontName.empty())
{
primaryFontName = name;
}

wil::com_ptr<IDWriteFontFamily> fontFamily;
THROW_IF_FAILED(fontCollection->GetFontFamily(index, fontFamily.addressof()));

Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalSettingsEditor/ProfileViewModel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
UpdateFontList();
}
const auto& currentFontList{ CompleteFontList() };
fallbackFont = currentFontList.First().Current();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another temporary hack.

for (const auto& font : currentFontList)
{
if (font.LocalizedName() == name)
Expand Down
69 changes: 47 additions & 22 deletions src/renderer/atlas/AtlasEngine.api.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,32 +482,17 @@ void AtlasEngine::SetWarningCallback(std::function<void(HRESULT, wil::zwstring_v
// commit 9e86c98 (PR #16196), because it showed that it's definitely not due to FindFamilyName() failing.
//
// The workaround is to catch the exception and retry it with our nearby fonts manually loaded in.
HRESULT hr = _updateFont(fontInfoDesired, fontInfo, features, axes);

if constexpr (Feature_NearbyFontLoading::IsEnabled())
{
try
{
_updateFont(fontInfoDesired, fontInfo, features, axes);
return S_OK;
}
CATCH_LOG();

// _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.
// 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.
try
if (FAILED(hr) && _updateWithNearbyFontCollection())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we technically need this at all anymore? the first failed font will automatically use the nearby collection, down in _resolveFontMetrics, right?

Copy link
Member Author

@lhecker lhecker Apr 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that at one point and it didn't work. My current leading theory is that DWrite's internal font cache thinks the font exists, tells us it exists, and only once it actually tries to use the font - for instance when we call GetMetrics() - it notices that the font doesn't actually exist / is inaccessible, and then it returns a bad HRESULT.

{
_api.s.write()->font.write()->fontCollection = FontCache::GetCached();
hr = _updateFont(fontInfoDesired, fontInfo, features, axes);
}
CATCH_LOG();
}

try
{
_updateFont(fontInfoDesired, fontInfo, features, axes);
return S_OK;
}
CATCH_RETURN();
return hr;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HRESULT makes it simpler to write the if failed and necessary to retry condition IMO.

}

void AtlasEngine::UpdateHyperlinkHoveredId(const uint16_t hoveredId) noexcept
Expand Down Expand Up @@ -536,7 +521,8 @@ void AtlasEngine::_resolveTransparencySettings() noexcept
}
}

void 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)
[[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
try
{
std::vector<DWRITE_FONT_FEATURE> fontFeatures;
if (!features.empty())
Expand Down Expand Up @@ -616,9 +602,12 @@ void AtlasEngine::_updateFont(const FontInfoDesired& fontInfoDesired, FontInfo&
_resolveFontMetrics(fontInfoDesired, fontInfo, font);
font->fontFeatures = std::move(fontFeatures);
font->fontAxisValues = std::move(fontAxisValues);

return S_OK;
}
CATCH_RETURN()

void AtlasEngine::_resolveFontMetrics(const FontInfoDesired& fontInfoDesired, FontInfo& fontInfo, FontSettings* fontMetrics) const
void AtlasEngine::_resolveFontMetrics(const FontInfoDesired& fontInfoDesired, FontInfo& fontInfo, FontSettings* fontMetrics)
{
const auto& faceName = fontInfoDesired.GetFaceName();
const auto requestedFamily = fontInfoDesired.GetFamily();
Expand Down Expand Up @@ -659,6 +648,16 @@ void AtlasEngine::_resolveFontMetrics(const FontInfoDesired& fontInfoDesired, Fo
BOOL exists = false;
THROW_IF_FAILED(fontCollection->FindFamilyName(fontName.c_str(), &index, &exists));

// In case of a portable build, the given font may not be installed and instead be bundled next to our executable.
if constexpr (Feature_NearbyFontLoading::IsEnabled())
{
if (!exists && _updateWithNearbyFontCollection())
{
fontCollection = _api.s->font->fontCollection;
THROW_IF_FAILED(fontCollection->FindFamilyName(fontName.c_str(), &index, &exists));
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix for the dialog.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay so now, on the first failed font in the list, we'll fall back to the nearby font collection for all the remaining ones


if (!exists)
{
if (!missingFontNames.empty())
Expand Down Expand Up @@ -869,3 +868,29 @@ void AtlasEngine::_resolveFontMetrics(const FontInfoDesired& fontInfoDesired, Fo
fontMetrics->colorGlyphs = fontInfoDesired.GetEnableColorGlyphs();
}
}

// Nearby fonts are described a couple of times throughout the file.
// This abstraction in particular helps us avoid retrying when it's pointless:
// After all, if the font collection didn't change (no nearby fonts, loading failed, it's already loaded),
// we don't need to try it again. It returns true if retrying is necessary.
[[nodiscard]] bool AtlasEngine::_updateWithNearbyFontCollection() noexcept
{
// _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.
// 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.
wil::com_ptr<IDWriteFontCollection> collection;
try
{
collection = FontCache::GetCached();
}
CATCH_LOG();

if (!collection || _api.s->font->fontCollection == collection)
{
return false;
}

_api.s.write()->font.write()->fontCollection = std::move(collection);
return true;
}
lhecker marked this conversation as resolved.
Show resolved Hide resolved
5 changes: 3 additions & 2 deletions src/renderer/atlas/AtlasEngine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,9 @@ namespace Microsoft::Console::Render::Atlas

// AtlasEngine.api.cpp
void _resolveTransparencySettings() noexcept;
void _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);
void _resolveFontMetrics(const FontInfoDesired& fontInfoDesired, FontInfo& fontInfo, FontSettings* fontMetrics = nullptr) const;
[[nodiscard]] HRESULT _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;
void _resolveFontMetrics(const FontInfoDesired& fontInfoDesired, FontInfo& fontInfo, FontSettings* fontMetrics = nullptr);
[[nodiscard]] bool _updateWithNearbyFontCollection() noexcept;

// AtlasEngine.r.cpp
ATLAS_ATTR_COLD void _recreateAdapter();
Expand Down
Loading