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

Clear line properly for download progress #1781

Merged
merged 1 commit into from
Apr 21, 2019
Merged

Clear line properly for download progress #1781

merged 1 commit into from
Apr 21, 2019

Conversation

solson
Copy link
Member

@solson solson commented Apr 20, 2019

The workaround here that was printing many spaces did not work properly if the line length got very long and then very short again. I noticed this problem on an old version of rustup that still suffered from #1696, so it might not technically be reproducible anymore, but I figured we might as well fix this for the future anyway!

I suspect the comment saying delete_line() didn't clear the line properly was attempting to call it after printing the current download progress, but the term crate docs say it deletes from the cursor
position to the end of the line.

To deal with that, we instead jump to the start of the line before printing the current download progress and call delete_line() there.

The workaround here that was printing many spaces did not work properly
if the line length got very long and then very short again. I noticed
this problem on an old version of rustup that still suffered from #1696,
so it might not technically be reproducible anymore, but I figured we
might as well fix this for the future anyway!

I suspect the comment saying `delete_line()` didn't clear the line
properly was attempting to call it *after* printing the current download
progress, but the `term` crate docs say it deletes from the cursor
position to the end of the line.

To deal with that, we instead jump to the start of the line *before*
printing the current download progress and call `delete_line()` there.
Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for this.

@kinnison kinnison merged commit c93feb0 into rust-lang:master Apr 21, 2019
kinnison added a commit to kinnison/rustup that referenced this pull request Apr 22, 2019
In rust-lang#1781, we switched to using `delete_line()` but unfortunately
that turns out to not work very well on Windows.  So this commit
turns us back to using spaces to erase things, but in a way which
ought to be moderately more robust than before.  There's still
no guarantee we're doing it right, but at least this time if
the terminal is approximately wide enough for the bar and no more,
we won't be flowing onto the next line with loads of spaces after
the rest of the tracker.

Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
kinnison added a commit to kinnison/rustup that referenced this pull request Apr 22, 2019
In rust-lang#1781, we switched to using `delete_line()` but unfortunately
that turns out to not work very well on Windows.  So this commit
turns us back to using spaces to erase things, but in a way which
ought to be moderately more robust than before.  There's still
no guarantee we're doing it right, but at least this time if
the terminal is approximately wide enough for the bar and no more,
we won't be flowing onto the next line with loads of spaces after
the rest of the tracker.

Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
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.

2 participants