Draft
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.
m_AlphabeticalTranslation was written but never read; drop it along with refreshAlphabeticalTranslation, ByName, and the LessThanWrapper helper.
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.
Replace the (game, mod, file) tuple backing m_PendingDownloads with a named struct, and add m_ByID as an O(1) m_DownloadID-to-info index kept in sync with every m_ActiveDownloads mutation. Encapsulate the id counter behind DownloadInfo::newDownloadID(), the only supported way to consume from s_NextDownloadID. Infrastructure only; external behaviour is unchanged.
startDownloadURLs / startDownloadNexusFile / addNXMDownload now reserve and return m_DownloadID instead of a stale index. Plugin callbacks fire with m_DownloadID; downloadPath looks up via m_ByID. nxmDownloadURLsAvailable threads the reserved id into the materializing DownloadInfo, and Nexus API failures wake waiting plugins via notifyPendingDownloadFailed. Incidental: startDownload now returns bool and frees newDownload on output-open failure; createMetaFile is deferred past that check so failed starts no longer leave an orphan .meta.
The old dual-use downloadFinished(int = 0) took either an explicit index or relied on sender() when called as a slot. Split into a sender-resolved slot and an id-based direct call, removing the ambiguous index-zero path.
Add a DownloadID type alias for the stable per-download handle and two public accessors (downloadIDAtRow, rowForDownloadID) so callers can translate between the view's row vocabulary and the model's id vocabulary without reaching into the manager's internals. DownloadList now embeds the DownloadID in QModelIndex::internalId() so any code holding an index can identify the download directly.
The four methods (cancel, pause, resume, resumeDownloadInt) now accept a DownloadID, resolve through m_ByID, and no longer care about row positions. Internal callers iterate DownloadInfo* or look up via id; DownloadsTab translates row -> id at the connect boundary so the view's int-shaped signals keep working unchanged. Also switches the remaining unsigned int signatures that refer to the download id (finishDownload, downloadInfoByID, PendingDownload::reservedID, m_ByID, newDownloadID, s_NextDownloadID) to the DownloadID alias. Drive-by fix: finishDownload's retry branch could read info->m_Tries after info had been deleted in the CANCELED/retries-exhausted branch above; now re-resolves via m_ByID.value(id) before touching any fields.
Member
|
Don't think I have any major issues with these changes but I guess we're waiting on the first PR to be merged? It would be nice to isolate these changes from the PR. |
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
2 participants
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.
Summary:
TBD