A setting doesn't seem feasible

lhecker · lhecker · commit b5295576691b · 2024-03-22T17:54:19.000+01:00
diff --git a/doc/cascadia/profiles.schema.json b/doc/cascadia/profiles.schema.json
@@ -2344,11 +2344,6 @@
           "description": "Force the terminal to use the legacy input encoding. Certain keys in some applications may stop working when enabling this setting.",
           "type": "boolean"
         },
-        "experimental.graphemes": {
-          "default": true,
-          "description": "When set to true, the terminal will use grapheme cluster boundaries for cursor movement. Otherwise, the terminal will use codepoint boundaries.",
-          "type": "boolean"
-        },
         "experimental.useBackgroundImageForWindow": {
           "default": false,
           "description": "When set to true, the background image for the currently focused profile is expanded to encompass the entire window, beneath other panes.",
diff --git a/src/cascadia/TerminalCore/ICoreSettings.idl b/src/cascadia/TerminalCore/ICoreSettings.idl
@@ -20,7 +20,6 @@ namespace Microsoft.Terminal.Core
         String WordDelimiters;
 
         Boolean ForceVTInput;
-        Boolean Graphemes;
         Boolean TrimBlockSelection;
         Boolean DetectURLs;
         Boolean VtPassthrough;
diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp
@@ -14,8 +14,6 @@
 #include <til/hash.h>
 #include <winrt/Microsoft.Terminal.Core.h>
 
-#include "../../types/inc/CodepointWidthDetector.hpp"
-
 using namespace winrt::Microsoft::Terminal::Core;
 using namespace Microsoft::Terminal::Core;
 using namespace Microsoft::Console;
@@ -99,7 +97,6 @@ void Terminal::UpdateSettings(ICoreSettings settings)
     _autoMarkPrompts = settings.AutoMarkPrompts();
 
     _getTerminalInput().ForceDisableWin32InputMode(settings.ForceVTInput());
-    CodepointWidthDetector::Singleton().SetEnableGraphemes(settings.Graphemes());
 
     if (settings.TabColor() == nullptr)
     {
diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.idl b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.idl
@@ -80,7 +80,6 @@ namespace Microsoft.Terminal.Settings.Model
         INHERITABLE_SETTING(Boolean, SoftwareRendering);
         INHERITABLE_SETTING(Boolean, UseBackgroundImageForWindow);
         INHERITABLE_SETTING(Boolean, ForceVTInput);
-        INHERITABLE_SETTING(Boolean, Graphemes);
         INHERITABLE_SETTING(Boolean, DebugFeaturesEnabled);
         INHERITABLE_SETTING(Boolean, StartOnUserLogin);
         INHERITABLE_SETTING(Boolean, AlwaysOnTop);
diff --git a/src/cascadia/TerminalSettingsModel/MTSMSettings.h b/src/cascadia/TerminalSettingsModel/MTSMSettings.h
@@ -28,7 +28,6 @@ Author(s):
     X(bool, SoftwareRendering, "experimental.rendering.software", false)                                                                                                                              \
     X(bool, UseBackgroundImageForWindow, "experimental.useBackgroundImageForWindow", false)                                                                                                           \
     X(bool, ForceVTInput, "experimental.input.forceVT", false)                                                                                                                                        \
-    X(bool, Graphemes, "experimental.graphemes", Feature_Graphemes::IsEnabled())                                                                                                                      \
     X(bool, TrimBlockSelection, "trimBlockSelection", true)                                                                                                                                           \
     X(bool, DetectURLs, "experimental.detectURLs", true)                                                                                                                                              \
     X(bool, AlwaysShowTabs, "alwaysShowTabs", true)                                                                                                                                                   \
diff --git a/src/cascadia/TerminalSettingsModel/TerminalSettings.cpp b/src/cascadia/TerminalSettingsModel/TerminalSettings.cpp
@@ -364,7 +364,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         _SoftwareRendering = globalSettings.SoftwareRendering();
         _UseBackgroundImageForWindow = globalSettings.UseBackgroundImageForWindow();
         _ForceVTInput = globalSettings.ForceVTInput();
-        _Graphemes = globalSettings.Graphemes();
         _TrimBlockSelection = globalSettings.TrimBlockSelection();
         _DetectURLs = globalSettings.DetectURLs();
         _EnableUnfocusedAcrylic = globalSettings.EnableUnfocusedAcrylic();
diff --git a/src/cascadia/TerminalSettingsModel/TerminalSettings.h b/src/cascadia/TerminalSettingsModel/TerminalSettings.h
@@ -159,7 +159,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         INHERITABLE_SETTING(Model::TerminalSettings, bool, SoftwareRendering, false);
         INHERITABLE_SETTING(Model::TerminalSettings, bool, UseBackgroundImageForWindow, false);
         INHERITABLE_SETTING(Model::TerminalSettings, bool, ForceVTInput, false);
-        INHERITABLE_SETTING(Model::TerminalSettings, bool, Graphemes, false);
 
         INHERITABLE_SETTING(Model::TerminalSettings, hstring, PixelShaderPath);
         INHERITABLE_SETTING(Model::TerminalSettings, hstring, PixelShaderImagePath);
diff --git a/src/cascadia/inc/ControlProperties.h b/src/cascadia/inc/ControlProperties.h
@@ -46,7 +46,6 @@
     X(bool, TrimBlockSelection, true)                                                                             \
     X(bool, SuppressApplicationTitle)                                                                             \
     X(bool, ForceVTInput, false)                                                                                  \
-    X(bool, Graphemes, false)                                                                                     \
     X(winrt::hstring, StartingTitle)                                                                              \
     X(bool, DetectURLs, true)                                                                                     \
     X(bool, VtPassthrough, false)                                                                                 \
diff --git a/src/types/CodepointWidthDetector.cpp b/src/types/CodepointWidthDetector.cpp
@@ -4,6 +4,9 @@
 #include "precomp.h"
 #include "inc/CodepointWidthDetector.hpp"
 
+// Due to the Feature_Graphemes::IsEnabled() feature flagging, some code may be disabled.
+#pragma warning(disable : 4702) // unreachable code
+
 // I was trying to minimize dependencies in this code so that it's easier to port to other terminal applications.
 // Also, it has to be fast / have minimal overhead, since it potentially parses every single input character.
 #pragma warning(disable : 26446) // Prefer to use gsl::at() instead of unchecked subscript operator (bounds.4).
@@ -946,25 +949,27 @@ CodepointWidthDetector& CodepointWidthDetector::Singleton() noexcept
     return s_codepointWidthDetector;
 }
 
-CodepointWidthDetector::CodepointWidthDetector() noexcept :
-    _enableGraphemes{ Feature_Graphemes::IsEnabled() }
-{
-}
-
 size_t CodepointWidthDetector::GraphemeNext(const std::wstring_view& str, size_t offset, int* width) noexcept
 {
-    if (!_enableGraphemes.load(std::memory_order_relaxed))
+    if constexpr (!Feature_Graphemes::IsEnabled())
     {
         return _graphemeNextOld(str, offset, width);
     }
 
+    int widthIgnored;
+    if (!width)
+    {
+        width = &widthIgnored;
+    }
+
     const auto beg = str.begin();
     const auto end = str.end();
     auto it = beg + std::min(offset, str.size());
 
     if (it == end)
     {
-        return 0;
+        *width = 1;
+        return offset;
     }
 
     char32_t cp;
@@ -1000,26 +1005,30 @@ size_t CodepointWidthDetector::GraphemeNext(const std::wstring_view& str, size_t
         lead = trail;
     }
 
-    if (width)
-    {
-        *width = totalWidth < 1 ? 1 : (totalWidth > 2 ? 2 : totalWidth);
-    }
+    *width = totalWidth < 1 ? 1 : (totalWidth > 2 ? 2 : totalWidth);
     return it - beg;
 }
 
 // This code is identical to GraphemeNext() but with the order of operations reversed since we're iterating backwards.
 size_t CodepointWidthDetector::GraphemePrev(const std::wstring_view& str, size_t offset, int* width) noexcept
 {
-    if (!_enableGraphemes.load(std::memory_order_relaxed))
+    if constexpr (!Feature_Graphemes::IsEnabled())
     {
         return _graphemePrevOld(str, offset, width);
     }
 
+    int widthIgnored;
+    if (!width)
+    {
+        width = &widthIgnored;
+    }
+
     const auto beg = str.begin();
     auto it = beg + std::min(offset, str.size());
 
     if (it == beg)
     {
+        *width = 1;
         return 0;
     }
 
@@ -1056,10 +1065,7 @@ size_t CodepointWidthDetector::GraphemePrev(const std::wstring_view& str, size_t
         trail = lead;
     }
 
-    if (width)
-    {
-        *width = totalWidth < 1 ? 1 : (totalWidth > 2 ? 2 : totalWidth);
-    }
+    *width = totalWidth < 1 ? 1 : (totalWidth > 2 ? 2 : totalWidth);
     return it - beg;
 }
 
@@ -1164,11 +1170,6 @@ catch (...)
     return 1;
 }
 
-void CodepointWidthDetector::SetEnableGraphemes(const bool enable) noexcept
-{
-    _enableGraphemes.store(enable, std::memory_order_relaxed);
-}
-
 // Method Description:
 // - Sets a function that should be used as the fallback mechanism for
 //      determining a particular glyph's width, should the glyph be an ambiguous
diff --git a/src/types/inc/CodepointWidthDetector.hpp b/src/types/inc/CodepointWidthDetector.hpp
@@ -7,23 +7,19 @@ struct CodepointWidthDetector
 {
     static CodepointWidthDetector& Singleton() noexcept;
 
-    CodepointWidthDetector() noexcept;
-
     size_t GraphemeNext(const std::wstring_view& str, size_t offset, int* width) noexcept;
     size_t GraphemePrev(const std::wstring_view& str, size_t offset, int* width) noexcept;
 
-    void SetEnableGraphemes(bool enable) noexcept;
     void SetFallbackMethod(std::function<bool(const std::wstring_view&)> pfnFallback) noexcept;
     void ClearFallbackCache() noexcept;
 
 private:
     __declspec(noinline) int _checkFallbackViaCache(char32_t codepoint) noexcept;
 
-    __declspec(noinline) size_t _graphemeNextOld(const std::wstring_view& str, size_t offset, int* width) noexcept;
-    __declspec(noinline) size_t _graphemePrevOld(const std::wstring_view& str, size_t offset, int* width) noexcept;
+    size_t _graphemeNextOld(const std::wstring_view& str, size_t offset, int* width) noexcept;
+    size_t _graphemePrevOld(const std::wstring_view& str, size_t offset, int* width) noexcept;
     int _getWidthOld(char32_t cp) noexcept;
 
     std::unordered_map<char32_t, uint8_t> _fallbackCache;
     std::function<bool(const std::wstring_view&)> _pfnFallbackMethod;
-    std::atomic<bool> _enableGraphemes;
 };
diff --git a/src/types/ut_types/CodepointWidthDetectorTests.cpp b/src/types/ut_types/CodepointWidthDetectorTests.cpp
@@ -6,6 +6,9 @@
 
 #include "../types/inc/CodepointWidthDetector.hpp"
 
+// Due to the Feature_Graphemes::IsEnabled() feature flagging, some code may be disabled.
+#pragma warning(disable : 4702) // unreachable code
+
 // FYI at the time of writing you may have to generate this table in cmd with
 //   go run CodepointWidthDetectorTests_gen.go > temp.txt
 // because PowerShell garbles Unicode text between piped commands.
@@ -1228,6 +1231,11 @@ class CodepointWidthDetectorTests
 
     TEST_METHOD(GraphemeBreakTest)
     {
+        if constexpr (!Feature_Graphemes::IsEnabled())
+        {
+            return;
+        }
+
         WEX::TestExecution::DisableVerifyExceptions disableVerifyExceptions{};
         WEX::TestExecution::SetVerifyOutput verifyOutputScope{ WEX::TestExecution::VerifyOutputSettings::LogOnlyFailures };
 
@@ -1277,6 +1285,11 @@ class CodepointWidthDetectorTests
 
     TEST_METHOD(BasicGraphemes)
     {
+        if constexpr (!Feature_Graphemes::IsEnabled())
+        {
+            return;
+        }
+
         static constexpr std::wstring_view text{ L"a\u0363e\u0364\u0364i\u0365" };
 
         auto& cwd = CodepointWidthDetector::Singleton();
@@ -1319,6 +1332,11 @@ class CodepointWidthDetectorTests
 
     TEST_METHOD(DevanagariConjunctLinker)
     {
+        if constexpr (!Feature_Graphemes::IsEnabled())
+        {
+            return;
+        }
+
         static constexpr std::wstring_view text{ L"\u0915\u094D\u094D\u0924" };
 
         auto& cwd = CodepointWidthDetector::Singleton();
