Implemented the replace (r and R) actions and replace mode #5
Loading…
x
Reference in New Issue
Block a user
No description provided.
Delete Branch "feature/replace"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
PR Type
Enhancement, Bug fix, Tests, Documentation
Description
Implemented
r(replace character) andR(replace mode) actions.Added comprehensive integration tests for replace functionalities.
Introduced
ReplaceModewith dedicated key bindings and cursor styling.Corrected cursor positioning for end-of-line motions.
Diagram Walkthrough
File Walkthrough
5 files
Introduced actions for single character replacement and replace modeAdded `ReplaceMode` to the editor's mode enumerationImplemented `handleReplaceKey` to process input in replace modeDefined key bindings for `ReplaceMode` actions and motionsAdded distinct cursor styling for `ReplaceMode` and `WaitingMode`2 files
Expanded `sendKeys` helper with navigation and tab keys for testingAdded extensive integration tests for 'r' and 'R' replacefunctionalities3 files
Fixed cursor position expectation for '$' motion in testsAdjusted cursor position expectation for 'v$' in visual mode testsCorrected `MoveToLineEnd` to position cursor on the last character1 files
Removed commented-out list of unused Chroma styles2 files
Updated feature list to mark 'r' and 'R' actions as implementedAdded a section explaining the design choices for replace mode/review
PR Reviewer Guide 🔍
(Review updated until commit
066b817200)Here are some key observations to aid the review process:
Undo Behavior
The
handleReplaceKeyfunction incorrectly callsbuf.UndoStack.BeginBlock(win.Cursor)for every character typed inReplaceModeif an undo block is not already recording. This means each character typed will be a separate undoable action, rather than the entireReplaceModesession being a single undoable block. This deviates from the expected behavior where a singleRcommand (and subsequent typing) should be undoable as one unit. TheBeginBlockshould ideally be called once when enteringReplaceMode(e.g., by theEnterReplaceaction), andEndBlockwhen exitingReplaceMode(e.g., when 'Esc' is pressed).Persistent review updated to latest commit
066b817200/improve
PR Code Suggestions ✨
Latest suggestions up to
066b817Use rune-aware string manipulation
Go strings are UTF-8 encoded, so direct byte-based slicing (
line[:pos],line[pos+1:]) andlen(line)can corrupt multi-byte characters. Convert the line to[]runefor character-accurate manipulation, then convert back tostring. Thisensures correct behavior with Unicode characters across
ReplaceChar,ReplaceModeChar,ReplaceNewline, andReplaceTabactions.internal/action/replace.go [38-44]
Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies a critical issue with byte-based string manipulation in Go when dealing with Unicode characters. Converting to
[]runeensures character-accurate replacement, which is essential for a text editor.Ensure cursor calculations are rune-aware
Cursor column positions and calculations, especially when using
len(string)forcharacter counts, should be based on rune counts, not byte lengths. This ensures
correct visual positioning and movement when multi-byte Unicode characters are
present. This also applies to
ReplaceTab.ExecuteandMoveToLineEnd.Execute.internal/action/replace.go [87]
Suggestion importance[1-10]: 9
__
Why: Using
len(string)for cursor column calculations can lead to incorrect positioning with multi-byte Unicode characters. The suggestion correctly proposes usinglen([]rune(a.Char))for accurate character counting, which is crucial for correct visual feedback.Correct tab key behavior
The
ReplaceTabaction currently replaces a single character with multiple spaces.Given the
TODOinkeymap.goand common editor behavior, a tab in replace mode shouldtypically insert spaces, shifting subsequent characters, rather than overwriting
just one. Adjust the logic to perform an insertion.
internal/action/replace.go [121-127]
Suggestion importance[1-10]: 8
__
Why: This suggestion correctly points out that the
ReplaceTabaction should insert spaces rather than overwrite a single character, aligning with standard editor behavior and theTODOcomment inkeymap.go. Theimproved_codealso correctly uses rune-aware operations.Previous suggestions
Suggestions up to commit
066b817Optimize character replacement and fix cursor position
The current implementation of
ReplaceChar.Executeuses inefficient stringconcatenations and repeated
buf.SetLinecalls within a loop, which can be slow forlarge counts or long lines. Additionally,
win.SetCursorCol(pos - 1)can result in anegative column if no characters were replaced (e.g., on an empty line or when
a.Countis 0). Optimize the string manipulation by converting the line to a[]runeslice, modifying it once, and ensure the cursor is only moved if replacements
occurred, landing on the last replaced character.
internal/action/replace.go [38-46]
Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies performance issues with string concatenations and repeated
buf.SetLinecalls. It also fixes a potential bug wherewin.SetCursorCol(pos - 1)could result in a negative column or incorrect position in edge cases.Fix repeat command behavior in replace mode
The
handleReplaceKeyfunction currently records individual keys intoInsertKeys. Forreplace mode (
R), the.command should repeat the entire sequence of characterstyped during the
Rsession, not just the last character. UsingInsertKeysfor thispurpose will lead to incorrect repeat behavior. A dedicated mechanism is needed to
store and replay the full replace mode input for the
.command.internal/input/handler.go [588-590]
Suggestion importance[1-10]: 8
__
Why: The suggestion accurately points out that using
m.SetInsertKeysfor replace mode will not correctly implement the Vim-like.command, which should repeat the entire sequence of characters typed inRmode. This is a crucial behavioral detail for a Vim-inspired editor.