cleanup osc calls for shell_integration#12810
Merged
fdncred merged 2 commits intonushell:mainfrom May 8, 2024
Merged
Conversation
Contributor
|
As you noted, after this PR (not saying that's an issue with your PR, it's likely a WezTerm one, just writing it down for future investigation) |
Contributor
Author
Agreed. To say here what I've said in other places. I think this problem with WezTerm is related to how reedline redraws the entire prompt with every character typed in the repl. I'm not sure why. I've looked and haven't found it yet. It could also be a bug in WezTerm but I kind of doubt that after seeing how many escapes we send. The good news at least is that now you can turn off osc133 and still have the rest of the ansi escapes. Prior to these two PRs you weren't able to do that. |
FilipAndersson245
pushed a commit
to FilipAndersson245/nushell
that referenced
this pull request
May 18, 2024
# Description This PR is a continuation of nushell#12629 and meant to address [Reilly's stated issue](nushell#12629 (comment)). With this PR, nushell should work more consistently with WezTerm on Windows. However, that means continued scrolling with typing if osc133 is enabled. If it's possible to run WezTerm inside of vscode, then having osc633 enabled will also cause the display to scroll with every character typed. I think the cause of this is that reedline paints the entire prompt on each character typed. We need to figure out how to fix that, but that's in reedline. For my purposes, I keep osc133 and osc633 set to true and don't use WezTerm on Windows. Thanks @rgwood for reporting the issue. I found several logic errors. It's often good to come back to PRs and look at them with fresh eyes. I think this is pretty close to logically correct now. However, I'm approaching burn out on ansi escape codes so i could've missed something. Kudos to [escape-artist](https://github.com/rgwood/escape-artist) for helping me debug an ansi escape codes that are actually being sent to the terminal. It was an invaluable tool. # User-Facing Changes <!-- List of all changes that impact the user experience here. This helps us keep track of breaking changes. --> # Tests + Formatting <!-- Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass (on Windows make sure to [enable developer mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging)) - `cargo run -- -c "use std testing; testing run-tests --path crates/nu-std"` to run the tests for the standard library > **Note** > from `nushell` you can also use the `toolkit` as follows > ```bash > use toolkit.nu # or use an `env_change` hook to activate it automatically > toolkit check pr > ``` --> # After Submitting <!-- If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date. -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR is a continuation of #12629 and meant to address Reilly's stated issue.
With this PR, nushell should work more consistently with WezTerm on Windows. However, that means continued scrolling with typing if osc133 is enabled. If it's possible to run WezTerm inside of vscode, then having osc633 enabled will also cause the display to scroll with every character typed. I think the cause of this is that reedline paints the entire prompt on each character typed. We need to figure out how to fix that, but that's in reedline.
For my purposes, I keep osc133 and osc633 set to true and don't use WezTerm on Windows.
Thanks @rgwood for reporting the issue. I found several logic errors. It's often good to come back to PRs and look at them with fresh eyes. I think this is pretty close to logically correct now. However, I'm approaching burn out on ansi escape codes so i could've missed something.
Kudos to escape-artist for helping me debug an ansi escape codes that are actually being sent to the terminal. It was an invaluable tool.
User-Facing Changes
Tests + Formatting
After Submitting