ink icon indicating copy to clipboard operation
ink copied to clipboard

Fix rendering when terminal is erased on first render

Open vadimdemedes opened this issue 1 year ago • 8 comments

Fixes https://github.com/vadimdemedes/ink/issues/585.

@matteodepalo did a great job documenting the issue, so I'm not going to repeat it here.

Context

Ink has two ways of rendering output, depending how tall it is:

  1. If output is shorter than terminal height, Ink erases the lines from last output and writes lines from the new output. Ink tracks how many lines did the last output have for this to work. This is handled in https://github.com/vadimdemedes/ink/blob/master/src/log-update.ts, which is a fork of log-update package.

  2. If output is equal or taller than terminal height, Ink erases the entire terminal history (or scrollback as it's commonly referred to) and writes new output. Note that Ink doesn't need to track how many lines to erase here, because it just erases the entire contents of the current terminal. https://github.com/vadimdemedes/ink/blob/8a0476024cf997e0dbc02e493ad3972349b02474/src/ink.tsx#L176-L182

Root cause

The key insight here is this:

Note that Ink doesn't need to track how many lines to erase here, because it just erases the entire contents of the current terminal.

In https://github.com/vadimdemedes/ink/issues/585, the very first render starts with the second rendering method and erases the terminal completely. On next render, the first method kicks in, but there's no information how many lines to erase, since second method doesn't store that information.

That's why the output from the first render doesn't get erased, since for logUpdate this variable is zero.

https://github.com/vadimdemedes/ink/blob/8a0476024cf997e0dbc02e493ad3972349b02474/src/log-update.ts#L12

Solution

This PR fixes this bug by separating the write operation that erases the terminal and writes the output. This way, even if the entire terminal is erased, logUpdate is still used to render the output and keep track of how many lines were written.

I've also added force option to logUpdate render function to force it to always render the string we give to it without bailing out if it equals to the last output. This can happen when Static output changes, but normal output isn't.

https://github.com/vadimdemedes/ink/blob/8a0476024cf997e0dbc02e493ad3972349b02474/src/log-update.ts#L23-L25

I've also added erase option to logUpdate render function, so that we can tell it to not do erasing of its own, if we erased the entire terminal already.

vadimdemedes avatar May 03 '23 20:05 vadimdemedes

Not sure why tests are failing on CI, while passing locally consistently. Will need to look into it. Help also appreciated to ship this fix faster!

vadimdemedes avatar May 03 '23 21:05 vadimdemedes

You can use https://github.com/nektos/act to debug the ci

so1ve avatar May 28 '23 16:05 so1ve

<comment deleted as some testing revealed it didn't work>

amcaplan avatar Jun 08 '23 10:06 amcaplan

@vadimdemedes strangely with this fix, tests pass locally if I run CI=true npm run test, but they don't seem to pass on CI.

The reason for that fix is that in CI we don't clear the terminal, we return earlier in the code, so it makes sense that the check for ansiEscapes.clearTerminal fails.

matteodepalo avatar Jul 17 '23 09:07 matteodepalo

Hey there, wondering if there are any updates on this work. I have a need on this being resolved. Is there anything I can do to help get this over the finish line? :)

DaniGuardiola avatar Nov 05 '23 04:11 DaniGuardiola

@DaniGuardiola unfortunately I haven't had the time to continue on this bug, the issue as I left it was one with tests. The code was working fine. CI is failing while local tests are passing, but I couldn't find the root cause. If you'd like to pick it up you can also check what I tried to do in a separate branch to maybe get some ideas.

matteodepalo avatar Nov 06 '23 10:11 matteodepalo

@DaniGuardiola The weird issue is that I fixed the original issue in this PR and confirmed it works on my laptop, but when I submitted this PR, it consistently fails on CI. Huge help would be to figure out what's going on in CI here to unblock this PR.

vadimdemedes avatar Nov 11 '23 17:11 vadimdemedes

I changed my approach and this stopped being a blocker for me. I don't think I'll have the time to look into it, sorry.

DaniGuardiola avatar Nov 14 '23 05:11 DaniGuardiola