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

Replace crossterm ScrollUp with universal workaround #601

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

NotLebedev
Copy link
Contributor

Replace scrolling via escape sequence with workaround that prints newlines to retain scrollback regardless of terminal implementation of scroll sequence. See nushell/9166 for discussion and related info.

This PR replaces using ScrollUp sequence "\[e<num>S" with printing desired amount of blank lines at the last line of terminal window. This workaround functions on all terminals that have scrollback buffer regardless of their ability to handle and implementation of ScrollUp sequence.

Replace scrolling via escape sequence with workaround that prints newlines to retain scrollback
regardless of terminal implementation of scroll sequence
Copy link
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for resolving this!

This sounds reasonable and your helper is amazingly documented.

@NotLebedev
Copy link
Contributor Author

Also anybody interested can test this changes by compiling nushell against this revision of reedline and running two following nushell scripts:

  1. $env.PROMPT_COMMAND = {|| "\n>\n>" }; clear; for i in (term size | get rows | 1..$in) {print $i} It will clear screen. Fill it with numbers and then scroll two lines. Scroll your terminall up a bit, all lines should be there. Before fix it looked like this on my windows terminal:
    image
    after change all numbers are present:
    image
  2. $env.PROMPT_COMMAND = {|| ">" }; clear; for i in (term size | get rows | 1..($in - 5)) {print $i} and then press F1 or ctrl + R (or invoke any menu with your preferred shortcut) This will scroll terminal several lines up. Difference is that this time scroll is initiated when cursor is not at the bottom of window, but it still should work. Before fix i could not scroll up at all (notice absence of scroll bar on right) because nothing was moved into scrollback buffer by ScrollUp:
    image
    after change all numbers are available and I can scroll up:
    image

@sholderbach sholderbach merged commit 4f31e20 into nushell:main Jul 12, 2023
6 checks passed
@fdncred
Copy link
Collaborator

fdncred commented Jul 12, 2023

Love it! Nice work @NotLebedev!

@fdncred
Copy link
Collaborator

fdncred commented Jul 12, 2023

@NotLebedev I'm seeing missing lines in Windows Terminal but seemingly only when maximized.

❯ detect columns --help

produces this problem on the first example.

Examples:
  Splits string across multiple columns
  > 'a b c' | detect columns -n
  │ # │ column0 │ column1 │ column2 │
  ├───┼─────────┼─────────┼─────────┤
  │ 0 │ a       │ b       │ c       │
  ╰───┴─────────┴─────────┴─────────╯

The top of the table is cutoff and it's the very first line.
image

Then I scroll up and see it cut off
image

@NotLebedev
Copy link
Contributor Author

@NotLebedev I'm seeing missing lines in Windows Terminal but seemingly only when maximized.

Is master branch (or your branch) of nushell using this patch? It seems like at least master does not. Here in Cargo.lock https://github.com/nushell/nushell/blob/545697c0b20b58f13b8dfbbefa700e2dbd08d864/Cargo.lock#L4215 revision is cf841 and this change is one commit later on 4f31e

@fdncred
Copy link
Collaborator

fdncred commented Jul 12, 2023

rebuilding on latest main to make sure. stay tuned.

@fdncred
Copy link
Collaborator

fdncred commented Jul 12, 2023

I think you're right @NotLebedev. I got the latest main, checkout the detect columns pr, merged main, then did cargo update -p reedline and now my reedline is on 4f31e. I missed doing the cargo update last time so my mistake. It's working great now. Sorry for the false alarm!

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.

3 participants