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

Any use of FindFontWithLocalizedName is broken and requires refactoring #16943

Closed
zadjii-msft opened this issue Mar 26, 2024 · 2 comments · Fixed by #17164
Closed

Any use of FindFontWithLocalizedName is broken and requires refactoring #16943

zadjii-msft opened this issue Mar 26, 2024 · 2 comments · Fixed by #17164
Assignees
@lhecker
Labels
Area-Fonts Related to the font Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree.
Milestone
Terminal v1.21

Comments

@zadjii-msft
Copy link
Member

From #16821

Font features and axes have multiple blocking bugs right now anyway (I mean even without this PR), so I figured that we can do a follow-up refactoring later where we fix both issues: The existing bugs and the incompatibilities introduced by this one. I'm half done making the changes to fix this and they're highly related
@zadjii-msft zadjii-msft added Area-Fonts Related to the font Product-Terminal The new Windows Terminal. Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. labels Mar 26, 2024
@zadjii-msft zadjii-msft added this to the Terminal v1.21 milestone Mar 26, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Mar 26, 2024
@zadjii-msft zadjii-msft added Severity-Blocking We won't ship a release like this! No-siree. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Mar 26, 2024
@lhecker lhecker mentioned this issue Apr 1, 2024
Merged
DHowett pushed a commit that referenced this issue Apr 1, 2024
@lhecker
Fix bugs introduced in #16821 (custom font fallback) (#16993)
9f3dbab 
* Since `FindFontWithLocalizedName` is broken (intentionally and
  temporarily until #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 ✅
@zadjii-msft
Copy link
Member Author

@lhecker I was told this was blocking for 1.21 - are we fine shipping 1.21 with this?

@lhecker
Copy link
Member

lhecker commented Apr 29, 2024

No. I'm currently working on this issue.

Technically this should be a trivial change but funneling this into XAML turns out is harder than writing the renderer lol. "Oh this doesn't have a ItemsSource you need to instantiate UI elements yourself. Oh this is only a getter, needs a manual injection. Oh you can't set Items in the Click handler or it gets unresponsive. Oh you can't have generic types in a DataTemplate, needs a custom wrapper. Oh you can't reference the VM from inside a DataTemplate. Oh…" Hilarious lol.

@lhecker lhecker mentioned this issue Apr 30, 2024
Merged
@lhecker lhecker changed the title code health: Any use of FindFontWithLocalizedName is broken and requires refactoring Any use of FindFontWithLocalizedName is broken and requires refactoring Apr 30, 2024
@lhecker lhecker added Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) and removed Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. labels Apr 30, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Tag-Fix Doesn't match tag requirements label Apr 30, 2024
github-merge-queue bot pushed a commit that referenced this issue May 1, 2024
@lhecker
Fix font axes/features settings UI (#17164)
d380394 
Due to #16821 everything about #16104 broke. This PR rights the wrongs
by rewriting all the `Font`-based code to not use `Font` at all.
Instead we split the font spec once into font families, do a lot of
complex logic to split font axes/features into used and unused ones
and construct all the UI elements. So. much. boilerplate. code.

Closes #16943

## Validation Steps Performed
There are more edge cases than I can list here... Some ideas:
* Edit the settings.json with invalid axis/feature keys ✅
* ...out of range values ✅
* Settings UI reloads when the settings.json changes ✅
* Adding axes/features works ✅
* Removing axes/features works ✅
* Resetting axes/features works ✅
* Axes/features apply in the renderer when saving ✅
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lhecker lhecker

Labels
Area-Fonts Related to the font Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree.
Projects
None yet
Milestone
Terminal v1.21
Development

Successfully merging a pull request may close this issue.

Fix font axes/features settings UI microsoft/terminal
2 participants
@lhecker @zadjii-msft