prevent close event while non-cancelable async tasks are running - #525
Merged
Conversation
Reviewer's GuideImplements non-cancelable async tasks and updates window-close behavior so the app will not close (or redundantly prompt) while a non-interruptible async operation, such as saving, is in progress, and only writes settings once the close has been accepted. Sequence diagram for window close with non-cancelable async tasksequenceDiagram
actor User
participant MainWindow
participant AsyncTask
participant AsyncBase
participant ProgressDialog
User->>MainWindow: closeEvent(event)
Note over MainWindow: First async check
MainWindow->>AsyncTask: is_allowed_to_cancel()
AsyncTask->>AsyncBase: is_allowed_to_cancel()
AsyncBase-->>AsyncTask: cancelDisposition == Allow ? true : false
AsyncTask-->>MainWindow: bool
alt async task non_cancelable
MainWindow-->>User: event->ignore() (window stays open)
else async task cancelable or no task
MainWindow->>MainWindow: maybeSave()
alt maybeSave returns false
MainWindow-->>User: event->ignore()
else maybeSave returns true
Note over MainWindow: Second async check
MainWindow->>AsyncTask: is_allowed_to_cancel()
AsyncTask->>AsyncBase: is_allowed_to_cancel()
AsyncBase-->>AsyncTask: bool
AsyncTask-->>MainWindow: bool
alt now non_cancelable (e.g. save just scheduled)
MainWindow-->>User: event->ignore()
else cancelable
MainWindow->>AsyncTask: request_cancel()
AsyncTask->>AsyncBase: request_cancel()
AsyncBase->>AsyncBase: is_allowed_to_cancel()
AsyncBase->>ProgressCounter: requestCancel()
MainWindow->>ProgressDialog: reject()
MainWindow->>MainWindow: writeSettings()
MainWindow-->>User: event->accept() (window closes)
end
end
end
Class diagram for async cancel disposition and close handlingclassDiagram
class MainWindow {
+void closeEvent(QCloseEvent *event)
+bool maybeSave()
+void writeSettings()
-AsyncTask m_asyncTask
-std::unique_ptr<QProgressDialog> m_progressDlg
}
class AsyncTask {
-std::unique_ptr<AsyncBase> m_task
+void begin(std::unique_ptr<AsyncBase> task)
+void tick()
+void request_cancel()
+bool is_allowed_to_cancel() const
+void reset()
}
class AsyncBase {
+AsyncBase(std::shared_ptr<ProgressCounter> pc, CancelDispositionEnum cancelDisposition)
+~AsyncBase()
+PollResultEnum poll()
+PollResultEnum poll(std::chrono::milliseconds timeout)
+void request_cancel()
+bool requested_cancel() const
+bool is_allowed_to_cancel() const
+void virt_request_cancel()
+PollResultEnum virt_poll(std::chrono::milliseconds timeout)
+const std::shared_ptr<ProgressCounter> progressCounter
+const CancelDispositionEnum cancelDisposition
}
class AsyncHelper {
+AsyncHelper(MainWindow &mw, QString name, std::unique_ptr<QSaveFile> pd, UniqueStorage ps, QString dialogText, CancelDispositionEnum allow_cancel)
+MainWindow &mainWindow
+QString fileName
}
class CancelDispositionEnum {
<<enumeration>>
Allow
Disallow
}
class ProgressCounter {
+void requestCancel()
+bool requestedCancel() const
}
MainWindow o-- AsyncTask
MainWindow o-- QProgressDialog
AsyncTask *-- AsyncBase
AsyncBase o-- ProgressCounter
AsyncHelper --|> AsyncBase
AsyncBase --> CancelDispositionEnum
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The closeEvent logic now has two separate blocks checking m_asyncTask cancelability and ignoring the event; consider centralizing this into a single helper (e.g., canCloseForAsyncState()) to avoid duplicated conditions and keep future changes to the close/async interaction in one place.
- AsyncBase::request_cancel() silently returns when the task is non-cancelable; you might want to either return a bool indicating whether a cancel was actually requested or log/rename the method so callers are not misled into thinking a cancel always occurs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The closeEvent logic now has two separate blocks checking m_asyncTask cancelability and ignoring the event; consider centralizing this into a single helper (e.g., canCloseForAsyncState()) to avoid duplicated conditions and keep future changes to the close/async interaction in one place.
- AsyncBase::request_cancel() silently returns when the task is non-cancelable; you might want to either return a bool indicating whether a cancel was actually requested or log/rename the method so callers are not misled into thinking a cancel always occurs.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Before this change, the game could close before the async saver had a chance to finish. This commit: - Adds a 'cancelDisposition' to AsyncBase to define if a task is interruptible. - Updates MainWindow::closeEvent to ignore the close request if an active async task is marked as non-cancelable. - Ensures settings are written only after the close event is accepted. - Prevents redundant save prompts when a save task is already in progress.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #525 +/- ##
==========================================
- Coverage 25.40% 25.39% -0.02%
==========================================
Files 519 519
Lines 43109 43130 +21
Branches 4699 4705 +6
==========================================
Hits 10952 10952
- Misses 32157 32178 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before this change, the game could close before the async saver had a chance to finish.
This commit:
Summary by Sourcery
Guard window closure while non-cancelable async tasks are running and improve async task cancellation handling during shutdown.
New Features:
Bug Fixes:
Enhancements: