Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
279 changes: 270 additions & 9 deletions src/core_editor/editor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use super::{edit_stack::EditStack, Clipboard, ClipboardMode, LineBuffer};
#[cfg(feature = "system_clipboard")]
use crate::core_editor::get_system_clipboard;
use crate::enums::{EditType, UndoBehavior};
use crate::{core_editor::get_local_clipboard, EditCommand};
use crate::{core_editor::get_local_clipboard, EditCommand, PromptEditMode, PromptViMode};
use std::ops::DerefMut;

/// Stateful editor executing changes to the underlying [`LineBuffer`]
Expand Down Expand Up @@ -626,20 +626,35 @@ impl Editor {
/// If a selection is active returns the selected range, otherwise None.
/// The range is guaranteed to be ascending.
pub fn get_selection(&self) -> Option<(usize, usize)> {
// Default to Vi-style behavior for backward compatibility
self.get_selection_with_mode(&PromptEditMode::Vi(PromptViMode::Normal))
Comment on lines +629 to +630
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an explicit need to be backwards compatible here?

Maybe the correct thing would be to split it into true selection (which covers the characters in a consistent way with a range using indices used by the edit ops), and visual selection which is mode aware.

It seems the bug prone part was the visual selection, if there are no other users in reedline itself we should consider what the right behavior would be for a public user of get_selection (and if that is unlikely we can make things private.

}

/// Get selection with awareness of the current edit mode
pub fn get_selection_with_mode(&self, edit_mode: &PromptEditMode) -> Option<(usize, usize)> {
self.selection_anchor.map(|selection_anchor| {
let buffer_len = self.line_buffer.len();

// In Vi Normal/Visual mode, selection includes the character under the cursor
// In Emacs mode and Vi Insert mode, selection is between cursor positions
let extend_selection = matches!(edit_mode, PromptEditMode::Vi(PromptViMode::Normal));

if self.insertion_point() > selection_anchor {
(
selection_anchor,
self.line_buffer.grapheme_right_index().min(buffer_len),
)
let end = if extend_selection {
self.line_buffer.grapheme_right_index().min(buffer_len)
} else {
self.insertion_point()
};
(selection_anchor, end)
} else {
(
self.insertion_point(),
let end = if extend_selection {
self.line_buffer
.grapheme_right_index_from_pos(selection_anchor)
.min(buffer_len),
)
.min(buffer_len)
} else {
selection_anchor
};
(self.insertion_point(), end)
}
})
}
Expand Down Expand Up @@ -1363,4 +1378,250 @@ mod test {
assert_eq!(editor.insertion_point(), 3); // Cursor should return to original position
assert_eq!(editor.cut_buffer.get().0, "\r\n");
}

#[test]
fn test_selection_emacs_mode() {
let mut editor = editor_with("123456789");

// Test selection moving right from position 3
editor.line_buffer.set_insertion_point(3);
editor.update_selection_anchor(true);
editor.move_right(true);

// In Emacs mode, selection should be exactly between positions (no extra character)
let emacs_mode = crate::PromptEditMode::Emacs;
assert_eq!(editor.get_selection_with_mode(&emacs_mode), Some((3, 4)));

// Move right again
editor.move_right(true);
assert_eq!(editor.get_selection_with_mode(&emacs_mode), Some((3, 5)));
}

#[test]
fn test_selection_vi_insert_mode() {
let mut editor = editor_with("123456789");

// Test selection moving right from position 3
editor.line_buffer.set_insertion_point(3);
editor.update_selection_anchor(true);
editor.move_right(true);

// In Vi Insert mode, selection should be exactly between positions (no extra character)
let vi_insert_mode = crate::PromptEditMode::Vi(crate::PromptViMode::Insert);
assert_eq!(
editor.get_selection_with_mode(&vi_insert_mode),
Some((3, 4))
);

// Move right again
editor.move_right(true);
assert_eq!(
editor.get_selection_with_mode(&vi_insert_mode),
Some((3, 5))
);
}

#[test]
fn test_selection_vi_normal_mode() {
let mut editor = editor_with("123456789");

// Test selection moving right from position 3
editor.line_buffer.set_insertion_point(3);
editor.update_selection_anchor(true);
editor.move_right(true);

// In Vi Normal mode, selection should include the character under cursor (extend by one)
let vi_normal_mode = crate::PromptEditMode::Vi(crate::PromptViMode::Normal);
assert_eq!(
editor.get_selection_with_mode(&vi_normal_mode),
Some((3, 5))
);

// Move right again
editor.move_right(true);
assert_eq!(
editor.get_selection_with_mode(&vi_normal_mode),
Some((3, 6))
);
}

#[test]
fn test_selection_left_direction_emacs() {
let mut editor = editor_with("123456789");

// Test selection moving left from position 5
editor.line_buffer.set_insertion_point(5);
editor.update_selection_anchor(true);
editor.move_left(true);

// In Emacs mode, selection should be exactly between positions
let emacs_mode = crate::PromptEditMode::Emacs;
assert_eq!(editor.get_selection_with_mode(&emacs_mode), Some((4, 5)));

// Move left again
editor.move_left(true);
assert_eq!(editor.get_selection_with_mode(&emacs_mode), Some((3, 5)));
}

#[test]
fn test_selection_left_direction_vi_normal() {
let mut editor = editor_with("123456789");

// Test selection moving left from position 5
editor.line_buffer.set_insertion_point(5);
editor.update_selection_anchor(true);
editor.move_left(true);

// In Vi Normal mode, selection should include the character under cursor
let vi_normal_mode = crate::PromptEditMode::Vi(crate::PromptViMode::Normal);
assert_eq!(
editor.get_selection_with_mode(&vi_normal_mode),
Some((4, 6))
);

// Move left again
editor.move_left(true);
assert_eq!(
editor.get_selection_with_mode(&vi_normal_mode),
Some((3, 6))
);
}

#[test]
fn test_selection_at_buffer_end() {
let mut editor = editor_with("123");

// Test selection at the end of buffer
editor.line_buffer.set_insertion_point(2);
editor.update_selection_anchor(true);
editor.move_right(true); // Move to end of buffer

let emacs_mode = crate::PromptEditMode::Emacs;
let vi_normal_mode = crate::PromptEditMode::Vi(crate::PromptViMode::Normal);

// In Emacs mode, selection should be exactly between positions
assert_eq!(editor.get_selection_with_mode(&emacs_mode), Some((2, 3)));

// In Vi Normal mode, it should not extend beyond buffer
assert_eq!(
editor.get_selection_with_mode(&vi_normal_mode),
Some((2, 3))
);
}

#[test]
fn test_selection_with_unicode() {
let mut editor = editor_with("🦀rust");

// Test selection with unicode characters
editor.line_buffer.set_insertion_point(0);
editor.update_selection_anchor(true);
editor.move_right(true); // Move past the crab emoji

let emacs_mode = crate::PromptEditMode::Emacs;
let vi_normal_mode = crate::PromptEditMode::Vi(crate::PromptViMode::Normal);

// In Emacs mode, selection should be exactly between positions
assert_eq!(editor.get_selection_with_mode(&emacs_mode), Some((0, 4)));

// In Vi Normal mode, should include the next character
assert_eq!(
editor.get_selection_with_mode(&vi_normal_mode),
Some((0, 5))
);
}

#[test]
fn test_backward_compatibility() {
let mut editor = editor_with("123456789");

// Test that get_selection() maintains Vi Normal mode behavior for backward compatibility
editor.line_buffer.set_insertion_point(3);
editor.update_selection_anchor(true);
editor.move_right(true);

// Default get_selection should behave like Vi Normal mode (extend by one)
assert_eq!(editor.get_selection(), Some((3, 5)));
}

#[test]
fn test_issue_893_regression() {
// Reproduce the exact issue described in #893
let mut editor = editor_with("123456789");

// Position cursor after "3" (position 3, so "123|456789")
editor.line_buffer.set_insertion_point(3);

// Start selection and move left (shift + left arrow equivalent)
editor.update_selection_anchor(true);
editor.move_left(true);

// In Emacs mode, should only select character "3" (position 2 to 3)
let emacs_mode = crate::PromptEditMode::Emacs;
assert_eq!(editor.get_selection_with_mode(&emacs_mode), Some((2, 3)));

// Reset and test moving right
let mut editor = editor_with("123456789");
editor.line_buffer.set_insertion_point(3);
editor.update_selection_anchor(true);
editor.move_right(true);

// In Emacs mode, should only select character "4" (position 3 to 4)
assert_eq!(editor.get_selection_with_mode(&emacs_mode), Some((3, 4)));

// But in Vi normal mode, should include the character under cursor (as per #843)
let vi_normal_mode = crate::PromptEditMode::Vi(crate::PromptViMode::Normal);
assert_eq!(
editor.get_selection_with_mode(&vi_normal_mode),
Some((3, 5))
);
}

#[test]
fn test_demonstration_fix_for_all_modes() {
// This test demonstrates that the fix works correctly for all edit modes
// Issue #893: Emacs mode was selecting two characters instead of one
// Issue #843: Vi normal mode needs to include character under cursor

let test_text = "abcdefgh";

// Test scenario: cursor at position 3 ('d'), select one character to the right
for mode_name in ["Emacs", "Vi Insert", "Vi Normal"] {
let mut editor = editor_with(test_text);
editor.line_buffer.set_insertion_point(3); // cursor after 'c', before 'd'
editor.update_selection_anchor(true);
editor.move_right(true); // select to the right

let (expected_start, expected_end, mode) = match mode_name {
"Emacs" => (3, 4, crate::PromptEditMode::Emacs),
"Vi Insert" => (3, 4, crate::PromptEditMode::Vi(crate::PromptViMode::Insert)),
"Vi Normal" => (3, 5, crate::PromptEditMode::Vi(crate::PromptViMode::Normal)),
_ => unreachable!(),
};

let selection = editor.get_selection_with_mode(&mode);
assert_eq!(
selection,
Some((expected_start, expected_end)),
"Mode: {}, expected selection from {} to {}, got {:?}",
mode_name,
expected_start,
expected_end,
selection
);

// Verify the selected text
let selected_text = &test_text[expected_start..expected_end];
let expected_text = match mode_name {
"Emacs" | "Vi Insert" => "d", // Just the character 'd'
"Vi Normal" => "de", // Character 'd' plus next character 'e'
_ => unreachable!(),
};
assert_eq!(
selected_text, expected_text,
"Mode: {}, expected text '{}', got '{}'",
mode_name, expected_text, selected_text
);
}
}
}
4 changes: 3 additions & 1 deletion src/engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1752,7 +1752,9 @@ impl Reedline {
let mut styled_text = self
.highlighter
.highlight(buffer_to_paint, cursor_position_in_buffer);
if let Some((from, to)) = self.editor.get_selection() {
// Use context-aware selection with current edit mode
let edit_mode = self.edit_mode.edit_mode();
if let Some((from, to)) = self.editor.get_selection_with_mode(&edit_mode) {
styled_text.style_range(from, to, self.visual_selection_style);
}

Expand Down
Loading