Clear to end of screen before redrawing prompt #476
Conversation
| @@ -47,18 +47,24 @@ func (c *Cursor) Back(n int) error { | |||
|
|
|||
| // NextLine moves cursor to beginning of the line n lines down. | |||
| func (c *Cursor) NextLine(n int) error { | |||
| if err := c.Down(1); err != nil { | |||
| if err := c.HorizontalAbsolute(0); err != nil { | |||
There was a problem hiding this comment.
Why first move horizontal and then try to move between lines?
There was a problem hiding this comment.
There seems to be weird behavior when going Down(0) or Up(0), where the cursor moves down or up 1 instead of remaining on the same line. I'm not sure if this is expected ANSI behavior for \x1b[0A, but this change was made to guard against this case.
It might be more appropriate to move this check for n == 0 to the Down and Up functions instead? I wasn't sure if this current behavior was relied on by other functions and felt that this change was safer.
| if n == 0 { | ||
| return nil | ||
| } | ||
| return c.Down(n) |
There was a problem hiding this comment.
Nice catch that this didn't use to forward the n parameter and just moved down 1 line each time 👍
multiline.go
Outdated
| // position the cursor on last line of input | ||
| cursorPadding := 3 | ||
|
|
||
| // ignore the empty line in an empty answer | ||
| if len(multiline) == 1 && multiline[0] == "" { | ||
| cursorPadding = 2 | ||
| } | ||
| cursor.PreviousLine(cursorPadding) |
There was a problem hiding this comment.
Let's make this simpler to read at the cost of tiny repetition:
| // position the cursor on last line of input | |
| cursorPadding := 3 | |
| // ignore the empty line in an empty answer | |
| if len(multiline) == 1 && multiline[0] == "" { | |
| cursorPadding = 2 | |
| } | |
| cursor.PreviousLine(cursorPadding) | |
| if len(multiline) == 1 && multiline[0] == "" { | |
| // ignore the empty line in an empty answer | |
| cursor.PreviousLine(2) | |
| } else { | |
| // position the cursor on last line of input | |
| cursor.PreviousLine(3) | |
| } |
I'm curious about where these constants 2 and 3 come from. From the old code, it looks like the number of lines to clear was calculated by taking the length of multiline and adding 2 to that (I guess to account for two blank lines). Now, it either deletes 2 or 3 lines up and not more than that. Is the size of multiline taken into account somewhere when redrawing?
There was a problem hiding this comment.
Those magic numbers were a hacky way to handle the empty case and confused me quite a bit... There was something odd going on when using a modified input value while rendering the template1. Removing the newline from the answer but using the unmodified value in the template seems to clear this logic up some!
Also wanted to share that Multiline only moves the cursor location to account for the two blank lines and lets resetPrompt move the remaining n lines before clearing the screen. Really good call about checking into how multiline is redrawn!
Footnotes
-
Removing the trailing newline from
multilinebefore appending torenderTemplatecaused this template to differ from what was actually being displayed for the empty input. The missing newline in therenderTemplatemeant thatcountLinesundercounted what was actually rendered and socursor.PreviousLine(3)over adjusted, how odd! ↩
terminal/display.go
Outdated
| ) | ||
|
|
||
| func EraseScreen(out FileWriter, mode EraseScreenMode) error { | ||
| _, err := fmt.Fprintf(out, "\x1b[%dJ", mode) |
There was a problem hiding this comment.
on Windows we can't just assume that printing \x1b[%dJ to the terminal will just work. I'm pretty sure that out-of-the-box, that won't work in both cmd.exe and PowerShell.exe terminal, and seeing how Survey right now works in both, we would have to come up with a solution to clear to the end of the terminal that would work in all terminals that Survey currently supports.
There was a problem hiding this comment.
Completely agree! I'd be interested in knowing if a modified version of this example could be used to clear only from the current cursor position to the end of the screen? I have hope that the FillConsoleOutputCharacter call is buffered and would fill the remaining screen with blanks in a single action, which should mean that option flickering is removed from select prompts on Windows, but I am unfamiliar with these calls and am open to any suggestions!
There was a problem hiding this comment.
Added support for Windows terminals and tested in both cmd.exe and Powershell!
Unfortunately, it seems that there's still the occasional flickering effect with this redraw logic, but it doesn't seem to be any better or worse than currently. Just a different way of achieving the same effect. This might be more pronounced on my older machine though!
| lineCount := csbi.window.bottom - csbi.cursorPosition.Y | ||
| termWidth := csbi.size.X | ||
| screenSize := lineCount * termWidth |
There was a problem hiding this comment.
The Windows version of this EraseScreen function clears the lines from the cursor (at the beginning of the prompt) to the end of the displayed window (which is the end of the outputted prompt). For reference, the consoleScreenBufferInfo structure contains terminal dimensions and coordinates of the window and cursor.
This seems to match the behavior of the POSIX EraseScreen for resetting the prompt, but doesn't account for the EraseScreenMode. Not sure if supporting the other modes (clearing to the beginning of the screen and the entire screen) is necessary for how this function is being used, but I'd be in favor of forgoing them to keep this implementation more simple. Definitely open to thoughts here!
Summary
This PR adds a
terminal.EraseScreenfunction to support the "Erase in Display" ANSI control sequence. This function replaces calls toterminal.ClearLineinresetPromptin order to clear previous prompt output entirely and atomically, instead of clearing each line in an iterative manner.Updating
resetPromptto clear to the end of the screen seems to significantly reduce the "flickering" effect noted in #436 since fewer updates to the terminal screen are made between each render.Additionally, the
MultiLineprompt was updated as a result of changes to theresetPromptlogic. This prompt now preserves leading and trailing spaces in answers and replaces the input template with an answered template after submission. The template was also updated to begin inputs on a newline, which happens to be a request of #336.Preview
Before
select-flicker.mp4
After
select-smooth.mp4
Reviewers
The following steps can be used to inspect these changes:
surveyandsurvey.Select,survey.MultiSelect, orsurvey.MultiLine, append the following to yourgo.mod, pointing to your localsurveysource:Notes
Flickering is not entirely removed since there is brief moment between clearing the past prompt and outputting the updated one where the erased screen might be shown. This seems to be a fairly rare occurrence though, only happening a handful of times in my testing. The "synchronized update" suggestion from @dnkl would likely prevent this, but I had some difficulties in implementing this.
This has not been tested with a Windows terminal, however I presume this escape code is properly handled as "virtual terminal sequences are recommended" when developing for Windows.
resetPrompt.These changes do not seem to impact the current behavior shown in Input suggestion problem #452.