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.

Sign up for GitHub

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 #166

Closed
wants to merge 1 commit into from
Closed

confirm: properly clear the terminal in case of a multi-line prompt string #166

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/prompts/confirm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,8 +210,10 @@ impl Confirm<'_> {
continue;
}
};

// `prompt` may be a multi-line string, hence we may need to clear more than the current line.
// `clear_last_lines` clears the n lines *before* the current line.
term.clear_line()?;
term.clear_last_lines(self.prompt.lines().count() - 1)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please convert this change to all the prompt types?

Copy link
Contributor Author

@grunweg grunweg Feb 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the feedback! I'm happy to, but I don't see how that makes sense.

I looked at all the other prompts, and and all of them support multi-line prompt strings without issue. This bug only manifests because when trying to clear the current prompt. Confirmation prompts do this, but none of the other prompts do. (The selection prompts, for example, do need to clear multiple lines, but don't need to clear the prompt.)

More specifically:

  • the only other prompt calling the clear_line method is input.rs; that part of code is not clearing a prompt.
  • clear_last_lines is used a couple of times, but always to reset the whole terminal (such as clear_last_lines(paging.capacity)?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that this logic feels not really specific to confirmation prompts. I'm not sure where to extract it to, though. Can you point me to a better place? Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Extraction is not what I had in mind. Just copying to other prompts is enough. But as you said, looks like the other prompts don't have this bug. I will need to investigate.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Investigated and looks like yes, bug only happens in this prompt. But instead of doing it like this, can you please replace term.clear_line() with render.clear() and see if that satisfies for you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, and the bug is not specific to the combination of "report=true" and "wait_for_newline=false".

Copy link
Collaborator

@pksunkara pksunkara Feb 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like the bug is with theme renderer. We might be doing different things for confirm prompt rather than the pattern we used in other prompts. (I consider select prompt to be highly polished).

In any case, I would rather appreciate if you could fix it correctly in there. I am not interested in accepting this solution which would just increase the codebase's technical debt. With that said, I understand if you are not willing to continue anymore. In that case, you will have to wait for me to work on 0.11.0 in a few months for fixing this issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack, fair enough. It so turned out that I had a spare moment today and looked into fixing this directly -- and the fix was reasonable; I just pushed that. Let me know if you'd like me to change anything.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm.. you sure you pushed it? I don't see it.

Copy link
Contributor Author

@grunweg grunweg Feb 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Well... I thought pushing would be easy, but I managed to bork my branch. I created a new version at #175.
(If you can point me at instructions for fixing that, I'm all ears. I'm using mercurial through hg-git; sometimes things get lost in translation.)

render.confirm_prompt(&self.prompt, value)?;
}
} else {
Expand All @@ -234,7 +236,10 @@ impl Confirm<'_> {
}
}

// `prompt` may be a multi-line string, hence we may need to clear more than the current line.
// `clear_last_lines` clears the n lines *before* the current line.
term.clear_line()?;
term.clear_last_lines(self.prompt.lines().count() - 1)?;
if self.report {
render.confirm_prompt_selection(&self.prompt, rv)?;
}
Expand Down