Add progress bar to artifact downloads#182836
Add progress bar to artifact downloads#182836Mr-Pepe wants to merge 4 commits intoflutter:masterfrom
Conversation
github-actions
bot
added
the
tool
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a progress bar for artifact downloads, enhancing the user experience by providing more detailed feedback on download status, including percentage, speed, and ETA. A new DownloadProgress class encapsulates the logic for tracking and formatting this information. The changes are well-tested, with new unit tests for the DownloadProgress class and integration tests for the ArtifactUpdater. The implementation correctly falls back to the previous spinner-based progress display on terminals that do not support the new progress bar.
The changes look good overall. I have a few minor suggestions to improve documentation and code readability.
Mr-Pepe
force-pushed
the
02-24-add_progress_bar_to_artifact_download
branch
3 times, most recently
from
91a3241 to
3af15e1
Compare
bkonyi
left a comment
bkonyi
left a comment
There was a problem hiding this comment.
This looks great! I've been wanting this for years 😁
Just a few minor comments from me.
| var downloadCompleted = false; | ||
| Status? status; | ||
|
|
||
| if (_canShowProgress) { |
There was a problem hiding this comment.
Any chance we could extract the "canShowProgress" and not-show-progress to its own classes? That way this logic is just about downloading the archive and anything you need for progress line can be ignored in the non-progress line abstraction?
There was a problem hiding this comment.
Done. This adds an extra abstraction layer but keeps the ArtifactUpdater nice and clean 👍
| while (retries > 0) { | ||
| status = _logger.startProgress(formattedMessage); | ||
| final progress = DownloadProgress(); | ||
| final downloadStopwatch = Stopwatch()..start(); |
There was a problem hiding this comment.
You never stop this if !_canShowProgress
| return Duration(milliseconds: (timeRemainingBytes * 1000 / speed).round()); | ||
| } | ||
|
|
||
| static const _subBlocks = ['▏', '▎', '▍', '▌', '▋', '▊', '▉']; |
There was a problem hiding this comment.
nit: surprised you didn't use ' ' at the start and just use -1 rather than remainder > 0 ? 1 : 0
There was a problem hiding this comment.
Fixed, if I understood you correctly.
There was a problem hiding this comment.
Close, I was thinking:
['', '▏', '▎', '▍', '▌', '▋', '▊', '▉']
// then below you just use remainder:
_subBlocks[remainder]I guess you could use ' ' since emptyBlocks assumes a partial takes up some space. This is how I've done it in the past for progress bars.
Mr-Pepe
force-pushed
the
02-24-add_progress_bar_to_artifact_download
branch
from
3af15e1 to
6f05141
Compare
|
Thanks for your reviews! I hope I have addressed all the points. I think the fallback display without progress bar could still be improved but it's probably not worth it. |
| /// - [_ProgressBarDisplay]: ANSI progress bar for terminals with color support. | ||
| /// - [_SpinnerDisplay]: Spinner-based display via [Logger.startProgress]. | ||
| abstract class _DownloadDisplay { | ||
| void start(); |
There was a problem hiding this comment.
Nit: these function declarations could use some basic documentation.
jtmcdole
left a comment
jtmcdole
left a comment
There was a problem hiding this comment.
LGTM with clarity on the nit.
3 participants
Follow-up to #181808
Resolves #14268
Adds a new
DownloadProgressclass that contains the logic for tracking and formatting download progress, i.e., a progress bar, % display, download speed, and ETA.The rest is mostly ANSI control logic.
Bumped the width of the lines to 80 characters.
Before:
After:
Pre-launch Checklist
///).