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

clear_screen to fit in Warp's manner #811

Closed
tisonkun opened this issue Aug 5, 2024 · 12 comments · Fixed by #813
Closed

clear_screen to fit in Warp's manner #811

tisonkun opened this issue Aug 5, 2024 · 12 comments · Fixed by #813
Labels
enhancement New feature or request

Comments

@tisonkun
Copy link
Contributor

tisonkun commented Aug 5, 2024

Warp always has an upper block for the last command:

image

Currently, Reedline's clear_screen will print empty lines to move the current insert point to the most upper position, which is hidden by the block quoted.

I don't know how mysql can "clear screen" (ctrl+l) properly as shown above, but this can be a compatibility request.

@tisonkun tisonkun added the enhancement New feature or request label Aug 5, 2024
@tisonkun
Copy link
Contributor Author

tisonkun commented Aug 5, 2024

rustyline's implementation uses self.write_and_flush("\x1b[H\x1b[J") and it seems work.

@fdncred
Copy link
Collaborator

fdncred commented Aug 5, 2024

you can try that out with keybindings to see if that makes a difference with $"(ansi cursor_home)(ansi clear_screen)"

@tisonkun
Copy link
Contributor Author

tisonkun commented Aug 5, 2024

@fdncred I saw the custom keybindings right now but I'm wondering what to put here ...

    let mut keybindings = reedline::default_emacs_keybindings();
    keybindings.add_binding(
        reedline::KeyModifiers::CONTROL,
        reedline::KeyCode::Char('l'),
        todo!("not yet find the effect alternative clear_screen impl"),
    );

    let mut state = Reedline::create()
        .with_validator(Box::new(..))
        .with_highlighter(Box::new(..))
        .with_edit_mode(Box::new(Emacs::new(keybindings)));

@tisonkun
Copy link
Contributor Author

tisonkun commented Aug 5, 2024

Perhaps EditCommand, but I don't interact with the buffer, I'd talk to the painter.

@fdncred
Copy link
Collaborator

fdncred commented Aug 5, 2024

I was giving you nushell commands to try, sorry.

It looks like the reedline clear_screen function isn't quite working like rustyline's.

    pub(crate) fn clear_screen(&mut self) -> Result<()> {
        self.stdout.queue(cursor::Hide)?;
        let (_, num_lines) = terminal::size()?;
        for _ in 0..2 * num_lines {
            self.stdout.queue(Print("\n"))?;
        }
        self.stdout.queue(MoveTo(0, 0))?; # This is go to home
        self.stdout.queue(cursor::Show)?;

        self.stdout.flush()?;
        self.initialize_prompt_position()
    }

Seems like it might should be closer to the clear_scrollback function but without the purge.

    pub(crate) fn clear_scrollback(&mut self) -> Result<()> {
        self.stdout
            .queue(crossterm::terminal::Clear(ClearType::All))?
            .queue(crossterm::terminal::Clear(ClearType::Purge))?
            .queue(cursor::MoveTo(0, 0))?
            .flush()?;
        self.initialize_prompt_position()
    }

@fdncred
Copy link
Collaborator

fdncred commented Aug 5, 2024

To mimic rustline you'd have to use MoveTo(0, 0) and then ClearType::FromCursorDown. You could play around and make your own function to see what happens. Both of these clear functions are in the painter.

One last thing related to keybindings. You could try ReedlineEvent::ExecuteHostCommand(string) with some derivative of "\x1b[H\x1b[J" that the terminal understands like in nushell it would be "\e[H\e[J" or $"(ansi cursor_home)(ansi clear_screen)". I'm guessing your best bet is to fix the clear_screen function or add another one as suggested above.

@tisonkun
Copy link
Contributor Author

tisonkun commented Aug 5, 2024

to fix the clear_screen function or add another one as suggested above

This should happen within reedline (so a PR). Does it what you mean ;)?

@fdncred
Copy link
Collaborator

fdncred commented Aug 5, 2024

to fix the clear_screen function or add another one as suggested above

This should happen within reedline (so a PR). Does it what you mean ;)?

I mean, you make the changes in your cloned reedline repo and see how it works. If it works well, then you can think about a PR for reedline.

@tisonkun
Copy link
Contributor Author

tisonkun commented Aug 5, 2024

    pub(crate) fn clear_screen(&mut self) -> Result<()> {
        self.stdout.queue(cursor::Hide)?;
        self.stdout.queue(Clear(ClearType::All))?;
        self.stdout.queue(MoveTo(0, 0))?;
        self.stdout.queue(cursor::Show)?;

        self.stdout.flush()?;
        self.initialize_prompt_position(None)
    }

This change set looks work well (for both Warp and MacOS's Terminal, I don't test other terminals since I don't have one). I'll prepare a patch later after #812 merged.

It's 11 PM now so perhaps I'll resume this task tomorrow :D

@fdncred
Copy link
Collaborator

fdncred commented Aug 5, 2024

If you want it to work like rustyline, ClearType::All is the wrong enum. It should be ClearType::FromCursorDown. Also MoveTo(0,0) would need to be first to be like rustyline.

@sholderbach
Copy link
Member

History time: that we print those newlines is down to my second PR ever to reedline in #30, basically the goal was to replicate the Ctrl-L behavior from bash in a somewhat faithful manner (#28).
^L is equivalent to form-feed in ASCII so should not delete any existing content but only move that off the screen and redraw the prompt at the top of the visible area.

Not sure if \e[2J or \e[J is the right one I have seen mentions of both.

Our ClearScreen should in my view do the same as the bash clear-screen/Ctrl-L and not be the same as clear-display

@tisonkun
Copy link
Contributor Author

tisonkun commented Aug 5, 2024

@sholderbach Thanks for the background!

I typically have a few options to start a PR for discussions:

  1. Replace clear_screen entirely, which may cause compatibility issues with all historical considerations.
  2. Switch the manner with a feature flag, but it looks like random changes.
  3. Add a new ReedlineEvent perhaps called ClearDisplay and allow users to rebind the keybindings as I try above (we just cannot directly talk to the painter).
  4. Try to determine the terminal type and use a proper implementation.

I'll send a PR first adopt 3 and see if we can reach 4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants