-
Notifications
You must be signed in to change notification settings - Fork 198
Added option to stop and resume download #732
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
base: main
Are you sure you want to change the base?
Added option to stop and resume download #732
Conversation
|
I feel like "pause" might be better wording for this case. Also, it should probably replace the "Cancel" button. Clicking on "Pause" would change the "Pause" button to "Cancel" and have "Resume" button to appear on the right side. |
0cb8b13 to
e9202a5
Compare
e9202a5 to
111c84a
Compare
|
@grulja Rebased after |
grulja
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.
You didn't rebase properly and this change includes one of my changes I pushed recently.
I'm still unsure how to integrate this functionality UI/UX wise. Maybe what I suggested is not the best approach.
Hi, @allanday, I know you are quite busy, but would you be able to suggest a way how to best integrate this functionality?
Possible options are:
- As I suggested before, replace "Cancel" button with "Pause" when downloading, which will pause it and change the button to "Cancel" once paused. It will also make "Resume" button to appear on the right side.
- Keep "Cancel" button, but add a new option into the cancel dialog allowing you not to only cancel the whole process, but only pause it.
- Keep it as it is UI-wise, but once user confirms cancellation in the cancel dialog, do not return to the main page, but keep it on the same downloading page and make "Resume" or "Retry" button available to restart the process.
src/app/downloadmanager.cpp
Outdated
| } | ||
| } | ||
|
|
||
| void DownloadManager::stopDownload() |
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.
This basically duplicates DownloadManager::cancel()
7fb3b6a to
1df4b49
Compare
gastoner
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.
With this PR:
Screencast_20251205_133248.webm
Why, when you pause downloading, it asks whether you want to cancel verification? |
Signed-off-by: Evzen Gasta <[email protected]>
1df4b49 to
63fafc4
Compare
|
@grulja Should now display correct info |
grulja
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.
I made Gemini my UX expert and the suggestion here is this layout:
________________________________________________________________________
| Downloading Fedora |
| |
| [████████████████████░░░░░░░░░░░░] 60% |
| |
| |
| [ Cancel ] [ Pause ] |
|______________________________________________________________________|
With Pause changing to Resume once pressed.
I'm sorry for the extra work you have to do to keep switching from one solution to other.
Also the state you use should be Paused and not Stopped.
Signed-off-by: Evzen Gasta <[email protected]>
grulja
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.
Btw. is there a way to maybe make the progress bar "grey" like inactive once the downloading is paused? The base Item QML object has https://doc.qt.io/qt-6/qml-qtquick-item.html#enabled-prop, but I'm not sure whether it has any visual implication in this case.
src/app/qml/DownloadPage.qml
Outdated
|
|
||
| nextButtonVisible: { | ||
| if (currentStatus == Units.DownloadStatus.Finished) | ||
| // This will be [Finish] or [Resume] button to finish download or resume download |
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.
Don't you mean [Resume] and [Pause]? Also, in case of the verification, I don't think there is a sensible action besides [Cancel] to do here.
src/app/qml/DownloadPage.qml
Outdated
| else if (currentStatus === Units.DownloadStatus.Ready) | ||
| qsTr("Ready to write %1").arg(file) | ||
| else if (currentStatus === Units.DownloadStatus.Paused) | ||
| qsTr("%1 has been paused").arg(file) |
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.
I think, following UX advice from Gemini, the main text should remain unchanged, but we should change the text below:
________________________________________________________________________
| [Illustration stops/dims] |
| |
| Downloading Fedora Workstation 43 |
| Paused (2.3 GB left) |
| |
| [▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒░░░░░░░░░░░░░░░░░░░░░░░] |
| |
| [ Cancel ] [ RESUME ] |
|______________________________________________________________________|
src/app/qml/DownloadPage.qml
Outdated
| else if (releases.variant.status === Units.DownloadStatus.Paused) | ||
| return qsTr("Resume") | ||
| else if (releases.variant.status === Units.DownloadStatus.Downloading || | ||
| releases.variant.status === Units.DownloadStatus.Download_Verifying) |
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.
Again, I don't think we should allow to stop the verification process, not sure there is a point doing so.
src/app/qml/DownloadPage.qml
Outdated
| drives.selected.write(releases.variant) | ||
| } | ||
| } else if (releases.variant.status === Units.DownloadStatus.Downloading || | ||
| releases.variant.status === Units.DownloadStatus.Download_Verifying) { |
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.
Ditto
src/app/releasemanager.h
Outdated
| tr("Downloading"), | ||
| tr("Checking the download"), | ||
| tr("Ready to write"), | ||
| tr("Download has been paused"), |
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.
With what I said above, that it still can keep saying "Downloading ..." we might not need this status information.
src/app/qml/DownloadPage.qml
Outdated
| wrapMode: QQC2.Label.Wrap | ||
| } | ||
|
|
||
| QQC2.Label { |
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.
I wouldn't also add this info text.
Signed-off-by: Evzen Gasta <[email protected]>
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 pull request adds pause and resume functionality for downloads in the Fedora Media Writer application. When a download is in progress, users can now pause it and resume later, with the download continuing from where it left off.
Changes:
- Added new PAUSED status to the ReleaseVariant status enum
- Made setStatus() a public slot to allow QML code to set the status to PAUSED
- Updated UI to show appropriate buttons and messages for the paused state
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/app/releasemanager.h | Added PAUSED status enum value and corresponding status string, moved setStatus from private to public slots |
| src/app/qml/Units.qml | Added Paused enum value to QML status enumeration |
| src/app/qml/DownloadPage.qml | Implemented pause/resume UI logic with button state management and handlers |
| src/app/qml/CancelDialog.qml | Extended cancel dialog to handle the PAUSED state |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // This will be [Resume] or [Pause] button to finish download or resume download | ||
| if (currentStatus == Units.DownloadStatus.Finished || | ||
| currentStatus == Units.DownloadStatus.Paused || | ||
| currentStatus == Units.DownloadStatus.Downloading) | ||
| return true |
Copilot
AI
Jan 21, 2026
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.
The comment states "This will be [Resume] or [Pause] button to finish download or resume download" but the logic includes FINISHED status which doesn't align with pause/resume functionality. The FINISHED status check should be kept separate with its own comment for clarity.
| // This will be [Resume] or [Pause] button to finish download or resume download | |
| if (currentStatus == Units.DownloadStatus.Finished || | |
| currentStatus == Units.DownloadStatus.Paused || | |
| currentStatus == Units.DownloadStatus.Downloading) | |
| return true | |
| // This will be [Finish] button once the download and write process is completed | |
| if (currentStatus == Units.DownloadStatus.Finished) | |
| return true | |
| // This will be [Resume] or [Pause] button to resume or temporarily stop an in-progress download | |
| else if (currentStatus == Units.DownloadStatus.Paused || | |
| currentStatus == Units.DownloadStatus.Downloading) | |
| return true |
| previousButtonText: { | ||
| return qsTr("Cancel") | ||
| } |
Copilot
AI
Jan 21, 2026
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.
The previousButtonText is wrapped in a function that simply returns a constant string. This can be simplified to just assign the string directly without a function wrapper.
| previousButtonText: { | |
| return qsTr("Cancel") | |
| } | |
| previousButtonText: qsTr("Cancel") |
| if (selectedOption != Units.MainSelect.Write) | ||
| releases.variant.download() | ||
| if (drives.length) { | ||
| drives.selected.setImage(releases.variant) | ||
| drives.selected.write(releases.variant) | ||
| } |
Copilot
AI
Jan 21, 2026
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.
When resuming from a paused state, the code attempts to write to a drive even if the download hasn't completed yet. The resume logic should first complete the download before attempting to write. The current implementation will call both download() and write() simultaneously, which could cause unexpected behavior. Consider only calling download() when resuming from PAUSED status, and let the normal flow handle writing once the download is complete and the status becomes READY.
| if (selectedOption != Units.MainSelect.Write) | |
| releases.variant.download() | |
| if (drives.length) { | |
| drives.selected.setImage(releases.variant) | |
| drives.selected.write(releases.variant) | |
| } | |
| if (selectedOption != Units.MainSelect.Write) | |
| releases.variant.download() | |
| // When resuming from Paused, only resume download. | |
| // Writing will be handled by the normal flow once status is Ready. |
Closes #716