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.

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

Fix rendering when terminal is erased on first render #586

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vadimdemedes
Copy link
Owner

@vadimdemedes vadimdemedes commented May 3, 2023

Fixes #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.

    ink/src/ink.tsx

    Lines 176 to 182 in 8a04760

    if (outputHeight >= this.options.stdout.rows) {
    this.options.stdout.write(
    ansiEscapes.clearTerminal + this.fullStaticOutput + output
    );
    this.lastOutput = output;
    return;
    }

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 #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.

let previousLineCount = 0;

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.

ink/src/log-update.ts

Lines 23 to 25 in 8a04760

if (output === previousOutput) {
return;
}

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
Copy link
Owner Author

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!

@so1ve
Copy link

so1ve commented May 28, 2023

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

@amcaplan
Copy link

amcaplan commented Jun 8, 2023

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

@matteodepalo
Copy link
Contributor

@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.

@DaniGuardiola
Copy link

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? :)

@matteodepalo
Copy link
Contributor

@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.

@vadimdemedes
Copy link
Owner Author

@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.

@DaniGuardiola
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Output terminal isn't cleared when second to last frame is taller than terminal
5 participants