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

Skip updating initial lines that have not changed #29

Closed
wants to merge 2 commits into from

Conversation

djmitche
Copy link
Contributor

In many cases, the most frequently-changing part of the logUpdate output is at the bottom, as that is where users expect to see output. This patch optimizes for that case by skipping updating any initial lines that have not changed. For output spanning many lines, this can help avoid unsightly flicker on update.

I've left the test changes as a separate commit -- they show me that this worked, but if you'd rather not merge those changes then I'm happy to drop that commit. Or squash it.

@djmitche
Copy link
Contributor Author

djmitche commented Apr 4, 2018

I'd like to build on this by monitoring the visible rows in the terminal and only including output on those rows. But before I do this -- @sindresorhus is this repo still being maintained? I don't see any recent pushes, and issues haven't seen comments in a long time, either.

@djmitche
Copy link
Contributor Author

djmitche commented Apr 9, 2018

I tested this with Ink and it seems to work OK.

@djmitche
Copy link
Contributor Author

This does, in fact, fail some of the tests I just added, so I'll revisit.

In many cases, the most frequently-changing part of the logUpdate output
is at the bottom, as that is where users expect to see output.  This
patch optimizes for that case by skipping updating any initial lines
that have not changed.  For output spanning many lines, this can help
avoid unsightly flicker on update.
@djmitche
Copy link
Contributor Author

Fixed to pass tests. This also works in some manual testing I've done.

@djmitche
Copy link
Contributor Author

djmitche commented May 2, 2018

@sindresorhus thoughts on this? If you don't like the direction I'm taking what is admittedly a very simple library, I could fork instead..

@djmitche
Copy link
Contributor Author

djmitche commented May 2, 2018

(as a reminder, since I forgot -- the ❌ is because node 4 is still in .travis.yml but isn't supported)

@sindresorhus
Copy link
Owner

I'm open to it. Just need to test it against dependents to make sure it doesn't break anything. And I've been out of time lately. I'll get to this one soon.

@djmitche djmitche closed this Sep 30, 2019
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.

None yet

2 participants