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

fix: prompt position on resize #578

Merged
merged 6 commits into from
May 24, 2023
Merged

fix: prompt position on resize #578

merged 6 commits into from
May 24, 2023

Conversation

Banyc
Copy link
Contributor

@Banyc Banyc commented Apr 29, 2023

Fix: nushell/nushell#9015

Previously, when the terminal window shrinks its height, the prompt eats the previous line.
Now, the prompt will not overwrite the previous line.

@Banyc
Copy link
Contributor Author

Banyc commented Apr 29, 2023

I haven't checked the u16 overflow and the UX_TESTING yet because it's time to go to bed here so I will do the checks and tests tomorrow.

@sholderbach
Copy link
Member

Thanks so much for giving this a shot, there have been many futile attempt, would be great if you can resolve that!

For UX testing, interesting edge cases are:

Resize related

What happens below the prompt

  • buffer contains multiple lines due to wrapping text
  • triggering the completion menu

Possible artifacts due to assumption reedline made from its development with a relatively consistent prompt.

  • (different prompts (there might be some assumptions left from our initial set of prompts))

Cursor position behavior is some what dependent on the scrollback of the terminal emulator

  • initially opened terminal with out scrolling
  • at the bottom of the screen where the scroll buffer will expand.

fix: resume the early return to exclude a case where the terminal got smaller but the prompt is still visible
@Banyc
Copy link
Contributor Author

Banyc commented Apr 30, 2023

After a bunch of random tests, I found out there is a trade off between not overwriting any previous output and producing fewer incomplete duplicate prompts on window resizing. I prefer the former since:

  • we can just ignore the previous incomplete prompt fragments;
  • the previous output is very precious.

So I decided to just set the prompt_start_row to the last line every time the terminal window resizes for now. After the change, #42 and nushell/nushell#9015 are no longer problems. However, #408, #528, and #157 will still be there. Then we take a look at how the other shells deal with this problem and come up with an incremental solution.

@Banyc
Copy link
Contributor Author

Banyc commented Apr 30, 2023

Basically every time the window changes in size, it's expected to see incomplete duplicate prompt fragments. The main focus of the checklist expects no outputs being overwritten by the prompt.

Thanks for the checklist so that I found a bug on horizontal resizing just now (and fixed)!

Checklist

Resize related

What happens below the prompt

  • buffer contains multiple lines due to wrapping text: ok with example/demo
  • triggering the completion menu: ok with example/completions

Possible artifacts due to assumption reedline made from its development with a relatively consistent prompt.

  • (different prompts (there might be some assumptions left from our initial set of prompts))
    • example/demo: ok
    • example/custom_prompt: ok but no completions

Cursor position behavior is some what dependent on the scrollback of the terminal emulator

  • initially opened terminal without scrolling: the cursor is at the right position
  • at the bottom of the screen where the scroll buffer will expand: no sure how to operate on this test

@Banyc Banyc marked this pull request as ready for review April 30, 2023 14:24
@sholderbach
Copy link
Member

I will have to put aside a bit of time for playing around but your suggestions sound good and the items on the list are promising!


I found out there is a trade off between not overwriting any previous output and producing fewer incomplete duplicate prompts on window resizing. I prefer the former since:

* we can just ignore the previous incomplete prompt fragments;

* the previous output is very precious.

Fully agree on your preferences here! Keeping the potentially valuable output to me would be higher priority than dealing fully gracefully with the rarer resizing. (I know that resizing can also be quite frequent if you change a tiling layout in desktop or terminal multiplexer...)


Regarding your question on the test suggestions:

at the bottom of the screen where the scroll buffer will expand: no sure how to operate on this test

I was thinking about pressing enter often enough that the prompt is at the last line of the screen to fit the buffer or multiline buffer there. The screen shouldn't start flowing over with content because text wrapping is not correclty accounted for in the prompt positioning.

@sophiajt
Copy link
Contributor

I played around with the demo a bit, and it looked pretty good. I think we could land and dogfood in nightly nushell to see how it works in practice.

Thanks for the PR!

@sophiajt sophiajt merged commit bd30694 into nushell:main May 24, 2023
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.

second last line overlapping
3 participants