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.

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

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

Merged
merged 3 commits into from
Apr 1, 2024

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Apr 1, 2024

  • Since FindFontWithLocalizedName is broken (intentionally and
    temporarily until Any use of FindFontWithLocalizedName is broken and requires refactoring #16943 is fixed) we have to be extra be careful
    not to return a nullptr Font.
  • Portable builds may not have a broken font cache, but also not have
    the given font (Cascadia Mono for instance) installed. This requires
    us to load the nearby fonts even if there aren't any exceptions.

Validation Steps Performed

  • Open src/cascadia/CascadiaResources.build.items
    and remove the Condition for .ttf files
  • Deploy on a clean Windows 10 VM
  • Cascadia Mono loads without issues ✅
  • Open the Settings > Defaults > Appearance,
    enter a non-existing font and hit Save
  • Doesn't crash ✅

@lhecker lhecker added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Priority-1 A description (P1) Area-Settings UI Anything specific to the SUI labels Apr 1, 2024
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.

@@ -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.

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.

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

src/renderer/atlas/AtlasEngine.api.cpp Show resolved Hide resolved
@zadjii-msft
Copy link
Member

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

assuming the instrumentation build passes, of course

// 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

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.

@DHowett
Copy link
Member

DHowett commented Apr 1, 2024

image
works!

@DHowett DHowett merged commit 9f3dbab into main Apr 1, 2024
13 of 17 checks passed
@DHowett DHowett deleted the dev/lhecker/2664-font-fallback-fixup branch April 1, 2024 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Area-Settings UI Anything specific to the SUI Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants