disable bracketed paste during the execution of a child #10998

LevitatingBusinessMan · 2023-11-08T14:53:43Z

I invite @WindSoilder, @fdncred and @theAkito to test this on their respective platforms.

I can't say I completely understand nushells cli code, but based on @WindSoilder I assumed eval_source was responsible for executing the child program.

This PR disables bracketed paste during the execution of children for reasons I explained in #10982.

To test:

1. cargo update
2. cargo run
3. cat
4. paste something inside

With this all bracketed paste issues should be fixed.

fdncred · 2023-11-08T14:58:35Z

crates/nu-cli/src/repl.rs

                    if engine_state.get_config().bracketed_paste {
-                        #[cfg(not(target_os = "windows"))]


This should probably stay because I don't believe bracketed paste is supported on Windows yet. IIRC, there was a crossterm issue about it, unless you're saying that's been fixed?

This should probably stay because I don't believe bracketed paste is supported on Windows yet. IIRC, there was a crossterm issue about it, unless you're saying that's been fixed?

I haven't looked into bracketed_paste much from the crossterm side. However it should be totally dependent on the terminal, not on the OS itself. And we can't assume that a windows user is using any specific terminal.

This is why I asked for multiple people to test it on multiple platforms, to see if something breaks.

I don't like the solution of giving Windows users a configuration setting that is silently ignored either. Either they can or they can't configure bracketed paste.

If bracketed paste isn't supported in a certain terminal (some unix terminals might not support it either) at its worst the control characters are probably just ignored. So there should be no harm in having it enabled on a terminal that does not support it.

According to crossterm-rs/crossterm#737, bracketed paste does work in WizTerm on Windows.

I'm not super sure how to test this to ensure my buffer has the appropriate escape sequences when I paste them. But these seem to work on Windows Terminal and WezTerm.

❯ $"(ansi csi)200~this is a test(ansi csi)201~" this is a test ❯ $"(ansi csi)?2004h(ansi csi)200~this is a test(ansi csi)201~(ansi csi)?2004l" this is a test

fdncred · 2023-11-08T15:36:14Z

It would also be good to have reedline updated in this PR since it's required, I think, right?

LevitatingBusinessMan · 2023-11-08T16:10:56Z

It would also be good to have reedline updated in this PR since it's required, I think, right?

This shouldn't have negative effects on the reedline crate before the patch. If you want to bump the reedline version in nushell you'll have to release a new reedline version. 0.25.1 or 0.26.0

fdncred · 2023-11-08T16:54:51Z

I'm saying that we should be testing with the latest version of reedline so cargo update -p reedline needs to be done somewhere in some PR and this seems like as good of one as any since it's related to the most recent changes in reedline.

LevitatingBusinessMan · 2023-11-08T18:43:22Z

I'm saying that we should be testing with the latest version of reedline so cargo update -p reedline needs to be done somewhere in some PR and this seems like as good of one as any since it's related to the most recent changes in reedline.

But reedline is still on version 0.25.0, and only the development build is configured to build reedline from git. (Edit: It's not)

Are you saying I should commit the updated Cargo.~~toml~~lock referencing the latest git commit for reedline?

WindSoilder · 2023-11-09T06:53:14Z

Are you saying I should commit the updated Cargo.toml referencing the latest git commit for reedline?

Actually you don't need to, I found that it already use latest git commit of reedline

LevitatingBusinessMan · 2023-11-10T00:04:14Z

Are you saying I should commit the updated Cargo.toml referencing the latest git commit for reedline?

Actually you don't need to, I found that it already use latest git commit of reedline

I think it uses whatever version is specified in the lockfile.

I compiled nushell from a fresh clone, and it still had nushell/reedline#648, the bug where bracketed_paste only works the first time because it gets disabled after the first time. Then I ran cargo update -p reedline updating the used commit:

    Updating reedline v0.25.0 (https://github.com/nushell/reedline.git?branch=main#973dbb5f) -> #c853d71e

I was wrong thinking that the patch.crates-io only applied to debug builds, it also applies to the release build. So @fdncred do you want me to commit the updated Cargo.lock to the PR?

LevitatingBusinessMan · 2023-11-10T00:22:36Z

I updated the commit hash, but I really think it would be better if reedline would just release regular patch version bumps.

fdncred · 2023-11-10T01:00:28Z

Then I ran cargo update -p reedline updating the used commit ...

This is why I asked you to do it the other day. Glad you figured it out.

I think at this point I'd like to see what @sholderbach's PR reveals nushell/reedline#659 and how it intersects with this PR.

Go from the ill-defined `enable/disable` pairs to `.use_...` builders This alleviates unclear properties when the underlying enhancements are enabled. Now they are enabed when entering `Reedline::read_line` and disabled when exiting that. Furthermore allow setting `$env.config.use_kitty_protocol` to have an effect when toggling during runtime. Previously it was only enabled when receiving a value from `config.nu`. I kept the warning code there to not pollute the log. We could move it into the REPL-loop if desired Not sure if we should actively block the enabling of `bracketed_paste` on Windows. Need to test what happens if it just doesn't do anything we could remove the `cfg!` switch. At least for WSL2 Windows Terminal already supports bracketed paste. `target_os = windows` is a bad predictor for `conhost.exe`. Depends on nushell/reedline#659 (pointing to personal fork) Closes nushell#10982 Supersedes nushell#10998

Go from the ill-defined `enable/disable` pairs to `.use_...` builders This alleviates unclear properties when the underlying enhancements are enabled. Now they are enabed when entering `Reedline::read_line` and disabled when exiting that. Furthermore allow setting `$env.config.use_kitty_protocol` to have an effect when toggling during runtime. Previously it was only enabled when receiving a value from `config.nu`. I kept the warning code there to not pollute the log. We could move it into the REPL-loop if desired Not sure if we should actively block the enabling of `bracketed_paste` on Windows. Need to test what happens if it just doesn't do anything we could remove the `cfg!` switch. At least for WSL2 Windows Terminal already supports bracketed paste. `target_os = windows` is a bad predictor for `conhost.exe`. Depends on nushell/reedline#659 (pointing to personal fork) Closes #10982 Supersedes #10998

sholderbach · 2023-11-15T15:11:47Z

Thanks for the PR @LevitatingBusinessMan

superseded by #11045

Go from the ill-defined `enable/disable` pairs to `.use_...` builders This alleviates unclear properties when the underlying enhancements are enabled. Now they are enabed when entering `Reedline::read_line` and disabled when exiting that. Furthermore allow setting `$env.config.use_kitty_protocol` to have an effect when toggling during runtime. Previously it was only enabled when receiving a value from `config.nu`. I kept the warning code there to not pollute the log. We could move it into the REPL-loop if desired Not sure if we should actively block the enabling of `bracketed_paste` on Windows. Need to test what happens if it just doesn't do anything we could remove the `cfg!` switch. At least for WSL2 Windows Terminal already supports bracketed paste. `target_os = windows` is a bad predictor for `conhost.exe`. Depends on nushell/reedline#659 (pointing to personal fork) Closes nushell#10982 Supersedes nushell#10998

disable bracketed paste during the execution of a child

6819996

fdncred reviewed Nov 8, 2023

View reviewed changes

update the git commit of reedline

bc62c35

sholderbach mentioned this pull request Nov 13, 2023

Move to clearer reedline keyboard enhancement API #11045

Merged

sholderbach mentioned this pull request Nov 14, 2023

Interactive MSVC Nushell doesn't work under Cygwin Mintty with disable_pcon (e.g. Git Bash) #10808

Open

sholderbach closed this Nov 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

disable bracketed paste during the execution of a child #10998

disable bracketed paste during the execution of a child #10998

LevitatingBusinessMan commented Nov 8, 2023

fdncred Nov 8, 2023

LevitatingBusinessMan Nov 8, 2023 •

edited

LevitatingBusinessMan Nov 8, 2023 •

edited

fdncred Nov 8, 2023

fdncred commented Nov 8, 2023

LevitatingBusinessMan commented Nov 8, 2023

fdncred commented Nov 8, 2023

LevitatingBusinessMan commented Nov 8, 2023 •

edited

WindSoilder commented Nov 9, 2023

LevitatingBusinessMan commented Nov 10, 2023

LevitatingBusinessMan commented Nov 10, 2023

fdncred commented Nov 10, 2023

sholderbach commented Nov 15, 2023

		if engine_state.get_config().bracketed_paste {
		#[cfg(not(target_os = "windows"))]

disable bracketed paste during the execution of a child #10998

disable bracketed paste during the execution of a child #10998

Conversation

LevitatingBusinessMan commented Nov 8, 2023

fdncred Nov 8, 2023

Choose a reason for hiding this comment

LevitatingBusinessMan Nov 8, 2023 • edited

Choose a reason for hiding this comment

LevitatingBusinessMan Nov 8, 2023 • edited

Choose a reason for hiding this comment

fdncred Nov 8, 2023

Choose a reason for hiding this comment

fdncred commented Nov 8, 2023

LevitatingBusinessMan commented Nov 8, 2023

fdncred commented Nov 8, 2023

LevitatingBusinessMan commented Nov 8, 2023 • edited

WindSoilder commented Nov 9, 2023

LevitatingBusinessMan commented Nov 10, 2023

LevitatingBusinessMan commented Nov 10, 2023

fdncred commented Nov 10, 2023

sholderbach commented Nov 15, 2023

LevitatingBusinessMan Nov 8, 2023 •

edited

LevitatingBusinessMan Nov 8, 2023 •

edited

LevitatingBusinessMan commented Nov 8, 2023 •

edited