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

Download progress take 3 #73

Merged
merged 4 commits into from
Feb 24, 2016
Merged

Download progress take 3 #73

merged 4 commits into from
Feb 24, 2016

Conversation

crumblingstatue
Copy link
Contributor

This implements the progress indication part of #71.

Since I don't know how to measure download speed without adding
another dependency (time), this only implements download progress display,
but not speed (which is less important).

Library

The library part just adds the various notifications for the download events.
Notification implements Display, but download events aren't supposed to be
printed out normally, so the Display implementations for them aren't utilized right now.

Client

To avoid having to implement architectural changes, I have once again opted for using Rc and friends. It shouldn't be a performance problem, but maybe someone has a better idea.

Other

I also ran rustfmt in a follow-up commit to clean up after myself, which also affected some unrelated code.

Maybe the frequency of printing out the download events could be lessened (maybe once per second), but again, this would probably require something like the time crate.

@crumblingstatue
Copy link
Contributor Author

Uh-Oh! It looks like rustfmt actually messed up some code, making it no longer compile.
Gotta investigate.

@crumblingstatue
Copy link
Contributor Author

One way to avoid Rc and friends in the client would be to encode all necessary information in the notification itself, and construct a new Term every time we handle the notification.

For the former, I imagine something like DownloadProgress(total, content_len). However, this would require assuming that the Content-Length is always available in the get request (which is not always the case?). We could also pass an optional content_len every time, but that doesn't reflect the truth either, since you only query the Content-Length once at the beginning, so you only need to check for absence once.

For the latter, I'm not sure how expensive it is constructing a new Term every time. Right now the implementation stores it using Rc<RefCell>. We could also write to stdout directly instead of using Term, especially since delete_line() doesn't seem to work, making it less useful. I don't know if using '\r' instead of carriage_return() is less portable/robust though.

@crumblingstatue
Copy link
Contributor Author

This is what the current implementation looks like in action: https://asciinema.org/a/3oayn89jweqvbj6t7oi8f0zzr

@brson
Copy link
Contributor

brson commented Feb 24, 2016

Looks good to me. The Rc is fine - this doesn't need to be hyper-optimized.

multirust-rs should probably not do this when it is not outputting to a tty - it will print lines and lines of updates. There's some tty detection code in my patch so maybe it can be fixed up later.

@Diggsey
Copy link
Contributor

Diggsey commented Feb 24, 2016

@crumblingstatue Great work, thanks!

WRT Rc, it should be possible to store the download status directly as a capture in the notification handler (the notification system should accept any FnMut, and FnMuts can capture mutable value with move semantics).

However, as @brson rightly points out, it really doesn't matter in this case! Any minor improvements can be made later, so I'll merge this in now.

Diggsey added a commit that referenced this pull request Feb 24, 2016
@Diggsey Diggsey merged commit 3a69c14 into rust-lang:master Feb 24, 2016
@crumblingstatue crumblingstatue deleted the download_progress_take_3 branch February 24, 2016 23:07
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.

3 participants