-
-
Notifications
You must be signed in to change notification settings - Fork 10.9k
Add progress bar to concurrent downloads #21183
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
Conversation
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.
Pull request overview
This PR adds visual progress bars to concurrent downloads in Homebrew, enhancing the user experience by providing real-time visual feedback on download progress. The implementation calculates the percentage of completion for each download and renders it as a Unicode box-drawing progress bar that occupies approximately 20% of the terminal width.
Key changes:
- Added progress bar rendering logic that displays download completion percentage using Unicode characters (━ for completed, ┈ for pending)
- Modified the progress string format to include the progress bar before the phase and size information
- Removed the square bracket notation from the progress display
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MikeMcQuaid
left a comment
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.
Looks great so far! Want to give this a better review and test but won't be until Monday. Maintainers: please don't merge until then, thanks!
MikeMcQuaid
left a comment
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.
Looks great and worked nicely locally. Only other thing that is tempting is to use a bit more of the possible space if available? With e.g. 100 characters of width the progress bar is pretty cramped.
2987429 to
969bf59
Compare
|
@MikeMcQuaid i was able to do some analysis of the lengths of the messages based on the extant formula and cask names and versions. 95% of messages will be less than 40-45 characters in length. i selected this since a 80 column terminal will still get a minimal progress bar, while having a low risk of covering the messages. if the message is longer, the bar will shrink. if the terminal is wider, the bar will stretch. i decided to keep the unicode bar style to try to imitate pypi and for readability. it looks clearer that the pure ascii version in my terminal. however, if that ascii version desired i can change that. here is a screenshot in a 80 column terminal |
MikeMcQuaid
left a comment
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.
Thanks for the research! I'd be tempted to say you just fill the entire space if possible based on the maximum length of items already in the queue (because we fully populate it before fetching). I may have missed something here so please correct me if so.
|
@MikeMcQuaid good idea on finding the max length. i did that and i also updated the bar to be ascii. |
MikeMcQuaid
left a comment
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.
Great work, thanks again @yuzawa-san!





brew lgtm(style, typechecking and tests) with your changes locally?I was looking over #20963 and enjoy the file size ratios (thanks!). I sometimes run on slow connections and would like some progress bars so I can look at quick glance on how the downloads are going. The prior PR's had no unit tests, and I tested this locally. Here is a video:
progress.mp4
I made sure the bar is 20% (1/5th) of the tty and that it is at least 4 characters long.
I used some unicode box drawing glyphs. I suppose we could use
=and-if we want full ASCII support, but the checkmark is already not ASCII, so we should be ok. I am open to feedback on the formatting, sizing, and appearance.