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

Jump to bottom

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
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c4fdc9c
theme.clear(): also clear the current line.
grunweg Feb 14, 2022
c0575d1
confirm: properly clear the terminal in case of a multi-line prompt s…
grunweg Feb 14, 2022
1be630a
Remove clear_line() calls preceding TermThemeRenderer.clear() in the …
grunweg May 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/prompts/confirm.rs
View file
Open in desktop
Expand Up @@ -210,8 +210,8 @@ impl Confirm<'_> {
continue;
}
};

term.clear_line()?;
// Since `prompt` may be a multi-line string, we clear the entire prompt and not just the current line.
render.clear()?;
render.confirm_prompt(&self.prompt, value)?;
}
} else {
Expand All @@ -234,7 +234,8 @@ impl Confirm<'_> {
}
}

term.clear_line()?;
// Since `prompt` may be a multi-line string, we clear the entire prompt and not just the current line.
render.clear()?;
if self.report {
render.confirm_prompt_selection(&self.prompt, rv)?;
}
Expand Down
2 changes: 0 additions & 2 deletions src/prompts/input.rs
View file
Open in desktop
Expand Up @@ -401,7 +401,6 @@ where
}
let input = chars.iter().collect::<String>();

term.clear_line()?;
render.clear()?;

if chars.is_empty() {
Expand Down Expand Up @@ -494,7 +493,6 @@ where
};

render.add_line();
term.clear_line()?;
render.clear()?;

if input.is_empty() {
Expand Down
7 changes: 7 additions & 0 deletions src/theme.rs
View file
Open in desktop
Expand Up @@ -839,9 +839,16 @@ impl<'a> TermThemeRenderer<'a> {
})
}

/// Clear the current theme.
///
/// Position the cursor at the beginning of the current line.
pub fn clear(&mut self) -> io::Result<()> {
// clear the current line first, so the cursor ends at the beginning of 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 •
edited

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.

self.term
.clear_last_lines(self.height + self.prompt_height)?;
// self.term now contains self.height + self.prompt_height empty lines after
// the current line. That doesn't really matter, as these are empty.
self.height = 0;
Ok(())
}
Expand Down