feat(editor): Implement dd, yy, and p Vim operations #1
Conversation
This change implements core Vim-style line operations for deleting (`dd`), yanking (`yy`), and pasting (`p`). - The `delete_line` (`dd`) functionality was improved to more intuitively handle deleting the last line of a file by removing the preceding newline. - The `yank_line` (`yy`) functionality was implemented to copy the current line, including its newline character, to the clipboard. - The `paste` (`p`) function was significantly improved to correctly handle line-wise pasting, inserting the clipboard content below the current line, which is consistent with Vim's behavior. - Added comprehensive unit tests for all new and modified operations at both the editor and application layers. These tests uncovered and led to the correction of several pre-existing and newly introduced bugs, improving overall robustness.
There was a problem hiding this comment.
Pull Request Overview
This PR implements core Vim-style line operations including dd (delete line), yy (yank line), and p (paste). The changes enhance the editor's Vim compatibility by providing proper line-wise operations with appropriate cursor positioning and clipboard management.
Key changes:
- Enhanced
delete_lineto handle edge cases like deleting the last line or only line - Added
yank_linefunctionality to copy lines with proper newline handling - Improved
pastefunction to support both line-wise and character-wise pasting modes - Added comprehensive test coverage for all new operations
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| crates/scriptoris/src/editor.rs | Core editor functionality with enhanced delete_line, new yank_line, improved paste, and extensive test coverage |
| crates/scriptoris/src/app.rs | Application layer integration for yy command handling and corresponding tests |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Character-wise paste. Insert *after* the cursor. | ||
| let mut char_idx = self.line_col_to_char_idx(self.cursor_line, self.cursor_col); | ||
| if self.rope.len_chars() > 0 && self.cursor_col < self.rope.line(self.cursor_line).len_chars() { | ||
| char_idx += 1; |
There was a problem hiding this comment.
There's an extra space at the beginning of this line. It should be indented consistently with the surrounding code.
| self.cursor_line = target_line.min(self.rope.len_lines().saturating_sub(1)); | ||
| if self.cursor_line < self.rope.len_lines() { | ||
| let pasted_line = self.rope.line(self.cursor_line); | ||
| self.cursor_col = pasted_line.to_string().chars().take_while(|c| c.is_whitespace()).count(); |
There was a problem hiding this comment.
Converting the entire line to a string just to count whitespace characters is inefficient. Consider iterating over the rope slice directly: pasted_line.chars().take_while(|c| c.is_whitespace()).count()
| self.cursor_col = pasted_line.to_string().chars().take_while(|c| c.is_whitespace()).count(); | |
| self.cursor_col = pasted_line.chars().take_while(|c| c.is_whitespace()).count(); |
| // This is the last line. If there's a preceding newline, remove it too. | ||
| if start_idx > 0 { | ||
| if self.rope.char(start_idx - 1) == '\n' { | ||
| start_idx -= 1; |
There was a problem hiding this comment.
This check could panic if start_idx is 0 and we try to access start_idx - 1. The outer if start_idx > 0 check protects against this, but the logic is fragile. Consider restructuring for clarity.
| start_idx -= 1; | |
| let prev_idx = start_idx.saturating_sub(1); | |
| if self.rope.char(prev_idx) == '\n' { | |
| start_idx = prev_idx; |
| // Ensure line-wise yanks always end in a newline for consistent pasting behavior. | ||
| if !self.clipboard.ends_with('\n') { | ||
| self.clipboard.push('\n'); | ||
| } |
There was a problem hiding this comment.
The get_slice method might return None for valid ranges in some rope implementations. Consider using slice(start_idx..end_idx) directly or handle the None case appropriately if this is expected behavior.
| } | |
| let slice = self.rope.slice(start_idx..end_idx); | |
| self.clipboard = slice.to_string(); | |
| // Ensure line-wise yanks always end in a newline for consistent pasting behavior. | |
| if !self.clipboard.ends_with('\n') { | |
| self.clipboard.push('\n'); |
This change implements core Vim-style line operations for deleting (
dd), yanking (yy), and pasting (p).delete_line(dd) functionality was improved to more intuitively handle deleting the last line of a file by removing the preceding newline.yank_line(yy) functionality was implemented to copy the current line, including its newline character, to the clipboard.paste(p) function was significantly improved to correctly handle line-wise pasting, inserting the clipboard content below the current line, which is consistent with Vim's behavior.Description
Type of Change
Related Issues
Fixes #(issue number)
Closes #(issue number)
Related to #(issue number)
Changes Made
Testing
Test Environment
Test Cases
Manual Testing Checklist
Screenshots/Demos
Breaking Changes
Checklist
Additional Notes