Skip to content

Commit

Permalink
Don't throw in GetProposedDimensions (#10260)
Browse files Browse the repository at this point in the history 
I cannot for the life of me repro the original bug. I've got fonts with bad permissions SxS, I've tried installing a font twice, I've tried stopping the font cache service. No idea how to manually repro the original bug.

BUT theoretically, this function should never throw. So lets just switch this to a `LOG_IF_FAILED`, and hope that this goes away? 

* [x] Fixes #10211?
* [x] built & ran manually.

Unclear if this can get cherry-picked trivially to 1.8. Code's pretty trivial though so if we need another PR for that, it can be arranged.
  • Loading branch information
@zadjii-msft
zadjii-msft authored May 28, 2021
1 parent d8647e0 commit 89ca2ae
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 50 deletions.
6 changes: 4 additions & 2 deletions src/cascadia/TerminalControl/TermControl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1845,9 +1845,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// will take up.
// TODO: MSFT:21254947 - use a static function to do this instead of
// instantiating a DxEngine
// GH#10211 - UNDER NO CIRCUMSTANCE should this fail. If it does, the
// whole app will crash instantaneously on launch, which is no good.
auto dxEngine = std::make_unique<::Microsoft::Console::Render::DxEngine>();
THROW_IF_FAILED(dxEngine->UpdateDpi(dpi));
THROW_IF_FAILED(dxEngine->UpdateFont(desiredFont, actualFont));
LOG_IF_FAILED(dxEngine->UpdateDpi(dpi));
LOG_IF_FAILED(dxEngine->UpdateFont(desiredFont, actualFont));

const auto scale = dxEngine->GetScaling();
const auto fontSize = actualFont.GetSize();
Expand Down
109 changes: 62 additions & 47 deletions src/renderer/dx/DxFontRenderData.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -644,63 +644,77 @@ CATCH_RETURN()
{
// First attempt to find exactly what the user asked for.
didFallback = false;
auto face = _FindFontFace(familyName, weight, stretch, style, localeName);
Microsoft::WRL::ComPtr<IDWriteFontFace1> face{ nullptr };

if (!face)
// GH#10211 - wrap this all up in a try/catch. If the nearby fonts are
// corrupted, then we don't want to throw out of this top half of this
// method. We still want to fall back to a font that's reasonable, below.
try
{
// If we missed, try looking a little more by trimming the last word off the requested family name a few times.
// Quite often, folks are specifying weights or something in the familyName and it causes failed resolution and
// an unexpected error dialog. We theoretically could detect the weight words and convert them, but this
// is the quick fix for the majority scenario.
// The long/full fix is backlogged to GH#9744
// Also this doesn't count as a fallback because we don't want to annoy folks with the warning dialog over
// this resolution.
while (!face && !familyName.empty())
{
const auto lastSpace = familyName.find_last_of(UNICODE_SPACE);

// value is unsigned and npos will be greater than size.
// if we didn't find anything to trim, leave.
if (lastSpace >= familyName.size())
{
break;
}

// trim string down to just before the found space
// (space found at 6... trim from 0 for 6 length will give us 0-5 as the new string)
familyName = familyName.substr(0, lastSpace);
face = _FindFontFace(familyName, weight, stretch, style, localeName, true);

// Try to find it with the shortened family name
face = _FindFontFace(familyName, weight, stretch, style, localeName);
}

// Alright, if our quick shot at trimming didn't work either...
// move onto looking up a font from our hardcoded list of fonts
// that should really always be available.
if (!face)
{
for (const auto fallbackFace : FALLBACK_FONT_FACES)
// If we missed, try looking a little more by trimming the last word off the requested family name a few times.
// Quite often, folks are specifying weights or something in the familyName and it causes failed resolution and
// an unexpected error dialog. We theoretically could detect the weight words and convert them, but this
// is the quick fix for the majority scenario.
// The long/full fix is backlogged to GH#9744
// Also this doesn't count as a fallback because we don't want to annoy folks with the warning dialog over
// this resolution.
while (!face && !familyName.empty())
{
familyName = fallbackFace;
face = _FindFontFace(familyName, weight, stretch, style, localeName);
const auto lastSpace = familyName.find_last_of(UNICODE_SPACE);

if (face)
// value is unsigned and npos will be greater than size.
// if we didn't find anything to trim, leave.
if (lastSpace >= familyName.size())
{
didFallback = true;
break;
}

familyName = fallbackFace;
weight = DWRITE_FONT_WEIGHT_NORMAL;
stretch = DWRITE_FONT_STRETCH_NORMAL;
style = DWRITE_FONT_STYLE_NORMAL;
face = _FindFontFace(familyName, weight, stretch, style, localeName);
// trim string down to just before the found space
// (space found at 6... trim from 0 for 6 length will give us 0-5 as the new string)
familyName = familyName.substr(0, lastSpace);

if (face)
{
didFallback = true;
break;
}
// Try to find it with the shortened family name
face = _FindFontFace(familyName, weight, stretch, style, localeName, true);
}
}
}
CATCH_LOG();

// Alright, if our quick shot at trimming didn't work either...
// move onto looking up a font from our hardcoded list of fonts
// that should really always be available.
if (!face)
{
for (const auto fallbackFace : FALLBACK_FONT_FACES)
{
familyName = fallbackFace;
// With these fonts, don't attempt the nearby lookup. We're looking
// for system fonts only. If one of the nearby fonts is causing us
// problems (like in GH#10211), then we don't want to go anywhere

// near it in this part.
face = _FindFontFace(familyName, weight, stretch, style, localeName, false);

if (face)
{
didFallback = true;
break;
}

familyName = fallbackFace;
weight = DWRITE_FONT_WEIGHT_NORMAL;
stretch = DWRITE_FONT_STRETCH_NORMAL;
style = DWRITE_FONT_STYLE_NORMAL;
face = _FindFontFace(familyName, weight, stretch, style, localeName, false);

if (face)
{
didFallback = true;
break;
}
}
}
Expand All @@ -723,7 +737,8 @@ CATCH_RETURN()
DWRITE_FONT_WEIGHT& weight,
DWRITE_FONT_STRETCH& stretch,
DWRITE_FONT_STYLE& style,
std::wstring& localeName) const
std::wstring& localeName,
const bool withNearbyLookup) const
{
Microsoft::WRL::ComPtr<IDWriteFontFace1> fontFace;

Expand All @@ -735,7 +750,7 @@ CATCH_RETURN()
THROW_IF_FAILED(fontCollection->FindFamilyName(familyName.data(), &familyIndex, &familyExists));

// If the system collection missed, try the files sitting next to our binary.
if (!familyExists)
if (withNearbyLookup && !familyExists)
{
auto&& nearbyCollection = NearbyCollection();

Expand Down
3 changes: 2 additions & 1 deletion src/renderer/dx/DxFontRenderData.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,8 @@ namespace Microsoft::Console::Render
DWRITE_FONT_WEIGHT& weight,
DWRITE_FONT_STRETCH& stretch,
DWRITE_FONT_STYLE& style,
std::wstring& localeName) const;
std::wstring& localeName,
const bool withNearbyLookup) const;

[[nodiscard]] std::wstring _GetFontFamilyName(gsl::not_null<IDWriteFontFamily*> const fontFamily,
std::wstring& localeName) const;
Expand Down

0 comments on commit 89ca2ae

Please sign in to comment.