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 ce6ba665d..2bcb9afe3 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, @@ -84,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)) { @@ -156,33 +156,77 @@ 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); +} + +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); } } @@ -225,12 +269,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 +352,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) @@ -334,11 +374,10 @@ void DownloadManager::refreshList() { TimeThis tt("DownloadManager::refreshList()"); - try { - emit aboutToUpdate(); - // avoid triggering other refreshes - ScopedDisableDirWatcher scopedDirWatcher(this); + DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); + ModelResetGuard guard(*this); + try { int downloadsBefore = m_ActiveDownloads.size(); // remove finished downloads @@ -436,9 +475,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).")); } @@ -463,13 +499,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,37 +590,46 @@ bool DownloadManager::addDownload(QNetworkReply* reply, const QStringList& URLs, baseName.truncate(queryIndex); } - startDisableDirWatcher(); - newDownload->setName(getDownloadFileName(baseName), false); - endDisableDirWatcher(); + 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, renameToUnique), false); + } startDownload(reply, newDownload, false); - // emit update(-1); return true; } 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; - } - } - emit aboutToUpdate(); + 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)) { + // 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, @@ -622,54 +668,28 @@ 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)) { - 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 { - startDisableDirWatcher(); - newDownload->setName(getDownloadFileName(newDownload->m_FileName, true), true); - endDisableDirWatcher(); - 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) @@ -714,9 +734,9 @@ void DownloadManager::addNXMDownload(const QString& url) for (auto tuple : m_PendingDownloads) { if (std::get<0>(tuple).compare(foundGame->gameShortName(), Qt::CaseInsensitive) == - 0, + 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()) @@ -781,12 +801,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(); @@ -802,8 +821,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)); @@ -840,38 +858,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) { @@ -898,23 +884,31 @@ void DownloadManager::restoreDownload(int index) QString filePath = m_OutputDirectory + "/" + download->m_FileName; - // avoid dirWatcher triggering refreshes - startDisableDirWatcher(); - 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); + } - endDisableDirWatcher(); + notifyRowChanged(index); } } } void DownloadManager::removeDownload(int index, bool deleteFile) { - try { - // avoid dirWatcher triggering refreshes - ScopedDisableDirWatcher scopedDirWatcher(this); + // 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; + } - emit aboutToUpdate(); + // coalesce the removal and the subsequent refresh into one reset + ModelResetGuard guard(*this); + + try { + DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); if (index < 0) { bool removeAll = (index == -1); @@ -935,20 +929,14 @@ 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); } catch (const std::exception& e) { log::error("failed to remove download: {}", e.what()); } + refreshList(); } @@ -1049,7 +1037,6 @@ void DownloadManager::resumeDownloadInt(int index) log::debug("resume at {} bytes", info->m_ResumePos); startDownload(m_NexusInterface->getAccessManager()->get(request), info, true); } - emit update(index); } DownloadManager::DownloadInfo* DownloadManager::downloadInfoByID(unsigned int id) @@ -1273,6 +1260,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); } @@ -1500,8 +1488,7 @@ void DownloadManager::markInstalled(int index) throw MyException(tr("mark installed: invalid download index %1").arg(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); @@ -1519,8 +1506,7 @@ void DownloadManager::markInstalled(QString fileName) } else { 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); @@ -1541,8 +1527,7 @@ void DownloadManager::markUninstalled(int index) throw MyException(tr("mark uninstalled: invalid download index %1").arg(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); @@ -1561,8 +1546,7 @@ void DownloadManager::markUninstalled(QString fileName) DownloadInfo* info = getDownloadInfo(filePath); 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); @@ -1573,15 +1557,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; } @@ -1604,28 +1589,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( @@ -1645,12 +1628,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, @@ -1710,7 +1697,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&) { @@ -1729,8 +1716,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); @@ -1756,13 +1742,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) { - emit update(i); - } - } } void DownloadManager::nxmDescriptionAvailable(QString, int, QVariant userData, @@ -1920,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; @@ -2156,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()) { @@ -2214,19 +2173,18 @@ void DownloadManager::nxmRequestFailed(QString gameName, int modID, int fileID, } else { info->m_State = STATE_READY; queryInfo(index); - emit update(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); } - emit update(index); break; } } @@ -2237,6 +2195,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]; @@ -2294,22 +2254,20 @@ 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); } else { - emit aboutToUpdate(); QString url = info->m_Urls[info->m_CurrentUrl]; if (info->m_FileInfo->userData.contains("downloadMap")) { foreach (const QVariant& server, @@ -2340,20 +2298,18 @@ 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 } - endDisableDirWatcher(); if (!isNexus) { setState(info, STATE_READY); } - emit update(index); + notifyRowChanged(index); } reply->close(); reply->deleteLater(); @@ -2385,10 +2341,11 @@ 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(); - refreshAlphabeticalTranslation(); + { + DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); + info->setName(getDownloadFileName(newName), true); + } + 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)); @@ -2400,12 +2357,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; @@ -2447,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 abf909c86..dddcf0afd 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 @@ -61,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, @@ -191,19 +262,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 +466,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); @@ -432,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 @@ -538,7 +624,6 @@ private slots: void downloadFinished(int index = 0); void downloadError(QNetworkReply::NetworkError error); void metaDataChanged(); - void directoryChanged(const QString& dirctory); void checkDownloadTimeout(); private: @@ -578,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); @@ -594,6 +675,8 @@ private slots: void writeData(DownloadInfo* info); + QString getValidGameShortName(const QString& gameNexusName) const; + private: static const int AUTOMATIC_RETRIES = 3; @@ -609,22 +692,17 @@ private slots: QString m_OutputDirectory; std::set m_RequestIDs; - QVector m_AlphabeticalTranslation; - QFileSystemWatcher m_DirWatcher; + 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; 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 +712,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..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) { - ScopedDisableDirWatcher scopedDirwatcher(&m_DownloadManager); + DirWatcherManager::Guard dirWatcherGuard = + m_DownloadManager.dirWatcher().scopedGuard(); try { QString fileName = m_DownloadManager.getFilePath(index);