-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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
Add support for customizing font fallback #16821
Conversation
This comment has been minimized.
This comment has been minimized.
_actualFont.GetFaceName()) }; | ||
auto noticeArgs = winrt::make<NoticeEventArgs>(NoticeLevel::Warning, message); | ||
_RaiseNoticeHandlers(*this, std::move(noticeArgs)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This went into TermControl.cpp
.
{ | ||
} | ||
|
||
WINRT_PROPERTY(uint64_t, Result); | ||
WINRT_PROPERTY(HRESULT, Result); | ||
WINRT_PROPERTY(winrt::hstring, Parameter); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will allow us to carry messages to the warning popup code in TermControl
.
} | ||
} | ||
return _hasPowerlineCharacters.value_or(false); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Font::HasPowerlineCharacters()
was moved to AppearanceViewModel::HasPowerlineCharacters()
, because "has" is now a function of >1 fonts (= can't be attached to a single Font
instance). The detection itself went into _refreshFontFaceDependents
below.
@@ -526,7 +618,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation | |||
|
|||
// find one axis that does not already exist, and add that | |||
// if there are no more possible axes to add, the button is disabled so there shouldn't be a way to get here | |||
const auto possibleAxesTagsAndNames = ProfileViewModel::FindFontWithLocalizedName(FontFace()).FontAxesTagsAndNames(); | |||
const auto possibleAxesTagsAndNames = ProfileViewModel::FindFontWithLocalizedName(_primaryFontName).FontAxesTagsAndNames(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are all placeholders until we refactor the font axis/feature code later on.
Refactoring it as part of this PR was not possible unfortunately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious as to what the refactor entails - do we need to combine all the possible axes/features for all the fonts in the given family?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. We'll have to basically detach the Font
object entirely from axes and features. In my opinion we should move the (re)generation of axes and features from Font
, ProfileViewModel
, and Appearance
all over into AppearanceViewModel
.
There, we have access to the parsed list of fonts families (which this PR adds) and this allows us to generate the union of axes and features. Then we can split the collection up into those that are currently in use and those that are still available for selection and expose both lists (in whatever form) to the view.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it may be worth mentioning that we create AppearanceViewModel
s eagerly when loading the settings UI, which is not great, because this causes all the DirectWrite code to run eagerly too. It'd be nice if they were created lazily somehow. (I mean we don't have to address that now - it's just a thing I noticed.)
@@ -39,20 +39,21 @@ struct ViewModelHelper | |||
|
|||
#define _BASE_OBSERVABLE_PROJECTED_SETTING(target, name) \ | |||
public: \ | |||
auto name() const noexcept \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WinRT function calls are never noexcept
so this was always a lie. 😅
src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw
Outdated
Show resolved
Hide resolved
// Commas are treated literally inside strings. | ||
break; | ||
} | ||
if (!family.empty()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delayedSpace is ignored here - which I believe is correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because we want to strip trailing whitespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a ✅ EXCEPT for the very scary sounding FindFontWithLocalizedName
comment that doesn't have a linked work item. If that's 1.21 blocking, then we need to make sure we get to it.
appearance.FontFace(fontSpec); | ||
} | ||
|
||
// TODO: Any use of FindFontWithLocalizedName is broken and requires refactoring in time for version 1.21. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on a scale of 1-scary, this comment sounds scary? Like, ship-blocking scary. Do we have a plan for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmk. I'll file something and tag you on it just to make sure we don't lose it.
* 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 ✅
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 ✅
This doesn't seem to work? I'm configuring it as follows: "profiles":
{
"defaults":
{
"colorScheme": "One Half Dark",
"experimental.retroTerminalEffect": false,
"font":
{
"face": "Comic Code Ligatures, FiraCode Nerd Font Mono",
"features": {
"liga": 1
},
"size": 10.0,
"weight": "light"
},
"opacity": 80,
"useAcrylic": true
}, and get the following error: I'm on version |
It's in Windows Terminal Preview right now (version 1.21). You can find it in the app store, or here: https://github.com/microsoft/terminal/releases/tag/v1.21.1772.0 |
Ah I thought I was going crazy, but makes sense. Thanks a lot! Works like a charm as well!! |
This adds support for specifying more than one font family using a
syntax that is similar to CSS'
font-family
property.The implementation is straight-forward and is effectively
just a wrapper around
IDWriteFontFallbackBuilder
.Closes #2664
PR Checklist