Implement vi-mode "Yank" (copy): #868

deephbz · 2024-12-31T10:41:43Z

Implement vi-mode "Yank" (copy):
- Vi Command;
- Editor Command;
- Editor methods;
- Mode changes back to Normal after yank

Note only last commit in this PR should be reviewed as part of this PR as this is on top of:

sholderbach

Thanks for giving this a shot and putting in the work! One thing that makes me hesitant about this, that the copying we do does not work with multipliers at all as each EditCommand will update the clipboard. Also operations you define in terms of two EditCommands suffer the same unfortunate fate.

One solution to that would be to do extra state management around clipboard but keep the old edit commands (suggested in #631 ) or otherwise rethink how we implement the vi mode interface to use an internal selection build up from the motion before performing the action. The latter would require way fewer additional EditCommand to be maintained, like the ones you propose here. Related issues where that came up was the impl of #849 where there are cases where the chaining of EditCommands did not achieve the expected result.

deephbz · 2025-01-01T01:31:33Z

Thanks for giving this a shot and putting in the work! One thing that makes me hesitant about this, that the copying we do does not work with multipliers at all as each EditCommand will update the clipboard. Also operations you define in terms of two EditCommands suffer the same unfortunate fate.

One solution to that would be to do extra state management around clipboard but keep the old edit commands (suggested in #631 ) or otherwise rethink how we implement the vi mode interface to use an internal selection build up from the motion before performing the action. The latter would require way fewer additional EditCommand to be maintained, like the ones you propose here. Related issues where that came up was the impl of #849 where there are cases where the chaining of EditCommands did not achieve the expected result.

What you say are valid concerns and I do agree in the future a re-think/refactor is needed to make Vi mode elegant and better.
However, I think it's still of great utility (and also harmless) to merge this PR in order to make Nushell's Vi mode "feature-complete", in a sense that 99% users' 99% use cases are covered. It's really common to yank and paste (like moving CLI options) so missing it is a barrier to onboard new years.

sholderbach · 2025-01-08T13:32:57Z

src/edit_mode/vi/command.rs

+            Self::YankInside(left) if is_valid_change_inside_left(left) => {
+                let right = bracket_for(left);
+                vec![
+                    ReedlineOption::Edit(EditCommand::CopyLeftBefore(*left)),
+                    ReedlineOption::Edit(EditCommand::CopyRightBefore(right)),
+                ]
+            }
+            Self::YankInside(right) if is_valid_change_inside_right(right) => {
+                let left = bracket_for(right);
+                vec![
+                    ReedlineOption::Edit(EditCommand::CopyLeftBefore(left)),
+                    ReedlineOption::Edit(EditCommand::CopyRightBefore(*right)),
+                ]
+            }


As those are guranteed to be broken for all cases where you are not on the beginning of the text object (putting the left half in the clipboard and then overwriting with the right half) I would omit them until we have a better solution. Yes to shipping but let's not ship broken stuff.

This was indeed my mistake initially. Thanks for helping review! This specific issue should be addressed in #874 , atomic Cut/CopyInside pair of brackets/quotes.

sholderbach · 2025-01-08T13:33:46Z

src/edit_mode/vi/parser.rs

+            (Some(Command::Delete), ParseResult::Incomplete)
+            | (Some(Command::DeleteChar), ParseResult::Incomplete)
+            | (Some(Command::DeleteToEnd), ParseResult::Incomplete)
+            | (Some(Command::Delete), ParseResult::Valid(_))
+            | (Some(Command::DeleteChar), ParseResult::Valid(_))
+            | (Some(Command::DeleteToEnd), ParseResult::Valid(_))
+            | (Some(Command::Yank), ParseResult::Valid(_))
+            | (Some(Command::Yank), ParseResult::Incomplete)
+            | (Some(Command::YankInside(_)), ParseResult::Valid(_))
+            | (Some(Command::YankInside(_)), ParseResult::Incomplete) => Some(ViMode::Normal),


Let's not forget to rebase/merge onto the updated main branch when landing #867

sholderbach · 2025-01-08T13:36:40Z

src/core_editor/editor.rs

+    pub(crate) fn copy_from_start(&mut self) {
+        let insertion_offset = self.line_buffer.insertion_point();
+        if insertion_offset > 0 {
+            self.cut_buffer.set(
+                &self.line_buffer.get_buffer()[..insertion_offset],
+                ClipboardMode::Normal,
+            );
+        }
+    }


I haven't gone through all of those to check if their indexing is correct. That's an annoying part to deal with this level of duplication for the different versions referring to the same text-objects/motions.

sholderbach · 2025-01-08T13:38:46Z

src/enums.rs

+    /// Copy from the start of the buffer to the insertion point
+    CopyFromStart,
+
+    /// Copy from the start of the current line to the insertion point
+    CopyFromLineStart,


For the users trying to configure their emacs mode having all those extra variants certainly adds some potential for confusion, and when we remove them for a simpler VI mode more breaking changes where we hope people don't try to depend on them.

deephbz · 2025-01-18T10:29:29Z

Thanks for giving this a shot and putting in the work! One thing that makes me hesitant about this, that the copying we do does not work with multipliers at all as each EditCommand will update the clipboard. Also operations you define in terms of two EditCommands suffer the same unfortunate fate.

One solution to that would be to do extra state management around clipboard but keep the old edit commands (suggested in #631 ) or otherwise rethink how we implement the vi mode interface to use an internal selection build up from the motion before performing the action. The latter would require way fewer additional EditCommand to be maintained, like the ones you propose here. Related issues where that came up was the impl of #849 where there are cases where the chaining of EditCommands did not achieve the expected result.

Indeed I see a common pattern Move/Cut/Copy + selection range (word, big word, line end, etc.). 3+ actions plus ~10 selections give us numerous EditCommands.

132ikl · 2025-02-08T16:02:59Z

is anything blocking this? I think we're all in agreement that the vi mode interface should be reworked eventually but this PR seems like a helpful stop-gap for now

deephbz · 2025-02-09T04:30:54Z

is anything blocking this? I think we're all in agreement that the vi mode interface should be reworked eventually but this PR seems like a helpful stop-gap for now

I don't think there's any block except for the dependent PRs #867 , #874.
I've personally used this branch for months on a daily basis without any issue and the Vi mode works perfectly. I'm open to owners modify my above PRs if that help get things merged.

sholderbach · 2025-02-10T12:12:48Z

Should be good for a rebase/merge following the merge of #867.

Is the refactor proposed/documented in #874 already part of this branch or am I misreading this?

Otherwise I am good with landing this (@132ikl if you want to look into something feel free to chime in otherwise I would give the green light after resolving the branch)

deephbz · 2025-02-11T03:20:00Z

Should be good for a rebase/merge following the merge of #867.

Is the refactor proposed/documented in #874 already part of this branch or am I misreading this?

Otherwise I am good with landing this (@132ikl if you want to look into something feel free to chime in otherwise I would give the green light after resolving the branch)

I've rebased this to be on top of #874. Current PR includes all of #874 with one additional commit for Vi-Yank. Once 874 is merged, we can merge this PR with a single-clean commit for yank.

132ikl · 2025-02-11T05:07:05Z

sounds good, let me or @sholderbach know when you update the branch

- Vi Command; - Editor Command; - Editor methods; - Mode changes back to Normal after yank

deephbz · 2025-02-11T05:40:35Z

sounds good, let me or @sholderbach know when you update the branch

Done ready for review :D

132ikl · 2025-02-11T14:34:08Z

thank you!!

deephbz force-pushed the feature/vi-yank branch from 7e2619d to af86266 Compare December 31, 2024 11:05

sholderbach reviewed Dec 31, 2024

View reviewed changes

deephbz requested a review from sholderbach January 4, 2025 01:46

sholderbach reviewed Jan 8, 2025

View reviewed changes

deephbz force-pushed the feature/vi-yank branch 2 times, most recently from bb76b01 to 1f73f9a Compare January 18, 2025 10:25

sholderbach mentioned this pull request Feb 10, 2025

Vi-Mode Feature: Atomic unified commands for ChangeInside/DeleteInside #874

Merged

deephbz force-pushed the feature/vi-yank branch from 1f73f9a to eecf12b Compare February 11, 2025 03:18

Implement vi-mode "Yank" (copy):

6cfb08c

- Vi Command; - Editor Command; - Editor methods; - Mode changes back to Normal after yank

deephbz force-pushed the feature/vi-yank branch from eecf12b to 6cfb08c Compare February 11, 2025 05:40

132ikl merged commit 446fb1d into nushell:main Feb 11, 2025
6 checks passed

deephbz deleted the feature/vi-yank branch February 12, 2025 00:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement vi-mode "Yank" (copy): #868

Implement vi-mode "Yank" (copy): #868

deephbz commented Dec 31, 2024 •

edited

Loading

sholderbach left a comment

deephbz commented Jan 1, 2025

sholderbach Jan 8, 2025

deephbz Jan 18, 2025 •

edited

Loading

sholderbach Jan 8, 2025

sholderbach Jan 8, 2025

sholderbach Jan 8, 2025

deephbz commented Jan 18, 2025

132ikl commented Feb 8, 2025

deephbz commented Feb 9, 2025

sholderbach commented Feb 10, 2025

deephbz commented Feb 11, 2025

132ikl commented Feb 11, 2025 •

edited

Loading

deephbz commented Feb 11, 2025

132ikl commented Feb 11, 2025

Implement vi-mode "Yank" (copy): #868

Implement vi-mode "Yank" (copy): #868

Conversation

deephbz commented Dec 31, 2024 • edited Loading

sholderbach left a comment

Choose a reason for hiding this comment

deephbz commented Jan 1, 2025

sholderbach Jan 8, 2025

Choose a reason for hiding this comment

deephbz Jan 18, 2025 • edited Loading

Choose a reason for hiding this comment

sholderbach Jan 8, 2025

Choose a reason for hiding this comment

sholderbach Jan 8, 2025

Choose a reason for hiding this comment

sholderbach Jan 8, 2025

Choose a reason for hiding this comment

deephbz commented Jan 18, 2025

132ikl commented Feb 8, 2025

deephbz commented Feb 9, 2025

sholderbach commented Feb 10, 2025

deephbz commented Feb 11, 2025

132ikl commented Feb 11, 2025 • edited Loading

deephbz commented Feb 11, 2025

132ikl commented Feb 11, 2025

deephbz commented Dec 31, 2024 •

edited

Loading

deephbz Jan 18, 2025 •

edited

Loading

132ikl commented Feb 11, 2025 •

edited

Loading