Batch undo for overwrite mode #97
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds batch undo support for overwrite mode by introducing a new OverwriteAction and refactoring buffer undo/redo management to handle composite edits.
- Merge consecutive delete/insert actions into a single
OverwriteActionin overwrite mode - Extend
Bufferwithundo_stack,redo_stack,undo_limit,push_undo, andundo_or_redo - Add
stringreaders toInsertActionandDeleteActionfor merging support
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/textbringer/modes/overwrite_mode.rb | Implements batch undo hook and defines OverwriteAction |
| lib/textbringer/buffer.rb | Introduces undo/redo stacks, limit, methods, and action readers |
Comments suppressed due to low confidence (3)
lib/textbringer/modes/overwrite_mode.rb:65
- There are no tests covering the new
OverwriteAction.mergebehavior or the overwrite-mode batching. Adding unit tests for consecutive overwrite operations will ensure merging works correctly and prevent regressions.
def merge(inserted_string, deleted_string)
lib/textbringer/buffer.rb:1795
- [nitpick] Using a generic
stringreader inDeleteActioncan be ambiguous. Renaming it todeleted_stringwould clarify its purpose when accessed in merge logic.
attr_reader :string
lib/textbringer/modes/overwrite_mode.rb:66
- [nitpick]
String#concatmutates the original string in place. If these strings are shared or frozen elsewhere, consider using non-mutating concatenation (+) or duplicating the string before merging to avoid side effects.
@inserted_string.concat(inserted_string)
| @@ -14,6 +14,23 @@ class OverwriteMode < MinorMode | |||
| buffer.delete_region(pos, buffer.point) | |||
| end | |||
| end | |||
| if !buffer.undoing? && buffer.undo_limit >= 2 | |||
There was a problem hiding this comment.
The guard checks undo_limit but not the current undo_stack size; if undo_limit >= 2 but the stack has fewer than two entries, accessing undo_stack[-2] returns nil and skips the merge silently. Consider also checking buffer.undo_stack.size >= 2.
Copilot uses AI. Check for mistakes.
| if !buffer.undoing? && buffer.undo_limit >= 2 | ||
| if !buffer.undo_stack[-1].is_a?(DeleteAction) || | ||
| !buffer.undo_stack[-2].is_a?(InsertAction) | ||
| next | ||
| end | ||
| delete_action = buffer.undo_stack.pop | ||
| insert_action = buffer.undo_stack.pop | ||
| action = buffer.undo_stack.last | ||
| if action.is_a?(OverwriteAction) | ||
| action.merge(insert_action.string, delete_action.string) | ||
| buffer.redo_stack.clear | ||
| else | ||
| new_action = OverwriteAction.new(buffer, insert_action.location, | ||
| insert_action.string, delete_action.string) | ||
| buffer.push_undo(new_action) | ||
| end | ||
| end | ||
| } |
There was a problem hiding this comment.
[nitpick] The batch-undo logic in this hook is nested and hard to follow. Extracting it into a named helper method (e.g., handle_overwrite_merge) would improve readability and make it easier to test in isolation.
| if !buffer.undoing? && buffer.undo_limit >= 2 | |
| if !buffer.undo_stack[-1].is_a?(DeleteAction) || | |
| !buffer.undo_stack[-2].is_a?(InsertAction) | |
| next | |
| end | |
| delete_action = buffer.undo_stack.pop | |
| insert_action = buffer.undo_stack.pop | |
| action = buffer.undo_stack.last | |
| if action.is_a?(OverwriteAction) | |
| action.merge(insert_action.string, delete_action.string) | |
| buffer.redo_stack.clear | |
| else | |
| new_action = OverwriteAction.new(buffer, insert_action.location, | |
| insert_action.string, delete_action.string) | |
| buffer.push_undo(new_action) | |
| end | |
| end | |
| } | |
| handle_overwrite_merge(buffer) | |
| } | |
| def handle_overwrite_merge(buffer) | |
| return if buffer.undoing? || buffer.undo_limit < 2 | |
| if !buffer.undo_stack[-1].is_a?(DeleteAction) || | |
| !buffer.undo_stack[-2].is_a?(InsertAction) | |
| return | |
| end | |
| delete_action = buffer.undo_stack.pop | |
| insert_action = buffer.undo_stack.pop | |
| action = buffer.undo_stack.last | |
| if action.is_a?(OverwriteAction) | |
| action.merge(insert_action.string, delete_action.string) | |
| buffer.redo_stack.clear | |
| else | |
| new_action = OverwriteAction.new(buffer, insert_action.location, | |
| insert_action.string, delete_action.string) | |
| buffer.push_undo(new_action) | |
| end | |
| end |
Copilot uses AI. Check for mistakes.
| @@ -24,4 +41,30 @@ def disable | |||
| remove_hook(:post_self_insert_hook, POST_INSERT_HOOK, local: true) | |||
| end | |||
| end | |||
|
|
|||
| class OverwriteAction < UndoableAction | |||
There was a problem hiding this comment.
[nitpick] Consider adding a class-level comment for OverwriteAction explaining its role in batch undo and how its merge method combines multiple actions.
Copilot uses AI. Check for mistakes.
9a82ca2 to
8660a0e
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds batch undo support for overwrite mode by merging consecutive insert and delete actions into a single undoable action.
- Introduce
merge_overwrite_actionrefinement onBufferand invoke it in the overwrite mode hook - Define a new
OverwriteActionclass withundo,redo, andmergemethods - Expose
stringreaders onInsertActionandDeleteActionto enable merging
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/textbringer/modes/overwrite_mode.rb | Add merge_overwrite_action, update hook call, and implement OverwriteAction |
| lib/textbringer/buffer.rb | Add attr_reader :string to InsertAction and DeleteAction |
Comments suppressed due to low confidence (2)
lib/textbringer/modes/overwrite_mode.rb:12
- [nitpick] Consider renaming the local variable
actiontolast_actionorprevious_actionfor clearer intent in the merge logic.
action = @undo_stack.last
lib/textbringer/modes/overwrite_mode.rb:52
- Add unit tests for
OverwriteAction, particularly around itsmergebehavior and integration viamerge_overwrite_action, to ensure batch undo works as intended.
class OverwriteAction < UndoableAction
| @@ -1,4 +1,27 @@ | |||
| module Textbringer | |||
| using Module.new { | |||
| refine Buffer do | |||
| def merge_overwrite_action | |||
There was a problem hiding this comment.
Add a comment or docstring to merge_overwrite_action explaining its behavior and when it should be triggered, to improve maintainability and clarity.
Copilot uses AI. Check for mistakes.
| @@ -1,4 +1,27 @@ | |||
| module Textbringer | |||
| using Module.new { | |||
There was a problem hiding this comment.
[nitpick] Extract the anonymous refinement module into a named module (e.g., BufferOverwriteRefinement) to improve discoverability and readability.
| using Module.new { | |
| module BufferOverwriteRefinement |
Copilot uses AI. Check for mistakes.
No description provided.