Skip to content

Conversation

collinmurch
Copy link
Contributor

@collinmurch collinmurch commented Jun 30, 2025

This PR fixes shift selection in vi (insert) & emacs mode. The gist is that we were incorrectly selecitng an additional character when selecting leftwards (holding shift and pressing left arrow). One extra character to the right of the start location was getting highlighted and would be affected by any delete or other operations.

Tested on macOS with both vi & emacs mode. No regression of vi (normal) shift selection functionality.

Fix for this issue: #922 which I think is a duplicate of #893.

@blindFS
Copy link
Contributor

blindFS commented Jun 30, 2025

Thanks, that seems similar but clearer than the solution by Claude #911 .

Noticed one minor issue, steps to reproduce:

  1. selection done in vi normal mode
  2. press c, the last selected character is not deleted.

@collinmurch
Copy link
Contributor Author

Yeah, you're totally right; I missed this test case. Should be fixed now, all the modes seem to work for me.

@blindFS
Copy link
Contributor

blindFS commented Jun 30, 2025

Yeah, you're totally right; I missed this test case. Should be fixed now, all the modes seem to work for me.

Your last commit didn't change anything in my setup. Say I have abcdefg in cli with cde selected in vi normal mode.
After pressing c, the result is ab|efg where | represents the cursor.

@blindFS
Copy link
Contributor

blindFS commented Jun 30, 2025

There's another complication: inconsistent behavior of selection in vi normal mode triggered by v or shift + arrow key.
A common issue for #911

@collinmurch
Copy link
Contributor Author

collinmurch commented Jun 30, 2025

Okay I pushed up a commit that actually does fix it. My prior one did fix shift selection going leftwards but seems to have just shifted the issue to being present moving to the right for vi normal mode + cut operations. This new commit fixes it for both, but adds some complexity that I don't love.

The core issue is that when we perform the cut operation, we immediately go into insert mode which then messes up the calculation for text to delete. Since we're in insert mode, it selects one character short even though the selection was made with vi normal mode.

Let me know what you think of this. Also, I'm not sure exactly what you're referring to with:

There's another complication: inconsistent behavior of selection in vi normal mode triggered by v or shift + arrow key.

From my experience, I can't even get the two to behave the same on a fresh build of the main branch. It seems that shift selection for vi normal mode has some other bug that makes it operate inconsistently with the cut and other operations.

@blindFS
Copy link
Contributor

blindFS commented Jun 30, 2025

There's another complication: inconsistent behavior of selection in vi normal mode triggered by v or shift + arrow key. A common issue for #911

Yeah I noticed this too but in my testing the same behavior is present in the current build of main. So I think it might be unrelated. Let me know if you disagree.

Yeah, probably rooted elsewhere. OMG...

@blindFS
Copy link
Contributor

blindFS commented Jun 30, 2025

I used to take this issue to test AI agents' capabilities, haha. Never found one that's good enough. Claude 4.0 >> Gemini 2.5 pro on this issue, but still lame.

@collinmurch
Copy link
Contributor Author

Yeah since I'm new to this repo claude 4 has helped out with some of this but ultimately I'm having to babysit it pretty aggressively. Keeps wanting to implement hacky one-off fixes.

@collinmurch
Copy link
Contributor Author

Any movement on this?

@collinmurch
Copy link
Contributor Author

Bumping this.

@collinmurch collinmurch force-pushed the main branch 2 times, most recently from 0b7721b to 7047385 Compare July 30, 2025 03:38
@Bahex Bahex self-assigned this Jul 31, 2025
@Bahex
Copy link
Member

Bahex commented Aug 1, 2025

Rather than separate selection_anchor and selection_mode fields, I think it might be better to define an enum like this:

pub enum Selection {
    Inclusive(usize),
    Exclusive(usize),
}

pub struct Editor {
    // ...
    selection: Option<Selection>
    // ...
}

that way the anchor and mode will always have to be updated together.

@collinmurch
Copy link
Contributor Author

collinmurch commented Aug 26, 2025

Good call! I've implemented it and retested with the suggestion. Let me know what you think.

Sorry for the delay, I've been travelling the last few weeks and haven't had much free time.

last_undo_behavior: UndoBehavior,
selection_anchor: Option<usize>,
selection: Option<Selection>,
edit_mode: PromptEditMode,
Copy link
Contributor

Choose a reason for hiding this comment

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

@Bahex Should we consider moving edit_mode from

reedline/src/engine.rs

Lines 128 to 129 in adeb8ea

// Edit Mode: Vi, Emacs
edit_mode: Box<dyn EditMode>,
here?
It doesn't make much sense to me that edit_mode is not native to the editor.

@blindFS
Copy link
Contributor

blindFS commented Aug 26, 2025

There's another complication: inconsistent behavior of selection in vi normal mode triggered by v or shift + arrow key. A common issue for #911

Yeah I noticed this too but in my testing the same behavior is present in the current build of main. So I think it might be unrelated. Let me know if you disagree.

Yeah, probably rooted elsewhere. OMG...

Good news, this one seems to be fixed by recent changes, with an itching edge case:

  1. enter vi normal mode
  2. press v then x, nothing deleted because the selection doesn't happen until we make some movements.

This makes me wonder, do we really need these boilerplates?

reedline/src/enums.rs

Lines 23 to 99 in adeb8ea

/// Move to the start of the buffer
MoveToStart {
/// Select the text between the current cursor position and destination
select: bool,
},
/// Move to the start of the current line
MoveToLineStart {
/// Select the text between the current cursor position and destination
select: bool,
},
/// Move to the end of the buffer
MoveToEnd {
/// Select the text between the current cursor position and destination
select: bool,
},
/// Move to the end of the current line
MoveToLineEnd {
/// Select the text between the current cursor position and destination
select: bool,
},
/// Move one character to the left
MoveLeft {
/// Select the text between the current cursor position and destination
select: bool,
},
/// Move one character to the right
MoveRight {
/// Select the text between the current cursor position and destination
select: bool,
},
/// Move one word to the left
MoveWordLeft {
/// Select the text between the current cursor position and destination
select: bool,
},
/// Move one WORD to the left
MoveBigWordLeft {
/// Select the text between the current cursor position and destination
select: bool,
},
/// Move one word to the right
MoveWordRight {
/// Select the text between the current cursor position and destination
select: bool,
},
/// Move one word to the right, stop at start of word
MoveWordRightStart {
/// Select the text between the current cursor position and destination
select: bool,
},
/// Move one WORD to the right, stop at start of WORD
MoveBigWordRightStart {
/// Select the text between the current cursor position and destination
select: bool,
},
/// Move one word to the right, stop at end of word
MoveWordRightEnd {
/// Select the text between the current cursor position and destination
select: bool,
},
/// Move one WORD to the right, stop at end of WORD
MoveBigWordRightEnd {
/// Select the text between the current cursor position and destination
select: bool,
},

I mean, entering selection and movements with selection should be logically different operations, while Shift + arrow keys just trigger them consecutively.

@collinmurch
Copy link
Contributor Author

Okay I updated the enum shape to be logically distinct. Noteably though this will require more than just a version bump in nushell as it changes the way we interface with the cursor. The changes are pretty small though:

nushell/nushell@main...collinmurch:nushell:main

Which would you prefer? Reverting to the old one with the coupled enum shape or this new one, which requires nushell changes?

I'd like to get this bug fixed. I can look at the vi issue that's currently present perhaps after.

@collinmurch collinmurch requested a review from blindFS September 8, 2025 02:05
src/enums.rs Outdated
EditCommand::MoveLeftBefore { .. } => {
write!(f, "MoveLeftBefore Value: <char>, Optional[select: <bool>]")
}
EditCommand::Move { movement, .. } => match movement {
Copy link
Contributor

@blindFS blindFS Sep 8, 2025

Choose a reason for hiding this comment

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

Sorry for not making it clear, but this isn't what I meant:

  1. We probably don't need the bool to mark whether a cursor movement is triggered with selection.
  2. Instead, selection status should be a native property of the editor itself.
  3. In vi normal mode, press v to toggle selection on/off (inclusive selection)
  4. In all modes, shift + arrow keys to firstly turn selection (exclusive) on, then handle the movement.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's too much for this bug fixing PR, I'll leave it for other reviewers to judge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I got it. Yeah I hear you I can probably take a stab at this too and throw up a draft MR. Otherwise I'd probably fall back on the previous implementation

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I got it. Yeah I hear you I can probably take a stab at this too and throw up a draft MR. Otherwise I'd probably fall back on the previous implementation

Yeah, I think reverting to previous implementation makes it easier to review.

@collinmurch collinmurch requested a review from blindFS October 15, 2025 03:58
@blindFS
Copy link
Contributor

blindFS commented Oct 15, 2025

It works well for me.

The implementation looks not very straightforward but I guess we don't have much options without completely redesigning the code base of selection and movement with selection, which I'm afraid none of us has the passion to work on.

We could use some TODO comments if we have the vision of a "better" design.

After all, I know little about the reedline code, let's await for Bahex to make the call.

@collinmurch
Copy link
Contributor Author

It works well for me.

The implementation looks not very straightforward but I guess we don't have much options without completely redesigning the code base of selection and movement with selection, which I'm afraid none of us has the passion to work on.

We could use some TODO comments if we have the vision of a "better" design.

After all, I know little about the reedline code, let's await for Bahex to make the call.

Yeah agreed. I just want this darn issue fixed. I've been using my own fork of nushell with this fix in place for so long now. Anyways, updated again. There was a clippy failure in the CI/CD.

@Bahex Bahex merged commit 0bb9aca into nushell:main Oct 15, 2025
6 checks passed
@Bahex
Copy link
Member

Bahex commented Oct 15, 2025

Thanks for all your work on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants