terkelg / prompts

❯ Lightweight, beautiful and user-friendly interactive prompts
MIT License
8.88k stars 304 forks source link

[Discussion] Prompt flicker solution #233

Open Js-Brecht opened 5 years ago

Js-Brecht commented 5 years ago

Description of issue

There is a problem with flickering in prompts, which reduces the quality of the UX. This is an analysis for a possible solution to the problem.

Please let me know if you notice anything erroneous, or ways that this could be improved.

Solution analysis

I think that the only way to get around the flickering is to redraw the prompts only when needed. I'm sure there's many ways to achieve this. Here is my idea.

First, would need to save some state that tracks how many rows the prompt (previously) occupied, and when that changes, only redraw the parts that need to change.

I'll use the text prompt as the example to work off of here.

I think that the entire prompt can be broken down into its component pieces. The prompt, the input, and what I call the message (basically, a message following the input prompt; i.e. the hint or error.)

Track the previous state of each prompt component, maybe something like this:

interface IHostComponentState {
  active: boolean;
  render: boolean;
  rows: number;
  output: string | ISubComponentState;
};

interface ISubComponentState extends Omit<IHostComponentState , 'rows'> {
    anchor: boolean;
}

type IComponentState = IHostComponentState [];

This could track each component, moving down the screen. When the new row count for each component is calculated, it can be compared to its corresponding component in the array, and if the rows is not equal to the new row count, then set the render switch to true, and set the rows to the new row count.

The render() method would need to execute this calculation, maybe by creating a "new state" object, then cycle over each index of the new state and see if the rows property of each component matches its previous state. If it doesn't, then redraw that component (and set render: true on all following components). As it hits each component, if render: true, then it knows it needs to redraw that component.

render: true could also be used if a specific component is changed, but the rows aren't necessarily changing. For example, if an error is generated, the message component's output property could be updated, and the render property set to true. If the rows property hasn't changed, then render() will know that it has to redraw that component, but not every component afterwards.

The active property would only be set on one element of the components array, indicating where the user's cursor would be kept for input. That means you could calculate how many lines up or down you would need to move to redraw certain parts of the display. This switch could also be used to remember where the cursor needs to be returned to after a component has been drawn below the prompt. So for every component that is redrawn, a count of the lines down/up from the active component will need to be recorded.

Since there wouldn't be a redraw every time a key is hit, the user would have free range over the entire input line. This also means they would be able to overwrite the prompt, so the recommended method for displaying the prompt would be using readline.setPrompt(), and readline.prompt(). This would also make the UX feel more natural, since the cursor wouldn't be locked at the end (even though, currently, you can still move the cursor left/right to change some of the input; you just don't see the cursor move).

Using the readline interface like this would mean either exposing it directly to all extending classes, or creating an interface for updating the current prompt. After a question is completed, readline.setPrompt('') could be used to reset the prompt to nothing so that it doesn't interfere with later question processes.

readline.prompt() example [This is a gist of a simple readline interface I wrote a while ago for a project](https://gist.github.com/Js-Brecht/56a0cfd3624249a5ab69660e448d0fca). It had very basic requirements, so I didn't have to account for any of the additional functionality that `prompts` provides. It is just an example of one way to use `readline`'s builtin prompt functionality.
"output" property recursive structure Since some components will need to be broken into pieces (specifically for this example, the `prompt` and the `input`, or the `prompt` and the `placeholder`), the `prompt` property may need to be recursive. I think that for components that have "sub-components", the `render` property will be key. That is, only sub-components with the `render` property set to true will be drawn. This will allow you to have multiple sub-components, but only draw certain ones at any given time. Therefore, if any sub-components `render` properties are changed, then the host component's `render` property will need to be changed to `true`. The `active` property for a sub-component will act the same as the `active` component for the "host" component. It will indicate which specific sub-component the cursor state needs to be saved for. The "host" `active` property of a component with sub-components will only indicate that one of the sub-components will be used to save cursor state. Sub-components will always be drawn on the same line, not on separate rows, so they don't need a `rows` property. However, they do need to be accounted for when calculating the "host" component's `rows` property. The `anchor` property for a sub-component will indicate whether the cursor position should be "anchored" (cursor state saved) at the beginning of the drawn text, or after the text has been drawn.

I think the trickiest part of this process will be caused by the cursor not being locked in place by redraws. You would need to keep track of the cursor's position in the input, so that you know two things:

  1. What row the cursor is currently on.
    • So you can calculate how far up/down to move the cursor when you need to redraw a component, and when you need to restore cursor position upon completion of render.
  2. What column the cursor is currently located at.
    • So you know what X position to return to when the cursor Y is restored.

I think this could be done with some simple "cursor state":

interface ICursorState {
  offset: number;
  column: number;
}

The offset would be where the cursor needs to land based on row 0 (where the output starts). So, with this, you always know the bounds of your entire prompt. You know how many rows all of your components take up by using the component state, and the cursor state will tell you the X and Y your cursor needs to land at, within the entire scope of your prompt.

On the first render, when the active prompt is drawn, the cursor state will need to be initialized. For every time the user hits left, or right (in some cases, up/down), the cursor state will need to be updated with the +/- offset and column. Then other inputs could be supported, like <HOME>, <END>, etc..


I think that this concept would be flexible enough to use in all elements (questions). Each element would only need to know how it's going to structure its components, how it's going to style them (plain text needs to be used for length/row calculations), and when one of them is going to change. All of the calculations/tracking will generally remain the same, for the most part, I would think.

elliot-nelson commented 5 years ago

Really thorough!

My only concern is that the number of components and states really balloons for something like an autocompleteMultiselect.

I don't know how easy, hard, or impossible it would be, but I think you'd have a lot more flexibility if every element produced a single text blob when it rendered, and you parsed it to decide what line(s) needed to be updated - like React does with the DOM in the browser.

The way new elements that look like anything can still be added and immediately get the benefit of your reduced flickering work.

Js-Brecht commented 5 years ago

I think I understand what you're saying. Keep a previousState text object of the entire prompt; then, when the render() method is called, generate a new state in memory. Compare them line by line, straight across; when one line differs, redraw it.

Is that what you had in mind? I can see how that would be simpler.

We would have to know if lines are going to wrap, because that will effect lines below, and to avoid unnecessary redrawing, we have to know if updates to a single line added/reduced the total number of lines drawn. So, there will have to be two representations of both the previous state, and the new state; one would be plain text (special characters replaced by spaces), the other would be the (desired) rendered text. If one line of rendered text from the previous state differs at all from the same line in the new state, then redraw that line; but also check to make sure the row span didn't change, so you know if the lines below the current one need to be redrawn as well.

Then, when you get to the end of your new state's rendered text, you just issue a clear-to-end-of-screen (CEOS) code, and you're done. That is, if you've just redrawn the last line. If you haven't, then you'd have to check if the number of lines in the new state is less than the previous state. If it is, then drop the cursor down to one past the last character on the last line, and CEOS

Js-Brecht commented 5 years ago

I have a pretty good idea how I would accomplish that.

Could use something like an sprintf (spool print, in this case) function here; a shared function that abstracts away the details of building the state objects, doing comparisons, and drawing lines. Then you don't have to worry about doing any of that when writing these classes; all you have to worry about is what text you're going to output on each line, and what it's going to look like.

elliot-nelson commented 5 years ago

I don't know if you've looked at https://github.com/vadimdemedes/ink, but in their experimental mode I think they aggressively look for sections of the screen to render. Might be some useful tricks in there (I have not read the source code myself so YMMV).

Js-Brecht commented 5 years ago

I know of ink, but that's about as much as I can say. They use it for the CLI over at Gatsby, so I've encountered it, but haven't really delved into it to see how it operates, or even paid much attention to how it is used. I vaguely recall running across an issue on Gatsby's roadmap talking about the ink experimental features, since there were issues with performance and flickering there, too.

It might be interesting to try ink out here. Some of the stuff you can do with it looks pretty cool. One of the big things with prompts is minimal dependencies, though, yeah? I'll check out their source and see how they handle diffs.


Reading through some of their source now, and I ran across this in their output source:

output[y + offsetY] = sliceAnsi(currentLine, 0, x) + line + sliceAnsi(currentLine, x + length);

sliceAnsi uses regular expressions to slice out the ansi codes, which works, but could be a performance hit. I can see how it would be required, though, if you don't expect to have full control over the text in the output. Wonder if the performance would even be an issue in a prompt type application, since a lot of the time in the app is actually spent waiting for input, not as much processing output.


What I was thinking about doing was feeding a function destructured styling characters+text output. So it would feel somewhat similar to how strings are concatenated before being printed to the screen, and would eliminate the need to do any string splitting/parsing; all it would be doing is string comparisons, which is ~10% faster than using regular expressions to pull strings apart

sprintf() function usage concept ```ts interface IFormatObj { value: string; formatFn: (val: string) => string; } type ISprintfChunk = function | string | IFormatObj | false; type ISprintfArg = (ISprintfChunk | false)[]; interface ISprintfFn { (...args: ISprintfArg[]) => { done: () => void; } } ``` So, each parameter in the function would be an array that represents one line; each index in the array would be the desired output to concatenate. * Strings are assumed plain text. * Functions would indicate a symbol/special character that should go to output; the routine will see a function, and for plain text use a space, and call the function to receive the actual output to render. * `IFormatObj` would be what gives you a little more flexibility, but would also be more verbose. It would let you use functions like `kleur` provides: you give it the value, which is what's used in the plain text, and it would call the provided function with that value as the argument to get the rendered text * If `false` is encountered, do nothing. This would let you put in boolean comparisons, without needing to account for negatives results. One thing about this is all of the symbols and special characters and such would need to be wrapped. Usage would remain the same, but the returned value would be a function. For example, where `style.symbol` is used, the related export in `style.js` would look like this: ```js const symbol = (done, aborted) => () => aborted ? symbols.aborted : done ? symbols.done : symbols.default; ``` Unless, of course, the symbol doesn't take up more than 1 space (no escape characters, for one). If it takes up **more** than one space, an `IFormatObj` would need to be used. For example, since `style.delimiter` uses a couple possible values, and either one could take up variable amount of space, it could be rewritten like this: ```js const delimiter = completing => { value: completing ? figures.ellipsis : figures.pointerSmall, fn: c.gray, } ``` Using `text.js` as an example, usage could look like this: ```ts const bold = (value) => { value, fn: color.bold }; const red = (value) => { value, fn: color.red }; const redItalic = (value) => { value, fn: color.red().italic }; const outputText = [ style.symbol(this.done, this.aborted), bold(this.msg), style.delimiter(this.done), this.red ? red(this.rendered) : this.rendered, ]; const outputError = this.error && this.errorMsg.split('\n') .map((l, i) => [i ? ' ' : figures.pointerSmall, redItalic(l)]); this.sprintf(outputText, outputError && ...outputError).done(); // Or it could be called multiple times. Output will be "spooled" as it is input this.sprintf([ style.symbol(this.done, this.aborted), bold(this.msg), style.delimiter(this.done), this.red ? red(this.rendered) : this.rendered, ]); this.error && this.errorMsg.split('\n') .forEach((l, i) => this.sprintf([i ? ' ' : figures.pointerSmall, redItalic(l)]); this.sprintf().done(); ``` As each line is input, the function will calculate where the cursor would need to land, and then draw that line if it's changed from the previous output. All that would be happening in the background is the current state would be built as an array, printing to the screen as it goes (if it needs to). When `done()` is called, the reference to that array is passed to the "previous state" as is. Then, when the function is called the next time, a new state array is created, and as it processes, it compares to the corresponding index in the previous state. * There's no object copying going on; just a reference passed from current state to previous state. * No history is captured, just current and previous state only * Line/row counts would be calculated ONE time per line, and stored in the state (`[lineCount, value]`), so line parsing & number crunching are done as little as possible. --- Using this type of method, the `render()` functions in the various elements could actually be simplified. They don't need to worry about line counting, crunching, up/downs, clearing lines/screens... none of that. Just generate the text, and include styles. Need add some way to mark "input lines", too, so that they aren't processed the same as the rest of the output. For example, redraws shouldn't happen on every keystroke (since that's where the flickering is coming from, really). If the only output that's changing is the input string, and its number of occupied rows doesn't change, then redrawing can be skipped. Tracking cursor position needs to be done, too. I'm sure it would probably need to be extended for various other scenarios, too. Just a concept for now. This would definitely change the API, because you wouldn't want to pass any hooks a `kleur` object. Instead, a reasonably simple API to call the function would need to be designed, or its usage would need to be clearly defined. Alternatively, the whole `sliceAnsi` method could be used, but only for strings that are passed back from the API hook 🤔. Let me know what you think.
elliot-nelson commented 5 years ago

You have put thought into this. :muscle:

Backing up a bit, it seems like the options are:

1) Do nothing at all. Some elements are experiencing flicker. *

2) Make a single wrapper all elements call with their final text output (so it's already newlined, ansi-colored, etc.). The wrapper needs to deconstruct the text and determine what rows of the screen it can update and which it can ignore. This will reduce some flicker, but is slower than option 3.

3) For maximum efficiency, all elements need to use a new non-native String "object" that represents text and formatting, and concatenates them into an object that will eventually be turned into text at the very end. Need to avoid normal string concatenation essentially throughout the whole project.

Right now I'm actually not familiar with the flicker (I don't know how often it happens, whether it's worse on Windows vs Linux vs MacOS, if it affects iTerm more than 3rd party terminal programs, etc.), so I don't know how bad the problem is. Just as a plain-old user of prompts, I'm much more inclined to go with Option 2 than Option 3 -- because Option 3 seems like the anti-aircraft gun here.

It also won't stop users from pre-coloring text before it even reaches you, i.e.:

    { message: `Do you like ${kleur.yellow('yellow')} bananas?` }

I personally wouldn't want to travel that road unless you could show that Option 3 was significantly better than Option 2, and solved a real problem with Option 1.

Js-Brecht commented 5 years ago

One other area where this "selective rendering" would be important is during input. If somebody is typing something into a prompt, and it is redrawing on every keystroke (this is really where the flickering becomes noticeable), then your (visible) caret is effectively limited to the end of the input. You can move back and forth, but you can't see your cursor move. Just seems a little strange, but it's not necessarily a critical issue. For input to feel more natural, it would have to stop rendering everything on every change, so it would just make sense to do selective rendering on the whole block; otherwise, it would have to take care of moving your caret for you.

No. 2 is definitely doable. No. 2 and No. 3 will basically be doing the same thing, except for No. 2 to work, you would have to do some pre-processing of the input, so ultimately it's more work.
However, I don't think either one would be too difficult to design.

It also won't stop users from pre-coloring text before it even reaches you

As you say, external inputs could be just about anything, so it seems like No. 2 would be necessary in any case. That way, inputs can be processed before they are used. Maybe it would make sense to have No. 3 as a lower level function that could be used if something needed to be optimized, but otherwise use No. 2 (which ultimately feeds No. 3) as the common use method.

But hey, I'm good with doing nothing, too 🤷‍♂. I don't ever use prompts for anything that's going into production. I just use it for my own dev tooling... stuff like that. The flicker was mentioned, and I've noticed it too, so I thought I'd write out how I would approach a selective rendering process.

Another option would be tag functions; kind of middle-of-the-road between No.2 and No.3.

Js-Brecht commented 5 years ago

Github needs a poll feature 😆

elliot-nelson commented 5 years ago

With the current design, as you say, TTY cursor is always turned off because it would always be floating at the bottom of the rendered text. I think there might be 2 approaches to fix that:

1) Keep TTY cursor turned off for all elements, but add in hand-crafted cursor positions ("draw" a cursor in the appropriate spot, perhaps even with animated blinking, using the ASCII block or another suitable char). Each element (like number, text, autocomplete) would control its own animations, whereas others (select, multiselect) would opt out of any cursor.

2) Allow all elements to specify the cursor position with a special unicode character in the string (or some other mechanism). In the rendering, strip this out, render the string, retrieve the cursor position, then move the cursor to the desired X,Y and display it. On the next frame rendered, you need to reverse this by moving the cursor back to the last position (the end of the stdout) and turning it back off, then doing the existing clearing behavior.

I actually think the first approach would be easier, but wouldn't exactly mimic the user's real cursor (would especially vary in different terminals / shells). The second approach would give you a real TTY cursor, but you would have to implement a lot of that logic you've written about -- you need to figure out what X,Y on-screen would be rendered by that certain cursor position in the string, etc.)

(This might be a total tangent, I'm mentioning it just because fixing flickering and fixing cursor can probably be tackled separately -- if you wanted to.)

Js-Brecht commented 5 years ago

Now that you mention it, cursor.save (\x1B7) is already in use here, for one. It would make perfect sense to just look for that.

I hadn't noticed that the cursor was turned off; hadn't really looked for that. I do know it's not turned off in the text and the number prompt. That's one reason I even noticed it... the cursor would remain at the bottom of the prompt. The cursor.restore code doesn't seem to remember the Y position, just the X.

I did some work to fix that, and it works. The prompts are sensitive to where the text lands, though. If it lands on the last line, it will screw up the line counts, so cursor.up will always be one line short when it redraws (unless you don't allow any terminal history; then it's just the one line off, and it corrects itself). So, I always try to make sure it doesn't land on the last line, but in the case of wrapping prompts / input, it can still happen. The problem also exists in the current version, except it will always count one too many lines up. But it's not like people are writing dissertations when answering a question, though. Kind of an edge case.

Selective rendering would support longer inputs, and longer/multiline prompts a lot better than the current method. I'd be interested to see if it will help with the one-off rendering problem on the last line, too.

terkelg commented 5 years ago

First of all thank you! It makes me really happy to read such thorough comments! Discussions like these are important and very helpful.

I share same concern as @elliot-nelson, that the number of components and states would become complex for some prompt types. One of the main things I want to address in the next major version of prompts is the developer experience. It should be easy and fun to develop new custom prompts. The API should be flexible, consistent and easy to maintain - That's a top priority!

@Js-Brecht you are spot on about the dependencies. I would prefer not to include too many dependencies. Another goal for Prompts is to be simple and small. I just looked at Ink and to me it seems overly complex – I like how you don't have to worry about rendering logic, but not so much that they try to mimic Reacts API and have you write JSX.

I'd say developer experience, user experience and simplicity over performance. Less complexity and less code means less bugs and easier to maintain codebase. That's what's really important. Flickering is a part of the user experience, and therefore something I'm interested in fixing but not if it introduces too much work. I wonder if the idea with the TTY cursor turned off and a "virtual" curser could work?

So many good points here! 🙏