Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly handle optional event modes #659

Merged
merged 11 commits into from
Nov 14, 2023

Conversation

sholderbach
Copy link
Member

Both bracketed paste and the kitty keyboard enhancement flag are not always supported.
This goes both for terminals that may not emit events according to the extensions thus we already provide the means to only optionally enable them.
But also the applications that may run between each Reedline::read_line call are in no way guaranteed to supported any enhanced terminal events coming in.
It should thus be part of reedline's contract to leave read_line in a clean state with the canonical terminal behavior.

The API for Reedline::enable_bracketed_paste was misleading as it for the most part acted independent of the Reedline state and immediately globally enabled bracketed paste, potentially polluting any application during evaluation. disable_bracketed_paste equally globally modified state but was only active if the bracketed paste state was entered with enable_bracketed_paste and would only run once.

For better readability transfer this pattern to the kitty keyboard enhancement protocol. (which was already properly handled but was overly eager to check if the enhancements are supported on every read_line)

Make sure we enter the mode when starting to take control and exit the mode when leaving the mode or being dropped.
The settings exposed to the user will just internally enable a setting (in the case of the kitty keyboard enhancement flags crossterm provides an easy way to check for support that we take into account with that)

Close nushell/nushell#10982 and related issues for non-nushell projects using reedline

As a follow up we can simplify the setters to ignore the Result or collapse it to a simpler fn use_...(self, enable: bool)

Both bracketed paste and the kitty keyboard enhancement flag are not
always supported.
This goes both for terminals that may not emit events according to their
spec and also applications that will run in the terminal independent of
reedline.

Make sure we enter the mode when starting to take control and exit the
mode when leaving the mode or being dropped.
The settings exposed to the user will just internally enable a setting
(in the case of the kitty keyboard enhancement flags crossterm provides
and easy way to check for support that we take into account with that)
Copy link

codecov bot commented Nov 8, 2023

Codecov Report

Merging #659 (cde7c89) into main (c853d71) will decrease coverage by 0.04%.
Report is 3 commits behind head on main.
The diff coverage is 19.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #659      +/-   ##
==========================================
- Coverage   49.22%   49.19%   -0.04%     
==========================================
  Files          43       46       +3     
  Lines        7912     7930      +18     
==========================================
+ Hits         3895     3901       +6     
- Misses       4017     4029      +12     
Files Coverage Δ
src/terminal_extensions/mod.rs 0.00% <0.00%> (ø)
src/engine.rs 4.99% <16.66%> (-0.16%) ⬇️
src/terminal_extensions/bracketed_paste.rs 23.80% <23.80%> (ø)
src/terminal_extensions/kitty.rs 18.51% <18.51%> (ø)

... and 1 file with indirect coverage changes

@fdncred
Copy link
Collaborator

fdncred commented Nov 8, 2023

Nice work @sholderbach!!

@fdncred
Copy link
Collaborator

fdncred commented Nov 12, 2023

are we going to land this before the release or after?

Public as `reedline::kitty_protocol_available`

Replace the method `Reedline::can_use_kitty_protocol` that doesn't
depend on `Reedline` in the future

Rewrite `KittyProtocolGuard` in terms of it.
As we internally check whether it is available, this should be fine in
nearly all terminals (if they can safely be probed)
sholderbach added a commit to sholderbach/nushell that referenced this pull request Nov 13, 2023
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
@sholderbach sholderbach merged commit 8792726 into nushell:main Nov 14, 2023
8 checks passed
@sholderbach sholderbach deleted the enhancement-handling branch November 14, 2023 18:53
sholderbach added a commit to nushell/nushell that referenced this pull request Nov 14, 2023
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
hardfau1t pushed a commit to hardfau1t/nushell that referenced this pull request Dec 14, 2023
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
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this pull request Feb 20, 2024
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
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.

Pasting with bracketed_paste creates escape codes that affect some programs
2 participants