Implement cell size customizations #14255

lhecker · 2022-10-19T17:37:24Z

Does what it says in the title. After this commit you can customize the height
and width of the terminal's cells. This commit supports parts of CSS'
<length-percentage> data type: Font-size relative sizes as multiples (1.2),
percentage (120%), or advance-width relative (1.2ch), as well as absolute
sizes in CSS pixels (px) or points (pt).

This PR is neither bug free in DxEngine, nor in AtlasEngine.
The former fails to implement glyph advance corrections (for instance #9381),
as well as disallowing glyphs to overlap rows. The latter has the same
overlap issue, but more severely as it tries to shrink glyphs to fit in.

Closes #3498
Closes #14068

Validation Steps Performed

Setting height to 1 creates 12pt tall rows ✅
Setting height to 1ch creates square cells ✅
Setting width to 1 creates square cells ✅
Setting width or height to Npx or Npt works ✅
Trailing zeroes are trimmed properly during serialization ✅
Patching the PR to allow >100 line heights and entering "100.123456"
displays 6 fractional digits ✅

lhecker · 2022-10-19T23:04:56Z

src/cascadia/TerminalSettingsEditor/Appearances.cpp

+            IncrementNumberRounder rounder;
+            rounder.Increment(1e-6);


See microsoft/microsoft-ui-xaml#7851

lhecker · 2022-10-20T12:36:04Z

Finally, after decade waiting for this 😂😘

Before you get too excited, I have to mention again that this PR doesn't work great by itself. Neither the old/stable nor new/preview text renderer work correctly with line heights less than approx. 1.2, which is what a significant portion of people in #3498 have asked for though. I'm working on that separately, but that will unfortunately take another couple months to complete on the side.

carlos-zamora

Mainly concerned about inheritance being done properly. It feels weird to introduce two settings in the cellSize object but then store them as two different members in the settings model.

Also confused why this isn't going in the FontConfig.

src/cascadia/TerminalSettingsModel/Profile.idl

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

src/cascadia/TerminalSettingsEditor/Appearances.xaml

src/cascadia/TerminalSettingsEditor/Appearances.cpp

…-heights

lhecker · 2022-12-06T23:32:55Z

I've moved the two fields into the font config object now. With the primary target being the line height customization, the resulting config might look something like this to the user:

{
    "profiles":
    {
        "defaults":
        {
            "font":
            {
                "face": "Consolas",
                "size": 13,
                "cellHeight": "1.4"
            }
        }
    }
}

…-heights

DHowett

I have to trust the math in AtlasEngine.api.cpp. I did however check the math in Dx :)

DHowett · 2023-02-14T22:17:10Z

src/cascadia/TerminalSettingsEditor/Appearances.idl

@@ -39,6 +39,7 @@ namespace Microsoft.Terminal.Settings.Editor

        OBSERVABLE_PROJECTED_APPEARANCE_SETTING(String, FontFace);
        OBSERVABLE_PROJECTED_APPEARANCE_SETTING(Single, FontSize);
+        OBSERVABLE_PROJECTED_APPEARANCE_SETTING(Double, LineHeight);


one's a single, one's a double!

Double is technically the better one, because that's what WinUI's setters and getters use. But we use float and Single throughout the project (which is alright - we don't need more than 2-3 decimal places for anything), including for the FontSize property. LineHeight on the other hand is a custom getter/setter and I felt like it'd be better to use Double if we can when interfacing with WinUI.

DHowett · 2023-02-14T22:17:24Z

src/cascadia/TerminalSettingsEditor/Appearances.xaml

@@ -219,7 +219,6 @@
                                    Visibility="{x:Bind Appearance.IsDefault, Mode=OneWay}">
                <muxc:NumberBox x:Name="_fontSizeBox"
                                x:Uid="Profile_FontSizeBox"
-                                AcceptsExpression="False"


Does it accept expressions now?

false is the default!

DHowett · 2023-02-14T22:18:10Z

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

+    <comment>A description for what the "line height" setting does. Presented near "Profile_LineHeight".</comment>
+  </data>
+  <data name="Profile_LineHeightBox.PlaceholderText" xml:space="preserve">
+    <value>1.2</value>


will users be confused that it says 1.2 but the default is not 1.2?

I felt like a placeholder made the input feel a little less confusing. 1.2 is reasonably close to the line height of most fonts (I've seen anything between about 1.15 and 1.35).

DHowett · 2023-02-14T22:19:06Z

src/cascadia/TerminalSettingsModel/Profile.cpp

-    // Font settings
-    const auto fontInfoImpl = winrt::get_self<FontConfig>(_FontInfo);
-    if (fontInfoImpl->HasAnyOptionSet())
+    if (auto fontJSON = winrt::get_self<FontConfig>(_FontInfo)->ToJson(); !fontJSON.empty())


interesting. serialize first, then check!

Yep! I felt like that it made the code a lot simpler.

DHowett · 2023-02-14T22:21:19Z

src/renderer/base/CSSLengthPercentage.cpp

+
+CSSLengthPercentage CSSLengthPercentage::FromString(const wchar_t* str)
+{
+    auto& errnoRef = errno; // Nonzero cost, pay it once.


What is the cost and how does this ensure we pay it only once? Is it the thread safety?

I think it's because errno is a thread-local variable and this gets the reference to the actual storage early. I took the code and the comment from the STL.

DHowett · 2023-02-14T22:22:05Z

src/renderer/base/CSSLengthPercentage.cpp

+        }
+        else
+        {
+            return {};


silent discard - OK?

i notice we have no way of expressing a failure to parse this via the settings warning dialog

It's not okay, but I tried the alternative by implementing a WinRT runtimeclass for CSSLengthPercentage and using that as the value for CellWidth/Height and that was quite the mess. I would not mind adding an extra check during the settings loading. I could call CSSLengthPercentage::FromString inside the FontConfig::LayerJson to check if it's valid since wcstof and wcscmp aren't /that/ expensive. But I wasn't personally convinced it's worth it (by asking myself "Would anyone notice if I don't check this?"). Would you say I should do it?

DHowett · 2023-02-14T22:23:04Z

src/renderer/base/CSSLengthPercentage.cpp

+
+float CSSLengthPercentage::Resolve(float factor) const noexcept
+{
+    return _value * factor;


I don't love how we have two functions, same name, one of which uses the local reference frame and one of which doesn't. Gotta look more to see what it means

DHowett · 2023-02-14T22:24:17Z

src/renderer/base/lib/base.vcxproj

@@ -11,6 +11,7 @@
  <Import Project="$(SolutionDir)src\common.build.pre.props" />
  <Import Project="$(SolutionDir)src\common.nugetversions.props" />
  <ItemGroup>
+    <ClCompile Include="..\CSSLengthPercentage.cpp" />


Do you need to add this to sources, or will it be okay to not link anything related to CSSLengthPercentage inside Windows?

src/renderer/inc/CSSLengthPercentage.h

DHowett · 2023-02-14T22:31:00Z

src/renderer/dx/DxFontRenderData.cpp

+    const auto lineHeight = desired.GetCellHeight().Resolve(defaultHeight, dpiF, heightDesired, widthAdvanceInPx);
+    const auto baseline = fullPixelAscent + (lineHeight - defaultHeight) / 2.0f;
+
+    lineSpacing.height = roundf(lineHeight);


This isn't going to result in a different outcome for users who haven't customized cellHeight, right?

Yeah, because defaultHeight is an integer and that's what Resolve() returns if no cellHeight is set.

daymxn · 2023-02-21T05:23:53Z

with line heights less than approx. 1.2, which is what a significant portion of people in #3498 have asked for though.

Just for the sake of clarity; while a lot of the vocal majority are wanting smaller line heights- there are folk like me out there who want the opposite. I want more distance between my lines. As such, I speak for us all when I say- thank you for doing this. It's the small QoL stuff like this that honestly adds up.

This just removes some leftover code that I forgot to remove before the merge. Additionally I forgot to add a newly added file to our `sources` build file.

Diablo-D3 · 2023-02-28T21:52:38Z

I would love to have access to this feature. Is 1.18 on the horizon?

zadjii-msft · 2023-02-28T21:55:01Z

I would love to have access to this feature. Is 1.18 on the horizon?

Yep, probably not for a couple months though. We're working on some... bigger... features that might take longer to bake. We're still discussing the finer points of our plan here though.

This is practically a from scratch rewrite of AtlasEngine. The initial approach used a very classic monospace text renderer, where the viewport is subdivided into cells and each cell is assigned one glyph texture, just like how real terminals used to work. While we knew that it would have problems with overly large glyphs, like those found in less often used languages, we didn't expect the absolutely massive number of fonts that this approach would break. For one, the assumption that monospace fonts are actually mostly monospace has turned out to be a complete lie and we can't force users to use better designed fonts. But more importantly, we can't just design an entire Unicode fallback font collection from scratch where every major glyph is monospace either. This is especially problematic for vertical overhangs which are extremely difficult to handle in a way that outperforms the much simpler alternative approach: Just implementing a bog-standard, modern, quad-based text renderer. Such an approach is both, less code and runs faster due to a less complex CPU-side. The text shaping engine (in our case DirectWrite) has to resolve text into glyph indices anyways, so using them directly for text rendering allows reduces the effort of turning it back into text ranges and hashing those. It's memory overhead is also reduced, because we can now break up long ligatures into their individual glyphs. Especially on AMD APUs I found this approach to run much faster. A list of issues I think are either obsolete (and could be closed) or resolved with this PR in combination with #14255: Closes #6864 Closes #6974 Closes #8993 Closes #9940 Closes #10128 Closes #12537 Closes #13064 Closes #13527 Closes #13662 Closes #13700 Closes #13989 Closes #14022 Closes #14057 Closes #14094 Closes #14098 Closes #14117 Closes #14533 Closes #14877 ## PR Checklist * Enabling software rendering enables D2D mode ✅ * Both D2D and D3D: * Background appears correctly ✅✅ * Text appears correctly * Cascadia Code Regular ✅✅ * Cascadia Code Bold ✅✅ * Cascadia Code Italic ✅✅ * Cascadia Code block chars leave (almost) no gaps ✅✅ * Terminus TTF at 13.5pt leaves no gaps between block chars ✅✅ * ``"`u{e0b2}`u{e0b0}"`` in Fira Code Nerd Font forms a square ✅✅ * Cursor appears correctly * Legacy small/medium/large ✅✅ * Vertical bar ✅✅ * Underscore ✅✅ * Empty box ✅✅ * Full box ✅✅ * Double underscore ✅✅ * Changing the cursor color works ✅✅ * Selection appears correctly ✅✅ * Scrolling in various manners always renders correctly ✅✅ * Changing the text antialising mode works ✅✅ * Semi-transparent backgrounds work ✅✅ * Scroll-zooming the font size works ✅✅ * Double-size characters work ✅✅ * Resizing while text is printing works ✅✅ * DWM `+Heatmap_ShowDirtyRegions` shows that only the cursor region is dirty when it's blinking ✅✅ * D2D * Margins are filled with background color ❌ They're filled with the neighboring's cell background color for convenience, as D2D doesn't support `D3D11_TEXTURE_ADDRESS_BORDER` * D3D * Margins are filled with background color ✅ * Soft fonts work ✅ * Custom shaders enable continous redraw if time constant is used ✅ * Retro shader appears correctly ✅ * Resizing while a custom shader is running works ✅

Implement cell size customizations

aaec2a5

lhecker force-pushed the dev/lhecker/3498-line-heights branch from 3660b47 to aaec2a5 Compare October 19, 2022 17:38

lhecker commented Oct 19, 2022

View reviewed changes

Fix unit tests

ba8e165

carlos-zamora requested changes Oct 20, 2022

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Oct 20, 2022

lhecker added 2 commits October 20, 2022 23:43

Address parts of the feedback

f6e730e

Merge remote-tracking branch 'origin/main' into dev/lhecker/3498-line…

6bbc1cd

…-heights

carlos-zamora added a commit that referenced this pull request Oct 25, 2022

PRE-MERGE #14255 Implement cell size customizations

0c42e72

ghost added Area-AtlasEngine Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) labels Nov 5, 2022

carlos-zamora added the Needs-Discussion Something that requires a team discussion before we can proceed label Nov 28, 2022

This comment has been minimized.

Sign in to view

Merge remote-tracking branch 'origin/main' into dev/lhecker/3498-line…

9554470

…-heights

lhecker force-pushed the dev/lhecker/3498-line-heights branch from 0c945b6 to 9554470 Compare December 6, 2022 16:00

This comment has been minimized.

Sign in to view

zadjii-msft added the zBugBash-Consider label Dec 6, 2022

Address feedback

504f483

Fix config saving

17f8e47

zadjii-msft assigned carlos-zamora Dec 9, 2022

carlos-zamora approved these changes Dec 9, 2022

View reviewed changes

carlos-zamora removed their assignment Dec 9, 2022

carlos-zamora removed the Needs-Discussion Something that requires a team discussion before we can proceed label Dec 9, 2022

carlos-zamora added a commit that referenced this pull request Dec 14, 2022

PRE-MERGE #14255 Implement cell size customizations

e88578e

carlos-zamora added a commit that referenced this pull request Dec 14, 2022

CARLOS: Fix merge conflict from #14255

48cd105

lhecker mentioned this pull request Dec 16, 2022

Font sizes in WT and conhost #14165

Open

zadjii-msft added this to the Terminal v1.18 milestone Jan 10, 2023

Merge remote-tracking branch 'origin/main' into dev/lhecker/3498-line…

dd34a30

…-heights

zadjii-msft approved these changes Feb 14, 2023

View reviewed changes

lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 14, 2023

lhecker merged commit b6e6dd8 into main Feb 14, 2023

lhecker deleted the dev/lhecker/3498-line-heights branch February 14, 2023 21:42

DHowett reviewed Feb 14, 2023

View reviewed changes

lhecker mentioned this pull request Feb 21, 2023

How to set the linespace? #14882

Closed

lhecker added a commit that referenced this pull request Feb 28, 2023

Address remaining review feedback for #14255

6ee1888

lhecker added a commit that referenced this pull request Feb 28, 2023

Address remaining review feedback for #14255 (#14931)

cf87590

This just removes some leftover code that I forgot to remove before the merge. Additionally I forgot to add a newly added file to our `sources` build file.

lhecker mentioned this pull request Mar 6, 2023

Rewrite AtlasEngine to allow arbitrary overhangs #14959

Merged

carlos-zamora mentioned this pull request Apr 5, 2023

Box-drawing characters in the Fira Mono font are not connected in the latest preview version of Terminal #13064

Closed

lhecker mentioned this pull request May 3, 2023

Frames and special characters from bitmap TTF font are not visible #7927

Closed

tusharsnx mentioned this pull request Jun 15, 2023

Enhance text editing experience on WindowsTerminal #15555

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement cell size customizations #14255

Implement cell size customizations #14255

lhecker commented Oct 19, 2022 •

edited

Loading

lhecker Oct 19, 2022 •

edited

Loading

lhecker commented Oct 20, 2022

carlos-zamora left a comment

This comment has been minimized.

This comment has been minimized.

lhecker commented Dec 6, 2022

DHowett left a comment

DHowett Feb 14, 2023

lhecker Feb 14, 2023

DHowett Feb 14, 2023

lhecker Feb 14, 2023

DHowett Feb 14, 2023

lhecker Feb 14, 2023

DHowett Feb 14, 2023

lhecker Feb 14, 2023

DHowett Feb 14, 2023

lhecker Feb 14, 2023

DHowett Feb 14, 2023

lhecker Feb 14, 2023 •

edited

Loading

DHowett Feb 14, 2023

DHowett Feb 14, 2023

DHowett Feb 14, 2023

lhecker Feb 14, 2023

daymxn commented Feb 21, 2023

Diablo-D3 commented Feb 28, 2023

zadjii-msft commented Feb 28, 2023

Implement cell size customizations #14255

Implement cell size customizations #14255

Conversation

lhecker commented Oct 19, 2022 • edited Loading

Validation Steps Performed

lhecker Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

lhecker commented Oct 20, 2022

carlos-zamora left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

lhecker commented Dec 6, 2022

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daymxn commented Feb 21, 2023

Diablo-D3 commented Feb 28, 2023

zadjii-msft commented Feb 28, 2023

lhecker commented Oct 19, 2022 •

edited

Loading

lhecker Oct 19, 2022 •

edited

Loading

lhecker Feb 14, 2023 •

edited

Loading