Addressed feedback, Fixed font fallback replacement

src/cascadia/TerminalControl/ControlCore.cpp

@@ -1304,7 +1304,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         }
     }
 
-    void ControlCore::SetBackgroundOpacity(double opacity)
+    void ControlCore::SetBackgroundOpacity(const double opacity)
     {
         if (_renderEngine)
         {

src/inc/til/rle.h

@@ -400,12 +400,6 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned"
             return _runs;
         }
 
-        void clear()
-        {
-            _runs.clear();
-            _total_length = 0;
-        }
-
         // Get the value at the position
         const_reference at(size_type position) const
         {

src/renderer/atlas/AtlasEngine.api.cpp

@@ -385,7 +385,8 @@ void AtlasEngine::SetWarningCallback(std::function<void(HRESULT)> pfn) noexcept
     RETURN_IF_FAILED(vec2_narrow(pixels.cx, pixels.cy, newSize));
 
     // At the time of writing:
-    // When Win+D is pressed a render pass is initiated. As conhost is in the background, GetClientRect will return {0,0}.
+    // When Win+D is pressed, `TriggerRedrawCursor` is called and a render pass is initiated.
+    // As conhost is in the background, GetClientRect will return {0,0} and we'll get called with {0,0}.
     // This isn't a valid value for _api.sizeInPixel and would crash _recreateSizeDependentResources().
     if (_api.sizeInPixel != newSize && newSize != u16x2{})
     {
@@ -541,6 +542,11 @@ void AtlasEngine::_resolveFontMetrics(const FontInfoDesired& fontInfoDesired, Fo
     DWRITE_FONT_METRICS metrics;
     fontFace->GetMetrics(&metrics);
 
+    // According to Wikipedia:
+    // > One em was traditionally defined as the width of the capital 'M' in the current typeface and point size,
+    // > because the 'M' was commonly cast the full-width of the square blocks [...] which are used in printing presses.
+    // Even today M is often the widest character in a font that supports ASCII.
+    // In the future a more robust solution could be written, until then this simple solution works for most cases.
     static constexpr u32 codePoint = L'M';
     u16 glyphIndex;
     THROW_IF_FAILED(fontFace->GetGlyphIndicesW(&codePoint, 1, &glyphIndex));

src/renderer/atlas/AtlasEngine.cpp

@@ -48,15 +48,35 @@ struct TextAnalyzer final : IDWriteTextAnalysisSource, IDWriteTextAnalysisSink
         Ensures(_text.size() <= UINT32_MAX);
     }
 
-#ifndef NDEBUG
-private:
+    // TextAnalyzer will be allocated on the stack and reference counting is pointless because of that.
+    // The debug version will assert that we don't leak any references though.
+#ifdef NDEBUG
+    ULONG __stdcall AddRef() noexcept override
+    {
+        return 1;
+    }
+
+    ULONG __stdcall Release() noexcept override
+    {
+        return 1;
+    }
+#else
     ULONG _refCount = 1;
 
-public:
     ~TextAnalyzer()
     {
         assert(_refCount == 1);
     }
+
+    ULONG __stdcall AddRef() noexcept override
+    {
+        return ++_refCount;
+    }
+
+    ULONG __stdcall Release() noexcept override
+    {
+        return --_refCount;
+    }
 #endif
 
     HRESULT __stdcall QueryInterface(const IID& riid, void** ppvObject) noexcept override
@@ -73,24 +93,6 @@ struct TextAnalyzer final : IDWriteTextAnalysisSource, IDWriteTextAnalysisSink
         return E_NOINTERFACE;
     }
 
-    ULONG __stdcall AddRef() noexcept override
-    {
-#ifdef NDEBUG
-        return 1;
-#else
-        return ++_refCount;
-#endif
-    }
-
-    ULONG __stdcall Release() noexcept override
-    {
-#ifdef NDEBUG
-        return 1;
-#else
-        return --_refCount;
-#endif
-    }
-
     HRESULT __stdcall GetTextAtPosition(UINT32 textPosition, const WCHAR** textString, UINT32* textLength) noexcept override
     {
         // Writing to address 0 is a crash in practice. Just what we want.
@@ -1019,10 +1021,6 @@ void AtlasEngine::_recreateFontDependentResources()
                 // > Lines are explicitly set to uniform spacing, regardless of contained font sizes.
                 // > This can be useful to avoid the uneven appearance that can occur from font fallback.
                 // We want that. Otherwise fallback fonts might be rendered with an incorrect baseline and get cut off vertically.
-                //
-                // "baseline" parameter:
-                // > Distance from top of line to baseline. A reasonable ratio to lineSpacing is 80%.
-                // So... let's set it to 80%.
                 THROW_IF_FAILED(textFormat->SetLineSpacing(DWRITE_LINE_SPACING_METHOD_UNIFORM, _r.cellSizeDIP.y, _api.fontMetrics.baselineInDIP));
 
                 if (!_api.fontAxisValues.empty())
@@ -1235,13 +1233,21 @@ void AtlasEngine::_flushBufferLine()
 
             if (!mappedFontFace)
             {
+                // Task: Replace all characters in this range with unicode replacement characters.
+                // Input (where "n" is a narrow and "ww" is a wide character):
+                //    _api.bufferLine       = "nwwnnw"
+                //    _api.bufferLineColumn = {0, 1, 1, 2, 3, 4, 4, 5}
+                //                             n  w  w  n  n  w  w
+                // Solution:
+                //   Iterate through bufferLineColumn until the value changes, because this indicates we passed over a
+                //   complete (narrow or wide) cell. To do so we'll use col1 (previous column) and col2 (next column).
+                //   Then we emit a replacement character by telling _emplaceGlyph that this range has no font face.
                 auto pos1 = idx;
                 auto col1 = _api.bufferLineColumn[pos1];
                 for (auto pos2 = idx + 1; pos2 <= mappedEnd; ++pos2)
                 {
                     if (const auto col2 = _api.bufferLineColumn[pos2]; col1 != col2)
                     {
-                        static constexpr auto replacement = L'\uFFFD';
                         _emplaceGlyph(nullptr, scale, pos1, pos2);
                         pos1 = pos2;
                         col1 = col2;
@@ -1376,11 +1382,14 @@ void AtlasEngine::_flushBufferLine()
 
 void AtlasEngine::_emplaceGlyph(IDWriteFontFace* fontFace, float scale, size_t bufferPos1, size_t bufferPos2)
 {
+    static constexpr auto replacement = L'\uFFFD';
+
     // This would seriously blow us up otherwise.
     Expects(bufferPos1 < bufferPos2 && bufferPos2 <= _api.bufferLine.size());
 
-    const auto chars = &_api.bufferLine[bufferPos1];
-    const auto charCount = bufferPos2 - bufferPos1;
+    const auto chars = fontFace ? &_api.bufferLine[bufferPos1] : &replacement;
+    const auto charCount = fontFace ? bufferPos2 - bufferPos1 : 1;
+
     // _flushBufferLine() ensures that bufferLineColumn.size() > bufferLine.size().
     const auto x1 = _api.bufferLineColumn[bufferPos1];
     const auto x2 = _api.bufferLineColumn[bufferPos2];

src/renderer/atlas/AtlasEngine.h

@@ -244,30 +244,32 @@ namespace Microsoft::Console::Render
             }
 
         private:
+            // These two functions don't need to use scoped objects or standard allocators,
+            // since this class is in fact an scoped allocator object itself.
 #pragma warning(push)
 #pragma warning(disable : 26402) // Return a scoped object instead of a heap-allocated if it has a move constructor (r.3).
 #pragma warning(disable : 26409) // Avoid calling new and delete explicitly, use std::make_unique<T> instead (r.11).
             static T* allocate(size_t size)
             {
                 if constexpr (Alignment <= __STDCPP_DEFAULT_NEW_ALIGNMENT__)
                 {
-                    return static_cast<T*>(::operator new[](size * sizeof(T)));
+                    return static_cast<T*>(::operator new(size * sizeof(T)));
                 }
                 else
                 {
-                    return static_cast<T*>(::operator new[](size * sizeof(T), static_cast<std::align_val_t>(Alignment)));
+                    return static_cast<T*>(::operator new(size * sizeof(T), static_cast<std::align_val_t>(Alignment)));
                 }
             }
 
             static void deallocate(T* data) noexcept
             {
                 if constexpr (Alignment <= __STDCPP_DEFAULT_NEW_ALIGNMENT__)
                 {
-                    ::operator delete[](data);
+                    ::operator delete(data);
                 }
                 else
                 {
-                    ::operator delete[](data, static_cast<std::align_val_t>(Alignment));
+                    ::operator delete(data, static_cast<std::align_val_t>(Alignment));
                 }
             }
 #pragma warning(pop)

src/renderer/atlas/shader_ps.hlsl

@@ -1,3 +1,6 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
 #define INVALID_COLOR 0xffffffff
 
 // These flags are shared with AtlasEngine::CellFlags.

src/renderer/atlas/shader_vs.hlsl

@@ -1,4 +1,9 @@
-float4 main(uint id : SV_VERTEXID) : SV_POSITION
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+// clang-format off
+float4 main(uint id: SV_VERTEXID): SV_POSITION
+// clang-format on
 {
     // The algorithm below is a fast way to generate a full screen triangle,
     // published by Bill Bilodeau "Vertex Shader Tricks" at GDC14.