From 8bdd2076293d5656ac3fbab4f909458333dfc7d2 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Mon, 23 Oct 2023 14:55:31 -0700 Subject: [PATCH 1/2] Fix a few VI key handlers to close edit group properly --- PSReadLine/Completion.cs | 2 +- PSReadLine/ReadLine.vi.cs | 8 +++- PSReadLine/Replace.vi.cs | 28 +++++++++---- test/BasicEditingTest.VI.cs | 82 +++++++++++++++++++++++++++++++++++++ 4 files changed, 111 insertions(+), 9 deletions(-) diff --git a/PSReadLine/Completion.cs b/PSReadLine/Completion.cs index f35c7cd52..6b3bb89b8 100644 --- a/PSReadLine/Completion.cs +++ b/PSReadLine/Completion.cs @@ -205,7 +205,7 @@ private void CompleteImpl(bool menuSelect) if (InViInsertMode()) // must close out the current edit group before engaging menu completion { ViCommandMode(); - ViInsertWithAppend(); + ViInsertWithAppendImpl(); } // Do not show suggestion text during tab completion. diff --git a/PSReadLine/ReadLine.vi.cs b/PSReadLine/ReadLine.vi.cs index c39932c75..571f06f5c 100644 --- a/PSReadLine/ReadLine.vi.cs +++ b/PSReadLine/ReadLine.vi.cs @@ -572,6 +572,12 @@ public static void ViInsertAtEnd(ConsoleKeyInfo? key = null, object arg = null) /// Append from the current line position. /// public static void ViInsertWithAppend(ConsoleKeyInfo? key = null, object arg = null) + { + _singleton._groupUndoHelper.StartGroup(ViInsertWithAppend, arg); + ViInsertWithAppendImpl(key, arg); + } + + private static void ViInsertWithAppendImpl(ConsoleKeyInfo? key = null, object arg = null) { ViInsertMode(key, arg); ForwardChar(key, arg); @@ -1304,7 +1310,7 @@ public static void ViAppendLine(ConsoleKeyInfo? key = null, object arg = null) } _singleton.SaveEditItem(EditItemInsertChar.Create('\n', insertPoint)); _singleton.Render(); - ViInsertWithAppend(); + ViInsertWithAppendImpl(); } private void MoveToEndOfPhrase() diff --git a/PSReadLine/Replace.vi.cs b/PSReadLine/Replace.vi.cs index efd23e469..8617f79c0 100644 --- a/PSReadLine/Replace.vi.cs +++ b/PSReadLine/Replace.vi.cs @@ -161,7 +161,7 @@ private static void ViReplaceWord(ConsoleKeyInfo? key, object arg) && !_singleton.IsDelimiter(_singleton._lastWordDelimiter, _singleton.Options.WordDelimiters) && _singleton._shouldAppend) { - ViInsertWithAppend(key, arg); + ViInsertWithAppendImpl(key, arg); } else { @@ -180,7 +180,7 @@ private static void ViReplaceGlob(ConsoleKeyInfo? key, object arg) } if (_singleton._current == _singleton._buffer.Length - 1) { - ViInsertWithAppend(key, arg); + ViInsertWithAppendImpl(key, arg); } else { @@ -194,7 +194,7 @@ private static void ViReplaceEndOfWord(ConsoleKeyInfo? key, object arg) DeleteEndOfWord(key, arg); if (_singleton._current == _singleton._buffer.Length - 1) { - ViInsertWithAppend(key, arg); + ViInsertWithAppendImpl(key, arg); } else { @@ -208,7 +208,7 @@ private static void ViReplaceEndOfGlob(ConsoleKeyInfo? key, object arg) ViDeleteEndOfGlob(key, arg); if (_singleton._current == _singleton._buffer.Length - 1) { - ViInsertWithAppend(key, arg); + ViInsertWithAppendImpl(key, arg); } else { @@ -270,13 +270,17 @@ private static void ViReplaceToChar(char keyChar, ConsoleKeyInfo? key = null, ob { if (_singleton._current < initialCurrent || _singleton._current >= _singleton._buffer.Length) { - ViInsertWithAppend(key, arg); + ViInsertWithAppendImpl(key, arg); } else { ViInsertMode(key, arg); } } + else + { + _singleton._groupUndoHelper.EndGroup(); + } } /// @@ -295,6 +299,10 @@ private static void ViReplaceToCharBack(char keyChar, ConsoleKeyInfo? key = null { ViInsertMode(key, arg); } + else + { + _singleton._groupUndoHelper.EndGroup(); + } } /// @@ -314,6 +322,10 @@ private static void ViReplaceToBeforeChar(char keyChar, ConsoleKeyInfo? key = nu { ViInsertMode(key, arg); } + else + { + _singleton._groupUndoHelper.EndGroup(); + } } /// @@ -332,8 +344,10 @@ private static void ViReplaceToBeforeCharBack(char keyChar, ConsoleKeyInfo? key { ViInsertMode(key, arg); } + else + { + _singleton._groupUndoHelper.EndGroup(); + } } - - } } diff --git a/test/BasicEditingTest.VI.cs b/test/BasicEditingTest.VI.cs index a41b6b078..53dc8a748 100644 --- a/test/BasicEditingTest.VI.cs +++ b/test/BasicEditingTest.VI.cs @@ -1121,5 +1121,87 @@ public void ViInsertModeMoveCursor() _.RightArrow, // 'RightArrow' again does nothing, but doesn't crash "c")); } + + [SkippableFact] + public void ViDefect1281_1() + { + TestSetup(KeyMode.Vi); + + Test("bcd", Keys( + "abcdabcd", _.Escape, + + // return to the [B]eginning of the word, + // then [c]hange text un[t]il just before the [2]nd [b] character + // this leaves the cursor at the current position (0) but erases + // the "abcda" text portion, / switches to edit mode and + // positions the cursor just before the "bcd" text portion. + + "Bc2tb", + + // going back to normal mode again without having modified the buffer further + // even though the [c] command started an edit group, going back to normal + // mode closes the pending edit group. + + _.Escape, CheckThat(() => AssertCursorLeftIs(0)), + + // attempt to [c]hange text un[t]il just before the [2]nd [b] character again + // because the [b] character only appears once further down in the buffer + // relative to where the cursor position is – currently set to 0 - the command + // fails. Therefore, we are still in normal mode. + // + // however, even though the command failed, it still started an edit group + // which is now pending further edit actions + + "c2tb", CheckThat(() => AssertLineIs("bcd")), + + // attempt to [c]hange text un[t]il just before the [2]nd [b] character a third time. + // this exercises a code path where starting a edit group while another + // pending edit group was previously started crashed PSRL. + + "c2tb" // should not crash + )); + } + + [SkippableFact] + public void ViDefect1281_2() + { + TestSetup(KeyMode.Vi); + + Test("abc", Keys( + "abc", _.Escape, + + // 'cff' triggers `ViReplaceToChar` to delete until the character 'f'. But 'abc' doesn't + // have the letter 'f', so the started edit group should be ended and cursor is not moved. + "cff", + CheckThat(() => AssertCursorLeftIs(2)), + + // the subsequent 'cc' calls `ViReplaceLine` to replace the current line with 'i', which + // starts a new edit group. + "ccip", + CheckThat(() => AssertLineIs("ip")), + + // now we undo the 'cci' step, and accept the current command line, which should be 'abc'. + _.Escape, "u" + )); + } + + [SkippableFact] + public void ViDefect1281_3() + { + TestSetup(KeyMode.Vi); + + Test("bcd", Keys( + "bcd", _.Escape, _.LeftArrow, _.LeftArrow, + + // 'a' triggers `ViInsertWithAppend` and we append 'iii' after 'b'. + "aiii", + CheckThat(() => AssertCursorLeftIs(4)), + CheckThat(() => AssertLineIs("biiicd")), + + // now we undo the 'aiii' step, and accept the current command line, + // which should be 'bcd'. + _.Escape, "u" + )); + } } } From 025b44609d5ad05726c065301fb22b3d91664339 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Tue, 24 Oct 2023 09:41:09 -0700 Subject: [PATCH 2/2] Update comments in the test as suggested --- test/BasicEditingTest.VI.cs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/test/BasicEditingTest.VI.cs b/test/BasicEditingTest.VI.cs index 53dc8a748..1ab2bca51 100644 --- a/test/BasicEditingTest.VI.cs +++ b/test/BasicEditingTest.VI.cs @@ -1149,16 +1149,17 @@ public void ViDefect1281_1() // relative to where the cursor position is – currently set to 0 - the command // fails. Therefore, we are still in normal mode. // - // however, even though the command failed, it still started an edit group - // which is now pending further edit actions + // as the command failed, the current edit group is now correctly closed. "c2tb", CheckThat(() => AssertLineIs("bcd")), // attempt to [c]hange text un[t]il just before the [2]nd [b] character a third time. // this exercises a code path where starting a edit group while another // pending edit group was previously started crashed PSRL. - - "c2tb" // should not crash + // + // this should no longer crash as any started pending group is now properly closed if + // the command fails + "c2tb" )); }