Implement vi-mode "Yank" (copy): #868
Conversation
- Vi Command; - Editor Command; - Editor methods; - Mode changes back to Normal after yank
deephbz
force-pushed
the
feature/vi-yank
branch
from
7e2619d to
af86266
Compare
There was a problem hiding this comment.
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. |
| 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)), | ||
| ] | ||
| } |
There was a problem hiding this comment.
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.
| (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), |
There was a problem hiding this comment.
Let's not forget to rebase/merge onto the updated main branch when landing #867
| 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, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| /// 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, |
There was a problem hiding this comment.
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.
Implement vi-mode "Yank" (copy):
- Vi Command;
- Editor Command;
- Editor methods;
- Mode changes back to Normal after yank
Depends on #867 (the first 3 commits of this PR are included in 867)