From 5f859a8c99906a46deae62ace2b9bfb2a9a9d8d5 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/13] 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 ce6ba665d..032baaa6f 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 @@ -803,7 +822,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)); @@ -899,11 +918,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(); } } } @@ -912,7 +929,7 @@ void DownloadManager::removeDownload(int index, bool deleteFile) { try { // avoid dirWatcher triggering refreshes - ScopedDisableDirWatcher scopedDirWatcher(this); + DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); emit aboutToUpdate(); @@ -1501,7 +1518,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); @@ -1520,7 +1537,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); @@ -1542,7 +1559,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); @@ -1562,7 +1579,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); @@ -1730,7 +1747,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); @@ -2340,14 +2357,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); @@ -2385,9 +2403,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)) { @@ -2400,12 +2419,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 abf909c86..0528c5b89 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 startDownloadNexusFile(const QString& gameName, int modID, int fileID); @@ -538,7 +583,6 @@ private slots: void downloadFinished(int index = 0); void downloadError(QNetworkReply::NetworkError error); void metaDataChanged(); - void directoryChanged(const QString& dirctory); void checkDownloadTimeout(); private: @@ -611,20 +655,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; @@ -634,14 +671,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 ca13e62fa0dcbc4dc1060aecb0c021405c0c5304 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/13] 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 032baaa6f..570c6448a 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)) { @@ -800,12 +820,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(); @@ -927,11 +946,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); @@ -952,21 +977,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) @@ -1066,7 +1085,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) @@ -1727,7 +1746,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&) { @@ -1777,7 +1796,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); } } } @@ -2231,19 +2250,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; } } @@ -2311,22 +2331,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, @@ -2371,7 +2390,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 0528c5b89..a5e5a9f4d 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, @@ -477,16 +496,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 @@ -657,6 +698,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 bb0590bd1229e0cd8819fc0c9e91e12355ee91a5 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/13] 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 570c6448a..7b13c40e6 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -840,7 +840,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()) { @@ -936,10 +935,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); } } } @@ -1085,7 +1087,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) @@ -1536,7 +1537,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); @@ -1555,7 +1555,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); @@ -1577,7 +1576,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); @@ -1597,7 +1595,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); @@ -1640,28 +1637,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( @@ -1681,12 +1676,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, @@ -1765,7 +1764,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()), @@ -1792,13 +1790,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, @@ -2250,7 +2241,6 @@ void DownloadManager::nxmRequestFailed(QString gameName, int modID, int fileID, } else { info->m_State = STATE_READY; queryInfo(index); - notifyRowChanged(index); return; } } @@ -2262,7 +2252,6 @@ void DownloadManager::nxmRequestFailed(QString gameName, int modID, int fileID, delete info; } else { setState(info, STATE_READY); - notifyRowChanged(index); } break; } @@ -2274,6 +2263,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]; @@ -2344,7 +2335,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")) { @@ -2376,14 +2366,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) { @@ -2427,6 +2414,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 9e6d62cebb391955c9cfad5321aaf12605b2a227 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/13] 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 7b13c40e6..28c77ba0f 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -752,10 +752,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 66e6f214ef01f73ba6e46e43078754a0357f942b 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/13] 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 28c77ba0f..c124c6587 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 7c07854cd73cd99662e750bfbf145c9ee1736984 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/13] 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 c124c6587..952d8d2c5 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 12ce5af020744b786067c6bfbb8d519405f641ba 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/13] 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 952d8d2c5..6bce1eca4 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -1598,15 +1598,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 688a8660831310d80535eeebe17983447adb593c 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/13] [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 6bce1eca4..7b1b08622 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); } @@ -745,7 +743,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 32537476ed657c13ae661b54f5c3f389cc53fc56 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/13] 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 7b1b08622..322ca6c90 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -868,38 +868,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) { @@ -2405,7 +2373,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 a5e5a9f4d..50c60dd05 100644 --- a/src/downloadmanager.h +++ b/src/downloadmanager.h @@ -663,10 +663,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); @@ -694,7 +690,6 @@ private slots: QString m_OutputDirectory; std::set m_RequestIDs; - QVector m_AlphabeticalTranslation; DirWatcherManager m_DirWatcher; From ce2068ff00150b20c3090f47adde9ba7eb2ef046 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/13] 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 322ca6c90..0267102d8 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -907,9 +907,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; } @@ -942,11 +942,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 04c9b3389cd42750f91c07a587fb65e42fbb5baf 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/13] 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 0267102d8..b63043f91 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -914,10 +914,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 abaf343f6d0341d2766dbc9875753d2d23206f25 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/13] 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 b63043f91..f3ecf2c03 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -1270,6 +1270,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 2a8d21159ca61a8db03a503db6bd7f0b07b3c788 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/13] 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 f3ecf2c03..2bcb9afe3 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)) { @@ -1909,15 +1899,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; @@ -2145,19 +2127,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()) { @@ -2428,3 +2398,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 50c60dd05..dddcf0afd 100644 --- a/src/downloadmanager.h +++ b/src/downloadmanager.h @@ -675,6 +675,8 @@ private slots: void writeData(DownloadInfo* info); + QString getValidGameShortName(const QString& gameNexusName) const; + private: static const int AUTOMATIC_RETRIES = 3;