Add ability to select and cut text in the input buffer #689

Tastaturtaste · 2023-12-27T19:35:14Z

All CutXXX commands like "CutFromStart" or "CutWordLeft" should use the OS copy/paste buffer not an internal buffer

This is already the case if the crate is compiled with the feature system_clipboard. This is, afaik, not (yet) done for nushell.

codecov · 2023-12-27T19:53:53Z

Codecov Report

Attention: 268 lines in your changes are missing coverage. Please review.

Comparison is base (dc27ed8) 49.32% compared to head (39ba732) 49.69%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     nushell/nushell#689      +/-   ##
==========================================
+ Coverage   49.32%   49.69%   +0.37%     
==========================================
  Files          46       46              
  Lines        7998     8433     +435     
==========================================
+ Hits         3945     4191     +246     
- Misses       4053     4242     +189

Files	Coverage Δ
src/edit_mode/emacs.rs	`93.85% <100.00%> (+0.22%)`	⬆️
src/edit_mode/keybindings.rs	`92.59% <100.00%> (+3.86%)`	⬆️
src/edit_mode/vi/parser.rs	`96.10% <ø> (ø)`
src/edit_mode/vi/vi_keybindings.rs	`100.00% <100.00%> (ø)`
src/painting/styled_text.rs	`61.65% <98.34%> (+61.65%)`	⬆️
src/edit_mode/vi/command.rs	`29.76% <0.00%> (-1.11%)`	⬇️
src/core_editor/line_buffer.rs	`87.69% <0.00%> (-1.94%)`	⬇️
src/engine.rs	`5.08% <6.66%> (+0.10%)`	⬆️
src/enums.rs	`16.86% <2.32%> (-2.61%)`	⬇️
src/edit_mode/vi/motion.rs	`38.63% <21.42%> (-5.49%)`	⬇️
... and 1 more

src/core_editor/editor.rs

fdncred · 2023-12-27T19:55:34Z

This looks like a promising start. Is it enabled in any of the reedline demos so we can try it out?

Tastaturtaste · 2023-12-27T19:59:49Z

You can try it out in the basic example of reedline. It is not gated behind a feature. But if you want to use Ctrl+V you have to compile with feature system_clipboard, at least on windows.

fdncred · 2023-12-27T20:07:49Z

Hmmm, I'm on macos and can't get it to work. I compiled with cargo run --examples basic --features=system_clipboard. I tried Ctrl+A, Shift+Left, Shift+Right and I'm not getting any selection.

Tastaturtaste · 2023-12-27T20:11:20Z

I have not yet implemented highlighting of the selection. If you hold down Shift and move the cursor to over text and afterwards press Ctrl + X you should have the text in the clipboard. At least pasting inside the terminal again worked without problem for me.

Edit: I just tested pasting outside of the terminal, that works for me, too.

fdncred · 2023-12-27T20:14:39Z

oh, i see. seems to work great with that little tidbit. 😆 thanks.

Tastaturtaste · 2024-01-06T11:56:30Z

Is there anything left to do before you can consider to merge it? The two failing clippy lints are not related to the code I touched, so I don't know what to do about them...

fdncred · 2024-01-06T12:54:09Z

Just change the code like the clippy lints suggest, replacing .get(0) with .first(). The CI needs to be green prior to landing.

Do you plan on making a PR for nushell based on this PR?

Tastaturtaste · 2024-01-06T18:44:42Z

Do you plan on making a PR for nushell based on this PR?

Yeah, that was my plan

fdncred · 2024-01-06T22:32:09Z

I'd like to know/see what changes are required in nushell before landing this.

fdncred · 2024-01-13T14:54:38Z

I've looked over this several times now and I'm about ready to land it. I see you have a builder pattern to implement it in nushell (and others). How are you feeling about this PR/Do you still have things you want to do or are you ready?

Tastaturtaste · 2024-01-13T20:30:02Z

I see you have a builder pattern to implement it in nushell

Yes, that should work just like any other EditCommand. However, the bindings Ctrl + C and Ctrl + A are used in the default config.nu for cancel_command and move_to_line_start respectively. I don't know how a differentiation between Ctrl + C to cancel a operation and Ctrl + C to copy a selection could be handled. Maybe there is some way to see if currently some text is selected and treat it as a copy command and otherwise as a cancel command?

How are you feeling about this PR/Do you still have things you want to do or are you ready?

Actually, I am currently working on making selection an option for all of the previously existing move commands. This should afford much more flexibility and I believe is better extensible.
Please take a look if you think this would be beneficial. I think it is an improvement.

fdncred · 2024-01-14T00:36:47Z

Would you mind talking a bit about how the selection, etc works in relation to graphemes? Seems like we have a lot of grapheme work here but I'm not sure all that is accommodated in your new code or not and I'd hate to break that type of thing.

Tastaturtaste · 2024-01-14T12:11:48Z

Of course, I will try to give a brief summary of the selection logic and how it relates to grapheme boundaries.

Any EditCommand of the MoveCursor type carries with it a parameter specifying if the move should select text that is moved over. This works as follows:

The Editor has a field selection_anchor of type Option<usize>, which tracks if a selection is currently happening, and if so, it carries the index of one end of the selected range into the line_buffer. The other end of the selected range is marked by the current cursor position.
When a EditCommand with a selection request (select == true) is processed, a check is made if we previously already started selecting text (indicated by Some(usize) in selection_anchor). If yes, the selection_anchor remains set as it is. If no, it is set to the current cursor position before moving the cursor.
When an EditCommand is processed that does anything else than select text by moving the cursor the selection_anchor is reset to None.

By this logic, the selection_anchor can only contain indices that were previously also cursor positions. Assuming

Cursor positions are always valid indices at grapheme boundaries
All commands that modify the line buffer also reset the selection_anchor

the indices bracketing the selected text in the line buffer are always valid indices at grapheme boundaries.
Assumption 2. is given because all MoveCursor edit commands do not modify the line buffer and conceptually probably never should. However, this could be a maintenance hazard if somehow a MoveCursor command with select == true were added that does modify the line buffer. This hazard could be contained if mutable access to the line_buffer would always have to be done through a custom type or function that could then make sure to always reset the selection_anchor.

fdncred · 2024-01-14T13:49:55Z

Thanks so much for the detailed explanation ❤️. It seems like we'd want to remove this maintenance hazard if we could, although I believe that to be outside the scope of this PR.

…pace'

fdncred · 2024-01-17T18:03:39Z

ok, let's go with this then!

fdncred · 2024-01-17T18:04:29Z

now you can update your nushell PR and we can troubleshoot there. hopefully we don't have to come back here to make any changes.

This PR should close #1171 # Description  This PR introduces the capability to select text using the existing move.. `EditCommand`s of `reedline`. Those commands are extended with an optional parameter specifying if text should be selected while navigating. This enables a workflow familiar from a wide variety of text editors, where holding `shift` while navigating selects all text between the initial cursor position when pressing `shift` and the current cursor position. Before this PR can be merged the [sibling PR for reedline](nushell/reedline#689) has to land first. # User-Facing Changes ## Additional `EditCommand`s 1. `SelectAll` 2. `CutSelection` 3. `CopySelection` ## New optional parameter on existing `EditCommand`s All `EditCommand`s of `EditType` `MoveCursor` have a new optional parameter named `select` of type `bool`. If this parameter is not set by a user it is treated as false, which corresponds to their behavior up to now. I am relatively new to `nushell` and as such may not know of existing behavior that might change through this PR. However, I believe there should be none. I come to this conclusion because 1. Existing commands are extended only with an *optional* additional parameter, users who currently use these EditCommands keep their existing behavior if they don't use it. 2. A few new commands are introduced which were previously not valid. 3. The default keybindings specified in `default_config.nu` are untouched. # Tests + Formatting Tests for the new optional parameter for the move commands are included to make sure that they truly are optional and an unused optional parameter conforms to the previous behavior.

nushell/nushell#11600 (comment)

…ell/nushell#11600 (comment) (#715)

fdncred · 2024-02-07T12:30:45Z

@Tastaturtaste when you have a minute, could you help us figure out why these cut/copy/select_all/paste keybindings are not working in nushell when it's launched with nu -n?

Tastaturtaste · 2024-02-07T18:44:26Z

I will have to see if I get to it tomorrow.

Tastaturtaste · 2024-02-08T18:41:46Z

Hmm, I just cloned a fresh copy of nushell and tried to reproduce the commands not working. But for me everything works as expected. Just to be sure I did the right steps to reproduce:

git clone https://github.com/nushell/nushell.git
cd nushell
cargo run -- -n
<Test commands here>

Are there specific steps to reproduce the issue?

fdncred · 2024-02-08T19:27:24Z

@amtoine can you comment on this?

@Tastaturtaste It works on flawlessly on Windows for me with nu -n or nu -n --no-std-lib. I was having problems on mac. I can test there later. I'll try ubuntu on WSL and see in a minute.

fdncred · 2024-02-08T19:36:40Z

@Tastaturtaste on WSL Ubuntu, when I typed shift left-arrow I got this. Note that this happens when I accept an item out of my history. e.g. i typed nu and the history hint was nu -c "def table [] { \"hi\" }; table" so I right-arrowed to complete it, hit left-arrow once to get inside the trailing double quote, then hit shift left-arrow for the crash.

with full backtrace

Tastaturtaste · 2024-02-08T21:42:07Z

I can't take a closer look today, but will look into it in the next few days. Sadly my weekend is packed already, so maybe after that.

The backtrace gives a good hint already. Somewhere in reedline::painting::styled_text::StyledText::style_range the indexing of the styled buffer (or rather of one styled fragment of the buffer) goes wrong. The idea in that function is to store each continuous fragment of the buffer with the same style as a pair of String and Style. When something is selected the style has to be updated to show the selection effect.

I can reproduce the crash with a shorter prompt on windows:

Type cargo run (do not press enter)
press left once (cursor is now between u and n
press shift + left

Edit: Or rather a crash in that function. I get a

.../reedline-0.29.0\src\painting\styled_text.rs:61:58:
attempt to subtract with overflow

Seems I botched some logic in that function :/

fdncred · 2024-02-08T22:12:41Z

@Tastaturtaste Sounds good. We appreciate your help whenever you can get to it!

kubouch · 2024-02-08T22:17:45Z

@Tastaturtaste Could you put that panic with repro steps in a separate issue and slap the "Critical" label on it? So that we can track easier in case you won't have time in the next few days.

fdncred · 2024-02-09T11:57:17Z

I think this is a similar panic in nushell #750

Tastaturtaste · 2024-02-12T16:38:08Z

#689 (comment)
I got this fixed on this branch. Now I need to try and reproduce the error @fdncred mentions. Please also try my fixed branch on your machine.

Tastaturtaste · 2024-02-12T16:59:30Z

@Tastaturtaste on WSL Ubuntu, when I typed shift left-arrow I got this. Note that this happens when I accept an item out of my history. e.g. i typed nu and the history hint was nu -c "def table [] { \"hi\" }; table" so I right-arrowed to complete it, hit left-arrow once to get inside the trailing double quote, then hit shift left-arrow for the crash.

with full backtrace

I reproduced the error on WSL. The fix for my subtraction with overflow error also fixes this one. Unless someone says otherwise I consider this bug fixed with #751.

fdncred · 2024-02-12T17:18:22Z

Thanks @Tastaturtaste. Do you have a nushell branch that I can easily test this in? I'm not quite sure how to reproduce it in reedline. I've tried but can't make it fail.

Tastaturtaste · 2024-02-12T17:20:54Z

I don't have one pushed. Locally I tested with the nushell main branch and set reedshell patch to my git branch. Do you need me to push a nushell branch to test?

fdncred · 2024-02-12T17:37:17Z

Do you need me to push a nushell branch to test?

If it's not too much trouble. It would make it easier for us to test.

Tastaturtaste · 2024-02-12T17:51:18Z

https://github.com/Tastaturtaste/nushell/tree/test-reedline-style-text-fix
Here, have a go. Should I make a quick PR against nushell so you can run CI tests?

fdncred · 2024-02-12T18:04:09Z

Let me try to reproduce the problem and try your branch and see how far that gets. Thanks so much! Gimme a few and I'll ping you back.

fdncred · 2024-02-12T19:03:42Z

Looks like it fixes it to me. Thanks

# Description This fixes a panic with the text selection. Reference nushell/reedline#751 nushell/reedline#750 nushell/reedline#689 (comment) # User-Facing Changes  # Tests + Formatting  # After Submitting

This PR should close nushell#1171 # Description  This PR introduces the capability to select text using the existing move.. `EditCommand`s of `reedline`. Those commands are extended with an optional parameter specifying if text should be selected while navigating. This enables a workflow familiar from a wide variety of text editors, where holding `shift` while navigating selects all text between the initial cursor position when pressing `shift` and the current cursor position. Before this PR can be merged the [sibling PR for reedline](nushell/reedline#689) has to land first. # User-Facing Changes ## Additional `EditCommand`s 1. `SelectAll` 2. `CutSelection` 3. `CopySelection` ## New optional parameter on existing `EditCommand`s All `EditCommand`s of `EditType` `MoveCursor` have a new optional parameter named `select` of type `bool`. If this parameter is not set by a user it is treated as false, which corresponds to their behavior up to now. I am relatively new to `nushell` and as such may not know of existing behavior that might change through this PR. However, I believe there should be none. I come to this conclusion because 1. Existing commands are extended only with an *optional* additional parameter, users who currently use these EditCommands keep their existing behavior if they don't use it. 2. A few new commands are introduced which were previously not valid. 3. The default keybindings specified in `default_config.nu` are untouched. # Tests + Formatting Tests for the new optional parameter for the move commands are included to make sure that they truly are optional and an unused optional parameter conforms to the previous behavior.

# Description This fixes a panic with the text selection. Reference nushell/reedline#751 nushell/reedline#750 nushell/reedline#689 (comment) # User-Facing Changes  # Tests + Formatting  # After Submitting

This was referenced Dec 27, 2023

Select with shift-cursor and cut, copy and paste #380

Closed

shift + navigation should highlight nushell/nushell#1171

Closed

fdncred reviewed Dec 27, 2023

View reviewed changes

src/core_editor/editor.rs Outdated Show resolved Hide resolved

amtoine changed the title ~~"Added ability to select and cut text in the input buffer"~~ Add ability to select and cut text in the input buffer Dec 27, 2023

Tastaturtaste force-pushed the selection branch from 3abf24c to 5e1cabd Compare December 28, 2023 20:54

Tastaturtaste marked this pull request as ready for review December 28, 2023 21:12

Tastaturtaste force-pushed the selection branch from 8926787 to e5d1b2d Compare January 6, 2024 18:34

Tastaturtaste force-pushed the selection branch from e5d1b2d to b06ce6a Compare January 13, 2024 14:46

Tastaturtaste mentioned this pull request Jan 14, 2024

Add shift + navigation functionality through reedline nushell/nushell#11535

Merged

fdncred mentioned this pull request Jan 14, 2024

Replace clipboard with copypasta #606

Closed

Tastaturtaste added 5 commits January 15, 2024 19:53

Add ability to select and cut text in the input buffer

546c1cc

Add visual selection effect

6bdaf5a

Add SelectMoveWord<Left/Right> command on Shift + Ctrl + Arrow

ab7dd83

Add ability to delete selection with EditCommands 'Delete' and 'Backs…

4c5aed4

…pace'

Make selection an option on every move EditCommand

e2e751a

fdncred merged commit 2f3eb3e into nushell:main Jan 17, 2024
8 checks passed

Tastaturtaste added a commit to Tastaturtaste/reedline that referenced this pull request Jan 22, 2024

Remove conflicting keybindings added with nushell#689 (comment), to fix

2b1b1c1

nushell/nushell#11600 (comment)

Tastaturtaste mentioned this pull request Jan 22, 2024

Remove conflicting keybindings added with selection feature #715

Merged

fdncred pushed a commit that referenced this pull request Jan 22, 2024

Remove conflicting keybindings added with #689 (comment), to fix nush…

135dd52

…ell/nushell#11600 (comment) (#715)

fdncred mentioned this pull request Feb 9, 2024

Panic when selecting text (assertion failed: self.is_char_boundary(at)) #750

Closed

fdncred mentioned this pull request Feb 12, 2024

update to latest reedline nushell/nushell#11844

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to select and cut text in the input buffer #689

Add ability to select and cut text in the input buffer #689

Tastaturtaste commented Dec 27, 2023

codecov bot commented Dec 27, 2023 •

edited

Loading

fdncred commented Dec 27, 2023

Tastaturtaste commented Dec 27, 2023

fdncred commented Dec 27, 2023

Tastaturtaste commented Dec 27, 2023 •

edited

Loading

fdncred commented Dec 27, 2023

Tastaturtaste commented Jan 6, 2024

fdncred commented Jan 6, 2024 •

edited

Loading

Tastaturtaste commented Jan 6, 2024

fdncred commented Jan 6, 2024

fdncred commented Jan 13, 2024

Tastaturtaste commented Jan 13, 2024 •

edited

Loading

fdncred commented Jan 14, 2024

Tastaturtaste commented Jan 14, 2024

fdncred commented Jan 14, 2024

fdncred commented Jan 17, 2024

fdncred commented Jan 17, 2024

fdncred commented Feb 7, 2024

Tastaturtaste commented Feb 7, 2024

Tastaturtaste commented Feb 8, 2024

fdncred commented Feb 8, 2024 •

edited

Loading

fdncred commented Feb 8, 2024 •

edited

Loading

Tastaturtaste commented Feb 8, 2024 •

edited

Loading

fdncred commented Feb 8, 2024

kubouch commented Feb 8, 2024

fdncred commented Feb 9, 2024

Tastaturtaste commented Feb 12, 2024 •

edited

Loading

Tastaturtaste commented Feb 12, 2024

fdncred commented Feb 12, 2024

Tastaturtaste commented Feb 12, 2024

fdncred commented Feb 12, 2024

Tastaturtaste commented Feb 12, 2024

fdncred commented Feb 12, 2024

fdncred commented Feb 12, 2024

Add ability to select and cut text in the input buffer #689

Add ability to select and cut text in the input buffer #689

Conversation

Tastaturtaste commented Dec 27, 2023

codecov bot commented Dec 27, 2023 • edited Loading

Codecov Report

fdncred commented Dec 27, 2023

Tastaturtaste commented Dec 27, 2023

fdncred commented Dec 27, 2023

Tastaturtaste commented Dec 27, 2023 • edited Loading

fdncred commented Dec 27, 2023

Tastaturtaste commented Jan 6, 2024

fdncred commented Jan 6, 2024 • edited Loading

Tastaturtaste commented Jan 6, 2024

fdncred commented Jan 6, 2024

fdncred commented Jan 13, 2024

Tastaturtaste commented Jan 13, 2024 • edited Loading

fdncred commented Jan 14, 2024

Tastaturtaste commented Jan 14, 2024

fdncred commented Jan 14, 2024

fdncred commented Jan 17, 2024

fdncred commented Jan 17, 2024

fdncred commented Feb 7, 2024

Tastaturtaste commented Feb 7, 2024

Tastaturtaste commented Feb 8, 2024

fdncred commented Feb 8, 2024 • edited Loading

fdncred commented Feb 8, 2024 • edited Loading

Tastaturtaste commented Feb 8, 2024 • edited Loading

fdncred commented Feb 8, 2024

kubouch commented Feb 8, 2024

fdncred commented Feb 9, 2024

Tastaturtaste commented Feb 12, 2024 • edited Loading

Tastaturtaste commented Feb 12, 2024

fdncred commented Feb 12, 2024

Tastaturtaste commented Feb 12, 2024

fdncred commented Feb 12, 2024

Tastaturtaste commented Feb 12, 2024

fdncred commented Feb 12, 2024

fdncred commented Feb 12, 2024

codecov bot commented Dec 27, 2023 •

edited

Loading

Tastaturtaste commented Dec 27, 2023 •

edited

Loading

fdncred commented Jan 6, 2024 •

edited

Loading

Tastaturtaste commented Jan 13, 2024 •

edited

Loading

fdncred commented Feb 8, 2024 •

edited

Loading

fdncred commented Feb 8, 2024 •

edited

Loading

Tastaturtaste commented Feb 8, 2024 •

edited

Loading

Tastaturtaste commented Feb 12, 2024 •

edited

Loading