Better behaviour on resize

[danielsomerfield]

There are still some edge cases I plan to continue to work on, but I have changed the behaviour so that when a resize happens, the editor queries the position. Otherwise it's very hard to know if there is some data above the screen throwing off the calculated offsets or potentially weird control characters messing with the offset.

My plan is to:

• test on more terminals
• figure out why on some terminals, there are some oddities after a CMD-K refresh
• dogfood it a bit to see if I find any other edge cases

[codecov]

Codecov Report

Merging #675 (6f6cb92) into main (37714c9) will increase coverage by 0.08%.
The diff coverage is 0.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #675      +/-   ##
==========================================
+ Coverage   49.16%   49.24%   +0.08%     
==========================================
  Files          46       46              
  Lines        7926     7913      -13     
==========================================
  Hits         3897     3897              
+ Misses       4029     4016      -13     

Files	Coverage Δ
src/painting/prompt_lines.rs	0.00% <0.00%> (ø)
src/painting/painter.rs	19.34% <0.00%> (+0.71%)	⬆️

[fdncred]

Are you testing with nushell or just running one of the demos. I tried it with cargo run --example demo on wezterm and so far it seems better.

[danielsomerfield]

So far just the demos. My plan was to build a nushell release against it and use it for a bit.

[stormasm]

@danielsomerfield thank you for taking a look at this problem. It has been an outstanding issue with reedline for a very long time 😄

if we are able to get this fixed or make more progress on it that would be great !

no one has really taken the time it takes to figure it out...

in order to solve this problem its going to take someone who has some experience with these types of issues in terminals or someone who is just willing to work on it until it is solved.

In either case, we are grateful for you taking the time to pursue it...

[fdncred]

I'm testing with nushell now. If anyone else wants to try, just modify the root Cargo.toml file (in nushell) with this and after saving it do a cargo update -p reedline.

# To use a development version of a dependency please use a global override here
# changing versions in each sub-crate of the workspace is tedious
[patch.crates-io]
reedline = { git = "https://github.com/nushell/reedline.git", rev = "6f1dd80c7eea06dc6a25966557363562308a761a" }
# nu-ansi-term = {git = "https://github.com/nushell/nu-ansi-term.git", branch = "main"}
# uu_cp = { git = "https://github.com/uutils/coreutils.git", branch = "main" }

[danielsomerfield]

I've been using it a bit with term, iterm, kitty, and alacrity and it's better, but there are still some idiosyncrasies. The most obvious is that on screen clear, it doesn't reset the cursor position. I can't seem to find a way to tell when that has occurred. Still looking...

@stormasm - you are welcome. I admit that terminals aren't really my expertise. I'm more of a stream-processing, IOT type of guy, but I'll put in the time and see what I can do.

[danielsomerfield]

Ok. I think I might have it. I need to check the crossterm::cursor::position not only during a resize, but prior to render. It's possible that something external has modified it (like a clear screen). I wonder if we could plumb in an event for cursor position change, then handle it more abstractly (rather than adding a bunch of clumsy checks). It seems like we have to be a little cautious since this is a blocking call that can fail.

I'll do a little investigation on this path and see if I can find a strategic place to check for cursor change that won't drag performance down.

[fdncred]

thanks for the update. one of the concerns we've talked about internally is performance of checking the cursor position all the time. having said that, we could probably lose some performance and it wouldn't matter much.

it would be nice if there was a way to hook into the cursor position change but I'm not sure what that is either. not sure if crossterm has something like this or not.

from my testing, this PR makes it better, but not perfect. but to be clear, we're not looking for perfect. we always just look for incrementally better so keep up the good work!

[stormasm]

@danielsomerfield sounds like you are making progress on a better understanding of how our code works !

it will take some time --- but the fact that someone is looking into it is very positive !!

whatever solution you do come up with will be welcomed by the entire Nushell community

as far as your comment on clearing the screen here is the one anecdote that has plagued my personal situation
on the apple mac terminal since day one when reedline was written...

the (CMD-K) clear screen does not work on a mac terminal --- there is reason for that --- and since the early days of reedline back in the spring of 2021 we have known about it... I used to remember why it didn't work but not at the moment...

So my solution for almost the past three years has been to use a trick @fdncred told me about back then..

He came up with it for me and maybe others who use it...

def cls [] {
    $"(ansi cls)(ansi clsb)(ansi reset)(ansi home)"
}

I know this probably won't help you --- but thought I would just give you more information about the apple mac terminal 😄

[fdncred]

Just to document what's going on in that custom command that storm showed.

• ansi cls = clear_entire_screen, ansi escape \e[2J
• ansi clsb = clear_entire_screen_plus_buffer, ansi escape \e[3J
• ansi reset = reset, ansi escape \e[0m
• ansi home = cursor_home, ansi escape \e[H

So, in the nushell repl, this could also be written as "\e[2J\e[3J\e[0m\e[H". It's less descriptive but does the same thing.

These are also in the spec.

Clear Screen

CSI Ps J  Erase in Display (ED), VT100.
            Ps = 0  ⇒  Erase Below (default).
            Ps = 1  ⇒  Erase Above.
            Ps = 2  ⇒  Erase All.
            Ps = 3  ⇒  Erase Saved Lines, xterm.

Reset

CSI Pm m  Character Attributes (SGR).
            Ps = 0  ⇒  Normal (default), VT100.

Cursor Home

CSI Ps ; Ps H
          Cursor Position [row;column] (default = [1,1]) (CUP).

[stormasm]

@fdncred thank you for adding the details of the cls command.

Others may find it helpful as well to get a better understanding of what it is doing...

[danielsomerfield]

Checked in a new version that checks the position on every refresh. It's pretty heavy handed, but perhaps it's good enough for now. I tried to see if I could do anything clever to prevent it from being called on every keystroke but still not have any weird edge-cases but I don't think I can without some serious surgery which I don't think I'm ready to take on, not knowing the codebase that well yet.

I'm running it with nushell now so I'll see how it goes and report back.

[danielsomerfield]

There are some edge cases that combine resize and clear screen (I think) that still add some extra spaces, but I don't have a clear repro yet. When I stumble across it I'll document and, if possible, fix it.

In the meantime, let me know when we've reached "good enough for now" or whether the performance risk is too great.

[fdncred]

I was looking at another PR and saw this in crossterm's code an wondered if it could help here? This is in crossterm's terminal.rs file.

/// A command that instructs the terminal emulator to begin a synchronized frame.
///
/// # Notes
///
/// * Commands must be executed/queued for execution otherwise they do nothing.
/// * Use [EndSynchronizedUpdate](./struct.EndSynchronizedUpdate.html) command to leave the entered alternate screen.
///
/// When rendering the screen of the terminal, the Emulator usually iterates through each visible grid cell and
/// renders its current state. With applications updating the screen a at higher frequency this can cause tearing.
///
/// This mode attempts to mitigate that.
///
/// When the synchronization mode is enabled following render calls will keep rendering the last rendered state.
/// The terminal Emulator keeps processing incoming text and sequences. When the synchronized update mode is disabled
/// again the renderer may fetch the latest screen buffer state again, effectively avoiding the tearing effect
/// by unintentionally rendering in the middle a of an application screen update.
///
/// # Examples
///
/// ```no_run
/// use std::io::{self, Write};
/// use crossterm::{execute, terminal::{BeginSynchronizedUpdate, EndSynchronizedUpdate}};
///
/// fn main() -> io::Result<()> {
///     execute!(io::stdout(), BeginSynchronizedUpdate)?;
///
///     // Anything performed here will not be rendered until EndSynchronizedUpdate is called.
///
///     execute!(io::stdout(), EndSynchronizedUpdate)?;
///     Ok(())
/// }
/// ```
///
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct BeginSynchronizedUpdate;

impl Command for BeginSynchronizedUpdate {
    fn write_ansi(&self, f: &mut impl fmt::Write) -> fmt::Result {
        f.write_str(csi!("?2026h"))
    }

    #[cfg(windows)]
    fn execute_winapi(&self) -> io::Result<()> {
        Ok(())
    }

    #[cfg(windows)]
    #[inline]
    fn is_ansi_code_supported(&self) -> bool {
        true
    }
}

/// A command that instructs the terminal to end a synchronized frame.
///
/// # Notes
///
/// * Commands must be executed/queued for execution otherwise they do nothing.
/// * Use [BeginSynchronizedUpdate](./struct.BeginSynchronizedUpdate.html) to enter the alternate screen.
///
/// When rendering the screen of the terminal, the Emulator usually iterates through each visible grid cell and
/// renders its current state. With applications updating the screen a at higher frequency this can cause tearing.
///
/// This mode attempts to mitigate that.
///
/// When the synchronization mode is enabled following render calls will keep rendering the last rendered state.
/// The terminal Emulator keeps processing incoming text and sequences. When the synchronized update mode is disabled
/// again the renderer may fetch the latest screen buffer state again, effectively avoiding the tearing effect
/// by unintentionally rendering in the middle a of an application screen update.
///
/// # Examples
///
/// ```no_run
/// use std::io::{self, Write};
/// use crossterm::{execute, terminal::{BeginSynchronizedUpdate, EndSynchronizedUpdate}};
///
/// fn main() -> io::Result<()> {
///     execute!(io::stdout(), BeginSynchronizedUpdate)?;
///
///     // Anything performed here will not be rendered until EndSynchronizedUpdate is called.
///
///     execute!(io::stdout(), EndSynchronizedUpdate)?;
///     Ok(())
/// }
/// ```
///
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct EndSynchronizedUpdate;

impl Command for EndSynchronizedUpdate {
    fn write_ansi(&self, f: &mut impl fmt::Write) -> fmt::Result {
        f.write_str(csi!("?2026l"))
    }

    #[cfg(windows)]
    fn execute_winapi(&self) -> io::Result<()> {
        Ok(())
    }

    #[cfg(windows)]
    #[inline]
    fn is_ansi_code_supported(&self) -> bool {
        true
    }
}

[fdncred]

let's give this a whirl. thanks.

[danielsomerfield]

Sorry for the non-responsiveness. I was out of town for a week. I think this is the right thing to do. I tried the queued-command approach first and it didn't seem to change the behaviour. I think it might be the right approach long term, but I believe would require a bit more of an invasive change. Using the command created a lot more complexity and the call to get position was still running on every keystroke anyway, which I believe means we'd need to do something to ensure that future calls were dropped if one was already queued up and there was nothing ahead of it in the queue that could alter it.

I've been running with this code for a while and it seems good on an 8 core intel mac. I can start looking into performance enhancements if that seems necessary for machines with less horsepower.

[fdncred]

ok, thanks for the update. I just pushed a nushell pr to allow us to dogfood this in the latest main branch there.

also, i've recently noticed that we're drawing the prompts on every keystroke, which seems very excessive. i know this is unrelated to your PR but i'm wondering if you've noticed this as well?

[danielsomerfield]

Yes, I did see that. I think that's related to why the queued commands didn't work any better than the brute force approach. I can look into it if it would be helpful.

[fdncred]

I can look into it if it would be helpful.

don't make me beg, because i will. 🤣 please look into it.

[rgwood]

Thanks for working on this @danielsomerfield and thank you for reviewing Darren. Nushell's had issues with resizing for quite a while and it's great to see this getting better.

[amtoine]

note that this PR appears to have introduced

• Nushell reset prompt_start_row to 0 when opening a file without newline nushell#11399

[danielsomerfield]

You are welcome. There are definitely some quirks still, including some potentially new ones. Two steps forward, one back. Happy to continue looking at new oddities as they come in.