-
Notifications
You must be signed in to change notification settings - Fork 819
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: incorrect line skipping in renderer flush #1233
Conversation
It seems that the bug was introduced in #1132 after this change: |
Totally, this is a regression. Appreciate all the work and insight on this one, @semihbkgr; we’re checking this out now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it with the provided example and everything seems to work as expected.
@semihbkgr Could you please add comments in the code explaining why you're doing this? I know you explained this in the issue, but it would be more beneficial to have some explanation in the code 🙂
I could reproduce the bug, yea |
@LeperGnome and does this patch fix it for you? |
@meowgorithm I found out that this bug was present in the original renderer before 1.2.0, but in a slightly different form... I'm tinkering around, still can't really wrap my head around it. I managed to fix it myself by: r.lastRender = strings.Join(newLines, "\n") // instead of r.buf.String() But I didn't test current patch yet. EDIT: |
Yea, as @semihbkgr pointed out in #1232 it seems like the issue lies in the fact, that |
Oh, wait.. @aymanbagabas did you reproduce the bug or not? I might've misunderstood your comment |
@LeperGnome Yes, I was able to reproduce the bug. This patch fixes the issue |
@@ -34,6 +34,7 @@ type standardRenderer struct { | |||
ticker *time.Ticker | |||
done chan struct{} | |||
lastRender string | |||
lastRenderedLines []string | |||
linesRendered int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can possibly remove the linesRendered
field and use len(lastRenderedLines)
instead. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will break tea.Println
. We reset the lastRenderedLines
when we call repaint
, which is the case when using tea.Println
. We need to keep track of the number of lines last rendered for tea.Println
to work.
As a side note - maybe it's a good thing to introduce tests for such corner-cases? I'll definitely help when working on new renderer. |
fixes #1232
To fix the bug described in the issue, we can simply store the currently rendered lines and use them for comparison in the upcoming flush operation.