Merged
Conversation
djc
reviewed
Jun 13, 2024
Member
djc
left a comment
There was a problem hiding this comment.
Thanks for the investigation!
Would you be able to add a test that can help us avoid regressing this in the future?
The move_cursor flag needs to be set in the wrapped Term draw state, as that's the one, that is then actually used to draw to the terminal. The move_cursor flag in the multi draw state doesn't have any effect on drawing to the terminal.
The cursor needs to move to the first line (which is one less than the number of lines, as the cursor is still on the last line), and then also move to the front of that line (as it was on the last char of the last line).
8882f60 to
cae9b99
Compare
Contributor
Author
|
Thanks for the review.
I tried to do that, but failed, because the output looks the same, whether it moves the cursor or uses clear, so the test was always green, even when it was broken (so it wasn't useful). But I just noticed |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I had the problem of my progress bars starting to flicker after adding some more bars, especially over an ssh connection. I then found #42 and from that then found #143, but the fix described there to set
m.set_move_cursor(true);didn't make a difference at all.After some debugging, I found out, that the move_cursor flag was used in the
draw_to_term()function, but this is only called from aDrawable::Term, while theset_move_cursorwas only set on theMultiStatewhich is only used in theDrawable::Multi. So in the current state, it never has any effect when actually drawing to the terminal.Then after forwarding the flag to the correct DrawState instance, I noticed, that the first bar always starts drawing one line too high on the last char, so the first line was just off-by-one. This was because the cursor is always on the last char of the last line, and then moving up the number of bars, will always end up one line too high. So just move up one line less, but then the cursor also still needs to be moved to the front of the line.
So this now fixes using of
set_move_cursor(true)again, so it can be used to fix the flickering.