confirm: properly clear the terminal in case of a multi-line prompt string #166
Conversation
…tring. Need to clear the current line before clearing previous lines, otherwise a stray final line is left behind. Not sure why.
|
I realised why we need to clear the current line first: clearing the last lines leaves the cursor on the first cleared line --- so to clear the current line first, we'd need to move the cursor down again. It's easier to clear the current line first, and then clear the last n lines. That way, the cursor will be placed at the last char in the terminal (which is what we want). |
| term.clear_line()?; | ||
| term.clear_last_lines(self.prompt.lines().count() - 1)?; |
There was a problem hiding this comment.
Can you please convert this change to all the prompt types?
There was a problem hiding this comment.
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_linemethod isinput.rs; that part of code is not clearing a prompt. clear_last_linesis used a couple of times, but always to reset the whole terminal (such asclear_last_lines(paging.capacity)?).
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Oh, and the bug is not specific to the combination of "report=true" and "wait_for_newline=false".
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hmm.. you sure you pushed it? I don't see it.
There was a problem hiding this comment.
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.)
|
Closing in favour of #175. |
Need to clear the current line before clearing previous lines, otherwise a stray final line is left behind. Not sure why.
Fixes #165.
I'm not sure how to add a test. Happy to add one, if somebody can point me at how to do this.
Barring that, I tested this manually, for all four combinations of report={true,false} and wait_for_newline={true,false}.