Conversation
QFileSystemWatcher suppression currently relies on public static start/end methods and a static counter. Seven call sites pair them raw, one of them outside the class. Any exception between a pair permanently disables the watcher, and the static counter implies a singleton DownloadManager. A new DirWatcherManager owns the watcher, the counter (now an instance member), and the filtering. The only way to suspend is an RAII Guard obtained via a scopedGuard() factory. All raw pairs migrate to guards. A TODO flags the existing processEvents() in the dtor as a known reentrancy hazard worth replacing later.
Replace the fragile two-signal protocol with a refcounted RAII ModelResetGuard. Split update(int) into aboutToResetModel/modelReset (guard only) and rowChanged(int); notifyRowChanged() is suppressed while a reset is active. Fixes "beginResetModel without endResetModel" warnings from three sites in downloadFinished/removeDownload that were pairing reset with a row update. removePending only opens a guard when an actual match is removed.
setState emits notifyRowChanged itself, uses indexByInfo (-1 when untracked), and re-looks up the row at each use so reply->abort() and plugin callbacks that re-enter and erase info don't produce stale signals. Remove the trailing emit loop from createMetaFile and the now-redundant notifyRowChanged calls scattered after setState. Add the two missing emits in restoreDownload (after m_Hidden) and metaDataChanged (after rename). Guard downloadFinished with a top-level DirWatcherGuard to prevent filesystem events from its writes racing with model updates.
The game-name comparison result was discarded by the comma operator, so the dedup only matched modId/fileId across all games.
Hoist the file-exists prompt out of startDownload so setup is straight-line. Connect finished() last and dispatch manually if the reply already finished.
Move the allocation past the early-return checks so path-mismatch and hidden-skip paths no longer leak a fresh DownloadInfo.
The collision-avoidance branch was using the raw baseName, so invalid characters sanitized out of the initial path leaked into the suffixed one.
Agreed. Could we use a mutex to ensure no duplicate download IDs are generated when using |
The download manager runs on a single thread, so in theory a simple increasing counter should work. But I need to check if there are assumptions about what the id represents (e.g. index inside arrays or stuff like that. |
|
So the download ID is not really used in a way where random IDs would break something, as far as I followed all the code paths. |
Yeah, I think that race could happen, but a monotonically increasing Id should prevent any issues. |
m_AlphabeticalTranslation was written but never read; drop it along with refreshAlphabeticalTranslation, ByName, and the LessThanWrapper helper.
mick-lue
left a comment
mick-lue
left a comment
There was a problem hiding this comment.
2 minor comments, otherwise LGTM
Moves the ModelResetGuard out of the try-catch so it also wraps the refreshList() call below. Without this, one reset fires when the guard destructs at the end of the try block and another fires from refreshList's own guard, producing two resets where one is sufficient.
openMetaFile creates the .meta file via QSettings when one does not exist; the disk write fires directoryChanged and triggers a spurious refreshList. Wrap it in a DirWatcherManager::Guard like the other meta-file editing paths.
|
I'm going to rerun the build just to make sure the Qt 6.11.0 change doesn't cause problems, but I doubt it. |
4 participants
Summary
Refactor of the downloads view-model plumbing plus fixes for several long-standing bugs. Best reviewed commit-by-commit.
Changes
ModelResetGuardRAII helper replaces scatteredaboutToUpdate()/update(int)calls; row notifications centralised insetState; directory watcher encapsulated inDirWatcherManager.addNXMDownloaddedup check: stray comma operator was discarding the game-name comparison; all three of game/mod/file IDs now participate.finished()signal on fast downloads: hoisted the "Download again?" prompt out ofstartDownloadintoaddDownloadsostartDownloadis straight-line setup with no event-loop pumping or re-entrancy.finished()is connected last and dispatched manually if the reply already finished (Qt does not emit to a later connection). Pending-queue entry is cleaned up on the cancel branch.DownloadInfo::createFromMetaleak: allocation moved past the early-return checks so path-mismatch and hidden-skip paths no longer leak.getDownloadFileName: suffix-collision branch now uses the sanitised basename like the initial path.Status
This incorporates some of the fixes from #2363.
I'll mark this as ready, will handle the move to stable monotonically increasing download ids in a separate PRs that builds on top of this one.