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

confirm: properly clear the terminal in case of a multi-line prompt string (take 2) #175

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

grunweg
Copy link
Contributor

@grunweg grunweg commented Feb 14, 2022

This a revised version of #166, see discussion there.
Fixes #165.

pub fn clear(&mut self) -> io::Result<()> {
// We must clear the current line first: `clear_last_lines` places the cursor on the first deleted line,
// whereas we want to have it on the current line.
self.term.clear_line()?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm... And this didn't effect the other prompts from having issues? I thought the next statement was the one which clears everything.

Copy link
Contributor Author

@grunweg grunweg Feb 17, 2022

Choose a reason for hiding this comment

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

Well, clear_last_lines() only clears the line before the current line. I would expect the current line to also be cleared, so this seems like an oversight to me. I just pushed a commit clarifying this a bit.

TBH, I'm a bit surprised the other prompts were not hit by this. I haven't checked why... but a quick spot-check with the selection and multi-selection prompt shows no issues.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Which is why i think the issue is actually in rendering the prompts themselves. Looks like the pattern for rendering other prompts is different to rendering the confirm prompt.

This was a simple oversight in the code.
Make sure to preserve the cursor at the beginning of the current line.
…input prompt.

With TermThemeRenderer.clear() also clearing the current line, they have
become superfluous.
@grunweg
Copy link
Contributor Author

grunweg commented May 6, 2022

I just squashed changes and reworded the commit message. Basically, I think this fixes two bugs/defects:

  • the confirmation prompt should clear the entire rendering, not just the current line
  • TermThemeRenderer::clear() should always clear the current line: currently, anything on the current line is left behind. In my opinion, that's simply an oversight.

Why did none of the other prompts notice this before?

  • the confirmation prompt was calling term.clear_line() (not TermThemeRenderer::clear()) --- hence didn't notice
  • the input prompt calls clear_line() right before calling clear() :-)
  • the password prompt asks for input, which is ended by a newline --> there is automatically a line break, and the new line is empty
  • the only prompts using TermThemeRenderer::write_formatted_string to render the prompt are input, password and confirm. All other prompts either use write_formatted_line, or write_formatted_prompt which calls write_formatted_line: both add a newline after the text that was written.

@grunweg
Copy link
Contributor Author

grunweg commented May 6, 2022

the input prompt calls clear_line() right before calling clear() :-)

I just pushed an additional commit which removes these now-superfluous calls.

@pksunkara pksunkara added this to the 0.12.0 milestone Sep 21, 2023
@WillLillis
Copy link

Just ran into this issue while testing a new feature for tree-sitter. Any chance this PR could be revived? Would be happy to open a rebased one and work through any necessary changes :)

@grunweg
Copy link
Contributor Author

grunweg commented Oct 1, 2024

I'm happy to rebase this PR. That said, my impression was that

  • there is no maintainer with much time to dedicate to this project
    (To be clear: I am not expecting differently; this is open source. It is totally fine if people have other projects, day jobs, care obligations or simply a life beyond open source.)
  • there is no good way to automatically test these changes: apparently, there's no such Rust infrastructure yet. In other words, it's hard to be confident that this change is correct, I doesn't regress other use cases, and later changes won't regress this bug fix. AIUI, this is the main reason this (and a handful of other PRs) have stalled.
  • there was talk of rewriting dialoguer using crossterm instead of console-rs, which would make this PR moot. I don't think that rewrite actually happened, though.

You're always free to use the patch section of your lockfile to fix just this bug locally. (Let me know if you need help applying this fix on the current master branch.)

@WillLillis
Copy link

Makes sense, thanks so much for the response and all the info!

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.

Confirmation prompt: non-last line of multiline strings get printed twice
3 participants