Skip to content

Another attempt at fixing nearby font loading - #12904

Merged
1 commit merged into
mainfrom
dev/lhecker/11648-nearby-fix-again
Apr 14, 2022
Merged

1 commit merged into
mainfrom
dev/lhecker/11648-nearby-fix-again

Conversation

@lhecker

@lhecker Leonard Hecker (lhecker) commented Apr 14, 2022 •

Copy link
Copy Markdown
Member

The original research for a solution all the way back in #11032 contained an
unfortunate flaw. The nearby font loading code was written under the assumption
that Cascadia is missing in the system font collection, leading to our issues.
Adding nearby fonts last into the collection would thus ensure that we use
the system fonts whenever possible, but only have nearby fonts as a fallback.

This didn't work and we figured that we'd have to always prefer loading nearby
fonts over system fonts. #12554 tried to achieve this, but failed to change
the order in which the font set is built. In order to prefer nearby fonts
over system ones, we have to add the system font collection last.

PR Checklist

Validation Steps Performed

  • Put Jetbrains Mono into the AppX directory of the Debug build
  • Jetbrains Mono shows up in the font selector and is useable

Additionally a more complex mini-test was built:
Using FontForge I've cloned arial.ttf and removed all characters except for
the letter "0". Afterwards I've build a custom font collection the same way
we do it in Terminal, extracted a FontFace named "Arial" and called
IDWriteFont::HasCharacter for the letter "1".
Loading the system font collection first results in TRUE and loading it last
results in FALSE (since my custom arial.ttf doesn't have the letter "1").
This confirms that we need to load the system font collection last.

@ghost ghost added Area-Fonts Related to the font Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. labels Apr 14, 2022
@lhecker

Copy link
Copy Markdown
Member Author

Here's the source code for the "mini-test":

#define UNICODE
#define NOMINMAX
#define WIN32_LEAN_AND_MEAN

#include <Windows.h>
#include <dwrite_3.h>

#include <cstdio>

int main() {
    IDWriteFactory7* factory;
    DWriteCreateFactory(DWRITE_FACTORY_TYPE_SHARED, __uuidof(factory), reinterpret_cast<IUnknown**>(&factory));

    IDWriteFontCollection1* systemFontCollection;
    factory->GetSystemFontCollection(false, &systemFontCollection);

    IDWriteFontSet* systemFontSet;
    systemFontCollection->GetFontSet(&systemFontSet);

    IDWriteFontSetBuilder2* fontSetBuilder;
    factory->CreateFontSetBuilder(&fontSetBuilder);

    // !!!! Change this order around to see the results !!!!
    fontSetBuilder->AddFontFile(L"arial1.ttf");
    fontSetBuilder->AddFontFile(L"arial2.ttf");
    fontSetBuilder->AddFontSet(systemFontSet);

    IDWriteFontSet* fontSet;
    fontSetBuilder->CreateFontSet(&fontSet);

    IDWriteFontCollection1* fontCollection;
    factory->CreateFontCollectionFromFontSet(fontSet, &fontCollection);

    UINT32 index;
    BOOL exists;
    fontCollection->FindFamilyName(L"Arial", &index, &exists);

    IDWriteFontFamily1* fontFamily;
    fontCollection->GetFontFamily(index, &fontFamily);

    IDWriteFont* font;
    fontFamily->GetFirstMatchingFont(DWRITE_FONT_WEIGHT_NORMAL, DWRITE_FONT_STRETCH_NORMAL, DWRITE_FONT_STYLE_NORMAL, &font);

    BOOL has0;
    BOOL has1;
    BOOL has2;
    font->HasCharacter(L'0', &has0);
    font->HasCharacter(L'1', &has1);
    font->HasCharacter(L'2', &has2);

    printf("0: %d\n1: %d\n2: %d\n", has0, has1, has2);

    return 0;
}

@zadjii-msft Mike Griese (zadjii-msft) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

triple-facepalm

@lhecker

Copy link
Copy Markdown
Member Author

image

LOG_IF_FAILED(fontSetBuilder->AddFontFile(file.get()));
}

// IDWriteFontSetBuilder ignores any families that have already been added.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Critically, this is true on Windows 10, Right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yep, tested!

@lhecker Leonard Hecker (lhecker) added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 14, 2022
@ghost

Copy link
Copy Markdown

Hello Leonard Hecker (@lhecker)!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost
ghost merged commit eeb8970 into main Apr 14, 2022
@ghost
ghost deleted the dev/lhecker/11648-nearby-fix-again branch April 14, 2022 22:05
Dustin L. Howett (DHowett) pushed a commit that referenced this pull request Apr 21, 2022
The original research for a solution all the way back in #11032 contained an
unfortunate flaw. The nearby font loading code was written under the assumption
that Cascadia is missing in the system font collection, leading to our issues.
Adding nearby fonts last into the collection would thus ensure that we use
the system fonts whenever possible, but only have nearby fonts as a fallback.

This didn't work and we figured that we'd have to always prefer loading nearby
fonts over system fonts. #12554 tried to achieve this, but failed to change
the order in which the font set is built. In order to prefer nearby fonts
over system ones, we have to add the system font collection last.

## PR Checklist

* [x] Closes #11648
* [x] I work here
* [x] Tests added/passed
* [x] Embarrassment for my incompetence

## Validation Steps Performed

* Put Jetbrains Mono into the AppX directory of the Debug build
* Jetbrains Mono shows up in the font selector and is useable

Additionally a more complex mini-test was built:
Using FontForge I've cloned arial.ttf and removed all characters except for
the letter "0". Afterwards I've build a custom font collection the same way
we do it in Terminal, extracted a `FontFace` named "Arial" and called
`IDWriteFont::HasCharacter` for the letter "1".
Loading the system font collection first results in `TRUE` and loading it last
results in `FALSE` (since my custom arial.ttf doesn't have the letter "1").
This confirms that we need to load the system font collection last.

(cherry picked from commit eeb8970)
Service-Card-Id: 80641214
Service-Version: 1.13
Dustin L. Howett (DHowett) pushed a commit that referenced this pull request Apr 21, 2022
The original research for a solution all the way back in #11032 contained an
unfortunate flaw. The nearby font loading code was written under the assumption
that Cascadia is missing in the system font collection, leading to our issues.
Adding nearby fonts last into the collection would thus ensure that we use
the system fonts whenever possible, but only have nearby fonts as a fallback.

This didn't work and we figured that we'd have to always prefer loading nearby
fonts over system fonts. #12554 tried to achieve this, but failed to change
the order in which the font set is built. In order to prefer nearby fonts
over system ones, we have to add the system font collection last.

## PR Checklist

* [x] Closes #11648
* [x] I work here
* [x] Tests added/passed
* [x] Embarrassment for my incompetence

## Validation Steps Performed

* Put Jetbrains Mono into the AppX directory of the Debug build
* Jetbrains Mono shows up in the font selector and is useable

Additionally a more complex mini-test was built:
Using FontForge I've cloned arial.ttf and removed all characters except for
the letter "0". Afterwards I've build a custom font collection the same way
we do it in Terminal, extracted a `FontFace` named "Arial" and called
`IDWriteFont::HasCharacter` for the letter "1".
Loading the system font collection first results in `TRUE` and loading it last
results in `FALSE` (since my custom arial.ttf doesn't have the letter "1").
This confirms that we need to load the system font collection last.

(cherry picked from commit eeb8970)
Service-Card-Id: 80641213
Service-Version: 1.12
@ghost

Copy link
Copy Markdown

🎉Windows Terminal v1.13.1143 has been released which incorporates this pull request.:tada:

Handy links:

@ghost

Copy link
Copy Markdown

🎉Windows Terminal Preview v1.14.143 has been released which incorporates this pull request.:tada:

Handy links:

@kczx3

kczx3 commented Jun 7, 2022

Copy link
Copy Markdown

Funny enough, I got back from a two week vacation today and the live start menu icon for Terminal showed like it was stuck on a partial upgrade or something. Opened Terminal and it seemed to work (showing version 1.13.11431.0) and correctly used Cascadia Mono, however my Visual Studio Code install could no longer find that font. Additionally, looking in my installed fonts within Windows Settings I could not find either Cascadia Code or Cascadia Mono. Repairing the Terminal install fixed the issue. Sadly, that's what I've had to do with the last two upgrades of Terminal as well. Perhaps 1.14 will finally be the version where that no longer occurs. Who knows, maybe my situation was just a fluke thing.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Fonts Related to the font AutoMerge Marked for automatic merge by the bot when requirements are met 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.

Terminal suddenly can't find the font Cascadia Mono

4 participants