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 font axes/features settings UI #17164

Merged
merged 5 commits into from
May 1, 2024
Merged

Fix font axes/features settings UI #17164

merged 5 commits into from
May 1, 2024

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Apr 30, 2024

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 ✅

#include "EnumEntry.h"
#include "ProfileViewModel.h"

#include "Appearances.g.cpp"
Copy link
Member Author

Choose a reason for hiding this comment

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

Welcome to the doomsday diff. This was awful to write, so I'm sure it's going to be awful to read.

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

I think I understand all of this, which is crazy.

@@ -349,7 +349,7 @@
"description": "Sets the DWrite font features for the given font. For example, { \"ss01\": 1, \"liga\":0 } will enable ss01 and disable ligatures.",
"type": "object",
"patternProperties": {
"^(([A-Za-z0-9]){4})$": {
"^[\\x00-\\x7E]{4}$": {
Copy link
Member

Choose a reason for hiding this comment

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

leonard you're scaring me

are there opentype features that we were excluding from pleasant company?

... should we keep excluding them?

Copy link
Member Author

Choose a reason for hiding this comment

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

When implementing the corresponding C++ code for this, I checked the OpenType and CSS 4 docs. Tags are apparently any 4 letter printable ASCII, so I changed the code and schema accordingly.

Choose a reason for hiding this comment

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

"printable" would start at \x20 right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh wow, I didn't notice that typo at all! Thanks!

@@ -731,6 +731,16 @@ namespace Microsoft::Terminal::Settings::Model::JsonUtils

Json::Value ToJson(const float val)
{
// Convert floats that are almost integers to proper integers, because that looks way neater in JSON.
Copy link
Member

Choose a reason for hiding this comment

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

oh i really like this

src/cascadia/TerminalSettingsModel/FontConfig.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsEditor/Appearances.xaml Outdated Show resolved Hide resolved

--*/

#pragma once
Copy link
Member

Choose a reason for hiding this comment

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

HAHAH line endings strikes again

Can you confirm whether this is correct? UNFORTUNATELY, our repo stores CRLF internally.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems it's the other way around. These files are LF in our repo and I accidentally turned this file into CRLF.

Copy link
Member Author

@lhecker lhecker Apr 30, 2024

Choose a reason for hiding this comment

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

src/cascadia/TerminalSettingsEditor/Appearances.cpp Outdated Show resolved Hide resolved
void AxisKeyValuePair::AxisIndex(int32_t axisIndex)
// You can't return a const-ref from a WinRT function, because the cppwinrt generated wrapper chokes on it.
// So, now we got two KeyDisplayString() functions, because I refuse to AddRef/Release this for no reason.
// I mean, really it makes no perf. difference, but I'm not kneeling down for an incompetent code generator.
Copy link
Member

Choose a reason for hiding this comment

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

mood

@DHowett DHowett requested a review from PankajBhojwani April 30, 2024 23:11
@lhecker lhecker force-pushed the dev/lhecker/16943-sui-axes branch from 41a3df2 to 0077ed7 Compare April 30, 2024 23:33
Copy link
Contributor

@PankajBhojwani PankajBhojwani left a comment

Choose a reason for hiding this comment

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

Played around with it, looks and feels great!

@lhecker lhecker added this pull request to the merge queue May 1, 2024
Merged via the queue into main with commit d380394 May 1, 2024
20 checks passed
@lhecker lhecker deleted the dev/lhecker/16943-sui-axes branch May 1, 2024 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Any use of FindFontWithLocalizedName is broken and requires refactoring
4 participants