From bf8a647788f2ca9632bb6c9ab508419e79566754 Mon Sep 17 00:00:00 2001
From: Mike Griese <migrie@microsoft.com>
Date: Mon, 13 May 2024 12:36:27 -0500
Subject: [PATCH] Clean up command history context passed to suggestions UI
 (#17245)

This is fallout from #16937.

* Typing a command then backspacing the chars then asking for
suggestions would think the current commandline ended with spaces,
making filtering very hard.
* The currently typed command would _also_ appear in the command
history, which isn't useful.

I actually did TDD for this and wrote the test first, then confirmed
again running through the build script, I wasn't hitting any of the
earlier issues.

Closes #17241
Closes #17243
---
 src/cascadia/TerminalControl/ControlCore.cpp  | 33 +++++++++---
 .../UnitTests_Control/ControlCoreTests.cpp    | 51 +++++++++++++++++++
 2 files changed, 77 insertions(+), 7 deletions(-)

diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp
index 575864ede1a..c4ad4b0799d 100644
--- a/src/cascadia/TerminalControl/ControlCore.cpp
+++ b/src/cascadia/TerminalControl/ControlCore.cpp
@@ -2235,20 +2235,39 @@ namespace winrt::Microsoft::Terminal::Control::implementation
 
         std::vector<winrt::hstring> commands;
         const auto bufferCommands{ textBuffer.Commands() };
-        for (const auto& commandInBuffer : bufferCommands)
-        {
-            const auto strEnd = commandInBuffer.find_last_not_of(UNICODE_SPACE);
+
+        auto trimToHstring = [](const auto& s) -> winrt::hstring {
+            const auto strEnd = s.find_last_not_of(UNICODE_SPACE);
             if (strEnd != std::string::npos)
             {
-                const auto trimmed = commandInBuffer.substr(0, strEnd + 1);
+                const auto trimmed = s.substr(0, strEnd + 1);
+                return winrt::hstring{ trimmed };
+            }
+            return winrt::hstring{ L"" };
+        };
+
+        const auto currentCommand = _terminal->CurrentCommand();
+        const auto trimmedCurrentCommand = trimToHstring(currentCommand);
 
-                commands.push_back(winrt::hstring{ trimmed });
+        for (const auto& commandInBuffer : bufferCommands)
+        {
+            if (const auto hstr{ trimToHstring(commandInBuffer) };
+                (!hstr.empty() && hstr != trimmedCurrentCommand))
+            {
+                commands.push_back(hstr);
             }
         }
 
-        auto context = winrt::make_self<CommandHistoryContext>(std::move(commands));
-        context->CurrentCommandline(winrt::hstring{ _terminal->CurrentCommand() });
+        // If the very last thing in the list of recent commands, is exactly the
+        // same as the current command, then let's not include it in the
+        // history. It's literally the thing the user has typed, RIGHT now.
+        if (!commands.empty() && commands.back() == trimmedCurrentCommand)
+        {
+            commands.pop_back();
+        }
 
+        auto context = winrt::make_self<CommandHistoryContext>(std::move(commands));
+        context->CurrentCommandline(trimmedCurrentCommand);
         return *context;
     }
 
diff --git a/src/cascadia/UnitTests_Control/ControlCoreTests.cpp b/src/cascadia/UnitTests_Control/ControlCoreTests.cpp
index 63f20f9058e..c9f2dbb2ae6 100644
--- a/src/cascadia/UnitTests_Control/ControlCoreTests.cpp
+++ b/src/cascadia/UnitTests_Control/ControlCoreTests.cpp
@@ -40,6 +40,7 @@ namespace ControlUnitTests
 
         TEST_METHOD(TestSelectCommandSimple);
         TEST_METHOD(TestSelectOutputSimple);
+        TEST_METHOD(TestCommandContext);
         TEST_METHOD(TestSelectOutputScrolling);
         TEST_METHOD(TestSelectOutputExactWrap);
 
@@ -509,6 +510,56 @@ namespace ControlUnitTests
             VERIFY_ARE_EQUAL(expectedEnd, end);
         }
     }
+    void ControlCoreTests::TestCommandContext()
+    {
+        auto [settings, conn] = _createSettingsAndConnection();
+        Log::Comment(L"Create ControlCore object");
+        auto core = createCore(*settings, *conn);
+        VERIFY_IS_NOT_NULL(core);
+        _standardInit(core);
+
+        Log::Comment(L"Print some text");
+
+        _writePrompt(conn, L"C:\\Windows");
+        conn->WriteInput(L"Foo-bar");
+        conn->WriteInput(L"\x1b]133;C\x7");
+
+        conn->WriteInput(L"\r\n");
+        conn->WriteInput(L"This is some text     \r\n");
+        conn->WriteInput(L"with varying amounts  \r\n");
+        conn->WriteInput(L"of whitespace         \r\n");
+
+        _writePrompt(conn, L"C:\\Windows");
+
+        Log::Comment(L"Check the command context");
+
+        const WEX::TestExecution::DisableVerifyExceptions disableExceptionsScope;
+        {
+            auto historyContext{ core->CommandHistory() };
+            VERIFY_ARE_EQUAL(1u, historyContext.History().Size());
+            VERIFY_ARE_EQUAL(L"", historyContext.CurrentCommandline());
+        }
+
+        Log::Comment(L"Write 'Bar' to the command...");
+        conn->WriteInput(L"Bar");
+        {
+            auto historyContext{ core->CommandHistory() };
+            // Bar shouldn't be in the history, it should be the current command
+            VERIFY_ARE_EQUAL(1u, historyContext.History().Size());
+            VERIFY_ARE_EQUAL(L"Bar", historyContext.CurrentCommandline());
+        }
+
+        Log::Comment(L"then delete it");
+        conn->WriteInput(L"\b \b");
+        conn->WriteInput(L"\b \b");
+        conn->WriteInput(L"\b \b");
+        {
+            auto historyContext{ core->CommandHistory() };
+            VERIFY_ARE_EQUAL(1u, historyContext.History().Size());
+            // The current commandline is now empty
+            VERIFY_ARE_EQUAL(L"", historyContext.CurrentCommandline());
+        }
+    }
 
     void ControlCoreTests::TestSelectOutputScrolling()
     {