From 829ee4f9c238d8e96687a10473df8d514e388c9b Mon Sep 17 00:00:00 2001 From: AL <26797547+Al12rs@users.noreply.github.com> Date: Sat, 18 Apr 2026 16:59:54 +0200 Subject: [PATCH 01/30] Encapsulate the downloads directory watcher in DirWatcherManager 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. --- src/downloadmanager.cpp | 141 ++++++++++++++++++++----------------- src/downloadmanager.h | 91 +++++++++++++++--------- src/modlistviewactions.cpp | 10 +-- src/organizercore.cpp | 2 +- 4 files changed, 143 insertions(+), 101 deletions(-) diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index c5d4c13ee..9544521a9 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -56,7 +56,6 @@ using namespace MOBase; static const char UNFINISHED[] = ".unfinished"; unsigned int DownloadManager::DownloadInfo::s_NextDownloadID = 1U; -int DownloadManager::m_DirWatcherDisabler = 0; DownloadManager::DownloadInfo* DownloadManager::DownloadInfo::createNew(const ModRepositoryFileInfo* fileInfo, @@ -156,34 +155,54 @@ DownloadManager::DownloadInfo::createFromMeta(const QString& filePath, bool show return info; } -ScopedDisableDirWatcher::ScopedDisableDirWatcher(DownloadManager* downloadManager) +DirWatcherManager::DirWatcherManager(QObject* parent) : QObject(parent) { - m_downloadManager = downloadManager; - m_downloadManager->startDisableDirWatcher(); - log::debug("Scoped Disable DirWatcher: Started"); + connect(&m_watcher, &QFileSystemWatcher::directoryChanged, this, + &DirWatcherManager::onDirectoryChanged); } -ScopedDisableDirWatcher::~ScopedDisableDirWatcher() +void DirWatcherManager::setPath(const QString& path) { - m_downloadManager->endDisableDirWatcher(); - m_downloadManager = nullptr; - log::debug("Scoped Disable DirWatcher: Stopped"); + const QStringList existing = m_watcher.directories(); + if (!existing.isEmpty()) { + m_watcher.removePaths(existing); + } + m_watcher.addPath(path); } -void DownloadManager::startDisableDirWatcher() +bool DirWatcherManager::isSuspended() const { - DownloadManager::m_DirWatcherDisabler++; + return m_suspendDepth > 0; } -void DownloadManager::endDisableDirWatcher() +void DirWatcherManager::onDirectoryChanged(const QString&) { - if (DownloadManager::m_DirWatcherDisabler > 0) { - if (DownloadManager::m_DirWatcherDisabler == 1) - QCoreApplication::processEvents(); - DownloadManager::m_DirWatcherDisabler--; - } else { - DownloadManager::m_DirWatcherDisabler = 0; + if (!isSuspended()) { + emit directoryChanged(); + } +} + +DirWatcherManager::Guard::Guard(DirWatcherManager& manager) : m_manager(manager) +{ + ++m_manager.m_suspendDepth; +} + +DirWatcherManager::Guard::~Guard() +{ + // drain queued events while still suspended so they are filtered out, + // then release. + // + // TODO: find alternative, pumping the event loop from a destructor is a reentrancy hazard; + // arbitrary slots may run during ~Guard. + if (m_manager.m_suspendDepth == 1) { + QCoreApplication::processEvents(); } + --m_manager.m_suspendDepth; +} + +DirWatcherManager::Guard DirWatcherManager::scopedGuard() +{ + return Guard(*this); } void DownloadManager::DownloadInfo::setName(QString newName, bool renameFile) @@ -225,12 +244,12 @@ QString DownloadManager::DownloadInfo::currentURL() } DownloadManager::DownloadManager(NexusInterface* nexusInterface, QObject* parent) - : m_NexusInterface(nexusInterface), m_DirWatcher(), m_ShowHidden(false), + : m_NexusInterface(nexusInterface), m_DirWatcher(this), m_ShowHidden(false), m_ParentWidget(nullptr) { m_OrganizerCore = dynamic_cast(parent); - connect(&m_DirWatcher, SIGNAL(directoryChanged(QString)), this, - SLOT(directoryChanged(QString))); + connect(&m_DirWatcher, &DirWatcherManager::directoryChanged, this, + &DownloadManager::refreshList); m_TimeoutTimer.setSingleShot(false); // connect(&m_TimeoutTimer, SIGNAL(timeout()), this, SLOT(checkDownloadTimeout())); m_TimeoutTimer.start(5 * 1000); @@ -308,15 +327,11 @@ void DownloadManager::pauseAll() void DownloadManager::setOutputDirectory(const QString& outputDirectory, const bool refresh) { - QStringList directories = m_DirWatcher.directories(); - if (directories.length() != 0) { - m_DirWatcher.removePaths(directories); - } m_OutputDirectory = QDir::fromNativeSeparators(outputDirectory); if (refresh) { refreshList(); } - m_DirWatcher.addPath(m_OutputDirectory); + m_DirWatcher.setPath(m_OutputDirectory); } void DownloadManager::setShowHidden(bool showHidden) @@ -337,7 +352,7 @@ void DownloadManager::refreshList() try { emit aboutToUpdate(); // avoid triggering other refreshes - ScopedDisableDirWatcher scopedDirWatcher(this); + DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); int downloadsBefore = m_ActiveDownloads.size(); @@ -463,13 +478,14 @@ void DownloadManager::queryDownloadListInfo() QMessageBox::Yes | QMessageBox::No) == QMessageBox::Yes) { TimeThis tt("DownloadManager::queryDownloadListInfo()"); log::info("Querying metadata for every download with incomplete info..."); - startDisableDirWatcher(); - for (size_t i = 0; i < m_ActiveDownloads.size(); i++) { - if (isInfoIncomplete(i)) { - queryInfoMd5(i, false); + { + DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); + for (size_t i = 0; i < m_ActiveDownloads.size(); i++) { + if (isInfoIncomplete(i)) { + queryInfoMd5(i, false); + } } } - endDisableDirWatcher(); log::info("Metadata has been retrieved successfully!"); } } @@ -553,9 +569,10 @@ bool DownloadManager::addDownload(QNetworkReply* reply, const QStringList& URLs, baseName.truncate(queryIndex); } - startDisableDirWatcher(); - newDownload->setName(getDownloadFileName(baseName), false); - endDisableDirWatcher(); + { + DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); + newDownload->setName(getDownloadFileName(baseName), false); + } startDownload(reply, newDownload, false); // emit update(-1); @@ -646,9 +663,11 @@ void DownloadManager::startDownload(QNetworkReply* reply, DownloadInfo* newDownl else setState(newDownload, STATE_CANCELING); } else { - startDisableDirWatcher(); - newDownload->setName(getDownloadFileName(newDownload->m_FileName, true), true); - endDisableDirWatcher(); + { + DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); + newDownload->setName(getDownloadFileName(newDownload->m_FileName, true), + true); + } if (newDownload->m_State == STATE_PAUSED) resumeDownload(indexByInfo(newDownload)); else @@ -811,7 +830,7 @@ void DownloadManager::addNXMDownload(const QString& url) void DownloadManager::removeFile(int index, bool deleteFile) { // Avoid triggering refreshes from DirWatcher - ScopedDisableDirWatcher scopedDirWatcher(this); + DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); if (index >= m_ActiveDownloads.size()) { throw MyException(tr("remove: invalid download index %1").arg(index)); @@ -907,11 +926,9 @@ void DownloadManager::restoreDownload(int index) QString filePath = m_OutputDirectory + "/" + download->m_FileName; // avoid dirWatcher triggering refreshes - startDisableDirWatcher(); + DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); QSettings metaSettings(filePath.append(".meta"), QSettings::IniFormat); metaSettings.setValue("removed", false); - - endDisableDirWatcher(); } } } @@ -920,7 +937,7 @@ void DownloadManager::removeDownload(int index, bool deleteFile) { try { // avoid dirWatcher triggering refreshes - ScopedDisableDirWatcher scopedDirWatcher(this); + DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); emit aboutToUpdate(); @@ -1509,7 +1526,7 @@ void DownloadManager::markInstalled(int index) } // Avoid triggering refreshes from DirWatcher - ScopedDisableDirWatcher scopedDirWatcher(this); + DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); DownloadInfo* info = m_ActiveDownloads.at(index); QSettings metaFile(info->m_Output.fileName() + ".meta", QSettings::IniFormat); @@ -1528,7 +1545,7 @@ void DownloadManager::markInstalled(QString fileName) DownloadInfo* info = getDownloadInfo(fileName); if (info != nullptr) { // Avoid triggering refreshes from DirWatcher - ScopedDisableDirWatcher scopedDirWatcher(this); + DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); QSettings metaFile(info->m_Output.fileName() + ".meta", QSettings::IniFormat); metaFile.setValue("installed", true); @@ -1550,7 +1567,7 @@ void DownloadManager::markUninstalled(int index) } // Avoid triggering refreshes from DirWatcher - ScopedDisableDirWatcher scopedDirWatcher(this); + DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); DownloadInfo* info = m_ActiveDownloads.at(index); QSettings metaFile(info->m_Output.fileName() + ".meta", QSettings::IniFormat); @@ -1570,7 +1587,7 @@ void DownloadManager::markUninstalled(QString fileName) if (info != nullptr) { // Avoid triggering refreshes from DirWatcher - ScopedDisableDirWatcher scopedDirWatcher(this); + DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); QSettings metaFile(info->m_Output.fileName() + ".meta", QSettings::IniFormat); metaFile.setValue("uninstalled", true); @@ -1738,7 +1755,7 @@ void DownloadManager::downloadReadyRead() void DownloadManager::createMetaFile(DownloadInfo* info) { // Avoid triggering refreshes from DirWatcher - ScopedDisableDirWatcher scopedDirWatcher(this); + DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); QSettings metaFile(QString("%1.meta").arg(info->m_Output.fileName()), QSettings::IniFormat); @@ -2364,14 +2381,15 @@ void DownloadManager::downloadFinished(int index) QString newName = getFileNameFromNetworkReply(reply); QString oldName = QFileInfo(info->m_Output).fileName(); - startDisableDirWatcher(); - if (!newName.isEmpty() && (oldName.isEmpty())) { - info->setName(getDownloadFileName(newName), true); - } else { - info->setName(m_OutputDirectory + "/" + info->m_FileName, - true); // don't rename but remove the ".unfinished" extension + { + DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); + if (!newName.isEmpty() && (oldName.isEmpty())) { + info->setName(getDownloadFileName(newName), true); + } else { + info->setName(m_OutputDirectory + "/" + info->m_FileName, + true); // don't rename but remove the ".unfinished" extension + } } - endDisableDirWatcher(); if (!isNexus) { setState(info, STATE_READY); @@ -2409,9 +2427,10 @@ void DownloadManager::metaDataChanged() if (info != nullptr) { QString newName = getFileNameFromNetworkReply(info->m_Reply); if (!newName.isEmpty() && (info->m_FileName.isEmpty())) { - startDisableDirWatcher(); - info->setName(getDownloadFileName(newName), true); - endDisableDirWatcher(); + { + DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); + info->setName(getDownloadFileName(newName), true); + } refreshAlphabeticalTranslation(); if (!info->m_Output.isOpen() && !info->m_Output.open(QIODevice::WriteOnly | QIODevice::Append)) { @@ -2424,12 +2443,6 @@ void DownloadManager::metaDataChanged() } } -void DownloadManager::directoryChanged(const QString&) -{ - if (DownloadManager::m_DirWatcherDisabler == 0) - refreshList(); -} - void DownloadManager::managedGameChanged(MOBase::IPluginGame const* managedGame) { m_ManagedGame = managedGame; diff --git a/src/downloadmanager.h b/src/downloadmanager.h index ad93b87d2..f6ad259a0 100644 --- a/src/downloadmanager.h +++ b/src/downloadmanager.h @@ -52,6 +52,58 @@ class NexusInterface; class PluginContainer; class OrganizerCore; +/** + * @brief QFileSystemWatcher with a nestable RAII suspension scope. + * + * Forwards directoryChanged() only while no Guard is alive. Use a Guard to + * bracket filesystem writes that would otherwise trigger a spurious refresh. + */ +class DirWatcherManager : public QObject +{ + Q_OBJECT + +public: + explicit DirWatcherManager(QObject* parent = nullptr); + + /// Set the directory being watched (replaces any previous path). + void setPath(const QString& path); + + /// True while one or more Guards are alive. + bool isSuspended() const; + + /** + * @brief RAII suspension guard. Nests safely; the only way to suspend + * forwarding. + */ + class [[nodiscard]] Guard + { + public: + explicit Guard(DirWatcherManager& manager); + ~Guard(); + Guard(const Guard&) = delete; + Guard& operator=(const Guard&) = delete; + Guard(Guard&&) = delete; + Guard& operator=(Guard&&) = delete; + + private: + DirWatcherManager& m_manager; + }; + + /// Returns a suspension Guard bound to the caller's scope. + [[nodiscard]] Guard scopedGuard(); + +signals: + /// Emitted when the watched directory changes and no Guard is active. + void directoryChanged(); + +private slots: + void onDirectoryChanged(const QString&); + +private: + QFileSystemWatcher m_watcher; + int m_suspendDepth = 0; +}; + /*! * \brief manages downloading of files and provides progress information for gui *elements @@ -191,19 +243,6 @@ class DownloadManager : public QObject **/ void setOutputDirectory(const QString& outputDirectory, const bool refresh = true); - /** - * @brief disables feedback from the downlods fileSystemWhatcher untill - *disableDownloadsWatcherEnd() is called - * - **/ - static void startDisableDirWatcher(); - - /** - * @brief re-enables feedback from the downlods fileSystemWhatcher after - *disableDownloadsWatcherStart() was called - **/ - static void endDisableDirWatcher(); - /** * @return current download directory **/ @@ -408,6 +447,12 @@ class DownloadManager : public QObject */ void queryDownloadListInfo(); + /** + * @return the directory watcher for the downloads folder; call + * scopedGuard() on it to suspend across filesystem writes. + */ + DirWatcherManager& dirWatcher() { return m_DirWatcher; } + public: // IDownloadManager interface: int startDownloadURLs(const QStringList& urls); int startDownloadURLWithMeta(const QString& url, const QString& name, @@ -541,7 +586,6 @@ private slots: void downloadFinished(int index = 0); void downloadError(QNetworkReply::NetworkError error); void metaDataChanged(); - void directoryChanged(const QString& dirctory); void checkDownloadTimeout(); private: @@ -614,20 +658,13 @@ private slots: std::set m_RequestIDs; QVector m_AlphabeticalTranslation; - QFileSystemWatcher m_DirWatcher; + DirWatcherManager m_DirWatcher; SignalDownloadCallback m_DownloadComplete; SignalDownloadCallback m_DownloadPaused; SignalDownloadCallback m_DownloadFailed; SignalDownloadCallback m_DownloadRemoved; - // The dirWatcher is actually triggering off normal Mo operations such as deleting - // downloads or editing .meta files so it needs to be disabled during operations that - // are known to cause the creation or deletion of files in the Downloads folder. - // Notably using QSettings to edit a file creates a temporarily .lock file that causes - // the Watcher to trigger multiple listRefreshes freezing the ui. - static int m_DirWatcherDisabler; - std::map m_DownloadFails; bool m_ShowHidden; @@ -637,14 +674,4 @@ private slots: QTimer m_TimeoutTimer; }; -class ScopedDisableDirWatcher -{ -public: - ScopedDisableDirWatcher(DownloadManager* downloadManager); - ~ScopedDisableDirWatcher(); - -private: - DownloadManager* m_downloadManager; -}; - #endif // DOWNLOADMANAGER_H diff --git a/src/modlistviewactions.cpp b/src/modlistviewactions.cpp index 6b26a1e33..9f2896785 100644 --- a/src/modlistviewactions.cpp +++ b/src/modlistviewactions.cpp @@ -818,11 +818,13 @@ void ModListViewActions::removeMods(const QModelIndexList& indices) const QMessageBox::Yes | QMessageBox::No) == QMessageBox::Yes) { // use mod names instead of indexes because those become invalid during the // removal - DownloadManager::startDisableDirWatcher(); - for (QString name : modNames) { - m_core.modList()->removeRowForce(ModInfo::getIndex(name), QModelIndex()); + { + DirWatcherManager::Guard dirWatcherGuard = + m_core.downloadManager()->dirWatcher().scopedGuard(); + for (QString name : modNames) { + m_core.modList()->removeRowForce(ModInfo::getIndex(name), QModelIndex()); + } } - DownloadManager::endDisableDirWatcher(); } } else if (!indices.isEmpty()) { m_core.modList()->removeRow(indices[0].data(ModList::IndexRole).toInt(), diff --git a/src/organizercore.cpp b/src/organizercore.cpp index a8fc67c27..e2bbfc298 100644 --- a/src/organizercore.cpp +++ b/src/organizercore.cpp @@ -866,7 +866,7 @@ OrganizerCore::doInstall(const QString& archivePath, GuessedValue modNa ModInfo::Ptr OrganizerCore::installDownload(int index, int priority) { - ScopedDisableDirWatcher scopedDirwatcher(&m_DownloadManager); + DirWatcherManager::Guard dirWatcherGuard = m_DownloadManager.dirWatcher().scopedGuard(); try { QString fileName = m_DownloadManager.getFilePath(index); From 07faf9c6354ca4d8bbe358453bd50b6be200c4d3 Mon Sep 17 00:00:00 2001 From: AL <26797547+Al12rs@users.noreply.github.com> Date: Sat, 18 Apr 2026 17:09:41 +0200 Subject: [PATCH 02/30] Replace aboutToUpdate/update(int) with ModelResetGuard 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. --- src/downloadlist.cpp | 21 +++++--- src/downloadlist.h | 18 ++++--- src/downloadmanager.cpp | 115 +++++++++++++++++++++++----------------- src/downloadmanager.h | 52 ++++++++++++++++-- 4 files changed, 139 insertions(+), 67 deletions(-) diff --git a/src/downloadlist.cpp b/src/downloadlist.cpp index b461be505..d85786fe6 100644 --- a/src/downloadlist.cpp +++ b/src/downloadlist.cpp @@ -35,8 +35,10 @@ DownloadList::DownloadList(OrganizerCore& core, QObject* parent) : QAbstractTableModel(parent), m_manager(*core.downloadManager()), m_settings(core.settings()) { - connect(&m_manager, SIGNAL(update(int)), this, SLOT(update(int))); - connect(&m_manager, SIGNAL(aboutToUpdate()), this, SLOT(aboutToUpdate())); + connect(&m_manager, &DownloadManager::aboutToResetModel, this, + &DownloadList::onAboutToResetModel); + connect(&m_manager, &DownloadManager::modelReset, this, &DownloadList::onModelReset); + connect(&m_manager, &DownloadManager::rowChanged, this, &DownloadList::onRowChanged); } int DownloadList::rowCount(const QModelIndex& parent) const @@ -239,16 +241,19 @@ QVariant DownloadList::data(const QModelIndex& index, int role) const return QVariant(); } -void DownloadList::aboutToUpdate() +void DownloadList::onAboutToResetModel() { - emit beginResetModel(); + beginResetModel(); } -void DownloadList::update(int row) +void DownloadList::onModelReset() { - if (row < 0) - emit endResetModel(); - else if (row < this->rowCount()) + endResetModel(); +} + +void DownloadList::onRowChanged(int row) +{ + if (row < this->rowCount()) emit dataChanged( this->index(row, 0, QModelIndex()), this->index(row, this->columnCount(QModelIndex()) - 1, QModelIndex())); diff --git a/src/downloadlist.h b/src/downloadlist.h index c9adaa7f2..538da71ac 100644 --- a/src/downloadlist.h +++ b/src/downloadlist.h @@ -83,16 +83,20 @@ class DownloadList : public QAbstractTableModel // bool lessThanPredicate(const QModelIndex& left, const QModelIndex& right); -public slots: +private slots: /** - * @brief used to inform the model that data has changed - * - * @param row the row that changed. This can be negative to update the whole view - **/ - void update(int row); + * @brief full reset (row count changed). Drops selection and scroll state. + */ + void onAboutToResetModel(); + void onModelReset(); - void aboutToUpdate(); + /** + * @brief single-row data change (row count unchanged). Preserves view state. + * + * @param row the row that changed + */ + void onRowChanged(int row); private: DownloadManager& m_manager; diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index 9544521a9..991448455 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -205,6 +205,30 @@ DirWatcherManager::Guard DirWatcherManager::scopedGuard() return Guard(*this); } +DownloadManager::ModelResetGuard::ModelResetGuard(DownloadManager& manager) + : m_manager(manager) +{ + if (m_manager.m_modelResetDepth++ == 0) { + emit m_manager.aboutToResetModel(); + } +} + +DownloadManager::ModelResetGuard::~ModelResetGuard() +{ + if (--m_manager.m_modelResetDepth == 0) { + emit m_manager.modelReset(); + } +} + +void DownloadManager::notifyRowChanged(int row) +{ + // suppressed while a reset is in progress; the outer reset will refresh + // all rows on release + if (m_modelResetDepth == 0) { + emit rowChanged(row); + } +} + void DownloadManager::DownloadInfo::setName(QString newName, bool renameFile) { QString oldMetaFileName = QString("%1.meta").arg(m_FileName); @@ -349,11 +373,10 @@ void DownloadManager::refreshList() { TimeThis tt("DownloadManager::refreshList()"); - try { - emit aboutToUpdate(); - // avoid triggering other refreshes - DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); + DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); + ModelResetGuard guard(*this); + try { int downloadsBefore = m_ActiveDownloads.size(); // remove finished downloads @@ -451,9 +474,6 @@ void DownloadManager::refreshList() }); log::debug("saw {} downloads", m_ActiveDownloads.size()); - - emit update(-1); - } catch (const std::bad_alloc&) { reportError(tr("Memory allocation error (in refreshing directory).")); } @@ -575,7 +595,6 @@ bool DownloadManager::addDownload(QNetworkReply* reply, const QStringList& URLs, } startDownload(reply, newDownload, false); - // emit update(-1); return true; } @@ -592,15 +611,15 @@ void DownloadManager::removePending(QString gameName, int modID, int fileID) break; } } - emit aboutToUpdate(); for (auto iter : m_PendingDownloads) { if (gameShortName.compare(std::get<0>(iter), Qt::CaseInsensitive) == 0 && (std::get<1>(iter) == modID) && (std::get<2>(iter) == fileID)) { + // only emit a reset if we actually have a row to remove + ModelResetGuard guard(*this); m_PendingDownloads.removeAt(m_PendingDownloads.indexOf(iter)); break; } } - emit update(-1); } void DownloadManager::startDownload(QNetworkReply* reply, DownloadInfo* newDownload, @@ -639,13 +658,14 @@ void DownloadManager::startDownload(QNetworkReply* reply, DownloadInfo* newDownl if (!resume) { newDownload->m_PreResumeSize = newDownload->m_Output.size(); - removePending(newDownload->m_FileInfo->gameName, newDownload->m_FileInfo->modID, - newDownload->m_FileInfo->fileID); - - emit aboutToUpdate(); - m_ActiveDownloads.append(newDownload); - - emit update(-1); + { + // coalesce the pending removal and the active append into one reset + ModelResetGuard guard(*this); + removePending(newDownload->m_FileInfo->gameName, + newDownload->m_FileInfo->modID, + newDownload->m_FileInfo->fileID); + m_ActiveDownloads.append(newDownload); + } emit downloadAdded(); if (QFile::exists(m_OutputDirectory + "/" + newDownload->m_FileName)) { @@ -808,12 +828,11 @@ void DownloadManager::addNXMDownload(const QString& url) } } - emit aboutToUpdate(); - - m_PendingDownloads.append( - std::make_tuple(foundGame->gameShortName(), nxmInfo.modId(), nxmInfo.fileId())); - - emit update(-1); + { + ModelResetGuard guard(*this); + m_PendingDownloads.append( + std::make_tuple(foundGame->gameShortName(), nxmInfo.modId(), nxmInfo.fileId())); + } emit downloadAdded(); ModRepositoryFileInfo* info = new ModRepositoryFileInfo(); @@ -935,11 +954,17 @@ void DownloadManager::restoreDownload(int index) void DownloadManager::removeDownload(int index, bool deleteFile) { + // validate the single-index case before entering the guard so we don't + // emit an empty reset on early return + if (index >= 0 && index >= m_ActiveDownloads.size()) { + reportError(tr("remove: invalid download index %1").arg(index)); + return; + } + try { - // avoid dirWatcher triggering refreshes DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); - - emit aboutToUpdate(); + // coalesce the removal and the subsequent refresh into one reset + ModelResetGuard guard(*this); if (index < 0) { bool removeAll = (index == -1); @@ -960,21 +985,15 @@ void DownloadManager::removeDownload(int index, bool deleteFile) } } } else { - if (index >= m_ActiveDownloads.size()) { - reportError(tr("remove: invalid download index %1").arg(index)); - // emit update(-1); - return; - } - removeFile(index, deleteFile); delete m_ActiveDownloads.at(index); m_ActiveDownloads.erase(m_ActiveDownloads.begin() + index); } - emit update(-1); + + refreshList(); } catch (const std::exception& e) { log::error("failed to remove download: {}", e.what()); } - refreshList(); } void DownloadManager::cancelDownload(int index) @@ -1074,7 +1093,7 @@ void DownloadManager::resumeDownloadInt(int index) log::debug("resume at {} bytes", info->m_ResumePos); startDownload(m_NexusInterface->getAccessManager()->get(request), info, true); } - emit update(index); + notifyRowChanged(index); } DownloadManager::DownloadInfo* DownloadManager::downloadInfoByID(unsigned int id) @@ -1735,7 +1754,7 @@ void DownloadManager::downloadProgress(qint64 bytesReceived, qint64 bytesTotal) TaskProgressManager::instance().updateProgress(info->m_TaskProgressId, bytesReceived, bytesTotal); - emit update(index); + notifyRowChanged(index); } } } catch (const std::bad_alloc&) { @@ -1785,7 +1804,7 @@ void DownloadManager::createMetaFile(DownloadInfo* info) // slightly hackish... for (int i = 0; i < m_ActiveDownloads.size(); ++i) { if (m_ActiveDownloads[i] == info) { - emit update(i); + notifyRowChanged(i); } } } @@ -2255,19 +2274,20 @@ void DownloadManager::nxmRequestFailed(QString gameName, int modID, int fileID, } else { info->m_State = STATE_READY; queryInfo(index); - emit update(index); + notifyRowChanged(index); return; } } if (info->m_FileInfo->modID == modID) { if (info->m_State < STATE_FETCHINGMODINFO) { + ModelResetGuard guard(*this); m_ActiveDownloads.erase(iter); delete info; } else { setState(info, STATE_READY); + notifyRowChanged(index); } - emit update(index); break; } } @@ -2335,22 +2355,21 @@ void DownloadManager::downloadFinished(int index) } if (info->m_State == STATE_CANCELED || (info->m_Tries == 0 && error)) { - emit aboutToUpdate(); - info->m_Output.remove(); - delete info; - m_ActiveDownloads.erase(m_ActiveDownloads.begin() + index); + { + ModelResetGuard guard(*this); + info->m_Output.remove(); + delete info; + m_ActiveDownloads.erase(m_ActiveDownloads.begin() + index); + } if (error) emit showMessage( tr("We were unable to download the file due to errors after four retries. " "There may be an issue with the Nexus servers.")); - emit update(-1); } else if (info->isPausedState() || info->m_State == STATE_PAUSING) { - emit aboutToUpdate(); info->m_Output.close(); createMetaFile(info); - emit update(index); + notifyRowChanged(index); } else { - emit aboutToUpdate(); QString url = info->m_Urls[info->m_CurrentUrl]; if (info->m_FileInfo->userData.contains("downloadMap")) { foreach (const QVariant& server, @@ -2395,7 +2414,7 @@ void DownloadManager::downloadFinished(int index) setState(info, STATE_READY); } - emit update(index); + notifyRowChanged(index); } reply->close(); reply->deleteLater(); diff --git a/src/downloadmanager.h b/src/downloadmanager.h index f6ad259a0..ff4f1bf1b 100644 --- a/src/downloadmanager.h +++ b/src/downloadmanager.h @@ -113,6 +113,25 @@ class DownloadManager : public QObject Q_OBJECT public: + /** + * @brief RAII full-reset guard. Use when the row count changes; drops + * view selection/scroll state. Nests safely: inner guards coalesce into + * the outermost scope so only one reset is emitted. + */ + class [[nodiscard]] ModelResetGuard + { + public: + explicit ModelResetGuard(DownloadManager& manager); + ~ModelResetGuard(); + ModelResetGuard(const ModelResetGuard&) = delete; + ModelResetGuard& operator=(const ModelResetGuard&) = delete; + ModelResetGuard(ModelResetGuard&&) = delete; + ModelResetGuard& operator=(ModelResetGuard&&) = delete; + + private: + DownloadManager& m_manager; + }; + enum DownloadState { STATE_STARTED = 0, @@ -480,16 +499,38 @@ class DownloadManager : public QObject void pauseAll(); + /** + * @brief notify the UI that a single row's data changed. Preserves view + * state; prefer over ModelResetGuard when the row count is unchanged. + * + * @param row the row that changed. This corresponds to the download index + */ + void notifyRowChanged(int row); + Q_SIGNALS: - void aboutToUpdate(); + /** + * @brief emitted before the download list model is about to be reset + * + * Emitted by ModelResetGuard on construction. Views should call + * beginResetModel() in response. + */ + void aboutToResetModel(); /** - * @brief signals that the specified download has changed + * @brief emitted after the download list model has been reset + * + * Emitted by ModelResetGuard on destruction. Views should call + * endResetModel() in response. + */ + void modelReset(); + + /** + * @brief signals that the specified download row's data has changed * * @param row the row that changed. This corresponds to the download index - **/ - void update(int row); + */ + void rowChanged(int row); /** * @brief signals the ui that a message should be displayed @@ -660,6 +701,9 @@ private slots: DirWatcherManager m_DirWatcher; + // nesting depth of active ModelResetGuard scopes; see its docs + int m_modelResetDepth = 0; + SignalDownloadCallback m_DownloadComplete; SignalDownloadCallback m_DownloadPaused; SignalDownloadCallback m_DownloadFailed; From c94858be4d0d7d9afeb1fab2c9047c268f101a03 Mon Sep 17 00:00:00 2001 From: AL <26797547+Al12rs@users.noreply.github.com> Date: Sat, 18 Apr 2026 17:15:29 +0200 Subject: [PATCH 03/30] Centralize row notifications in setState and fix missed emits 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. --- src/downloadmanager.cpp | 70 +++++++++++++++++------------------------ 1 file changed, 29 insertions(+), 41 deletions(-) diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index 991448455..4e68dd9d2 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -848,7 +848,6 @@ void DownloadManager::addNXMDownload(const QString& url) void DownloadManager::removeFile(int index, bool deleteFile) { - // Avoid triggering refreshes from DirWatcher DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); if (index >= m_ActiveDownloads.size()) { @@ -944,10 +943,13 @@ void DownloadManager::restoreDownload(int index) QString filePath = m_OutputDirectory + "/" + download->m_FileName; - // avoid dirWatcher triggering refreshes - DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); - QSettings metaSettings(filePath.append(".meta"), QSettings::IniFormat); - metaSettings.setValue("removed", false); + { + DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); + QSettings metaSettings(filePath.append(".meta"), QSettings::IniFormat); + metaSettings.setValue("removed", false); + } + + notifyRowChanged(index); } } } @@ -1093,7 +1095,6 @@ void DownloadManager::resumeDownloadInt(int index) log::debug("resume at {} bytes", info->m_ResumePos); startDownload(m_NexusInterface->getAccessManager()->get(request), info, true); } - notifyRowChanged(index); } DownloadManager::DownloadInfo* DownloadManager::downloadInfoByID(unsigned int id) @@ -1544,7 +1545,6 @@ void DownloadManager::markInstalled(int index) throw MyException(tr("mark installed: invalid download index %1").arg(index)); } - // Avoid triggering refreshes from DirWatcher DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); DownloadInfo* info = m_ActiveDownloads.at(index); @@ -1563,7 +1563,6 @@ void DownloadManager::markInstalled(QString fileName) } else { DownloadInfo* info = getDownloadInfo(fileName); if (info != nullptr) { - // Avoid triggering refreshes from DirWatcher DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); QSettings metaFile(info->m_Output.fileName() + ".meta", QSettings::IniFormat); @@ -1585,7 +1584,6 @@ void DownloadManager::markUninstalled(int index) throw MyException(tr("mark uninstalled: invalid download index %1").arg(index)); } - // Avoid triggering refreshes from DirWatcher DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); DownloadInfo* info = m_ActiveDownloads.at(index); @@ -1605,7 +1603,6 @@ void DownloadManager::markUninstalled(QString fileName) DownloadInfo* info = getDownloadInfo(filePath); if (info != nullptr) { - // Avoid triggering refreshes from DirWatcher DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); QSettings metaFile(info->m_Output.fileName() + ".meta", QSettings::IniFormat); @@ -1648,28 +1645,26 @@ QString DownloadManager::getFileNameFromNetworkReply(QNetworkReply* reply) void DownloadManager::setState(DownloadManager::DownloadInfo* info, DownloadManager::DownloadState state) { - int row = 0; - for (int i = 0; i < m_ActiveDownloads.size(); ++i) { - if (m_ActiveDownloads[i] == info) { - row = i; - break; - } - } info->m_State = state; + // Row is re-looked up at each use: reply->abort() and plugin callbacks can + // re-enter and erase info (and info may not be tracked yet on first state). switch (state) { case STATE_PAUSED: { info->m_Reply->abort(); info->m_Output.close(); - m_DownloadPaused(row); + if (const int r = indexByInfo(info); r >= 0) + m_DownloadPaused(r); } break; case STATE_ERROR: { info->m_Reply->abort(); info->m_Output.close(); - m_DownloadFailed(row); + if (const int r = indexByInfo(info); r >= 0) + m_DownloadFailed(r); } break; case STATE_CANCELED: { info->m_Reply->abort(); - m_DownloadFailed(row); + if (const int r = indexByInfo(info); r >= 0) + m_DownloadFailed(r); } break; case STATE_FETCHINGMODINFO: { m_RequestIDs.insert(m_NexusInterface->requestDescription( @@ -1689,12 +1684,16 @@ void DownloadManager::setState(DownloadManager::DownloadInfo* info, } break; case STATE_READY: { createMetaFile(info); - m_DownloadComplete(row); + if (const int r = indexByInfo(info); r >= 0) + m_DownloadComplete(r); } break; default: /* NOP */ break; } - emit stateChanged(row, state); + if (const int row = indexByInfo(info); row >= 0) { + emit stateChanged(row, state); + notifyRowChanged(row); + } } DownloadManager::DownloadInfo* DownloadManager::findDownload(QObject* reply, @@ -1773,7 +1772,6 @@ void DownloadManager::downloadReadyRead() void DownloadManager::createMetaFile(DownloadInfo* info) { - // Avoid triggering refreshes from DirWatcher DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); QSettings metaFile(QString("%1.meta").arg(info->m_Output.fileName()), @@ -1800,13 +1798,6 @@ void DownloadManager::createMetaFile(DownloadInfo* info) metaFile.setValue("paused", (info->m_State == DownloadManager::STATE_PAUSED) || (info->m_State == DownloadManager::STATE_ERROR)); metaFile.setValue("removed", info->m_Hidden); - - // slightly hackish... - for (int i = 0; i < m_ActiveDownloads.size(); ++i) { - if (m_ActiveDownloads[i] == info) { - notifyRowChanged(i); - } - } } void DownloadManager::nxmDescriptionAvailable(QString, int, QVariant userData, @@ -2274,7 +2265,6 @@ void DownloadManager::nxmRequestFailed(QString gameName, int modID, int fileID, } else { info->m_State = STATE_READY; queryInfo(index); - notifyRowChanged(index); return; } } @@ -2286,7 +2276,6 @@ void DownloadManager::nxmRequestFailed(QString gameName, int modID, int fileID, delete info; } else { setState(info, STATE_READY); - notifyRowChanged(index); } break; } @@ -2298,6 +2287,8 @@ void DownloadManager::nxmRequestFailed(QString gameName, int modID, int fileID, void DownloadManager::downloadFinished(int index) { + DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); + DownloadInfo* info; if (index > 0) info = m_ActiveDownloads[index]; @@ -2368,7 +2359,6 @@ void DownloadManager::downloadFinished(int index) } else if (info->isPausedState() || info->m_State == STATE_PAUSING) { info->m_Output.close(); createMetaFile(info); - notifyRowChanged(index); } else { QString url = info->m_Urls[info->m_CurrentUrl]; if (info->m_FileInfo->userData.contains("downloadMap")) { @@ -2400,14 +2390,11 @@ void DownloadManager::downloadFinished(int index) QString newName = getFileNameFromNetworkReply(reply); QString oldName = QFileInfo(info->m_Output).fileName(); - { - DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); - if (!newName.isEmpty() && (oldName.isEmpty())) { - info->setName(getDownloadFileName(newName), true); - } else { - info->setName(m_OutputDirectory + "/" + info->m_FileName, - true); // don't rename but remove the ".unfinished" extension - } + if (!newName.isEmpty() && (oldName.isEmpty())) { + info->setName(getDownloadFileName(newName), true); + } else { + info->setName(m_OutputDirectory + "/" + info->m_FileName, + true); // don't rename but remove the ".unfinished" extension } if (!isNexus) { @@ -2451,6 +2438,7 @@ void DownloadManager::metaDataChanged() info->setName(getDownloadFileName(newName), true); } refreshAlphabeticalTranslation(); + notifyRowChanged(index); if (!info->m_Output.isOpen() && !info->m_Output.open(QIODevice::WriteOnly | QIODevice::Append)) { reportError(tr("failed to re-open %1").arg(info->m_FileName)); From f7bb36b1b181a2aeb79c478b007bc157116962ea Mon Sep 17 00:00:00 2001 From: AL <26797547+Al12rs@users.noreply.github.com> Date: Sat, 18 Apr 2026 17:20:02 +0200 Subject: [PATCH 04/30] Fix comma operator in addNXMDownload pending-dedup check The game-name comparison result was discarded by the comma operator, so the dedup only matched modId/fileId across all games. --- src/downloadmanager.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index 4e68dd9d2..00b24a727 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -760,10 +760,9 @@ void DownloadManager::addNXMDownload(const QString& url) } for (auto tuple : m_PendingDownloads) { - if (std::get<0>(tuple).compare(foundGame->gameShortName(), Qt::CaseInsensitive) == - 0, + if (std::get<0>(tuple).compare(foundGame->gameShortName(), Qt::CaseInsensitive) == 0 && std::get<1>(tuple) == nxmInfo.modId() && - std::get<2>(tuple) == nxmInfo.fileId()) { + std::get<2>(tuple) == nxmInfo.fileId()) { const auto infoStr = tr("There is already a download queued for this file.\n\nMod %1\nFile %2") .arg(nxmInfo.modId()) From 1e8ada534819caf27ebff989838dbededab292a8 Mon Sep 17 00:00:00 2001 From: AL <26797547+Al12rs@users.noreply.github.com> Date: Sat, 18 Apr 2026 18:27:22 +0200 Subject: [PATCH 05/30] Fix lost finished() signal on fast downloads Hoist the file-exists prompt out of startDownload so setup is straight-line. Connect finished() last and dispatch manually if the reply already finished. --- src/downloadmanager.cpp | 68 ++++++++++++++++++----------------------- 1 file changed, 30 insertions(+), 38 deletions(-) diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index 00b24a727..ca5958175 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -589,9 +589,29 @@ bool DownloadManager::addDownload(QNetworkReply* reply, const QStringList& URLs, baseName.truncate(queryIndex); } + bool renameToUnique = false; + if (QFile::exists(m_OutputDirectory + "/" + MOBase::sanitizeFileName(baseName))) { + if (QMessageBox::question( + m_ParentWidget, tr("Download again?"), + tr("A file with the same name \"%1\" has already been downloaded. " + "Do you want to download it again? The new file will receive a " + "different name.") + .arg(baseName), + QMessageBox::Yes | QMessageBox::No) == QMessageBox::No) { + removePending(newDownload->m_FileInfo->gameName, + newDownload->m_FileInfo->modID, + newDownload->m_FileInfo->fileID); + reply->abort(); + reply->deleteLater(); + delete newDownload; + return false; + } + renameToUnique = true; + } + { DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); - newDownload->setName(getDownloadFileName(baseName), false); + newDownload->setName(getDownloadFileName(baseName, renameToUnique), false); } startDownload(reply, newDownload, false); @@ -668,47 +688,19 @@ void DownloadManager::startDownload(QNetworkReply* reply, DownloadInfo* newDownl } emit downloadAdded(); - if (QFile::exists(m_OutputDirectory + "/" + newDownload->m_FileName)) { - setState(newDownload, STATE_PAUSING); - QCoreApplication::processEvents(); - if (QMessageBox::question( - m_ParentWidget, tr("Download again?"), - tr("A file with the same name \"%1\" has already been downloaded. " - "Do you want to download it again? The new file will receive a " - "different name.") - .arg(newDownload->m_FileName), - QMessageBox::Yes | QMessageBox::No) == QMessageBox::No) { - if (reply->isFinished()) - setState(newDownload, STATE_CANCELED); - else - setState(newDownload, STATE_CANCELING); - } else { - { - DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); - newDownload->setName(getDownloadFileName(newDownload->m_FileName, true), - true); - } - if (newDownload->m_State == STATE_PAUSED) - resumeDownload(indexByInfo(newDownload)); - else - setState(newDownload, STATE_DOWNLOADING); - } - } else - connect(newDownload->m_Reply, SIGNAL(finished()), this, SLOT(downloadFinished())); - QCoreApplication::processEvents(); + } - if (newDownload->m_State != STATE_DOWNLOADING && - newDownload->m_State != STATE_READY && - newDownload->m_State != STATE_FETCHINGMODINFO && reply->isFinished()) { - int index = indexByInfo(newDownload); - if (index >= 0) { - downloadFinished(index); - } - return; + // Qt will not emit finished() to a slot connected after the reply has already + // finished, so detect that case and drive the handler manually. + if (reply->isFinished()) { + int index = indexByInfo(newDownload); + if (index >= 0) { + downloadFinished(index); } - } else + } else { connect(newDownload->m_Reply, SIGNAL(finished()), this, SLOT(downloadFinished())); + } } void DownloadManager::addNXMDownload(const QString& url) From 6eb51916538c8b1cb3ccd49c31765bdd206a16b6 Mon Sep 17 00:00:00 2001 From: AL <26797547+Al12rs@users.noreply.github.com> Date: Sat, 18 Apr 2026 18:48:10 +0200 Subject: [PATCH 06/30] Fix memory leak in DownloadInfo::createFromMeta Move the allocation past the early-return checks so path-mismatch and hidden-skip paths no longer leak a fresh DownloadInfo. --- src/downloadmanager.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index ca5958175..40acc8d75 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -83,21 +83,22 @@ DownloadManager::DownloadInfo::createFromMeta(const QString& filePath, bool show const QString outputDirectory, std::optional fileSize) { - DownloadInfo* info = new DownloadInfo; - QString metaFileName = filePath + ".meta"; QFileInfo metaFileInfo(metaFileName); if (QDir::fromNativeSeparators(metaFileInfo.path()) .compare(QDir::fromNativeSeparators(outputDirectory), Qt::CaseInsensitive) != 0) return nullptr; + QSettings metaFile(metaFileName, QSettings::IniFormat); - if (!showHidden && metaFile.value("removed", false).toBool()) { + const bool hidden = metaFile.value("removed", false).toBool(); + if (!showHidden && hidden) { return nullptr; - } else { - info->m_Hidden = metaFile.value("removed", false).toBool(); } + DownloadInfo* info = new DownloadInfo; + info->m_Hidden = hidden; + QString fileName = QFileInfo(filePath).fileName(); if (fileName.endsWith(UNFINISHED)) { From 78f434330432432b5794a8e61defbee66da5df2c Mon Sep 17 00:00:00 2001 From: AL <26797547+Al12rs@users.noreply.github.com> Date: Sat, 18 Apr 2026 18:55:53 +0200 Subject: [PATCH 07/30] Sanitize suffix path in getDownloadFileName The collision-avoidance branch was using the raw baseName, so invalid characters sanitized out of the initial path leaked into the suffixed one. --- src/downloadmanager.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index 40acc8d75..7a1f3bac9 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -1606,15 +1606,16 @@ void DownloadManager::markUninstalled(QString fileName) QString DownloadManager::getDownloadFileName(const QString& baseName, bool rename) const { - QString fullPath = m_OutputDirectory + "/" + MOBase::sanitizeFileName(baseName); + const QString sanitized = MOBase::sanitizeFileName(baseName); + QString fullPath = m_OutputDirectory + "/" + sanitized; if (QFile::exists(fullPath) && rename) { int i = 1; while (QFile::exists( - QString("%1/%2_%3").arg(m_OutputDirectory).arg(i).arg(baseName))) { + QString("%1/%2_%3").arg(m_OutputDirectory).arg(i).arg(sanitized))) { ++i; } - fullPath = QString("%1/%2_%3").arg(m_OutputDirectory).arg(i).arg(baseName); + fullPath = QString("%1/%2_%3").arg(m_OutputDirectory).arg(i).arg(sanitized); } return fullPath; } From 5a44a6d934d29056871d0e85764079fc7674582a Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 18 Apr 2026 17:08:02 +0000 Subject: [PATCH 08/30] [pre-commit.ci] Auto fixes from pre-commit.com hooks. --- src/downloadmanager.cpp | 13 ++++++------- src/organizercore.cpp | 3 ++- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index 7a1f3bac9..5abab8318 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -193,8 +193,8 @@ DirWatcherManager::Guard::~Guard() // drain queued events while still suspended so they are filtered out, // then release. // - // TODO: find alternative, pumping the event loop from a destructor is a reentrancy hazard; - // arbitrary slots may run during ~Guard. + // TODO: find alternative, pumping the event loop from a destructor is a reentrancy + // hazard; arbitrary slots may run during ~Guard. if (m_manager.m_suspendDepth == 1) { QCoreApplication::processEvents(); } @@ -599,8 +599,7 @@ bool DownloadManager::addDownload(QNetworkReply* reply, const QStringList& URLs, "different name.") .arg(baseName), QMessageBox::Yes | QMessageBox::No) == QMessageBox::No) { - removePending(newDownload->m_FileInfo->gameName, - newDownload->m_FileInfo->modID, + removePending(newDownload->m_FileInfo->gameName, newDownload->m_FileInfo->modID, newDownload->m_FileInfo->fileID); reply->abort(); reply->deleteLater(); @@ -682,8 +681,7 @@ void DownloadManager::startDownload(QNetworkReply* reply, DownloadInfo* newDownl { // coalesce the pending removal and the active append into one reset ModelResetGuard guard(*this); - removePending(newDownload->m_FileInfo->gameName, - newDownload->m_FileInfo->modID, + removePending(newDownload->m_FileInfo->gameName, newDownload->m_FileInfo->modID, newDownload->m_FileInfo->fileID); m_ActiveDownloads.append(newDownload); } @@ -753,7 +751,8 @@ void DownloadManager::addNXMDownload(const QString& url) } for (auto tuple : m_PendingDownloads) { - if (std::get<0>(tuple).compare(foundGame->gameShortName(), Qt::CaseInsensitive) == 0 && + if (std::get<0>(tuple).compare(foundGame->gameShortName(), Qt::CaseInsensitive) == + 0 && std::get<1>(tuple) == nxmInfo.modId() && std::get<2>(tuple) == nxmInfo.fileId()) { const auto infoStr = diff --git a/src/organizercore.cpp b/src/organizercore.cpp index e2bbfc298..dff3485eb 100644 --- a/src/organizercore.cpp +++ b/src/organizercore.cpp @@ -866,7 +866,8 @@ OrganizerCore::doInstall(const QString& archivePath, GuessedValue modNa ModInfo::Ptr OrganizerCore::installDownload(int index, int priority) { - DirWatcherManager::Guard dirWatcherGuard = m_DownloadManager.dirWatcher().scopedGuard(); + DirWatcherManager::Guard dirWatcherGuard = + m_DownloadManager.dirWatcher().scopedGuard(); try { QString fileName = m_DownloadManager.getFilePath(index); From 6b5ffc41f46afde736e937dcf112f20931cc74b8 Mon Sep 17 00:00:00 2001 From: AL <26797547+Al12rs@users.noreply.github.com> Date: Sat, 18 Apr 2026 22:03:53 +0200 Subject: [PATCH 09/30] Remove unused alphabetical translation vector m_AlphabeticalTranslation was written but never read; drop it along with refreshAlphabeticalTranslation, ByName, and the LessThanWrapper helper. --- src/downloadmanager.cpp | 33 --------------------------------- src/downloadmanager.h | 5 ----- 2 files changed, 38 deletions(-) diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index 5abab8318..51db2ca95 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -876,38 +876,6 @@ void DownloadManager::removeFile(int index, bool deleteFile) m_DownloadRemoved(index); } -class LessThanWrapper -{ -public: - LessThanWrapper(DownloadManager* manager) : m_Manager(manager) {} - bool operator()(int LHS, int RHS) - { - return m_Manager->getFileName(LHS).compare(m_Manager->getFileName(RHS), - Qt::CaseInsensitive) < 0; - } - -private: - DownloadManager* m_Manager; -}; - -bool DownloadManager::ByName(int LHS, int RHS) -{ - return m_ActiveDownloads[LHS]->m_FileName < m_ActiveDownloads[RHS]->m_FileName; -} - -void DownloadManager::refreshAlphabeticalTranslation() -{ - m_AlphabeticalTranslation.clear(); - int pos = 0; - for (QVector::iterator iter = m_ActiveDownloads.begin(); - iter != m_ActiveDownloads.end(); ++iter, ++pos) { - m_AlphabeticalTranslation.push_back(pos); - } - - std::sort(m_AlphabeticalTranslation.begin(), m_AlphabeticalTranslation.end(), - LessThanWrapper(this)); -} - void DownloadManager::restoreDownload(int index) { @@ -2429,7 +2397,6 @@ void DownloadManager::metaDataChanged() DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); info->setName(getDownloadFileName(newName), true); } - refreshAlphabeticalTranslation(); notifyRowChanged(index); if (!info->m_Output.isOpen() && !info->m_Output.open(QIODevice::WriteOnly | QIODevice::Append)) { diff --git a/src/downloadmanager.h b/src/downloadmanager.h index ff4f1bf1b..0255b42af 100644 --- a/src/downloadmanager.h +++ b/src/downloadmanager.h @@ -666,10 +666,6 @@ private slots: void removeFile(int index, bool deleteFile); - void refreshAlphabeticalTranslation(); - - bool ByName(int LHS, int RHS); - QString getFileNameFromNetworkReply(QNetworkReply* reply); void setState(DownloadInfo* info, DownloadManager::DownloadState state); @@ -697,7 +693,6 @@ private slots: QString m_OutputDirectory; std::set m_RequestIDs; - QVector m_AlphabeticalTranslation; DirWatcherManager m_DirWatcher; From af789246f998842b5ad164b7b2be2203777db1ad Mon Sep 17 00:00:00 2001 From: AL <26797547+Al12rs@users.noreply.github.com> Date: Sun, 19 Apr 2026 09:59:05 +0200 Subject: [PATCH 10/30] Address PR feedback: fix redundant check and move refresh outside try catch. --- src/downloadmanager.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index 51db2ca95..efd2a30a6 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -915,9 +915,9 @@ void DownloadManager::restoreDownload(int index) void DownloadManager::removeDownload(int index, bool deleteFile) { - // validate the single-index case before entering the guard so we don't - // emit an empty reset on early return - if (index >= 0 && index >= m_ActiveDownloads.size()) { + // validate before entering the guard so we don't emit an empty reset on + // early return + if (index >= m_ActiveDownloads.size()) { reportError(tr("remove: invalid download index %1").arg(index)); return; } @@ -950,11 +950,11 @@ void DownloadManager::removeDownload(int index, bool deleteFile) delete m_ActiveDownloads.at(index); m_ActiveDownloads.erase(m_ActiveDownloads.begin() + index); } - - refreshList(); } catch (const std::exception& e) { log::error("failed to remove download: {}", e.what()); } + + refreshList(); } void DownloadManager::cancelDownload(int index) From 7d8d4bb545832fc73b14ffdbfd92d7a7ed6fbc93 Mon Sep 17 00:00:00 2001 From: AL <26797547+Al12rs@users.noreply.github.com> Date: Sun, 19 Apr 2026 18:23:03 +0200 Subject: [PATCH 11/30] Coalesce the removeDownload reset with the following refreshList 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. --- src/downloadmanager.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index efd2a30a6..a3b061c8c 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -922,10 +922,11 @@ void DownloadManager::removeDownload(int index, bool deleteFile) return; } + // coalesce the removal and the subsequent refresh into one reset + ModelResetGuard guard(*this); + try { DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); - // coalesce the removal and the subsequent refresh into one reset - ModelResetGuard guard(*this); if (index < 0) { bool removeAll = (index == -1); From 4242abeab11ab970fe0ff0aa34ce643fc3e87677 Mon Sep 17 00:00:00 2001 From: AL <26797547+Al12rs@users.noreply.github.com> Date: Sun, 19 Apr 2026 18:24:50 +0200 Subject: [PATCH 12/30] Guard the .meta creation in openMetaFile against the directory watcher 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. --- src/downloadmanager.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index a3b061c8c..dd86f001d 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -1278,6 +1278,7 @@ void DownloadManager::openMetaFile(int index) shell::Open(metaPath); return; } else { + DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); QSettings metaFile(metaPath, QSettings::IniFormat); metaFile.setValue("removed", false); } From d4ce82a262e4f9a38df77476538cfc6aed10e6cb Mon Sep 17 00:00:00 2001 From: Jonathan Feenstra <26406078+JonathanFeenstra@users.noreply.github.com> Date: Sun, 26 Apr 2026 18:53:40 +0200 Subject: [PATCH 13/30] Extract getValidGameShortName method in download manager (#2380) --- src/downloadmanager.cpp | 51 +++++++++++++++-------------------------- src/downloadmanager.h | 2 ++ 2 files changed, 20 insertions(+), 33 deletions(-) diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index dd86f001d..2ddfc29b5 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -620,17 +620,7 @@ bool DownloadManager::addDownload(QNetworkReply* reply, const QStringList& URLs, void DownloadManager::removePending(QString gameName, int modID, int fileID) { - QString gameShortName = gameName; - QStringList games(m_ManagedGame->validShortNames()); - games += m_ManagedGame->gameShortName(); - for (auto game : games) { - MOBase::IPluginGame* gamePlugin = m_OrganizerCore->getGame(game); - if (gamePlugin != nullptr && - gamePlugin->gameNexusName().compare(gameName, Qt::CaseInsensitive) == 0) { - gameShortName = gamePlugin->gameShortName(); - break; - } - } + QString gameShortName = getValidGameShortName(gameName); for (auto iter : m_PendingDownloads) { if (gameShortName.compare(std::get<0>(iter), Qt::CaseInsensitive) == 0 && (std::get<1>(iter) == modID) && (std::get<2>(iter) == fileID)) { @@ -1917,15 +1907,7 @@ void DownloadManager::nxmFileInfoAvailable(QString gameName, int modID, int file info->repository = "Nexus"; - QStringList games(m_ManagedGame->validShortNames()); - games += m_ManagedGame->gameShortName(); - for (auto game : games) { - MOBase::IPluginGame* gamePlugin = m_OrganizerCore->getGame(game); - if (gamePlugin != nullptr && - gamePlugin->gameNexusName().compare(gameName, Qt::CaseInsensitive) == 0) { - info->gameName = gamePlugin->gameShortName(); - } - } + info->gameName = getValidGameShortName(gameName); info->modID = modID; info->fileID = fileID; @@ -2169,19 +2151,7 @@ void DownloadManager::nxmFileInfoFromMd5Available(QString gameName, QVariant use info->m_FileInfo->uploader = modDetails["uploaded_by"].toString(); info->m_FileInfo->uploaderUrl = modDetails["uploaded_users_profile_url"].toString(); - QString gameShortName = gameName; - QStringList games(m_ManagedGame->validShortNames()); - games += m_ManagedGame->gameShortName(); - for (auto game : games) { - MOBase::IPluginGame* gamePlugin = m_OrganizerCore->getGame(game); - if (gamePlugin != nullptr && - gamePlugin->gameNexusName().compare(gameName, Qt::CaseInsensitive) == 0) { - gameShortName = gamePlugin->gameShortName(); - break; - } - } - - info->m_FileInfo->gameName = gameShortName; + info->m_FileInfo->gameName = getValidGameShortName(gameName); // If the file description is not present, send another query to get it if (!fileDetails["description"].isValid()) { @@ -2452,3 +2422,18 @@ void DownloadManager::writeData(DownloadInfo* info) } } } + +QString DownloadManager::getValidGameShortName(const QString& gameNexusName) const +{ + QStringList games(m_ManagedGame->validShortNames()); + games.prepend(m_ManagedGame->gameShortName()); + for (auto& game : games) { + MOBase::IPluginGame* gamePlugin = m_OrganizerCore->getGame(game); + if (gamePlugin != nullptr && + gamePlugin->gameNexusName().compare(gameNexusName, Qt::CaseInsensitive) == 0) { + return gamePlugin->gameShortName(); + } + } + + return gameNexusName; +} diff --git a/src/downloadmanager.h b/src/downloadmanager.h index 0255b42af..750d02f9c 100644 --- a/src/downloadmanager.h +++ b/src/downloadmanager.h @@ -678,6 +678,8 @@ private slots: void writeData(DownloadInfo* info); + QString getValidGameShortName(const QString& gameNexusName) const; + private: static const int AUTOMATIC_RETRIES = 3; From e3039036a6dadd9fe13f706825a35aa38c046532 Mon Sep 17 00:00:00 2001 From: AL <26797547+Al12rs@users.noreply.github.com> Date: Sun, 19 Apr 2026 16:09:38 +0200 Subject: [PATCH 14/30] Add stable download id index and PendingDownload struct 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. --- src/downloadlist.cpp | 8 +++--- src/downloadmanager.cpp | 55 +++++++++++++++++++++++------------------ src/downloadmanager.h | 35 +++++++++++++++++++++++--- 3 files changed, 67 insertions(+), 31 deletions(-) diff --git a/src/downloadlist.cpp b/src/downloadlist.cpp index d85786fe6..c6a041de8 100644 --- a/src/downloadlist.cpp +++ b/src/downloadlist.cpp @@ -112,14 +112,14 @@ QVariant DownloadList::data(const QModelIndex& index, int role) const bool pendingDownload = index.row() >= m_manager.numTotalDownloads(); if (role == Qt::DisplayRole) { if (pendingDownload) { - std::tuple nexusids = + const DownloadManager::PendingDownload pending = m_manager.getPendingDownload(index.row() - m_manager.numTotalDownloads()); switch (index.column()) { case COL_NAME: return tr("< game %1 mod %2 file %3 >") - .arg(std::get<0>(nexusids)) - .arg(std::get<1>(nexusids)) - .arg(std::get<2>(nexusids)); + .arg(pending.gameName) + .arg(pending.modID) + .arg(pending.fileID); case COL_SIZE: return tr("Unknown"); case COL_STATUS: diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index 2ddfc29b5..14445f764 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -57,12 +57,17 @@ static const char UNFINISHED[] = ".unfinished"; unsigned int DownloadManager::DownloadInfo::s_NextDownloadID = 1U; +unsigned int DownloadManager::DownloadInfo::newDownloadID() +{ + return s_NextDownloadID++; +} + DownloadManager::DownloadInfo* DownloadManager::DownloadInfo::createNew(const ModRepositoryFileInfo* fileInfo, const QStringList& URLs) { DownloadInfo* info = new DownloadInfo; - info->m_DownloadID = s_NextDownloadID++; + info->m_DownloadID = newDownloadID(); info->m_StartTime.start(); info->m_PreResumeSize = 0LL; info->m_Progress = std::make_pair(0, "0.0 B/s "); @@ -119,7 +124,7 @@ DownloadManager::DownloadInfo::createFromMeta(const QString& filePath, bool show } } - info->m_DownloadID = s_NextDownloadID++; + info->m_DownloadID = newDownloadID(); info->m_Output.setFileName(filePath); info->m_TotalSize = fileSize ? *fileSize : QFileInfo(filePath).size(); info->m_PreResumeSize = info->m_TotalSize; @@ -287,6 +292,7 @@ DownloadManager::~DownloadManager() delete *iter; } m_ActiveDownloads.clear(); + m_ByID.clear(); } void DownloadManager::setParentWidget(QWidget* w) @@ -385,6 +391,7 @@ void DownloadManager::refreshList() iter != m_ActiveDownloads.end();) { if (((*iter)->m_State == STATE_READY) || ((*iter)->m_State == STATE_INSTALLED) || ((*iter)->m_State == STATE_UNINSTALLED)) { + m_ByID.remove((*iter)->m_DownloadID); delete *iter; iter = m_ActiveDownloads.erase(iter); } else { @@ -469,6 +476,7 @@ void DownloadManager::refreshList() } cx.self.m_ActiveDownloads.push_front(info); + cx.self.m_ByID.insert(info->m_DownloadID, info); cx.seen.insert(std::move(lc)); cx.seen.insert( QFileInfo(info->m_Output.fileName()).fileName().toLower().toStdWString()); @@ -621,12 +629,14 @@ bool DownloadManager::addDownload(QNetworkReply* reply, const QStringList& URLs, void DownloadManager::removePending(QString gameName, int modID, int fileID) { QString gameShortName = getValidGameShortName(gameName); - for (auto iter : m_PendingDownloads) { - if (gameShortName.compare(std::get<0>(iter), Qt::CaseInsensitive) == 0 && - (std::get<1>(iter) == modID) && (std::get<2>(iter) == fileID)) { + + for (int i = 0; i < m_PendingDownloads.size(); ++i) { + const PendingDownload& pending = m_PendingDownloads.at(i); + if (gameShortName.compare(pending.gameName, Qt::CaseInsensitive) == 0 && + pending.modID == modID && pending.fileID == fileID) { // only emit a reset if we actually have a row to remove ModelResetGuard guard(*this); - m_PendingDownloads.removeAt(m_PendingDownloads.indexOf(iter)); + m_PendingDownloads.removeAt(i); break; } } @@ -674,6 +684,7 @@ void DownloadManager::startDownload(QNetworkReply* reply, DownloadInfo* newDownl removePending(newDownload->m_FileInfo->gameName, newDownload->m_FileInfo->modID, newDownload->m_FileInfo->fileID); m_ActiveDownloads.append(newDownload); + m_ByID.insert(newDownload->m_DownloadID, newDownload); } emit downloadAdded(); @@ -740,11 +751,9 @@ void DownloadManager::addNXMDownload(const QString& url) return; } - for (auto tuple : m_PendingDownloads) { - if (std::get<0>(tuple).compare(foundGame->gameShortName(), Qt::CaseInsensitive) == - 0 && - std::get<1>(tuple) == nxmInfo.modId() && - std::get<2>(tuple) == nxmInfo.fileId()) { + for (const PendingDownload& pending : m_PendingDownloads) { + if (pending.gameName.compare(foundGame->gameShortName(), Qt::CaseInsensitive) == 0 && + pending.modID == nxmInfo.modId() && pending.fileID == nxmInfo.fileId()) { const auto infoStr = tr("There is already a download queued for this file.\n\nMod %1\nFile %2") .arg(nxmInfo.modId()) @@ -811,8 +820,9 @@ void DownloadManager::addNXMDownload(const QString& url) { ModelResetGuard guard(*this); - m_PendingDownloads.append( - std::make_tuple(foundGame->gameShortName(), nxmInfo.modId(), nxmInfo.fileId())); + PendingDownload pending{foundGame->gameShortName(), nxmInfo.modId(), + nxmInfo.fileId()}; + m_PendingDownloads.append(pending); } emit downloadAdded(); ModRepositoryFileInfo* info = new ModRepositoryFileInfo(); @@ -929,6 +939,7 @@ void DownloadManager::removeDownload(int index, bool deleteFile) if ((removeAll && (downloadState >= STATE_READY)) || (removeState == downloadState)) { removeFile(index, deleteFile); + m_ByID.remove((*iter)->m_DownloadID); delete *iter; iter = m_ActiveDownloads.erase(iter); } else { @@ -937,8 +948,10 @@ void DownloadManager::removeDownload(int index, bool deleteFile) } } } else { + DownloadInfo* info = m_ActiveDownloads.at(index); removeFile(index, deleteFile); - delete m_ActiveDownloads.at(index); + m_ByID.remove(info->m_DownloadID); + delete info; m_ActiveDownloads.erase(m_ActiveDownloads.begin() + index); } } catch (const std::exception& e) { @@ -1049,15 +1062,7 @@ void DownloadManager::resumeDownloadInt(int index) DownloadManager::DownloadInfo* DownloadManager::downloadInfoByID(unsigned int id) { - auto iter = std::find_if(m_ActiveDownloads.begin(), m_ActiveDownloads.end(), - [id](DownloadInfo* info) { - return info->m_DownloadID == id; - }); - if (iter != m_ActiveDownloads.end()) { - return *iter; - } else { - return nullptr; - } + return m_ByID.value(id, nullptr); } void DownloadManager::queryInfo(int index) @@ -1314,7 +1319,7 @@ int DownloadManager::numPendingDownloads() const return m_PendingDownloads.size(); } -std::tuple DownloadManager::getPendingDownload(int index) +DownloadManager::PendingDownload DownloadManager::getPendingDownload(int index) { if ((index < 0) || (index >= m_PendingDownloads.size())) { throw MyException(tr("get pending: invalid download index %1").arg(index)); @@ -2204,6 +2209,7 @@ void DownloadManager::nxmRequestFailed(QString gameName, int modID, int fileID, if (info->m_FileInfo->modID == modID) { if (info->m_State < STATE_FETCHINGMODINFO) { ModelResetGuard guard(*this); + m_ByID.remove(info->m_DownloadID); m_ActiveDownloads.erase(iter); delete info; } else { @@ -2281,6 +2287,7 @@ void DownloadManager::downloadFinished(int index) { ModelResetGuard guard(*this); info->m_Output.remove(); + m_ByID.remove(info->m_DownloadID); delete info; m_ActiveDownloads.erase(m_ActiveDownloads.begin() + index); } diff --git a/src/downloadmanager.h b/src/downloadmanager.h index 750d02f9c..2de9cf5b6 100644 --- a/src/downloadmanager.h +++ b/src/downloadmanager.h @@ -24,6 +24,7 @@ along with Mod Organizer. If not, see . #include #include #include +#include #include #include #include @@ -40,6 +41,7 @@ along with Mod Organizer. If not, see . #include #include #include +#include #include using namespace boost::accumulators; @@ -150,6 +152,21 @@ class DownloadManager : public QObject STATE_UNINSTALLED }; + /** + * @brief A download that has been requested but has not yet produced a + * DownloadInfo. + * + * Created when the user initiates an NXM download; drained either when the + * Nexus API returns the actual download URL (at which point a DownloadInfo + * is created) or when the request is cancelled or fails. + */ + struct PendingDownload + { + QString gameName; + int modID; + int fileID; + }; + private: struct DownloadInfo { @@ -187,6 +204,14 @@ class DownloadManager : public QObject bool m_Hidden; + /** + * @brief Issue a new download id. + * + * The only supported way to obtain one; ids are monotonically increasing + * within a session and never reused. + */ + static unsigned int newDownloadID(); + static DownloadInfo* createNew(const MOBase::ModRepositoryFileInfo* fileInfo, const QStringList& URLs); static DownloadInfo* createFromMeta(const QString& filePath, bool showHidden, @@ -329,9 +354,9 @@ class DownloadManager : public QObject * @brief retrieve the info of a pending download * @param index index of the pending download (index in the range [0, * numPendingDownloads()[) - * @return pair of modid, fileid + * @return the PendingDownload entry at the given index */ - std::tuple getPendingDownload(int index); + PendingDownload getPendingDownload(int index); /** * @brief retrieve the full path to the download specified by index @@ -689,10 +714,14 @@ private slots: OrganizerCore* m_OrganizerCore; QWidget* m_ParentWidget; - QVector> m_PendingDownloads; + QVector m_PendingDownloads; QVector m_ActiveDownloads; + // Secondary index into m_ActiveDownloads keyed by m_DownloadID; kept in sync + // with every m_ActiveDownloads mutation. + QHash m_ByID; + QString m_OutputDirectory; std::set m_RequestIDs; From 61dd068c0af074b01f01e4da82e031ca3d02e23d Mon Sep 17 00:00:00 2001 From: AL <26797547+Al12rs@users.noreply.github.com> Date: Sun, 19 Apr 2026 16:43:18 +0200 Subject: [PATCH 15/30] Return stable ids from the plugin-facing download API 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. --- src/downloadmanager.cpp | 134 +++++++++++++++++++++++++++------------- src/downloadmanager.h | 50 ++++++++++++--- 2 files changed, 134 insertions(+), 50 deletions(-) diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index 14445f764..315e5a5bc 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -64,10 +64,11 @@ unsigned int DownloadManager::DownloadInfo::newDownloadID() DownloadManager::DownloadInfo* DownloadManager::DownloadInfo::createNew(const ModRepositoryFileInfo* fileInfo, - const QStringList& URLs) + const QStringList& URLs, + std::optional reservedID) { DownloadInfo* info = new DownloadInfo; - info->m_DownloadID = newDownloadID(); + info->m_DownloadID = reservedID.has_value() ? *reservedID : newDownloadID(); info->m_StartTime.start(); info->m_PreResumeSize = 0LL; info->m_Progress = std::make_pair(0, "0.0 B/s "); @@ -520,7 +521,8 @@ void DownloadManager::queryDownloadListInfo() } bool DownloadManager::addDownload(const QStringList& URLs, QString gameName, int modID, - int fileID, const ModRepositoryFileInfo* fileInfo) + int fileID, const ModRepositoryFileInfo* fileInfo, + std::optional reservedID) { QString fileName = QFileInfo(URLs.first()).fileName(); if (fileName.isEmpty()) { @@ -558,7 +560,7 @@ bool DownloadManager::addDownload(const QStringList& URLs, QString gameName, int QNetworkRequest::AlwaysNetwork); request.setHttp2Configuration(h2Conf); return addDownload(m_NexusInterface->getAccessManager()->get(request), URLs, fileName, - gameName, modID, fileID, fileInfo); + gameName, modID, fileID, fileInfo, reservedID); } bool DownloadManager::addDownload(QNetworkReply* reply, @@ -575,11 +577,12 @@ bool DownloadManager::addDownload(QNetworkReply* reply, bool DownloadManager::addDownload(QNetworkReply* reply, const QStringList& URLs, const QString& fileName, QString gameName, int modID, - int fileID, const ModRepositoryFileInfo* fileInfo) + int fileID, const ModRepositoryFileInfo* fileInfo, + std::optional reservedID) { // download invoked from an already open network reply (i.e. download link in the // browser) - DownloadInfo* newDownload = DownloadInfo::createNew(fileInfo, URLs); + DownloadInfo* newDownload = DownloadInfo::createNew(fileInfo, URLs, reservedID); QString baseName = fileName; if (!fileInfo->fileName.isEmpty()) { @@ -607,8 +610,7 @@ bool DownloadManager::addDownload(QNetworkReply* reply, const QStringList& URLs, "different name.") .arg(baseName), QMessageBox::Yes | QMessageBox::No) == QMessageBox::No) { - removePending(newDownload->m_FileInfo->gameName, newDownload->m_FileInfo->modID, - newDownload->m_FileInfo->fileID); + removePending(gameName, modID, fileID); reply->abort(); reply->deleteLater(); delete newDownload; @@ -622,8 +624,19 @@ bool DownloadManager::addDownload(QNetworkReply* reply, const QStringList& URLs, newDownload->setName(getDownloadFileName(baseName, renameToUnique), false); } - startDownload(reply, newDownload, false); - return true; + return startDownload(reply, newDownload, false); +} + +void DownloadManager::notifyPendingDownloadFailed(const QString& gameName, int modID, + int fileID) +{ + for (const PendingDownload& pending : m_PendingDownloads) { + if (pending.gameName.compare(gameName, Qt::CaseInsensitive) == 0 && + pending.modID == modID && pending.fileID == fileID) { + m_DownloadFailed(pending.reservedID); + return; + } + } } void DownloadManager::removePending(QString gameName, int modID, int fileID) @@ -642,7 +655,7 @@ void DownloadManager::removePending(QString gameName, int modID, int fileID) } } -void DownloadManager::startDownload(QNetworkReply* reply, DownloadInfo* newDownload, +bool DownloadManager::startDownload(QNetworkReply* reply, DownloadInfo* newDownload, bool resume) { reply->setReadBufferSize( @@ -659,15 +672,25 @@ void DownloadManager::startDownload(QNetworkReply* reply, DownloadInfo* newDownl } newDownload->m_StartTime.start(); - createMetaFile(newDownload); if (!newDownload->m_Output.open(mode)) { reportError(tr("failed to download %1: could not open output file: %2") .arg(reply->url().toString()) .arg(newDownload->m_Output.fileName())); - return; + if (!resume) { + // newDownload has not been appended to m_ActiveDownloads yet: drop it + // here, otherwise it leaks. The resume path is owned elsewhere. + reply->abort(); + reply->deleteLater(); + delete newDownload; + } + return false; } + // Write the .meta only once we know the download is actually going to start. + // Otherwise a failed start leaves an orphan .meta on disk. + createMetaFile(newDownload); + connect(newDownload->m_Reply, SIGNAL(downloadProgress(qint64, qint64)), this, SLOT(downloadProgress(qint64, qint64))); connect(newDownload->m_Reply, SIGNAL(errorOccurred(QNetworkReply::NetworkError)), @@ -701,9 +724,10 @@ void DownloadManager::startDownload(QNetworkReply* reply, DownloadInfo* newDownl } else { connect(newDownload->m_Reply, SIGNAL(finished()), this, SLOT(downloadFinished())); } + return true; } -void DownloadManager::addNXMDownload(const QString& url) +unsigned int DownloadManager::addNXMDownload(const QString& url) { NXMUrl nxmInfo(url); @@ -748,9 +772,11 @@ void DownloadManager::addNXMDownload(const QString& url) .arg(nxmInfo.game()) .arg(m_ManagedGame->gameShortName()), QMessageBox::Ok); - return; + return 0; } + // If a pending entry already covers this file, return its id so callers that + // poll by id see the same download regardless of how many times they queue. for (const PendingDownload& pending : m_PendingDownloads) { if (pending.gameName.compare(foundGame->gameShortName(), Qt::CaseInsensitive) == 0 && pending.modID == nxmInfo.modId() && pending.fileID == nxmInfo.fileId()) { @@ -764,7 +790,7 @@ void DownloadManager::addNXMDownload(const QString& url) QMessageBox::information(m_ParentWidget, tr("Already Queued"), infoStr, QMessageBox::Ok); - return; + return pending.reservedID; } } @@ -813,15 +839,19 @@ void DownloadManager::addNXMDownload(const QString& url) log::debug("{}", debugStr); QMessageBox::information(m_ParentWidget, tr("Already Started"), infoStr, QMessageBox::Ok); - return; + return download->m_DownloadID; } } } + // Reserve the id now (before any async work) so the caller can refer to the + // eventual download before the Nexus API responds and a DownloadInfo exists. + const unsigned int reservedID = DownloadInfo::newDownloadID(); + { ModelResetGuard guard(*this); PendingDownload pending{foundGame->gameShortName(), nxmInfo.modId(), - nxmInfo.fileId()}; + nxmInfo.fileId(), reservedID}; m_PendingDownloads.append(pending); } emit downloadAdded(); @@ -835,6 +865,7 @@ void DownloadManager::addNXMDownload(const QString& url) m_RequestIDs.insert(m_NexusInterface->requestFileInfo( foundGame->gameShortName(), nxmInfo.modId(), nxmInfo.fileId(), this, QVariant::fromValue(test), "")); + return reservedID; } void DownloadManager::removeFile(int index, bool deleteFile) @@ -873,7 +904,7 @@ void DownloadManager::removeFile(int index, bool deleteFile) if (!download->m_Hidden) metaSettings.setValue("removed", true); } - m_DownloadRemoved(index); + m_DownloadRemoved(download->m_DownloadID); } void DownloadManager::restoreDownload(int index) @@ -1602,47 +1633,48 @@ QString DownloadManager::getFileNameFromNetworkReply(QNetworkReply* reply) void DownloadManager::setState(DownloadManager::DownloadInfo* info, DownloadManager::DownloadState state) { - info->m_State = state; - // Row is re-looked up at each use: reply->abort() and plugin callbacks can - // re-enter and erase info (and info may not be tracked yet on first state). + // Cache the id before reply->abort(): aborting fires signals synchronously + // and a slot may erase info, leaving this pointer dangling. The id is stable + // and m_ByID.contains() lets us check whether info is still present afterward. + const unsigned int id = info->m_DownloadID; + info->m_State = state; switch (state) { case STATE_PAUSED: { info->m_Reply->abort(); info->m_Output.close(); - if (const int r = indexByInfo(info); r >= 0) - m_DownloadPaused(r); + if (m_ByID.contains(id)) + m_DownloadPaused(id); } break; case STATE_ERROR: { info->m_Reply->abort(); info->m_Output.close(); - if (const int r = indexByInfo(info); r >= 0) - m_DownloadFailed(r); + if (m_ByID.contains(id)) + m_DownloadFailed(id); } break; case STATE_CANCELED: { info->m_Reply->abort(); - if (const int r = indexByInfo(info); r >= 0) - m_DownloadFailed(r); + if (m_ByID.contains(id)) + m_DownloadFailed(id); } break; case STATE_FETCHINGMODINFO: { m_RequestIDs.insert(m_NexusInterface->requestDescription( - info->m_FileInfo->gameName, info->m_FileInfo->modID, this, info->m_DownloadID, - QString())); + info->m_FileInfo->gameName, info->m_FileInfo->modID, this, id, QString())); } break; case STATE_FETCHINGFILEINFO: { m_RequestIDs.insert(m_NexusInterface->requestFiles(info->m_FileInfo->gameName, info->m_FileInfo->modID, this, - info->m_DownloadID, QString())); + id, QString())); } break; case STATE_FETCHINGMODINFO_MD5: { log::debug("Searching {} for MD5 of {}", info->m_GamesToQuery[0], QString(info->m_Hash.toHex())); m_RequestIDs.insert(m_NexusInterface->requestInfoFromMd5( - info->m_GamesToQuery[0], info->m_Hash, this, info->m_DownloadID, QString())); + info->m_GamesToQuery[0], info->m_Hash, this, id, QString())); } break; case STATE_READY: { createMetaFile(info); - if (const int r = indexByInfo(info); r >= 0) - m_DownloadComplete(r); + if (m_ByID.contains(id)) + m_DownloadComplete(id); } break; default: /* NOP */ break; @@ -1957,9 +1989,12 @@ bool ServerByPreference(const ServerList::container& preferredServers, int DownloadManager::startDownloadURLs(const QStringList& urls) { + const unsigned int reservedID = DownloadInfo::newDownloadID(); ModRepositoryFileInfo info; - addDownload(urls, "", -1, -1, &info); - return m_ActiveDownloads.size() - 1; + if (!addDownload(urls, "", -1, -1, &info, reservedID)) { + return 0; + } + return static_cast(reservedID); } int DownloadManager::startDownloadURLWithMeta(const QString& url, const QString& name, @@ -1981,15 +2016,17 @@ int DownloadManager::startDownloadURLWithMeta(const QString& url, const QString& int DownloadManager::startDownloadNexusFile(const QString& gameName, int modID, int fileID) { - int newID = m_ActiveDownloads.size(); - addNXMDownload( - QString("nxm://%1/mods/%2/files/%3").arg(gameName).arg(modID).arg(fileID)); - return newID; + return static_cast(addNXMDownload( + QString("nxm://%1/mods/%2/files/%3").arg(gameName).arg(modID).arg(fileID))); } QString DownloadManager::downloadPath(int id) { - return getFilePath(id); + DownloadInfo* info = downloadInfoByID(static_cast(id)); + if (info == nullptr) { + return QString(); + } + return m_OutputDirectory + "/" + info->m_FileName; } boost::signals2::connection @@ -2053,6 +2090,7 @@ void DownloadManager::nxmDownloadURLsAvailable(QString gameName, int modID, int qobject_cast(qvariant_cast(userData)); QVariantList resultList = resultData.toList(); if (resultList.length() == 0) { + notifyPendingDownloadFailed(gameName, modID, fileID); removePending(gameName, modID, fileID); emit showMessage(tr("No download server available. Please try again later.")); return; @@ -2070,7 +2108,18 @@ void DownloadManager::nxmDownloadURLsAvailable(QString gameName, int modID, int foreach (const QVariant& server, resultList) { URLs.append(server.toMap()["URI"].toString()); } - addDownload(URLs, gameName, modID, fileID, info); + + // Carry the pending entry's reserved id into the DownloadInfo so callers who + // received it from addNXMDownload can continue to identify the download. + std::optional reservedID; + for (const PendingDownload& pending : m_PendingDownloads) { + if (pending.gameName.compare(gameName, Qt::CaseInsensitive) == 0 && + pending.modID == modID && pending.fileID == fileID) { + reservedID = pending.reservedID; + break; + } + } + addDownload(URLs, gameName, modID, fileID, info, reservedID); } void DownloadManager::nxmFileInfoFromMd5Available(QString gameName, QVariant userData, @@ -2219,6 +2268,7 @@ void DownloadManager::nxmRequestFailed(QString gameName, int modID, int fileID, } } + notifyPendingDownloadFailed(gameName, modID, fileID); removePending(gameName, modID, fileID); emit showMessage(tr("Failed to request file info from nexus: %1").arg(errorString)); } diff --git a/src/downloadmanager.h b/src/downloadmanager.h index 2de9cf5b6..de2edd7dc 100644 --- a/src/downloadmanager.h +++ b/src/downloadmanager.h @@ -158,13 +158,16 @@ class DownloadManager : public QObject * * Created when the user initiates an NXM download; drained either when the * Nexus API returns the actual download URL (at which point a DownloadInfo - * is created) or when the request is cancelled or fails. + * is created using reservedID as its download id so that external references + * handed out before the download existed remain valid) or when the request + * is cancelled or fails. */ struct PendingDownload { QString gameName; int modID; int fileID; + unsigned int reservedID; }; private: @@ -212,8 +215,18 @@ class DownloadManager : public QObject */ static unsigned int newDownloadID(); + /** + * @brief Create a new DownloadInfo for a fresh download. + * + * When reservedID is provided it is used as the download id. Callers that + * need to hand out an id before the DownloadInfo exists (e.g. the NXM flow + * reserves an id when the request is queued, long before the Nexus API + * returns the actual URL) should reserve via newDownloadID() and pass it + * here. Otherwise a fresh id is drawn internally. + */ static DownloadInfo* createNew(const MOBase::ModRepositoryFileInfo* fileInfo, - const QStringList& URLs); + const QStringList& URLs, + std::optional reservedID = {}); static DownloadInfo* createFromMeta(const QString& filePath, bool showHidden, const QString outputDirectory, std::optional fileSize = {}); @@ -322,18 +335,22 @@ class DownloadManager : public QObject bool addDownload(QNetworkReply* reply, const QStringList& URLs, const QString& fileName, QString gameName, int modID, int fileID = 0, const MOBase::ModRepositoryFileInfo* fileInfo = - new MOBase::ModRepositoryFileInfo()); + new MOBase::ModRepositoryFileInfo(), + std::optional reservedID = {}); /** * @brief start a download using a nxm-link * - * starts a download using a nxm-link. The download manager will first query the nexus - * page for file information. + * Starts a download using a nxm-link. The download manager will first query the + * nexus page for file information. The returned id identifies the eventual + * download; it is reserved immediately so external references remain valid even + * before the Nexus API responds. * @param url a nxm link looking like this: nxm://skyrim/mods/1234/files/4711 + * @return the reserved download id * @todo the game name encoded into the link is currently ignored, all downloads are *incorrectly assumed to be for the identified game **/ - void addNXMDownload(const QString& url); + unsigned int addNXMDownload(const QString& url); /** * @brief retrieve the total number of downloads, both finished and unfinished @@ -671,7 +688,14 @@ private slots: QString getDownloadFileName(const QString& baseName, bool rename = false) const; private: - void startDownload(QNetworkReply* reply, DownloadInfo* newDownload, bool resume); + /** + * @brief Begin downloading into newDownload from reply. + * + * On the !resume path newDownload becomes owned by m_ActiveDownloads on + * success; on failure (e.g. the output file cannot be opened) it is deleted + * before returning. Returns whether the download actually started. + */ + bool startDownload(QNetworkReply* reply, DownloadInfo* newDownload, bool resume); void resumeDownloadInt(int index); /** @@ -683,7 +707,8 @@ private slots: *only happens if there is a duplicate and the user decides not to download again **/ bool addDownload(const QStringList& URLs, QString gameName, int modID, int fileID, - const MOBase::ModRepositoryFileInfo* fileInfo); + const MOBase::ModRepositoryFileInfo* fileInfo, + std::optional reservedID = {}); // important: the caller has to lock the list-mutex, otherwise the // DownloadInfo-pointer might get invalidated at any time @@ -699,6 +724,15 @@ private slots: void removePending(QString gameName, int modID, int fileID); + /** + * @brief Fire onDownloadFailed for a pending entry, if any matches. + * + * Used on Nexus API failures so callers holding a reserved id from + * addNXMDownload do not wait indefinitely for a result. No-op if no pending + * entry matches the (gameName, modID, fileID) triple. + */ + void notifyPendingDownloadFailed(const QString& gameName, int modID, int fileID); + static QString getFileTypeString(int fileType); void writeData(DownloadInfo* info); From 3811df741b58b18dd0d7f2b052fc2e461d0dd1e3 Mon Sep 17 00:00:00 2001 From: AL <26797547+Al12rs@users.noreply.github.com> Date: Sun, 19 Apr 2026 17:52:09 +0200 Subject: [PATCH 16/30] Split downloadFinished into onReplyFinished slot and finishDownload 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. --- src/downloadmanager.cpp | 231 ++++++++++++++++++++-------------------- src/downloadmanager.h | 16 ++- 2 files changed, 130 insertions(+), 117 deletions(-) diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index 315e5a5bc..252a8613f 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -717,12 +717,9 @@ bool DownloadManager::startDownload(QNetworkReply* reply, DownloadInfo* newDownl // Qt will not emit finished() to a slot connected after the reply has already // finished, so detect that case and drive the handler manually. if (reply->isFinished()) { - int index = indexByInfo(newDownload); - if (index >= 0) { - downloadFinished(index); - } + finishDownload(newDownload->m_DownloadID); } else { - connect(newDownload->m_Reply, SIGNAL(finished()), this, SLOT(downloadFinished())); + connect(newDownload->m_Reply, SIGNAL(finished()), this, SLOT(onReplyFinished())); } return true; } @@ -1049,7 +1046,7 @@ void DownloadManager::resumeDownloadInt(int index) if (info->m_TotalSize <= info->m_Output.size() && info->m_Reply != nullptr && info->m_Reply->isFinished() && info->m_State != STATE_ERROR) { setState(info, STATE_DOWNLOADING); - downloadFinished(index); + finishDownload(info->m_DownloadID); return; } @@ -2273,134 +2270,136 @@ void DownloadManager::nxmRequestFailed(QString gameName, int modID, int fileID, emit showMessage(tr("Failed to request file info from nexus: %1").arg(errorString)); } -void DownloadManager::downloadFinished(int index) +void DownloadManager::onReplyFinished() +{ + DownloadInfo* info = findDownload(this->sender()); + if (info == nullptr) { + log::warn("finished signal from unknown reply"); + return; + } + finishDownload(info->m_DownloadID); +} + +void DownloadManager::finishDownload(unsigned int id) { DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); - DownloadInfo* info; - if (index > 0) - info = m_ActiveDownloads[index]; - else { - info = findDownload(this->sender(), &index); - if (info == nullptr && index == 0) { - info = m_ActiveDownloads[index]; - } + DownloadInfo* info = m_ByID.value(id, nullptr); + if (info == nullptr) { + log::warn("no download with id {}", id); + return; } + int index = indexByInfo(info); - if (info != nullptr) { - QNetworkReply* reply = info->m_Reply; - QByteArray data; - if (reply->isOpen() && info->m_HasData) { - data = reply->readAll(); - info->m_Output.write(data); - } - info->m_Output.close(); - TaskProgressManager::instance().forgetMe(info->m_TaskProgressId); - - bool error = false; - if ((info->m_State != STATE_CANCELING) && (info->m_State != STATE_PAUSING)) { - bool textData = reply->header(QNetworkRequest::ContentTypeHeader) - .toString() - .startsWith("text", Qt::CaseInsensitive); - if (textData) + QNetworkReply* reply = info->m_Reply; + QByteArray data; + if (reply->isOpen() && info->m_HasData) { + data = reply->readAll(); + info->m_Output.write(data); + } + info->m_Output.close(); + TaskProgressManager::instance().forgetMe(info->m_TaskProgressId); + + bool error = false; + if ((info->m_State != STATE_CANCELING) && (info->m_State != STATE_PAUSING)) { + bool textData = reply->header(QNetworkRequest::ContentTypeHeader) + .toString() + .startsWith("text", Qt::CaseInsensitive); + if (textData) + emit showMessage( + tr("Warning: Content type is: %1") + .arg(reply->header(QNetworkRequest::ContentTypeHeader).toString())); + if ((info->m_Output.size() == 0) || + ((reply->error() != QNetworkReply::NoError) && + (reply->error() != QNetworkReply::OperationCanceledError))) { + if (reply->error() == QNetworkReply::UnknownContentError) emit showMessage( - tr("Warning: Content type is: %1") - .arg(reply->header(QNetworkRequest::ContentTypeHeader).toString())); - if ((info->m_Output.size() == 0) || - ((reply->error() != QNetworkReply::NoError) && - (reply->error() != QNetworkReply::OperationCanceledError))) { - if (reply->error() == QNetworkReply::UnknownContentError) - emit showMessage( - tr("Download header content length: %1 downloaded file size: %2") - .arg(reply->header(QNetworkRequest::ContentLengthHeader).toLongLong()) - .arg(info->m_Output.size())); - if (info->m_Tries == 0) { - emit showMessage(tr("Download failed: %1 (%2)") - .arg(reply->errorString()) - .arg(reply->error())); - } - error = true; - setState(info, STATE_ERROR); + tr("Download header content length: %1 downloaded file size: %2") + .arg(reply->header(QNetworkRequest::ContentLengthHeader).toLongLong()) + .arg(info->m_Output.size())); + if (info->m_Tries == 0) { + emit showMessage(tr("Download failed: %1 (%2)") + .arg(reply->errorString()) + .arg(reply->error())); } + error = true; + setState(info, STATE_ERROR); } + } - if (info->m_State == STATE_CANCELING) { - setState(info, STATE_CANCELED); - } else if (info->m_State == STATE_PAUSING) { - if (info->m_Output.isOpen() && info->m_HasData) { - info->m_Output.write(info->m_Reply->readAll()); - } - setState(info, STATE_PAUSED); + if (info->m_State == STATE_CANCELING) { + setState(info, STATE_CANCELED); + } else if (info->m_State == STATE_PAUSING) { + if (info->m_Output.isOpen() && info->m_HasData) { + info->m_Output.write(info->m_Reply->readAll()); } + setState(info, STATE_PAUSED); + } - if (info->m_State == STATE_CANCELED || (info->m_Tries == 0 && error)) { - { - ModelResetGuard guard(*this); - info->m_Output.remove(); - m_ByID.remove(info->m_DownloadID); - delete info; - m_ActiveDownloads.erase(m_ActiveDownloads.begin() + index); - } - if (error) - emit showMessage( - tr("We were unable to download the file due to errors after four retries. " - "There may be an issue with the Nexus servers.")); - } else if (info->isPausedState() || info->m_State == STATE_PAUSING) { - info->m_Output.close(); - createMetaFile(info); - } else { - QString url = info->m_Urls[info->m_CurrentUrl]; - if (info->m_FileInfo->userData.contains("downloadMap")) { - foreach (const QVariant& server, - info->m_FileInfo->userData["downloadMap"].toList()) { - QVariantMap serverMap = server.toMap(); - if (serverMap["URI"].toString() == url) { - int deltaTime = info->m_StartTime.elapsed() / 1000; - if (deltaTime > 5) { - emit downloadSpeed(serverMap["short_name"].toString(), - (info->m_TotalSize - info->m_PreResumeSize) / - deltaTime); - } // no division by zero please! Also, if the download is shorter than a - // few seconds, the result is way to inprecise - break; - } + if (info->m_State == STATE_CANCELED || (info->m_Tries == 0 && error)) { + { + ModelResetGuard guard(*this); + info->m_Output.remove(); + m_ByID.remove(info->m_DownloadID); + delete info; + m_ActiveDownloads.erase(m_ActiveDownloads.begin() + index); + } + if (error) + emit showMessage( + tr("We were unable to download the file due to errors after four retries. " + "There may be an issue with the Nexus servers.")); + } else if (info->isPausedState() || info->m_State == STATE_PAUSING) { + info->m_Output.close(); + createMetaFile(info); + } else { + QString url = info->m_Urls[info->m_CurrentUrl]; + if (info->m_FileInfo->userData.contains("downloadMap")) { + foreach (const QVariant& server, + info->m_FileInfo->userData["downloadMap"].toList()) { + QVariantMap serverMap = server.toMap(); + if (serverMap["URI"].toString() == url) { + int deltaTime = info->m_StartTime.elapsed() / 1000; + if (deltaTime > 5) { + emit downloadSpeed(serverMap["short_name"].toString(), + (info->m_TotalSize - info->m_PreResumeSize) / deltaTime); + } // no division by zero please! Also, if the download is shorter than a + // few seconds, the result is way to inprecise + break; } } + } - bool isNexus = info->m_FileInfo->repository == "Nexus"; - // need to change state before changing the file name, otherwise .unfinished is - // appended - if (isNexus) { - setState(info, STATE_FETCHINGMODINFO); - } else { - setState(info, STATE_NOFETCH); - } - - QString newName = getFileNameFromNetworkReply(reply); - QString oldName = QFileInfo(info->m_Output).fileName(); - - if (!newName.isEmpty() && (oldName.isEmpty())) { - info->setName(getDownloadFileName(newName), true); - } else { - info->setName(m_OutputDirectory + "/" + info->m_FileName, - true); // don't rename but remove the ".unfinished" extension - } + bool isNexus = info->m_FileInfo->repository == "Nexus"; + // need to change state before changing the file name, otherwise .unfinished is + // appended + if (isNexus) { + setState(info, STATE_FETCHINGMODINFO); + } else { + setState(info, STATE_NOFETCH); + } - if (!isNexus) { - setState(info, STATE_READY); - } + QString newName = getFileNameFromNetworkReply(reply); + QString oldName = QFileInfo(info->m_Output).fileName(); - notifyRowChanged(index); + if (!newName.isEmpty() && (oldName.isEmpty())) { + info->setName(getDownloadFileName(newName), true); + } else { + info->setName(m_OutputDirectory + "/" + info->m_FileName, + true); // don't rename but remove the ".unfinished" extension } - reply->close(); - reply->deleteLater(); - if ((info->m_Tries > 0) && error) { - --info->m_Tries; - resumeDownloadInt(index); + if (!isNexus) { + setState(info, STATE_READY); } - } else { - log::warn("no download index {}", index); + + notifyRowChanged(index); + } + reply->close(); + reply->deleteLater(); + + if ((info->m_Tries > 0) && error) { + --info->m_Tries; + resumeDownloadInt(index); } } @@ -2453,7 +2452,7 @@ void DownloadManager::checkDownloadTimeout() m_ActiveDownloads[i]->m_Reply != nullptr && m_ActiveDownloads[i]->m_Reply->isOpen()) { pauseDownload(i); - downloadFinished(i); + finishDownload(m_ActiveDownloads[i]->m_DownloadID); resumeDownload(i); } } diff --git a/src/downloadmanager.h b/src/downloadmanager.h index de2edd7dc..854ac71c7 100644 --- a/src/downloadmanager.h +++ b/src/downloadmanager.h @@ -666,7 +666,21 @@ private slots: void downloadProgress(qint64 bytesReceived, qint64 bytesTotal); void downloadReadyRead(); - void downloadFinished(int index = 0); + /** + * @brief Slot wired to QNetworkReply::finished(). + * + * Resolves the originating reply through sender() and then dispatches to + * finishDownload. Use the public finishDownload directly for non-slot calls. + */ + void onReplyFinished(); + + /** + * @brief Run the post-download bookkeeping for the given download. + * + * Writes any remaining data, transitions the download's state, and emits + * the appropriate plugin signals. + */ + void finishDownload(unsigned int id); void downloadError(QNetworkReply::NetworkError error); void metaDataChanged(); void checkDownloadTimeout(); From e45d0960c6c70d4f0a0bcfe52ed7c834a6daf69e Mon Sep 17 00:00:00 2001 From: AL <26797547+Al12rs@users.noreply.github.com> Date: Sun, 19 Apr 2026 18:02:07 +0200 Subject: [PATCH 17/30] Introduce DownloadID alias and row/id accessors 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. --- src/downloadlist.cpp | 4 +++- src/downloadmanager.cpp | 26 ++++++++++++++++++++++++++ src/downloadmanager.h | 23 +++++++++++++++++++++++ 3 files changed, 52 insertions(+), 1 deletion(-) diff --git a/src/downloadlist.cpp b/src/downloadlist.cpp index c6a041de8..bf72c42cd 100644 --- a/src/downloadlist.cpp +++ b/src/downloadlist.cpp @@ -58,7 +58,9 @@ int DownloadList::columnCount(const QModelIndex&) const QModelIndex DownloadList::index(int row, int column, const QModelIndex&) const { - return createIndex(row, column, row); + // Embed the stable DownloadID in internalId() so any consumer of the index + // can identify the download without having to track row shifts. + return createIndex(row, column, m_manager.downloadIDAtRow(row)); } QModelIndex DownloadList::parent(const QModelIndex&) const diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index 252a8613f..f656ee003 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -2070,6 +2070,32 @@ int DownloadManager::indexByInfo(const DownloadInfo* info) const return -1; } +DownloadManager::DownloadID DownloadManager::downloadIDAtRow(int row) const +{ + if (row >= 0 && row < m_ActiveDownloads.size()) { + return m_ActiveDownloads[row]->m_DownloadID; + } + const int pendingRow = row - m_ActiveDownloads.size(); + if (pendingRow >= 0 && pendingRow < m_PendingDownloads.size()) { + return m_PendingDownloads[pendingRow].reservedID; + } + return 0; +} + +int DownloadManager::rowForDownloadID(DownloadID id) const +{ + if (DownloadInfo* info = m_ByID.value(id, nullptr)) { + return indexByInfo(info); + } + const int base = m_ActiveDownloads.size(); + for (int i = 0; i < m_PendingDownloads.size(); ++i) { + if (m_PendingDownloads[i].reservedID == id) { + return base + i; + } + } + return -1; +} + void DownloadManager::nxmDownloadURLsAvailable(QString gameName, int modID, int fileID, QVariant userData, QVariant resultData, int requestID) diff --git a/src/downloadmanager.h b/src/downloadmanager.h index 854ac71c7..ac1024048 100644 --- a/src/downloadmanager.h +++ b/src/downloadmanager.h @@ -152,6 +152,14 @@ class DownloadManager : public QObject STATE_UNINSTALLED }; + /** + * @brief Stable identifier for a download. + * + * Monotonically increasing within a session and never reused. Distinct from + * row indices, which are positional and shift as the list is mutated. + */ + using DownloadID = unsigned int; + /** * @brief A download that has been requested but has not yet produced a * DownloadInfo. @@ -375,6 +383,21 @@ class DownloadManager : public QObject */ PendingDownload getPendingDownload(int index); + /** + * @brief Resolve a view row to a stable DownloadID. + * + * Rows cover active downloads followed by pending ones. Returns 0 if the row + * is out of range. + */ + DownloadID downloadIDAtRow(int row) const; + + /** + * @brief Resolve a stable DownloadID to its current view row. + * + * @return the current row, or -1 if no download with that id is tracked. + */ + int rowForDownloadID(DownloadID id) const; + /** * @brief retrieve the full path to the download specified by index * From 08e08e23a726ebbab82d37ef3797309babb7704a Mon Sep 17 00:00:00 2001 From: AL <26797547+Al12rs@users.noreply.github.com> Date: Sun, 19 Apr 2026 18:14:27 +0200 Subject: [PATCH 18/30] Convert cancel/pause/resume action methods to take DownloadID 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. --- src/downloadmanager.cpp | 101 ++++++++++++++++++++-------------------- src/downloadmanager.h | 34 +++++++------- src/downloadstab.cpp | 20 +++++--- 3 files changed, 81 insertions(+), 74 deletions(-) diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index f656ee003..395d146d7 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -55,9 +55,9 @@ using namespace MOBase; static const char UNFINISHED[] = ".unfinished"; -unsigned int DownloadManager::DownloadInfo::s_NextDownloadID = 1U; +DownloadManager::DownloadID DownloadManager::DownloadInfo::s_NextDownloadID = 1U; -unsigned int DownloadManager::DownloadInfo::newDownloadID() +DownloadManager::DownloadID DownloadManager::DownloadInfo::newDownloadID() { return s_NextDownloadID++; } @@ -65,7 +65,7 @@ unsigned int DownloadManager::DownloadInfo::newDownloadID() DownloadManager::DownloadInfo* DownloadManager::DownloadInfo::createNew(const ModRepositoryFileInfo* fileInfo, const QStringList& URLs, - std::optional reservedID) + std::optional reservedID) { DownloadInfo* info = new DownloadInfo; info->m_DownloadID = reservedID.has_value() ? *reservedID : newDownloadID(); @@ -327,9 +327,9 @@ void DownloadManager::pauseAll() { // first loop: pause all downloads - for (int i = 0; i < m_ActiveDownloads.count(); ++i) { - if (m_ActiveDownloads[i]->m_State < STATE_READY) { - pauseDownload(i); + for (DownloadInfo* info : m_ActiveDownloads) { + if (info->m_State < STATE_READY) { + pauseDownload(info->m_DownloadID); } } @@ -522,7 +522,7 @@ void DownloadManager::queryDownloadListInfo() bool DownloadManager::addDownload(const QStringList& URLs, QString gameName, int modID, int fileID, const ModRepositoryFileInfo* fileInfo, - std::optional reservedID) + std::optional reservedID) { QString fileName = QFileInfo(URLs.first()).fileName(); if (fileName.isEmpty()) { @@ -578,7 +578,7 @@ bool DownloadManager::addDownload(QNetworkReply* reply, bool DownloadManager::addDownload(QNetworkReply* reply, const QStringList& URLs, const QString& fileName, QString gameName, int modID, int fileID, const ModRepositoryFileInfo* fileInfo, - std::optional reservedID) + std::optional reservedID) { // download invoked from an already open network reply (i.e. download link in the // browser) @@ -724,7 +724,7 @@ bool DownloadManager::startDownload(QNetworkReply* reply, DownloadInfo* newDownl return true; } -unsigned int DownloadManager::addNXMDownload(const QString& url) +DownloadManager::DownloadID DownloadManager::addNXMDownload(const QString& url) { NXMUrl nxmInfo(url); @@ -843,7 +843,7 @@ unsigned int DownloadManager::addNXMDownload(const QString& url) // Reserve the id now (before any async work) so the caller can refer to the // eventual download before the Nexus API responds and a DownloadInfo exists. - const unsigned int reservedID = DownloadInfo::newDownloadID(); + const DownloadID reservedID = DownloadInfo::newDownloadID(); { ModelResetGuard guard(*this); @@ -989,27 +989,27 @@ void DownloadManager::removeDownload(int index, bool deleteFile) refreshList(); } -void DownloadManager::cancelDownload(int index) +void DownloadManager::cancelDownload(DownloadID id) { - if ((index < 0) || (index >= m_ActiveDownloads.size())) { - reportError(tr("cancel: invalid download index %1").arg(index)); + DownloadInfo* info = m_ByID.value(id, nullptr); + if (info == nullptr) { + reportError(tr("cancel: invalid download id %1").arg(id)); return; } - if (m_ActiveDownloads.at(index)->m_State == STATE_DOWNLOADING) { - setState(m_ActiveDownloads.at(index), STATE_CANCELING); + if (info->m_State == STATE_DOWNLOADING) { + setState(info, STATE_CANCELING); } } -void DownloadManager::pauseDownload(int index) +void DownloadManager::pauseDownload(DownloadID id) { - if ((index < 0) || (index >= m_ActiveDownloads.size())) { - reportError(tr("pause: invalid download index %1").arg(index)); + DownloadInfo* info = m_ByID.value(id, nullptr); + if (info == nullptr) { + reportError(tr("pause: invalid download id %1").arg(id)); return; } - DownloadInfo* info = m_ActiveDownloads.at(index); - if (info->m_State == STATE_DOWNLOADING) { if ((info->m_Reply != nullptr) && (info->m_Reply->isRunning())) { setState(info, STATE_PAUSING); @@ -1023,24 +1023,24 @@ void DownloadManager::pauseDownload(int index) } } -void DownloadManager::resumeDownload(int index) +void DownloadManager::resumeDownload(DownloadID id) { - if ((index < 0) || (index >= m_ActiveDownloads.size())) { - reportError(tr("resume: invalid download index %1").arg(index)); + DownloadInfo* info = m_ByID.value(id, nullptr); + if (info == nullptr) { + reportError(tr("resume: invalid download id %1").arg(id)); return; } - DownloadInfo* info = m_ActiveDownloads[index]; - info->m_Tries = AUTOMATIC_RETRIES; - resumeDownloadInt(index); + info->m_Tries = AUTOMATIC_RETRIES; + resumeDownloadInt(id); } -void DownloadManager::resumeDownloadInt(int index) +void DownloadManager::resumeDownloadInt(DownloadID id) { - if ((index < 0) || (index >= m_ActiveDownloads.size())) { - reportError(tr("resume (int): invalid download index %1").arg(index)); + DownloadInfo* info = m_ByID.value(id, nullptr); + if (info == nullptr) { + reportError(tr("resume (int): invalid download id %1").arg(id)); return; } - DownloadInfo* info = m_ActiveDownloads[index]; // Check for finished download; if (info->m_TotalSize <= info->m_Output.size() && info->m_Reply != nullptr && @@ -1088,7 +1088,7 @@ void DownloadManager::resumeDownloadInt(int index) } } -DownloadManager::DownloadInfo* DownloadManager::downloadInfoByID(unsigned int id) +DownloadManager::DownloadInfo* DownloadManager::downloadInfoByID(DownloadID id) { return m_ByID.value(id, nullptr); } @@ -1633,8 +1633,8 @@ void DownloadManager::setState(DownloadManager::DownloadInfo* info, // Cache the id before reply->abort(): aborting fires signals synchronously // and a slot may erase info, leaving this pointer dangling. The id is stable // and m_ByID.contains() lets us check whether info is still present afterward. - const unsigned int id = info->m_DownloadID; - info->m_State = state; + const DownloadID id = info->m_DownloadID; + info->m_State = state; switch (state) { case STATE_PAUSED: { info->m_Reply->abort(); @@ -1986,7 +1986,7 @@ bool ServerByPreference(const ServerList::container& preferredServers, int DownloadManager::startDownloadURLs(const QStringList& urls) { - const unsigned int reservedID = DownloadInfo::newDownloadID(); + const DownloadID reservedID = DownloadInfo::newDownloadID(); ModRepositoryFileInfo info; if (!addDownload(urls, "", -1, -1, &info, reservedID)) { return 0; @@ -2019,7 +2019,7 @@ int DownloadManager::startDownloadNexusFile(const QString& gameName, int modID, QString DownloadManager::downloadPath(int id) { - DownloadInfo* info = downloadInfoByID(static_cast(id)); + DownloadInfo* info = downloadInfoByID(static_cast(id)); if (info == nullptr) { return QString(); } @@ -2134,7 +2134,7 @@ void DownloadManager::nxmDownloadURLsAvailable(QString gameName, int modID, int // Carry the pending entry's reserved id into the DownloadInfo so callers who // received it from addNXMDownload can continue to identify the download. - std::optional reservedID; + std::optional reservedID; for (const PendingDownload& pending : m_PendingDownloads) { if (pending.gameName.compare(gameName, Qt::CaseInsensitive) == 0 && pending.modID == modID && pending.fileID == fileID) { @@ -2306,7 +2306,7 @@ void DownloadManager::onReplyFinished() finishDownload(info->m_DownloadID); } -void DownloadManager::finishDownload(unsigned int id) +void DownloadManager::finishDownload(DownloadID id) { DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); @@ -2423,9 +2423,12 @@ void DownloadManager::finishDownload(unsigned int id) reply->close(); reply->deleteLater(); - if ((info->m_Tries > 0) && error) { - --info->m_Tries; - resumeDownloadInt(index); + // The CANCELED / retries-exhausted branch above may have deleted info; use + // m_ByID to check liveness before reading any more fields or retrying. + if (DownloadInfo* aliveInfo = m_ByID.value(id, nullptr); + aliveInfo != nullptr && error && aliveInfo->m_Tries > 0) { + --aliveInfo->m_Tries; + resumeDownloadInt(id); } } @@ -2470,16 +2473,14 @@ void DownloadManager::managedGameChanged(MOBase::IPluginGame const* managedGame) void DownloadManager::checkDownloadTimeout() { - for (int i = 0; i < m_ActiveDownloads.size(); ++i) { - if (m_ActiveDownloads[i]->m_StartTime.elapsed() - - m_ActiveDownloads[i]->m_DownloadTimeLast > - 5 * 1000 && - m_ActiveDownloads[i]->m_State == STATE_DOWNLOADING && - m_ActiveDownloads[i]->m_Reply != nullptr && - m_ActiveDownloads[i]->m_Reply->isOpen()) { - pauseDownload(i); - finishDownload(m_ActiveDownloads[i]->m_DownloadID); - resumeDownload(i); + for (DownloadInfo* info : m_ActiveDownloads) { + if (info->m_StartTime.elapsed() - info->m_DownloadTimeLast > 5 * 1000 && + info->m_State == STATE_DOWNLOADING && info->m_Reply != nullptr && + info->m_Reply->isOpen()) { + const DownloadID id = info->m_DownloadID; + pauseDownload(id); + finishDownload(id); + resumeDownload(id); } } } diff --git a/src/downloadmanager.h b/src/downloadmanager.h index ac1024048..29551d3fa 100644 --- a/src/downloadmanager.h +++ b/src/downloadmanager.h @@ -175,7 +175,7 @@ class DownloadManager : public QObject QString gameName; int modID; int fileID; - unsigned int reservedID; + DownloadID reservedID; }; private: @@ -186,7 +186,7 @@ class DownloadManager : public QObject accumulator_set> m_DownloadTimeAcc; qint64 m_DownloadLast; qint64 m_DownloadTimeLast; - unsigned int m_DownloadID; + DownloadID m_DownloadID; QString m_FileName; QFile m_Output; QNetworkReply* m_Reply; @@ -221,7 +221,7 @@ class DownloadManager : public QObject * The only supported way to obtain one; ids are monotonically increasing * within a session and never reused. */ - static unsigned int newDownloadID(); + static DownloadID newDownloadID(); /** * @brief Create a new DownloadInfo for a fresh download. @@ -234,7 +234,7 @@ class DownloadManager : public QObject */ static DownloadInfo* createNew(const MOBase::ModRepositoryFileInfo* fileInfo, const QStringList& URLs, - std::optional reservedID = {}); + std::optional reservedID = {}); static DownloadInfo* createFromMeta(const QString& filePath, bool showHidden, const QString outputDirectory, std::optional fileSize = {}); @@ -249,14 +249,14 @@ class DownloadManager : public QObject **/ void setName(QString newName, bool renameFile); - unsigned int downloadID() { return m_DownloadID; } + DownloadID downloadID() { return m_DownloadID; } bool isPausedState(); QString currentURL(); private: - static unsigned int s_NextDownloadID; + static DownloadID s_NextDownloadID; private: DownloadInfo() @@ -344,7 +344,7 @@ class DownloadManager : public QObject const QString& fileName, QString gameName, int modID, int fileID = 0, const MOBase::ModRepositoryFileInfo* fileInfo = new MOBase::ModRepositoryFileInfo(), - std::optional reservedID = {}); + std::optional reservedID = {}); /** * @brief start a download using a nxm-link @@ -358,7 +358,7 @@ class DownloadManager : public QObject * @todo the game name encoded into the link is currently ignored, all downloads are *incorrectly assumed to be for the identified game **/ - unsigned int addNXMDownload(const QString& url); + DownloadID addNXMDownload(const QString& url); /** * @brief retrieve the total number of downloads, both finished and unfinished @@ -643,13 +643,13 @@ public slots: * @brief cancel the specified download. This will lead to the corresponding file to *be deleted * - * @param index index of the download to cancel + * @param id id of the download to cancel **/ - void cancelDownload(int index); + void cancelDownload(DownloadID id); - void pauseDownload(int index); + void pauseDownload(DownloadID id); - void resumeDownload(int index); + void resumeDownload(DownloadID id); void queryInfo(int index); @@ -703,7 +703,7 @@ private slots: * Writes any remaining data, transitions the download's state, and emits * the appropriate plugin signals. */ - void finishDownload(unsigned int id); + void finishDownload(DownloadID id); void downloadError(QNetworkReply::NetworkError error); void metaDataChanged(); void checkDownloadTimeout(); @@ -733,7 +733,7 @@ private slots: * before returning. Returns whether the download actually started. */ bool startDownload(QNetworkReply* reply, DownloadInfo* newDownload, bool resume); - void resumeDownloadInt(int index); + void resumeDownloadInt(DownloadID id); /** * @brief start a download from a url @@ -745,7 +745,7 @@ private slots: **/ bool addDownload(const QStringList& URLs, QString gameName, int modID, int fileID, const MOBase::ModRepositoryFileInfo* fileInfo, - std::optional reservedID = {}); + std::optional reservedID = {}); // important: the caller has to lock the list-mutex, otherwise the // DownloadInfo-pointer might get invalidated at any time @@ -757,7 +757,7 @@ private slots: void setState(DownloadInfo* info, DownloadManager::DownloadState state); - DownloadInfo* downloadInfoByID(unsigned int id); + DownloadInfo* downloadInfoByID(DownloadID id); void removePending(QString gameName, int modID, int fileID); @@ -791,7 +791,7 @@ private slots: // Secondary index into m_ActiveDownloads keyed by m_DownloadID; kept in sync // with every m_ActiveDownloads mutation. - QHash m_ByID; + QHash m_ByID; QString m_OutputDirectory; std::set m_RequestIDs; diff --git a/src/downloadstab.cpp b/src/downloadstab.cpp index 4f988bf09..ee0c250c3 100644 --- a/src/downloadstab.cpp +++ b/src/downloadstab.cpp @@ -49,12 +49,17 @@ DownloadsTab::DownloadsTab(OrganizerCore& core, Ui::MainWindow* mwui) SLOT(removeDownload(int, bool))); connect(ui.list, SIGNAL(restoreDownload(int)), m_core.downloadManager(), SLOT(restoreDownload(int))); - connect(ui.list, SIGNAL(cancelDownload(int)), m_core.downloadManager(), - SLOT(cancelDownload(int))); - connect(ui.list, SIGNAL(pauseDownload(int)), m_core.downloadManager(), - SLOT(pauseDownload(int))); - connect(ui.list, &DownloadListView::resumeDownload, [&](int i) { - resumeDownload(i); + // The view reports actions by row; translate to DownloadID at the boundary. + connect(ui.list, &DownloadListView::cancelDownload, this, [this](int row) { + auto* dm = m_core.downloadManager(); + dm->cancelDownload(dm->downloadIDAtRow(row)); + }); + connect(ui.list, &DownloadListView::pauseDownload, this, [this](int row) { + auto* dm = m_core.downloadManager(); + dm->pauseDownload(dm->downloadIDAtRow(row)); + }); + connect(ui.list, &DownloadListView::resumeDownload, this, [this](int row) { + resumeDownload(row); }); } @@ -105,6 +110,7 @@ void DownloadsTab::queryInfos() void DownloadsTab::resumeDownload(int downloadIndex) { m_core.loggedInAction(ui.list, [this, downloadIndex] { - m_core.downloadManager()->resumeDownload(downloadIndex); + auto* dm = m_core.downloadManager(); + dm->resumeDownload(dm->downloadIDAtRow(downloadIndex)); }); } From b9391ca0a56be86b93198d20cd6cad38a7ace528 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sun, 19 Apr 2026 16:30:40 +0000 Subject: [PATCH 19/30] [pre-commit.ci] Auto fixes from pre-commit.com hooks. --- src/downloadmanager.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index 395d146d7..b10dae5e7 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -775,7 +775,8 @@ DownloadManager::DownloadID DownloadManager::addNXMDownload(const QString& url) // If a pending entry already covers this file, return its id so callers that // poll by id see the same download regardless of how many times they queue. for (const PendingDownload& pending : m_PendingDownloads) { - if (pending.gameName.compare(foundGame->gameShortName(), Qt::CaseInsensitive) == 0 && + if (pending.gameName.compare(foundGame->gameShortName(), Qt::CaseInsensitive) == + 0 && pending.modID == nxmInfo.modId() && pending.fileID == nxmInfo.fileId()) { const auto infoStr = tr("There is already a download queued for this file.\n\nMod %1\nFile %2") @@ -1658,9 +1659,8 @@ void DownloadManager::setState(DownloadManager::DownloadInfo* info, info->m_FileInfo->gameName, info->m_FileInfo->modID, this, id, QString())); } break; case STATE_FETCHINGFILEINFO: { - m_RequestIDs.insert(m_NexusInterface->requestFiles(info->m_FileInfo->gameName, - info->m_FileInfo->modID, this, - id, QString())); + m_RequestIDs.insert(m_NexusInterface->requestFiles( + info->m_FileInfo->gameName, info->m_FileInfo->modID, this, id, QString())); } break; case STATE_FETCHINGMODINFO_MD5: { log::debug("Searching {} for MD5 of {}", info->m_GamesToQuery[0], From 6f13f8eca7b0eb3efc4b571cfbb27785b391bd15 Mon Sep 17 00:00:00 2001 From: AL <26797547+Al12rs@users.noreply.github.com> Date: Tue, 21 Apr 2026 14:33:11 +0200 Subject: [PATCH 20/30] fix warnings about unused variables and size_t types --- src/downloadmanager.cpp | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index b10dae5e7..a96d5efcb 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -385,8 +385,6 @@ void DownloadManager::refreshList() ModelResetGuard guard(*this); try { - int downloadsBefore = m_ActiveDownloads.size(); - // remove finished downloads for (QVector::iterator iter = m_ActiveDownloads.begin(); iter != m_ActiveDownloads.end();) { @@ -492,7 +490,7 @@ void DownloadManager::refreshList() void DownloadManager::queryDownloadListInfo() { int incompleteCount = 0; - for (size_t i = 0; i < m_ActiveDownloads.size(); i++) { + for (int i = 0; i < m_ActiveDownloads.size(); i++) { if (isInfoIncomplete(i)) { incompleteCount++; } @@ -510,7 +508,7 @@ void DownloadManager::queryDownloadListInfo() log::info("Querying metadata for every download with incomplete info..."); { DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); - for (size_t i = 0; i < m_ActiveDownloads.size(); i++) { + for (int i = 0; i < m_ActiveDownloads.size(); i++) { if (isInfoIncomplete(i)) { queryInfoMd5(i, false); } @@ -1715,7 +1713,6 @@ void DownloadManager::downloadProgress(qint64 bytesReceived, qint64 bytesTotal) if (bytesTotal > info->m_TotalSize) { info->m_TotalSize = bytesTotal; } - int oldProgress = info->m_Progress.first; info->m_Progress.first = ((info->m_ResumePos + bytesReceived) * 100) / (info->m_ResumePos + bytesTotal); From 332dad5b3f93338847d631225d23a0c825c67710 Mon Sep 17 00:00:00 2001 From: AL <26797547+Al12rs@users.noreply.github.com> Date: Sun, 26 Apr 2026 16:38:53 +0200 Subject: [PATCH 21/30] cleanup dead code --- src/downloadmanager.cpp | 18 ------------------ src/downloadmanager.h | 6 ------ 2 files changed, 24 deletions(-) diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index a96d5efcb..5ee100cf0 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -43,7 +43,6 @@ along with Mod Organizer. If not, see . #include #include #include -#include #include #include @@ -281,9 +280,6 @@ DownloadManager::DownloadManager(NexusInterface* nexusInterface, QObject* parent m_OrganizerCore = dynamic_cast(parent); connect(&m_DirWatcher, &DirWatcherManager::directoryChanged, this, &DownloadManager::refreshList); - m_TimeoutTimer.setSingleShot(false); - // connect(&m_TimeoutTimer, SIGNAL(timeout()), this, SLOT(checkDownloadTimeout())); - m_TimeoutTimer.start(5 * 1000); } DownloadManager::~DownloadManager() @@ -2468,20 +2464,6 @@ void DownloadManager::managedGameChanged(MOBase::IPluginGame const* managedGame) m_ManagedGame = managedGame; } -void DownloadManager::checkDownloadTimeout() -{ - for (DownloadInfo* info : m_ActiveDownloads) { - if (info->m_StartTime.elapsed() - info->m_DownloadTimeLast > 5 * 1000 && - info->m_State == STATE_DOWNLOADING && info->m_Reply != nullptr && - info->m_Reply->isOpen()) { - const DownloadID id = info->m_DownloadID; - pauseDownload(id); - finishDownload(id); - resumeDownload(id); - } - } -} - void DownloadManager::writeData(DownloadInfo* info) { if (info != nullptr) { diff --git a/src/downloadmanager.h b/src/downloadmanager.h index 29551d3fa..017eb9cbc 100644 --- a/src/downloadmanager.h +++ b/src/downloadmanager.h @@ -32,7 +32,6 @@ along with Mod Organizer. If not, see . #include #include #include -#include #include #include #include @@ -706,7 +705,6 @@ private slots: void finishDownload(DownloadID id); void downloadError(QNetworkReply::NetworkError error); void metaDataChanged(); - void checkDownloadTimeout(); private: void createMetaFile(DownloadInfo* info); @@ -806,13 +804,9 @@ private slots: SignalDownloadCallback m_DownloadFailed; SignalDownloadCallback m_DownloadRemoved; - std::map m_DownloadFails; - bool m_ShowHidden; MOBase::IPluginGame const* m_ManagedGame; - - QTimer m_TimeoutTimer; }; #endif // DOWNLOADMANAGER_H From d193455329885c6aa2618e72d9c50b7bb0ae6f05 Mon Sep 17 00:00:00 2001 From: AL <26797547+Al12rs@users.noreply.github.com> Date: Sun, 26 Apr 2026 17:29:09 +0200 Subject: [PATCH 22/30] avoid calling processEvents when releasing the DirWatcherGuard --- src/downloadmanager.cpp | 18 +++++++++--------- src/downloadmanager.h | 2 ++ 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index 5ee100cf0..d6f35d855 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -188,6 +188,11 @@ void DirWatcherManager::onDirectoryChanged(const QString&) } } +void DirWatcherManager::releaseSuspension() +{ + --m_suspendDepth; +} + DirWatcherManager::Guard::Guard(DirWatcherManager& manager) : m_manager(manager) { ++m_manager.m_suspendDepth; @@ -195,15 +200,10 @@ DirWatcherManager::Guard::Guard(DirWatcherManager& manager) : m_manager(manager) DirWatcherManager::Guard::~Guard() { - // drain queued events while still suspended so they are filtered out, - // then release. - // - // TODO: find alternative, pumping the event loop from a destructor is a reentrancy - // hazard; arbitrary slots may run during ~Guard. - if (m_manager.m_suspendDepth == 1) { - QCoreApplication::processEvents(); - } - --m_manager.m_suspendDepth; + // Queued so file system notifications already posted during the + // suspension are dispatched (and filtered) before the suspension lifts. + QMetaObject::invokeMethod(&m_manager, &DirWatcherManager::releaseSuspension, + Qt::QueuedConnection); } DirWatcherManager::Guard DirWatcherManager::scopedGuard() diff --git a/src/downloadmanager.h b/src/downloadmanager.h index 017eb9cbc..1029b4a14 100644 --- a/src/downloadmanager.h +++ b/src/downloadmanager.h @@ -101,6 +101,8 @@ private slots: void onDirectoryChanged(const QString&); private: + void releaseSuspension(); + QFileSystemWatcher m_watcher; int m_suspendDepth = 0; }; From 6203577afea5a11de906c30ad75a33c692941a99 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sun, 26 Apr 2026 15:29:22 +0000 Subject: [PATCH 23/30] [pre-commit.ci] Auto fixes from pre-commit.com hooks. --- src/downloadmanager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index d6f35d855..7741f3604 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -200,7 +200,7 @@ DirWatcherManager::Guard::Guard(DirWatcherManager& manager) : m_manager(manager) DirWatcherManager::Guard::~Guard() { - // Queued so file system notifications already posted during the + // Queued so file system notifications already posted during the // suspension are dispatched (and filtered) before the suspension lifts. QMetaObject::invokeMethod(&m_manager, &DirWatcherManager::releaseSuspension, Qt::QueuedConnection); From fb78cbfbf148f2668f43ca79c4de3f844d2e7d0d Mon Sep 17 00:00:00 2001 From: AL <26797547+Al12rs@users.noreply.github.com> Date: Sun, 26 Apr 2026 17:55:59 +0200 Subject: [PATCH 24/30] use QEventLoop instead of manual ProcessEvents --- src/downloadmanager.cpp | 40 +++++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index 7741f3604..691269620 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -38,11 +38,13 @@ along with Mod Organizer. If not, see . #include #include #include +#include #include #include #include #include #include +#include #include #include @@ -321,35 +323,35 @@ bool DownloadManager::downloadsInProgressNoPause() void DownloadManager::pauseAll() { - - // first loop: pause all downloads for (DownloadInfo* info : m_ActiveDownloads) { if (info->m_State < STATE_READY) { pauseDownload(info->m_DownloadID); } } - ::Sleep(100); - - bool done = false; - QTime startTime = QTime::currentTime(); - // further loops: busy waiting for all downloads to complete. This could be neater... - while (!done && (startTime.secsTo(QTime::currentTime()) < 5)) { - QCoreApplication::processEvents(); - done = true; - foreach (DownloadInfo* info, m_ActiveDownloads) { - if ((info->m_State < STATE_CANCELED) || - (info->m_State == STATE_FETCHINGFILEINFO) || - (info->m_State == STATE_FETCHINGMODINFO) || - (info->m_State == STATE_FETCHINGMODINFO_MD5)) { - done = false; - break; + auto stillTransitioning = [this] { + for (const DownloadInfo* info : m_ActiveDownloads) { + if (info->m_State < STATE_CANCELED || + info->m_State == STATE_FETCHINGFILEINFO || + info->m_State == STATE_FETCHINGMODINFO || + info->m_State == STATE_FETCHINGMODINFO_MD5) { + return true; } } - if (!done) { - ::Sleep(100); + return false; + }; + + QEventLoop loop; + QTimer::singleShot(5000, &loop, &QEventLoop::quit); + const auto conn = connect(this, &DownloadManager::stateChanged, &loop, [&] { + if (!stillTransitioning()) { + loop.quit(); } + }); + if (stillTransitioning()) { + loop.exec(); } + disconnect(conn); } void DownloadManager::setOutputDirectory(const QString& outputDirectory, From 8d5f540a0d2f96e6b3c3af995fe0e0eeee9c6fe4 Mon Sep 17 00:00:00 2001 From: AL <26797547+Al12rs@users.noreply.github.com> Date: Sun, 26 Apr 2026 18:31:42 +0200 Subject: [PATCH 25/30] don't call processEvents in download started and defer handling finish state in event loop --- src/downloadmanager.cpp | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index 691269620..11a5ebb04 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -35,7 +35,6 @@ along with Mod Organizer. If not, see . #include #include -#include #include #include #include @@ -694,6 +693,7 @@ bool DownloadManager::startDownload(QNetworkReply* reply, DownloadInfo* newDownl connect(newDownload->m_Reply, SIGNAL(readyRead()), this, SLOT(downloadReadyRead())); connect(newDownload->m_Reply, SIGNAL(metaDataChanged()), this, SLOT(metaDataChanged())); + connect(newDownload->m_Reply, SIGNAL(finished()), this, SLOT(onReplyFinished())); if (!resume) { newDownload->m_PreResumeSize = newDownload->m_Output.size(); @@ -706,16 +706,14 @@ bool DownloadManager::startDownload(QNetworkReply* reply, DownloadInfo* newDownl m_ByID.insert(newDownload->m_DownloadID, newDownload); } emit downloadAdded(); - - QCoreApplication::processEvents(); } - // Qt will not emit finished() to a slot connected after the reply has already - // finished, so detect that case and drive the handler manually. + // Reply may have finished before we connected; defer a manual finish via a + // queued call rather than reentering startDownload synchronously. if (reply->isFinished()) { - finishDownload(newDownload->m_DownloadID); - } else { - connect(newDownload->m_Reply, SIGNAL(finished()), this, SLOT(onReplyFinished())); + QMetaObject::invokeMethod( + this, [this, id = newDownload->m_DownloadID] { finishDownload(id); }, + Qt::QueuedConnection); } return true; } From c3e14b351e43502f54f69f34277cce5ad545772a Mon Sep 17 00:00:00 2001 From: AL <26797547+Al12rs@users.noreply.github.com> Date: Sun, 26 Apr 2026 18:39:59 +0200 Subject: [PATCH 26/30] Cleanup pending download in case of failure. --- src/downloadmanager.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index 11a5ebb04..5315eba0b 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -675,6 +675,12 @@ bool DownloadManager::startDownload(QNetworkReply* reply, DownloadInfo* newDownl if (!resume) { // newDownload has not been appended to m_ActiveDownloads yet: drop it // here, otherwise it leaks. The resume path is owned elsewhere. + notifyPendingDownloadFailed(newDownload->m_FileInfo->gameName, + newDownload->m_FileInfo->modID, + newDownload->m_FileInfo->fileID); + removePending(newDownload->m_FileInfo->gameName, + newDownload->m_FileInfo->modID, + newDownload->m_FileInfo->fileID); reply->abort(); reply->deleteLater(); delete newDownload; From 711442351169a6d98ae2a012e8e46e6047c6322e Mon Sep 17 00:00:00 2001 From: AL <26797547+Al12rs@users.noreply.github.com> Date: Sun, 26 Apr 2026 18:41:52 +0200 Subject: [PATCH 27/30] Add missing notifyPendingDownloadFailed if user cancels --- src/downloadmanager.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index 5315eba0b..df08643dc 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -605,6 +605,7 @@ bool DownloadManager::addDownload(QNetworkReply* reply, const QStringList& URLs, "different name.") .arg(baseName), QMessageBox::Yes | QMessageBox::No) == QMessageBox::No) { + notifyPendingDownloadFailed(gameName, modID, fileID); removePending(gameName, modID, fileID); reply->abort(); reply->deleteLater(); From ef5da94a55fa65364f0dcb6db75accca05ed34e4 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sun, 26 Apr 2026 16:44:21 +0000 Subject: [PATCH 28/30] [pre-commit.ci] Auto fixes from pre-commit.com hooks. --- src/downloadmanager.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index df08643dc..d83750052 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -330,8 +330,7 @@ void DownloadManager::pauseAll() auto stillTransitioning = [this] { for (const DownloadInfo* info : m_ActiveDownloads) { - if (info->m_State < STATE_CANCELED || - info->m_State == STATE_FETCHINGFILEINFO || + if (info->m_State < STATE_CANCELED || info->m_State == STATE_FETCHINGFILEINFO || info->m_State == STATE_FETCHINGMODINFO || info->m_State == STATE_FETCHINGMODINFO_MD5) { return true; @@ -679,8 +678,7 @@ bool DownloadManager::startDownload(QNetworkReply* reply, DownloadInfo* newDownl notifyPendingDownloadFailed(newDownload->m_FileInfo->gameName, newDownload->m_FileInfo->modID, newDownload->m_FileInfo->fileID); - removePending(newDownload->m_FileInfo->gameName, - newDownload->m_FileInfo->modID, + removePending(newDownload->m_FileInfo->gameName, newDownload->m_FileInfo->modID, newDownload->m_FileInfo->fileID); reply->abort(); reply->deleteLater(); @@ -719,7 +717,10 @@ bool DownloadManager::startDownload(QNetworkReply* reply, DownloadInfo* newDownl // queued call rather than reentering startDownload synchronously. if (reply->isFinished()) { QMetaObject::invokeMethod( - this, [this, id = newDownload->m_DownloadID] { finishDownload(id); }, + this, + [this, id = newDownload->m_DownloadID] { + finishDownload(id); + }, Qt::QueuedConnection); } return true; From 8d20fc0172b06e83931659f8d3b6c8d921c20fcc Mon Sep 17 00:00:00 2001 From: AL <26797547+Al12rs@users.noreply.github.com> Date: Sun, 26 Apr 2026 19:03:56 +0200 Subject: [PATCH 29/30] Refactor pending download failure handling and cover rename failures --- src/downloadmanager.cpp | 38 ++++++++++++++++++++++---------------- src/downloadmanager.h | 9 +++++++++ 2 files changed, 31 insertions(+), 16 deletions(-) diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index d83750052..d4bb3d4e9 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -604,19 +604,19 @@ bool DownloadManager::addDownload(QNetworkReply* reply, const QStringList& URLs, "different name.") .arg(baseName), QMessageBox::Yes | QMessageBox::No) == QMessageBox::No) { - notifyPendingDownloadFailed(gameName, modID, fileID); - removePending(gameName, modID, fileID); - reply->abort(); - reply->deleteLater(); - delete newDownload; + cancelPendingDownload(newDownload, reply); return false; } renameToUnique = true; } - { + try { DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); newDownload->setName(getDownloadFileName(baseName, renameToUnique), false); + } catch (const std::exception& e) { + log::error("exception preparing download: {}", e.what()); + cancelPendingDownload(newDownload, reply); + return false; } return startDownload(reply, newDownload, false); @@ -634,6 +634,21 @@ void DownloadManager::notifyPendingDownloadFailed(const QString& gameName, int m } } +void DownloadManager::cancelPendingDownload(DownloadInfo* newDownload, + QNetworkReply* reply) +{ + notifyPendingDownloadFailed(newDownload->m_FileInfo->gameName, + newDownload->m_FileInfo->modID, + newDownload->m_FileInfo->fileID); + removePending(newDownload->m_FileInfo->gameName, newDownload->m_FileInfo->modID, + newDownload->m_FileInfo->fileID); + delete newDownload; + if (reply != nullptr) { + reply->abort(); + reply->deleteLater(); + } +} + void DownloadManager::removePending(QString gameName, int modID, int fileID) { QString gameShortName = getValidGameShortName(gameName); @@ -673,16 +688,7 @@ bool DownloadManager::startDownload(QNetworkReply* reply, DownloadInfo* newDownl .arg(reply->url().toString()) .arg(newDownload->m_Output.fileName())); if (!resume) { - // newDownload has not been appended to m_ActiveDownloads yet: drop it - // here, otherwise it leaks. The resume path is owned elsewhere. - notifyPendingDownloadFailed(newDownload->m_FileInfo->gameName, - newDownload->m_FileInfo->modID, - newDownload->m_FileInfo->fileID); - removePending(newDownload->m_FileInfo->gameName, newDownload->m_FileInfo->modID, - newDownload->m_FileInfo->fileID); - reply->abort(); - reply->deleteLater(); - delete newDownload; + cancelPendingDownload(newDownload, reply); } return false; } diff --git a/src/downloadmanager.h b/src/downloadmanager.h index 1029b4a14..18a3b5a03 100644 --- a/src/downloadmanager.h +++ b/src/downloadmanager.h @@ -770,6 +770,15 @@ private slots: */ void notifyPendingDownloadFailed(const QString& gameName, int modID, int fileID); + /** + * @brief Roll back a download that has not yet been activated. + * + * Ensures a caller awaiting the reservedID receives an onDownloadFailed + * callback. Must not be called once the download has been registered as + * active. + */ + void cancelPendingDownload(DownloadInfo* newDownload, QNetworkReply* reply); + static QString getFileTypeString(int fileType); void writeData(DownloadInfo* info); From 5832489b378842450c24e006d5193fc29ec0fe81 Mon Sep 17 00:00:00 2001 From: AL <26797547+Al12rs@users.noreply.github.com> Date: Mon, 4 May 2026 15:09:58 +0200 Subject: [PATCH 30/30] fix rebase bug, addNXMDownload not returning the correct type --- src/downloadmanager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/downloadmanager.cpp b/src/downloadmanager.cpp index d4bb3d4e9..d8f199465 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -741,7 +741,7 @@ DownloadManager::DownloadID DownloadManager::addNXMDownload(const QString& url) tr("This is a Nexus collections link. These are not yet supported by MO.")); QMessageBox::information(m_ParentWidget, tr("Collections Not Supported"), collectionStr, QMessageBox::Ok); - return; + return 0; } QStringList validGames;