diff options
| author | Al <26797547+Al12rs@users.noreply.github.com> | 2026-05-07 17:38:51 +0200 |
|---|---|---|
| committer | GitHub <noreply@github.com> | 2026-05-07 17:38:51 +0200 |
| commit | 9c6f48a4409626b7c9ff04cabeb6a4542e1df736 (patch) | |
| tree | 0398e1c4a7a6b89a2b5ca9eb95e57e87f23f3024 /src | |
| parent | 3070a60ab7e721e1c2766c7bf85373ed0cd9be6a (diff) | |
Stable DownloadId refactor (#2375)
* 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.
* 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.
* 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.
* 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.
* 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.
* 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.
* 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.
* [pre-commit.ci] Auto fixes from pre-commit.com hooks.
* Remove unused alphabetical translation vector
m_AlphabeticalTranslation was written but never read; drop it along with
refreshAlphabeticalTranslation, ByName, and the LessThanWrapper helper.
* Address PR feedback: fix redundant check and move refresh outside try catch.
* 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.
* 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.
* Extract getValidGameShortName method in download manager (#2380)
* Add stable download id index and PendingDownload struct
Replace the (game, mod, file) tuple backing m_PendingDownloads with a
named struct, and add m_ByID as an O(1) m_DownloadID-to-info index kept
in sync with every m_ActiveDownloads mutation. Encapsulate the id
counter behind DownloadInfo::newDownloadID(), the only supported way to
consume from s_NextDownloadID.
Infrastructure only; external behaviour is unchanged.
* Return stable ids from the plugin-facing download API
startDownloadURLs / startDownloadNexusFile / addNXMDownload now reserve
and return m_DownloadID instead of a stale index. Plugin callbacks fire
with m_DownloadID; downloadPath looks up via m_ByID. nxmDownloadURLsAvailable
threads the reserved id into the materializing DownloadInfo, and Nexus
API failures wake waiting plugins via notifyPendingDownloadFailed.
Incidental: startDownload now returns bool and frees newDownload on
output-open failure; createMetaFile is deferred past that check so
failed starts no longer leave an orphan .meta.
* Split downloadFinished into onReplyFinished slot and finishDownload
The old dual-use downloadFinished(int = 0) took either an explicit index
or relied on sender() when called as a slot. Split into a sender-resolved
slot and an id-based direct call, removing the ambiguous index-zero path.
* Introduce DownloadID alias and row/id accessors
Add a DownloadID type alias for the stable per-download handle and two
public accessors (downloadIDAtRow, rowForDownloadID) so callers can
translate between the view's row vocabulary and the model's id
vocabulary without reaching into the manager's internals. DownloadList
now embeds the DownloadID in QModelIndex::internalId() so any code
holding an index can identify the download directly.
* Convert cancel/pause/resume action methods to take DownloadID
The four methods (cancel, pause, resume, resumeDownloadInt) now accept
a DownloadID, resolve through m_ByID, and no longer care about row
positions. Internal callers iterate DownloadInfo* or look up via id;
DownloadsTab translates row -> id at the connect boundary so the
view's int-shaped signals keep working unchanged.
Also switches the remaining unsigned int signatures that refer to the
download id (finishDownload, downloadInfoByID, PendingDownload::reservedID,
m_ByID, newDownloadID, s_NextDownloadID) to the DownloadID alias.
Drive-by fix: finishDownload's retry branch could read info->m_Tries
after info had been deleted in the CANCELED/retries-exhausted branch
above; now re-resolves via m_ByID.value(id) before touching any fields.
* [pre-commit.ci] Auto fixes from pre-commit.com hooks.
* fix warnings about unused variables and size_t types
* cleanup dead code
* avoid calling processEvents when releasing the DirWatcherGuard
* [pre-commit.ci] Auto fixes from pre-commit.com hooks.
* use QEventLoop instead of manual ProcessEvents
* don't call processEvents in download started and defer handling finish state in event loop
* Cleanup pending download in case of failure.
* Add missing notifyPendingDownloadFailed if user cancels
* [pre-commit.ci] Auto fixes from pre-commit.com hooks.
* Refactor pending download failure handling and cover rename failures
* fix rebase bug, addNXMDownload not returning the correct type
---------
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Jonathan Feenstra <26406078+JonathanFeenstra@users.noreply.github.com>
Diffstat (limited to 'src')
| -rw-r--r-- | src/downloadlist.cpp | 33 | ||||
| -rw-r--r-- | src/downloadlist.h | 18 | ||||
| -rw-r--r-- | src/downloadmanager.cpp | 958 | ||||
| -rw-r--r-- | src/downloadmanager.h | 307 | ||||
| -rw-r--r-- | src/downloadstab.cpp | 20 | ||||
| -rw-r--r-- | src/modlistviewactions.cpp | 10 | ||||
| -rw-r--r-- | src/organizercore.cpp | 3 |
7 files changed, 792 insertions, 557 deletions
diff --git a/src/downloadlist.cpp b/src/downloadlist.cpp index b461be50..bf72c42c 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 @@ -56,7 +58,9 @@ int DownloadList::columnCount(const QModelIndex&) const QModelIndex DownloadList::index(int row, int column, const QModelIndex&) const { - return createIndex(row, column, row); + // Embed the stable DownloadID in internalId() so any consumer of the index + // can identify the download without having to track row shifts. + return createIndex(row, column, m_manager.downloadIDAtRow(row)); } QModelIndex DownloadList::parent(const QModelIndex&) const @@ -110,14 +114,14 @@ QVariant DownloadList::data(const QModelIndex& index, int role) const bool pendingDownload = index.row() >= m_manager.numTotalDownloads(); if (role == Qt::DisplayRole) { if (pendingDownload) { - std::tuple<QString, int, int> nexusids = + const DownloadManager::PendingDownload pending = m_manager.getPendingDownload(index.row() - m_manager.numTotalDownloads()); switch (index.column()) { case COL_NAME: return tr("< game %1 mod %2 file %3 >") - .arg(std::get<0>(nexusids)) - .arg(std::get<1>(nexusids)) - .arg(std::get<2>(nexusids)); + .arg(pending.gameName) + .arg(pending.modID) + .arg(pending.fileID); case COL_SIZE: return tr("Unknown"); case COL_STATUS: @@ -239,16 +243,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 c9adaa7f..538da71a 100644 --- a/src/downloadlist.h +++ b/src/downloadlist.h @@ -83,16 +83,20 @@ public: // 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 170e38f2..4e667e0f 100644 --- a/src/downloadmanager.cpp +++ b/src/downloadmanager.cpp @@ -35,9 +35,9 @@ along with Mod Organizer. If not, see <http://www.gnu.org/licenses/>. #include <taskprogressmanager.h> #include <utility.h> -#include <QCoreApplication> #include <QDesktopServices> #include <QDirIterator> +#include <QEventLoop> #include <QFileInfo> #include <QHttp2Configuration> #include <QInputDialog> @@ -55,15 +55,20 @@ using namespace MOBase; static const char UNFINISHED[] = ".unfinished"; -unsigned int DownloadManager::DownloadInfo::s_NextDownloadID = 1U; -int DownloadManager::m_DirWatcherDisabler = 0; +DownloadManager::DownloadID DownloadManager::DownloadInfo::s_NextDownloadID = 1U; + +DownloadManager::DownloadID DownloadManager::DownloadInfo::newDownloadID() +{ + return s_NextDownloadID++; +} DownloadManager::DownloadInfo* DownloadManager::DownloadInfo::createNew(const ModRepositoryFileInfo* fileInfo, - const QStringList& URLs) + const QStringList& URLs, + std::optional<DownloadID> reservedID) { DownloadInfo* info = new DownloadInfo; - info->m_DownloadID = s_NextDownloadID++; + info->m_DownloadID = reservedID.has_value() ? *reservedID : newDownloadID(); info->m_StartTime.start(); info->m_PreResumeSize = 0LL; info->m_Progress = std::make_pair<int, QString>(0, "0.0 B/s "); @@ -84,21 +89,22 @@ DownloadManager::DownloadInfo::createFromMeta(const QString& filePath, bool show const QString outputDirectory, std::optional<uint64_t> 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)) { @@ -119,7 +125,7 @@ DownloadManager::DownloadInfo::createFromMeta(const QString& filePath, bool show } } - info->m_DownloadID = s_NextDownloadID++; + info->m_DownloadID = newDownloadID(); info->m_Output.setFileName(filePath); info->m_TotalSize = fileSize ? *fileSize : QFileInfo(filePath).size(); info->m_PreResumeSize = info->m_TotalSize; @@ -156,33 +162,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(); + } +} + +void DirWatcherManager::releaseSuspension() +{ + --m_suspendDepth; +} + +DirWatcherManager::Guard::Guard(DirWatcherManager& manager) : m_manager(manager) +{ + ++m_manager.m_suspendDepth; +} + +DirWatcherManager::Guard::~Guard() +{ + // Queued so file system notifications already posted during the + // suspension are dispatched (and filtered) before the suspension lifts. + QMetaObject::invokeMethod(&m_manager, &DirWatcherManager::releaseSuspension, + Qt::QueuedConnection); +} + +DirWatcherManager::Guard DirWatcherManager::scopedGuard() +{ + 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,15 +275,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<OrganizerCore*>(parent); - connect(&m_DirWatcher, SIGNAL(directoryChanged(QString)), this, - SLOT(directoryChanged(QString))); - m_TimeoutTimer.setSingleShot(false); - // connect(&m_TimeoutTimer, SIGNAL(timeout()), this, SLOT(checkDownloadTimeout())); - m_TimeoutTimer.start(5 * 1000); + connect(&m_DirWatcher, &DirWatcherManager::directoryChanged, this, + &DownloadManager::refreshList); } DownloadManager::~DownloadManager() @@ -243,6 +290,7 @@ DownloadManager::~DownloadManager() delete *iter; } m_ActiveDownloads.clear(); + m_ByID.clear(); } void DownloadManager::setParentWidget(QWidget* w) @@ -274,49 +322,44 @@ bool DownloadManager::downloadsInProgressNoPause() void DownloadManager::pauseAll() { - - // first loop: pause all downloads - for (int i = 0; i < m_ActiveDownloads.count(); ++i) { - if (m_ActiveDownloads[i]->m_State < STATE_READY) { - pauseDownload(i); + for (DownloadInfo* info : m_ActiveDownloads) { + if (info->m_State < STATE_READY) { + pauseDownload(info->m_DownloadID); } } - ::Sleep(100); - - bool done = false; - QTime startTime = QTime::currentTime(); - // further loops: busy waiting for all downloads to complete. This could be neater... - while (!done && (startTime.secsTo(QTime::currentTime()) < 5)) { - QCoreApplication::processEvents(); - done = true; - foreach (DownloadInfo* info, m_ActiveDownloads) { - if ((info->m_State < STATE_CANCELED) || - (info->m_State == STATE_FETCHINGFILEINFO) || - (info->m_State == STATE_FETCHINGMODINFO) || - (info->m_State == STATE_FETCHINGMODINFO_MD5)) { - done = false; - break; + auto stillTransitioning = [this] { + for (const DownloadInfo* info : m_ActiveDownloads) { + if (info->m_State < STATE_CANCELED || info->m_State == STATE_FETCHINGFILEINFO || + info->m_State == STATE_FETCHINGMODINFO || + info->m_State == STATE_FETCHINGMODINFO_MD5) { + return true; } } - if (!done) { - ::Sleep(100); + return false; + }; + + QEventLoop loop; + QTimer::singleShot(5000, &loop, &QEventLoop::quit); + const auto conn = connect(this, &DownloadManager::stateChanged, &loop, [&] { + if (!stillTransitioning()) { + loop.quit(); } + }); + if (stillTransitioning()) { + loop.exec(); } + disconnect(conn); } void DownloadManager::setOutputDirectory(const QString& outputDirectory, 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,18 +377,16 @@ void DownloadManager::refreshList() { TimeThis tt("DownloadManager::refreshList()"); - try { - emit aboutToUpdate(); - // avoid triggering other refreshes - ScopedDisableDirWatcher scopedDirWatcher(this); - - int downloadsBefore = m_ActiveDownloads.size(); + DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); + ModelResetGuard guard(*this); + try { // remove finished downloads for (QVector<DownloadInfo*>::iterator iter = m_ActiveDownloads.begin(); iter != m_ActiveDownloads.end();) { if (((*iter)->m_State == STATE_READY) || ((*iter)->m_State == STATE_INSTALLED) || ((*iter)->m_State == STATE_UNINSTALLED)) { + m_ByID.remove((*iter)->m_DownloadID); delete *iter; iter = m_ActiveDownloads.erase(iter); } else { @@ -430,15 +471,13 @@ void DownloadManager::refreshList() } cx.self.m_ActiveDownloads.push_front(info); + cx.self.m_ByID.insert(info->m_DownloadID, info); cx.seen.insert(std::move(lc)); cx.seen.insert( QFileInfo(info->m_Output.fileName()).fileName().toLower().toStdWString()); }); log::debug("saw {} downloads", m_ActiveDownloads.size()); - - emit update(-1); - } catch (const std::bad_alloc&) { reportError(tr("Memory allocation error (in refreshing directory).")); } @@ -447,7 +486,7 @@ void DownloadManager::refreshList() void DownloadManager::queryDownloadListInfo() { int incompleteCount = 0; - for (size_t i = 0; i < m_ActiveDownloads.size(); i++) { + for (int i = 0; i < m_ActiveDownloads.size(); i++) { if (isInfoIncomplete(i)) { incompleteCount++; } @@ -463,19 +502,21 @@ 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 (int i = 0; i < m_ActiveDownloads.size(); i++) { + if (isInfoIncomplete(i)) { + queryInfoMd5(i, false); + } } } - endDisableDirWatcher(); log::info("Metadata has been retrieved successfully!"); } } bool DownloadManager::addDownload(const QStringList& URLs, QString gameName, int modID, - int fileID, const ModRepositoryFileInfo* fileInfo) + int fileID, const ModRepositoryFileInfo* fileInfo, + std::optional<DownloadID> reservedID) { QString fileName = QFileInfo(URLs.first()).fileName(); if (fileName.isEmpty()) { @@ -513,7 +554,7 @@ bool DownloadManager::addDownload(const QStringList& URLs, QString gameName, int QNetworkRequest::AlwaysNetwork); request.setHttp2Configuration(h2Conf); return addDownload(m_NexusInterface->getAccessManager()->get(request), URLs, fileName, - gameName, modID, fileID, fileInfo); + gameName, modID, fileID, fileInfo, reservedID); } bool DownloadManager::addDownload(QNetworkReply* reply, @@ -530,11 +571,12 @@ bool DownloadManager::addDownload(QNetworkReply* reply, bool DownloadManager::addDownload(QNetworkReply* reply, const QStringList& URLs, const QString& fileName, QString gameName, int modID, - int fileID, const ModRepositoryFileInfo* fileInfo) + int fileID, const ModRepositoryFileInfo* fileInfo, + std::optional<DownloadID> reservedID) { // download invoked from an already open network reply (i.e. download link in the // browser) - DownloadInfo* newDownload = DownloadInfo::createNew(fileInfo, URLs); + DownloadInfo* newDownload = DownloadInfo::createNew(fileInfo, URLs, reservedID); QString baseName = fileName; if (!fileInfo->fileName.isEmpty()) { @@ -553,40 +595,77 @@ 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) { + cancelPendingDownload(newDownload, reply); + return false; + } + renameToUnique = true; + } - startDownload(reply, newDownload, false); - // emit update(-1); - return true; + try { + DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); + newDownload->setName(getDownloadFileName(baseName, renameToUnique), false); + } catch (const std::exception& e) { + log::error("exception preparing download: {}", e.what()); + cancelPendingDownload(newDownload, reply); + return false; + } + + return startDownload(reply, newDownload, false); } -void DownloadManager::removePending(QString gameName, int modID, int fileID) +void DownloadManager::notifyPendingDownloadFailed(const 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; + for (const PendingDownload& pending : m_PendingDownloads) { + if (pending.gameName.compare(gameName, Qt::CaseInsensitive) == 0 && + pending.modID == modID && pending.fileID == fileID) { + m_DownloadFailed(pending.reservedID); + return; } } - 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)) { - m_PendingDownloads.removeAt(m_PendingDownloads.indexOf(iter)); +} + +void DownloadManager::cancelPendingDownload(DownloadInfo* newDownload, + QNetworkReply* reply) +{ + notifyPendingDownloadFailed(newDownload->m_FileInfo->gameName, + newDownload->m_FileInfo->modID, + newDownload->m_FileInfo->fileID); + removePending(newDownload->m_FileInfo->gameName, newDownload->m_FileInfo->modID, + newDownload->m_FileInfo->fileID); + delete newDownload; + if (reply != nullptr) { + reply->abort(); + reply->deleteLater(); + } +} + +void DownloadManager::removePending(QString gameName, int modID, int fileID) +{ + QString gameShortName = getValidGameShortName(gameName); + + for (int i = 0; i < m_PendingDownloads.size(); ++i) { + const PendingDownload& pending = m_PendingDownloads.at(i); + if (gameShortName.compare(pending.gameName, Qt::CaseInsensitive) == 0 && + pending.modID == modID && pending.fileID == fileID) { + // only emit a reset if we actually have a row to remove + ModelResetGuard guard(*this); + m_PendingDownloads.removeAt(i); break; } } - emit update(-1); } -void DownloadManager::startDownload(QNetworkReply* reply, DownloadInfo* newDownload, +bool DownloadManager::startDownload(QNetworkReply* reply, DownloadInfo* newDownload, bool resume) { reply->setReadBufferSize( @@ -603,15 +682,21 @@ void DownloadManager::startDownload(QNetworkReply* reply, DownloadInfo* newDownl } newDownload->m_StartTime.start(); - createMetaFile(newDownload); if (!newDownload->m_Output.open(mode)) { reportError(tr("failed to download %1: could not open output file: %2") .arg(reply->url().toString()) .arg(newDownload->m_Output.fileName())); - return; + if (!resume) { + cancelPendingDownload(newDownload, reply); + } + return false; } + // Write the .meta only once we know the download is actually going to start. + // Otherwise a failed start leaves an orphan .meta on disk. + createMetaFile(newDownload); + connect(newDownload->m_Reply, SIGNAL(downloadProgress(qint64, qint64)), this, SLOT(downloadProgress(qint64, qint64))); connect(newDownload->m_Reply, SIGNAL(errorOccurred(QNetworkReply::NetworkError)), @@ -619,60 +704,35 @@ void DownloadManager::startDownload(QNetworkReply* reply, DownloadInfo* newDownl connect(newDownload->m_Reply, SIGNAL(readyRead()), this, SLOT(downloadReadyRead())); connect(newDownload->m_Reply, SIGNAL(metaDataChanged()), this, SLOT(metaDataChanged())); + connect(newDownload->m_Reply, SIGNAL(finished()), this, SLOT(onReplyFinished())); if (!resume) { newDownload->m_PreResumeSize = newDownload->m_Output.size(); - 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); + m_ByID.insert(newDownload->m_DownloadID, 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; - } - } else - connect(newDownload->m_Reply, SIGNAL(finished()), this, SLOT(downloadFinished())); + // Reply may have finished before we connected; defer a manual finish via a + // queued call rather than reentering startDownload synchronously. + if (reply->isFinished()) { + QMetaObject::invokeMethod( + this, + [this, id = newDownload->m_DownloadID] { + finishDownload(id); + }, + Qt::QueuedConnection); + } + return true; } -void DownloadManager::addNXMDownload(const QString& url) +DownloadManager::DownloadID DownloadManager::addNXMDownload(const QString& url) { NXMUrl nxmInfo(url); @@ -681,7 +741,7 @@ void DownloadManager::addNXMDownload(const QString& url) tr("This is a Nexus collections link. These are not yet supported by MO.")); QMessageBox::information(m_ParentWidget, tr("Collections Not Supported"), collectionStr, QMessageBox::Ok); - return; + return 0; } QStringList validGames; @@ -717,14 +777,15 @@ void DownloadManager::addNXMDownload(const QString& url) .arg(nxmInfo.game()) .arg(m_ManagedGame->gameShortName()), QMessageBox::Ok); - return; + return 0; } - for (auto tuple : m_PendingDownloads) { - if (std::get<0>(tuple).compare(foundGame->gameShortName(), Qt::CaseInsensitive) == - 0, - std::get<1>(tuple) == nxmInfo.modId() && - std::get<2>(tuple) == nxmInfo.fileId()) { + // If a pending entry already covers this file, return its id so callers that + // poll by id see the same download regardless of how many times they queue. + for (const PendingDownload& pending : m_PendingDownloads) { + if (pending.gameName.compare(foundGame->gameShortName(), Qt::CaseInsensitive) == + 0 && + pending.modID == nxmInfo.modId() && pending.fileID == nxmInfo.fileId()) { const auto infoStr = tr("There is already a download queued for this file.\n\nMod %1\nFile %2") .arg(nxmInfo.modId()) @@ -735,7 +796,7 @@ void DownloadManager::addNXMDownload(const QString& url) QMessageBox::information(m_ParentWidget, tr("Already Queued"), infoStr, QMessageBox::Ok); - return; + return pending.reservedID; } } @@ -784,17 +845,21 @@ void DownloadManager::addNXMDownload(const QString& url) log::debug("{}", debugStr); QMessageBox::information(m_ParentWidget, tr("Already Started"), infoStr, QMessageBox::Ok); - return; + return download->m_DownloadID; } } } - emit aboutToUpdate(); - - m_PendingDownloads.append( - std::make_tuple(foundGame->gameShortName(), nxmInfo.modId(), nxmInfo.fileId())); + // Reserve the id now (before any async work) so the caller can refer to the + // eventual download before the Nexus API responds and a DownloadInfo exists. + const DownloadID reservedID = DownloadInfo::newDownloadID(); - emit update(-1); + { + ModelResetGuard guard(*this); + PendingDownload pending{foundGame->gameShortName(), nxmInfo.modId(), + nxmInfo.fileId(), reservedID}; + m_PendingDownloads.append(pending); + } emit downloadAdded(); ModRepositoryFileInfo* info = new ModRepositoryFileInfo(); @@ -806,12 +871,12 @@ void DownloadManager::addNXMDownload(const QString& url) m_RequestIDs.insert(m_NexusInterface->requestFileInfo( foundGame->gameShortName(), nxmInfo.modId(), nxmInfo.fileId(), this, QVariant::fromValue(test), "")); + return reservedID; } void DownloadManager::removeFile(int index, bool deleteFile) { - // 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)); @@ -845,39 +910,7 @@ void DownloadManager::removeFile(int index, bool deleteFile) if (!download->m_Hidden) metaSettings.setValue("removed", true); } - 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<DownloadInfo*>::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)); + m_DownloadRemoved(download->m_DownloadID); } void DownloadManager::restoreDownload(int index) @@ -906,23 +939,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; + } + + // coalesce the removal and the subsequent refresh into one reset + ModelResetGuard guard(*this); - emit aboutToUpdate(); + try { + DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); if (index < 0) { bool removeAll = (index == -1); @@ -935,6 +976,7 @@ void DownloadManager::removeDownload(int index, bool deleteFile) if ((removeAll && (downloadState >= STATE_READY)) || (removeState == downloadState)) { removeFile(index, deleteFile); + m_ByID.remove((*iter)->m_DownloadID); delete *iter; iter = m_ActiveDownloads.erase(iter); } else { @@ -943,44 +985,40 @@ 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; - } - + DownloadInfo* info = m_ActiveDownloads.at(index); removeFile(index, deleteFile); - delete m_ActiveDownloads.at(index); + m_ByID.remove(info->m_DownloadID); + delete info; m_ActiveDownloads.erase(m_ActiveDownloads.begin() + index); } - emit update(-1); } catch (const std::exception& e) { log::error("failed to remove download: {}", e.what()); } + refreshList(); } -void DownloadManager::cancelDownload(int index) +void DownloadManager::cancelDownload(DownloadID id) { - if ((index < 0) || (index >= m_ActiveDownloads.size())) { - reportError(tr("cancel: invalid download index %1").arg(index)); + DownloadInfo* info = m_ByID.value(id, nullptr); + if (info == nullptr) { + reportError(tr("cancel: invalid download id %1").arg(id)); return; } - if (m_ActiveDownloads.at(index)->m_State == STATE_DOWNLOADING) { - setState(m_ActiveDownloads.at(index), STATE_CANCELING); + if (info->m_State == STATE_DOWNLOADING) { + setState(info, STATE_CANCELING); } } -void DownloadManager::pauseDownload(int index) +void DownloadManager::pauseDownload(DownloadID id) { - if ((index < 0) || (index >= m_ActiveDownloads.size())) { - reportError(tr("pause: invalid download index %1").arg(index)); + DownloadInfo* info = m_ByID.value(id, nullptr); + if (info == nullptr) { + reportError(tr("pause: invalid download id %1").arg(id)); return; } - DownloadInfo* info = m_ActiveDownloads.at(index); - if (info->m_State == STATE_DOWNLOADING) { if ((info->m_Reply != nullptr) && (info->m_Reply->isRunning())) { setState(info, STATE_PAUSING); @@ -994,30 +1032,30 @@ void DownloadManager::pauseDownload(int index) } } -void DownloadManager::resumeDownload(int index) +void DownloadManager::resumeDownload(DownloadID id) { - if ((index < 0) || (index >= m_ActiveDownloads.size())) { - reportError(tr("resume: invalid download index %1").arg(index)); + DownloadInfo* info = m_ByID.value(id, nullptr); + if (info == nullptr) { + reportError(tr("resume: invalid download id %1").arg(id)); return; } - DownloadInfo* info = m_ActiveDownloads[index]; - info->m_Tries = AUTOMATIC_RETRIES; - resumeDownloadInt(index); + info->m_Tries = AUTOMATIC_RETRIES; + resumeDownloadInt(id); } -void DownloadManager::resumeDownloadInt(int index) +void DownloadManager::resumeDownloadInt(DownloadID id) { - if ((index < 0) || (index >= m_ActiveDownloads.size())) { - reportError(tr("resume (int): invalid download index %1").arg(index)); + DownloadInfo* info = m_ByID.value(id, nullptr); + if (info == nullptr) { + reportError(tr("resume (int): invalid download id %1").arg(id)); return; } - DownloadInfo* info = m_ActiveDownloads[index]; // Check for finished download; if (info->m_TotalSize <= info->m_Output.size() && info->m_Reply != nullptr && info->m_Reply->isFinished() && info->m_State != STATE_ERROR) { setState(info, STATE_DOWNLOADING); - downloadFinished(index); + finishDownload(info->m_DownloadID); return; } @@ -1057,20 +1095,11 @@ 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) +DownloadManager::DownloadInfo* DownloadManager::downloadInfoByID(DownloadID id) { - auto iter = std::find_if(m_ActiveDownloads.begin(), m_ActiveDownloads.end(), - [id](DownloadInfo* info) { - return info->m_DownloadID == id; - }); - if (iter != m_ActiveDownloads.end()) { - return *iter; - } else { - return nullptr; - } + return m_ByID.value(id, nullptr); } void DownloadManager::queryInfo(int index) @@ -1281,6 +1310,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); } @@ -1326,7 +1356,7 @@ int DownloadManager::numPendingDownloads() const return m_PendingDownloads.size(); } -std::tuple<QString, int, int> DownloadManager::getPendingDownload(int index) +DownloadManager::PendingDownload DownloadManager::getPendingDownload(int index) { if ((index < 0) || (index >= m_PendingDownloads.size())) { throw MyException(tr("get pending: invalid download index %1").arg(index)); @@ -1508,8 +1538,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); @@ -1527,8 +1556,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); @@ -1549,8 +1577,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); @@ -1569,8 +1596,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); @@ -1581,15 +1607,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; } @@ -1612,53 +1639,55 @@ 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; + // Cache the id before reply->abort(): aborting fires signals synchronously + // and a slot may erase info, leaving this pointer dangling. The id is stable + // and m_ByID.contains() lets us check whether info is still present afterward. + const DownloadID id = info->m_DownloadID; + info->m_State = state; switch (state) { case STATE_PAUSED: { info->m_Reply->abort(); info->m_Output.close(); - m_DownloadPaused(row); + if (m_ByID.contains(id)) + m_DownloadPaused(id); } break; case STATE_ERROR: { info->m_Reply->abort(); info->m_Output.close(); - m_DownloadFailed(row); + if (m_ByID.contains(id)) + m_DownloadFailed(id); } break; case STATE_CANCELED: { info->m_Reply->abort(); - m_DownloadFailed(row); + if (m_ByID.contains(id)) + m_DownloadFailed(id); } break; case STATE_FETCHINGMODINFO: { m_RequestIDs.insert(m_NexusInterface->requestDescription( - info->m_FileInfo->gameName, info->m_FileInfo->modID, this, info->m_DownloadID, - QString())); + info->m_FileInfo->gameName, info->m_FileInfo->modID, this, id, QString())); } break; case STATE_FETCHINGFILEINFO: { - m_RequestIDs.insert(m_NexusInterface->requestFiles(info->m_FileInfo->gameName, - info->m_FileInfo->modID, this, - info->m_DownloadID, QString())); + m_RequestIDs.insert(m_NexusInterface->requestFiles( + info->m_FileInfo->gameName, info->m_FileInfo->modID, this, id, QString())); } break; case STATE_FETCHINGMODINFO_MD5: { log::debug("Searching {} for MD5 of {}", info->m_GamesToQuery[0], QString(info->m_Hash.toHex())); m_RequestIDs.insert(m_NexusInterface->requestInfoFromMd5( - info->m_GamesToQuery[0], info->m_Hash, this, info->m_DownloadID, QString())); + info->m_GamesToQuery[0], info->m_Hash, this, id, QString())); } break; case STATE_READY: { createMetaFile(info); - m_DownloadComplete(row); + if (m_ByID.contains(id)) + m_DownloadComplete(id); } 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, @@ -1694,7 +1723,6 @@ void DownloadManager::downloadProgress(qint64 bytesReceived, qint64 bytesTotal) if (bytesTotal > info->m_TotalSize) { info->m_TotalSize = bytesTotal; } - int oldProgress = info->m_Progress.first; info->m_Progress.first = ((info->m_ResumePos + bytesReceived) * 100) / (info->m_ResumePos + bytesTotal); @@ -1718,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&) { @@ -1737,8 +1765,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); @@ -1764,13 +1791,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, @@ -1928,15 +1948,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; @@ -1981,9 +1993,12 @@ bool ServerByPreference(const ServerList::container& preferredServers, int DownloadManager::startDownloadURLs(const QStringList& urls) { + const DownloadID reservedID = DownloadInfo::newDownloadID(); ModRepositoryFileInfo info; - addDownload(urls, "", -1, -1, &info); - return m_ActiveDownloads.size() - 1; + if (!addDownload(urls, "", -1, -1, &info, reservedID)) { + return 0; + } + return static_cast<int>(reservedID); } int DownloadManager::startDownloadURLWithMeta(const QString& url, const QString& game, @@ -2043,15 +2058,17 @@ int DownloadManager::startDownloadURLWithMeta(const QString& url, const QString& int DownloadManager::startDownloadNexusFile(const QString& gameName, int modID, int fileID) { - int newID = m_ActiveDownloads.size(); - addNXMDownload( - QString("nxm://%1/mods/%2/files/%3").arg(gameName).arg(modID).arg(fileID)); - return newID; + return static_cast<int>(addNXMDownload( + QString("nxm://%1/mods/%2/files/%3").arg(gameName).arg(modID).arg(fileID))); } QString DownloadManager::downloadPath(int id) { - return getFilePath(id); + DownloadInfo* info = downloadInfoByID(static_cast<DownloadID>(id)); + if (info == nullptr) { + return QString(); + } + return m_OutputDirectory + "/" + info->m_FileName; } boost::signals2::connection @@ -2098,6 +2115,32 @@ int DownloadManager::indexByInfo(const DownloadInfo* info) const return -1; } +DownloadManager::DownloadID DownloadManager::downloadIDAtRow(int row) const +{ + if (row >= 0 && row < m_ActiveDownloads.size()) { + return m_ActiveDownloads[row]->m_DownloadID; + } + const int pendingRow = row - m_ActiveDownloads.size(); + if (pendingRow >= 0 && pendingRow < m_PendingDownloads.size()) { + return m_PendingDownloads[pendingRow].reservedID; + } + return 0; +} + +int DownloadManager::rowForDownloadID(DownloadID id) const +{ + if (DownloadInfo* info = m_ByID.value(id, nullptr)) { + return indexByInfo(info); + } + const int base = m_ActiveDownloads.size(); + for (int i = 0; i < m_PendingDownloads.size(); ++i) { + if (m_PendingDownloads[i].reservedID == id) { + return base + i; + } + } + return -1; +} + void DownloadManager::nxmDownloadURLsAvailable(QString gameName, int modID, int fileID, QVariant userData, QVariant resultData, int requestID) @@ -2115,6 +2158,7 @@ void DownloadManager::nxmDownloadURLsAvailable(QString gameName, int modID, int qobject_cast<ModRepositoryFileInfo*>(qvariant_cast<QObject*>(userData)); QVariantList resultList = resultData.toList(); if (resultList.length() == 0) { + notifyPendingDownloadFailed(gameName, modID, fileID); removePending(gameName, modID, fileID); emit showMessage(tr("No download server available. Please try again later.")); return; @@ -2132,7 +2176,18 @@ void DownloadManager::nxmDownloadURLsAvailable(QString gameName, int modID, int foreach (const QVariant& server, resultList) { URLs.append(server.toMap()["URI"].toString()); } - addDownload(URLs, gameName, modID, fileID, info); + + // Carry the pending entry's reserved id into the DownloadInfo so callers who + // received it from addNXMDownload can continue to identify the download. + std::optional<DownloadID> reservedID; + for (const PendingDownload& pending : m_PendingDownloads) { + if (pending.gameName.compare(gameName, Qt::CaseInsensitive) == 0 && + pending.modID == modID && pending.fileID == fileID) { + reservedID = pending.reservedID; + break; + } + } + addDownload(URLs, gameName, modID, fileID, info, reservedID); } void DownloadManager::nxmFileInfoFromMd5Available(QString gameName, QVariant userData, @@ -2218,19 +2273,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()) { @@ -2276,156 +2319,161 @@ 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_ByID.remove(info->m_DownloadID); m_ActiveDownloads.erase(iter); delete info; } else { setState(info, STATE_READY); } - emit update(index); break; } } + notifyPendingDownloadFailed(gameName, modID, fileID); removePending(gameName, modID, fileID); emit showMessage(tr("Failed to request file info from nexus: %1").arg(errorString)); } -void DownloadManager::downloadFinished(int index) +void DownloadManager::onReplyFinished() { - DownloadInfo* info; - if (index > 0) - info = m_ActiveDownloads[index]; - else { - info = findDownload(this->sender(), &index); - if (info == nullptr && index == 0) { - info = m_ActiveDownloads[index]; - } + DownloadInfo* info = findDownload(this->sender()); + if (info == nullptr) { + log::warn("finished signal from unknown reply"); + return; } + finishDownload(info->m_DownloadID); +} - if (info != nullptr) { - QNetworkReply* reply = info->m_Reply; - QByteArray data; - if (reply->isOpen() && info->m_HasData) { - data = reply->readAll(); - info->m_Output.write(data); - } - info->m_Output.close(); - TaskProgressManager::instance().forgetMe(info->m_TaskProgressId); +void DownloadManager::finishDownload(DownloadID id) +{ + DirWatcherManager::Guard dirWatcherGuard = m_DirWatcher.scopedGuard(); - bool error = false; - if ((info->m_State != STATE_CANCELING) && (info->m_State != STATE_PAUSING)) { - bool textData = reply->header(QNetworkRequest::ContentTypeHeader) - .toString() - .startsWith("text", Qt::CaseInsensitive); - if (textData) + DownloadInfo* info = m_ByID.value(id, nullptr); + if (info == nullptr) { + log::warn("no download with id {}", id); + return; + } + int index = indexByInfo(info); + + QNetworkReply* reply = info->m_Reply; + QByteArray data; + if (reply->isOpen() && info->m_HasData) { + data = reply->readAll(); + info->m_Output.write(data); + } + info->m_Output.close(); + TaskProgressManager::instance().forgetMe(info->m_TaskProgressId); + + bool error = false; + if ((info->m_State != STATE_CANCELING) && (info->m_State != STATE_PAUSING)) { + bool textData = reply->header(QNetworkRequest::ContentTypeHeader) + .toString() + .startsWith("text", Qt::CaseInsensitive); + if (textData) + emit showMessage( + tr("Warning: Content type is: %1") + .arg(reply->header(QNetworkRequest::ContentTypeHeader).toString())); + if ((info->m_Output.size() == 0) || + ((reply->error() != QNetworkReply::NoError) && + (reply->error() != QNetworkReply::OperationCanceledError))) { + if (reply->error() == QNetworkReply::UnknownContentError) emit showMessage( - tr("Warning: Content type is: %1") - .arg(reply->header(QNetworkRequest::ContentTypeHeader).toString())); - if ((info->m_Output.size() == 0) || - ((reply->error() != QNetworkReply::NoError) && - (reply->error() != QNetworkReply::OperationCanceledError))) { - if (reply->error() == QNetworkReply::UnknownContentError) - emit showMessage( - tr("Download header content length: %1 downloaded file size: %2") - .arg(reply->header(QNetworkRequest::ContentLengthHeader).toLongLong()) - .arg(info->m_Output.size())); - if (info->m_Tries == 0) { - emit showMessage(tr("Download failed: %1 (%2)") - .arg(reply->errorString()) - .arg(reply->error())); - } - error = true; - setState(info, STATE_ERROR); + tr("Download header content length: %1 downloaded file size: %2") + .arg(reply->header(QNetworkRequest::ContentLengthHeader).toLongLong()) + .arg(info->m_Output.size())); + if (info->m_Tries == 0) { + emit showMessage(tr("Download failed: %1 (%2)") + .arg(reply->errorString()) + .arg(reply->error())); } + error = true; + setState(info, STATE_ERROR); } + } - if (info->m_State == STATE_CANCELING) { - setState(info, STATE_CANCELED); - } else if (info->m_State == STATE_PAUSING) { - if (info->m_Output.isOpen() && info->m_HasData) { - info->m_Output.write(info->m_Reply->readAll()); - } - setState(info, STATE_PAUSED); + if (info->m_State == STATE_CANCELING) { + setState(info, STATE_CANCELED); + } else if (info->m_State == STATE_PAUSING) { + if (info->m_Output.isOpen() && info->m_HasData) { + info->m_Output.write(info->m_Reply->readAll()); } + setState(info, STATE_PAUSED); + } - if (info->m_State == STATE_CANCELED || (info->m_Tries == 0 && error)) { - emit aboutToUpdate(); + if (info->m_State == STATE_CANCELED || (info->m_Tries == 0 && error)) { + { + ModelResetGuard guard(*this); info->m_Output.remove(); + m_ByID.remove(info->m_DownloadID); delete info; m_ActiveDownloads.erase(m_ActiveDownloads.begin() + index); - if (error) - emit showMessage( - tr("We were unable to download the file due to errors after four retries. " - "There may be an issue with the Nexus servers.")); - 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, - info->m_FileInfo->userData["downloadMap"].toList()) { - QVariantMap serverMap = server.toMap(); - if (serverMap["URI"].toString() == url) { - int deltaTime = info->m_StartTime.elapsed() / 1000; - if (deltaTime > 5) { - emit downloadSpeed(serverMap["short_name"].toString(), - (info->m_TotalSize - info->m_PreResumeSize) / - deltaTime); - } // no division by zero please! Also, if the download is shorter than a - // few seconds, the result is way to inprecise - break; - } + } + if (error) + emit showMessage( + tr("We were unable to download the file due to errors after four retries. " + "There may be an issue with the Nexus servers.")); + } else if (info->isPausedState() || info->m_State == STATE_PAUSING) { + info->m_Output.close(); + createMetaFile(info); + } else { + QString url = info->m_Urls[info->m_CurrentUrl]; + if (info->m_FileInfo->userData.contains("downloadMap")) { + foreach (const QVariant& server, + info->m_FileInfo->userData["downloadMap"].toList()) { + QVariantMap serverMap = server.toMap(); + if (serverMap["URI"].toString() == url) { + int deltaTime = info->m_StartTime.elapsed() / 1000; + if (deltaTime > 5) { + emit downloadSpeed(serverMap["short_name"].toString(), + (info->m_TotalSize - info->m_PreResumeSize) / deltaTime); + } // no division by zero please! Also, if the download is shorter than a + // few seconds, the result is way to inprecise + break; } } + } - bool isNexus = info->m_FileInfo->repository == "Nexus"; - // need to change state before changing the file name, otherwise .unfinished is - // appended - if (isNexus) { - setState(info, STATE_FETCHINGMODINFO); - } else { - setState(info, STATE_NOFETCH); - } - - QString newName = getFileNameFromNetworkReply(reply); - QString oldName = QFileInfo(info->m_Output).fileName(); - - 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(); + bool isNexus = info->m_FileInfo->repository == "Nexus"; + // need to change state before changing the file name, otherwise .unfinished is + // appended + if (isNexus) { + setState(info, STATE_FETCHINGMODINFO); + } else { + setState(info, STATE_NOFETCH); + } - if (!isNexus) { - setState(info, STATE_READY); - } + QString newName = getFileNameFromNetworkReply(reply); + QString oldName = QFileInfo(info->m_Output).fileName(); - emit update(index); + if (!newName.isEmpty() && (oldName.isEmpty())) { + info->setName(getDownloadFileName(newName), true); + } else { + info->setName(m_OutputDirectory + "/" + info->m_FileName, + true); // don't rename but remove the ".unfinished" extension } - reply->close(); - reply->deleteLater(); - if ((info->m_Tries > 0) && error) { - --info->m_Tries; - resumeDownloadInt(index); + if (!isNexus) { + setState(info, STATE_READY); } - } else { - log::warn("no download index {}", index); + + notifyRowChanged(index); + } + reply->close(); + reply->deleteLater(); + + // The CANCELED / retries-exhausted branch above may have deleted info; use + // m_ByID to check liveness before reading any more fields or retrying. + if (DownloadInfo* aliveInfo = m_ByID.value(id, nullptr); + aliveInfo != nullptr && error && aliveInfo->m_Tries > 0) { + --aliveInfo->m_Tries; + resumeDownloadInt(id); } } @@ -2447,10 +2495,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)); @@ -2462,33 +2511,11 @@ void DownloadManager::metaDataChanged() } } -void DownloadManager::directoryChanged(const QString&) -{ - if (DownloadManager::m_DirWatcherDisabler == 0) - refreshList(); -} - void DownloadManager::managedGameChanged(MOBase::IPluginGame const* managedGame) { m_ManagedGame = managedGame; } -void DownloadManager::checkDownloadTimeout() -{ - for (int i = 0; i < m_ActiveDownloads.size(); ++i) { - if (m_ActiveDownloads[i]->m_StartTime.elapsed() - - m_ActiveDownloads[i]->m_DownloadTimeLast > - 5 * 1000 && - m_ActiveDownloads[i]->m_State == STATE_DOWNLOADING && - m_ActiveDownloads[i]->m_Reply != nullptr && - m_ActiveDownloads[i]->m_Reply->isOpen()) { - pauseDownload(i); - downloadFinished(i); - resumeDownload(i); - } - } -} - void DownloadManager::writeData(DownloadInfo* info) { if (info != nullptr) { @@ -2509,3 +2536,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 a65d4fcb..9c8e391d 100644 --- a/src/downloadmanager.h +++ b/src/downloadmanager.h @@ -24,6 +24,7 @@ along with Mod Organizer. If not, see <http://www.gnu.org/licenses/>. #include <QElapsedTimer> #include <QFile> #include <QFileSystemWatcher> +#include <QHash> #include <QMap> #include <QNetworkReply> #include <QObject> @@ -31,7 +32,6 @@ along with Mod Organizer. If not, see <http://www.gnu.org/licenses/>. #include <QSettings> #include <QStringList> #include <QTime> -#include <QTimer> #include <QUrl> #include <QVector> #include <boost/accumulators/accumulators.hpp> @@ -40,6 +40,7 @@ along with Mod Organizer. If not, see <http://www.gnu.org/licenses/>. #include <boost/signals2.hpp> #include <idownloadmanager.h> #include <modrepositoryfileinfo.h> +#include <optional> #include <set> using namespace boost::accumulators; @@ -52,6 +53,60 @@ 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: + void releaseSuspension(); + + QFileSystemWatcher m_watcher; + int m_suspendDepth = 0; +}; + /*! * \brief manages downloading of files and provides progress information for gui *elements @@ -61,6 +116,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, @@ -79,6 +153,32 @@ public: STATE_UNINSTALLED }; + /** + * @brief Stable identifier for a download. + * + * Monotonically increasing within a session and never reused. Distinct from + * row indices, which are positional and shift as the list is mutated. + */ + using DownloadID = unsigned int; + + /** + * @brief A download that has been requested but has not yet produced a + * DownloadInfo. + * + * Created when the user initiates an NXM download; drained either when the + * Nexus API returns the actual download URL (at which point a DownloadInfo + * is created using reservedID as its download id so that external references + * handed out before the download existed remain valid) or when the request + * is cancelled or fails. + */ + struct PendingDownload + { + QString gameName; + int modID; + int fileID; + DownloadID reservedID; + }; + private: struct DownloadInfo { @@ -87,7 +187,7 @@ private: accumulator_set<qint64, stats<tag::rolling_mean>> m_DownloadTimeAcc; qint64 m_DownloadLast; qint64 m_DownloadTimeLast; - unsigned int m_DownloadID; + DownloadID m_DownloadID; QString m_FileName; QFile m_Output; QNetworkReply* m_Reply; @@ -116,8 +216,26 @@ private: bool m_Hidden; + /** + * @brief Issue a new download id. + * + * The only supported way to obtain one; ids are monotonically increasing + * within a session and never reused. + */ + static DownloadID newDownloadID(); + + /** + * @brief Create a new DownloadInfo for a fresh download. + * + * When reservedID is provided it is used as the download id. Callers that + * need to hand out an id before the DownloadInfo exists (e.g. the NXM flow + * reserves an id when the request is queued, long before the Nexus API + * returns the actual URL) should reserve via newDownloadID() and pass it + * here. Otherwise a fresh id is drawn internally. + */ static DownloadInfo* createNew(const MOBase::ModRepositoryFileInfo* fileInfo, - const QStringList& URLs); + const QStringList& URLs, + std::optional<DownloadID> reservedID = {}); static DownloadInfo* createFromMeta(const QString& filePath, bool showHidden, const QString outputDirectory, std::optional<uint64_t> fileSize = {}); @@ -132,14 +250,14 @@ private: **/ void setName(QString newName, bool renameFile); - unsigned int downloadID() { return m_DownloadID; } + DownloadID downloadID() { return m_DownloadID; } bool isPausedState(); QString currentURL(); private: - static unsigned int s_NextDownloadID; + static DownloadID s_NextDownloadID; private: DownloadInfo() @@ -192,19 +310,6 @@ public: 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 **/ QString getOutputDirectory() const { return m_OutputDirectory; } @@ -239,18 +344,22 @@ public: bool addDownload(QNetworkReply* reply, const QStringList& URLs, const QString& fileName, QString gameName, int modID, int fileID = 0, const MOBase::ModRepositoryFileInfo* fileInfo = - new MOBase::ModRepositoryFileInfo()); + new MOBase::ModRepositoryFileInfo(), + std::optional<DownloadID> reservedID = {}); /** * @brief start a download using a nxm-link * - * starts a download using a nxm-link. The download manager will first query the nexus - * page for file information. + * Starts a download using a nxm-link. The download manager will first query the + * nexus page for file information. The returned id identifies the eventual + * download; it is reserved immediately so external references remain valid even + * before the Nexus API responds. * @param url a nxm link looking like this: nxm://skyrim/mods/1234/files/4711 + * @return the reserved download id * @todo the game name encoded into the link is currently ignored, all downloads are *incorrectly assumed to be for the identified game **/ - void addNXMDownload(const QString& url); + DownloadID addNXMDownload(const QString& url); /** * @brief retrieve the total number of downloads, both finished and unfinished @@ -271,9 +380,24 @@ public: * @brief retrieve the info of a pending download * @param index index of the pending download (index in the range [0, * numPendingDownloads()[) - * @return pair of modid, fileid + * @return the PendingDownload entry at the given index + */ + PendingDownload getPendingDownload(int index); + + /** + * @brief Resolve a view row to a stable DownloadID. + * + * Rows cover active downloads followed by pending ones. Returns 0 if the row + * is out of range. */ - std::tuple<QString, int, int> getPendingDownload(int index); + DownloadID downloadIDAtRow(int row) const; + + /** + * @brief Resolve a stable DownloadID to its current view row. + * + * @return the current row, or -1 if no download with that id is tracked. + */ + int rowForDownloadID(DownloadID id) const; /** * @brief retrieve the full path to the download specified by index @@ -408,6 +532,12 @@ public: */ void queryDownloadListInfo(); + /** + * @return the directory watcher for the downloads folder; call + * scopedGuard() on it to suspend across filesystem writes. + */ + DirWatcherManager& dirWatcher() { return m_DirWatcher; } + public: // IDownloadManager interface: int startDownloadURLs(const QStringList& urls); int startDownloadURLWithMeta(const QString& url, const QString& game, @@ -435,16 +565,38 @@ public: // IDownloadManager interface: 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 @@ -492,13 +644,13 @@ public slots: * @brief cancel the specified download. This will lead to the corresponding file to *be deleted * - * @param index index of the download to cancel + * @param id id of the download to cancel **/ - void cancelDownload(int index); + void cancelDownload(DownloadID id); - void pauseDownload(int index); + void pauseDownload(DownloadID id); - void resumeDownload(int index); + void resumeDownload(DownloadID id); void queryInfo(int index); @@ -538,11 +690,23 @@ private slots: void downloadProgress(qint64 bytesReceived, qint64 bytesTotal); void downloadReadyRead(); - void downloadFinished(int index = 0); + /** + * @brief Slot wired to QNetworkReply::finished(). + * + * Resolves the originating reply through sender() and then dispatches to + * finishDownload. Use the public finishDownload directly for non-slot calls. + */ + void onReplyFinished(); + + /** + * @brief Run the post-download bookkeeping for the given download. + * + * Writes any remaining data, transitions the download's state, and emits + * the appropriate plugin signals. + */ + void finishDownload(DownloadID id); void downloadError(QNetworkReply::NetworkError error); void metaDataChanged(); - void directoryChanged(const QString& dirctory); - void checkDownloadTimeout(); private: void createMetaFile(DownloadInfo* info); @@ -561,8 +725,15 @@ public: QString getDownloadFileName(const QString& baseName, bool rename = false) const; private: - void startDownload(QNetworkReply* reply, DownloadInfo* newDownload, bool resume); - void resumeDownloadInt(int index); + /** + * @brief Begin downloading into newDownload from reply. + * + * On the !resume path newDownload becomes owned by m_ActiveDownloads on + * success; on failure (e.g. the output file cannot be opened) it is deleted + * before returning. Returns whether the download actually started. + */ + bool startDownload(QNetworkReply* reply, DownloadInfo* newDownload, bool resume); + void resumeDownloadInt(DownloadID id); /** * @brief start a download from a url @@ -573,7 +744,8 @@ private: *only happens if there is a duplicate and the user decides not to download again **/ bool addDownload(const QStringList& URLs, QString gameName, int modID, int fileID, - const MOBase::ModRepositoryFileInfo* fileInfo); + const MOBase::ModRepositoryFileInfo* fileInfo, + std::optional<DownloadID> reservedID = {}); // important: the caller has to lock the list-mutex, otherwise the // DownloadInfo-pointer might get invalidated at any time @@ -581,22 +753,38 @@ private: 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); - DownloadInfo* downloadInfoByID(unsigned int id); + DownloadInfo* downloadInfoByID(DownloadID id); void removePending(QString gameName, int modID, int fileID); + /** + * @brief Fire onDownloadFailed for a pending entry, if any matches. + * + * Used on Nexus API failures so callers holding a reserved id from + * addNXMDownload do not wait indefinitely for a result. No-op if no pending + * entry matches the (gameName, modID, fileID) triple. + */ + void notifyPendingDownloadFailed(const QString& gameName, int modID, int fileID); + + /** + * @brief Roll back a download that has not yet been activated. + * + * Ensures a caller awaiting the reservedID receives an onDownloadFailed + * callback. Must not be called once the download has been registered as + * active. + */ + void cancelPendingDownload(DownloadInfo* newDownload, QNetworkReply* reply); + static QString getFileTypeString(int fileType); void writeData(DownloadInfo* info); + QString getValidGameShortName(const QString& gameNexusName) const; + private: static const int AUTOMATIC_RETRIES = 3; @@ -606,45 +794,30 @@ private: OrganizerCore* m_OrganizerCore; QWidget* m_ParentWidget; - QVector<std::tuple<QString, int, int>> m_PendingDownloads; + QVector<PendingDownload> m_PendingDownloads; QVector<DownloadInfo*> m_ActiveDownloads; + // Secondary index into m_ActiveDownloads keyed by m_DownloadID; kept in sync + // with every m_ActiveDownloads mutation. + QHash<DownloadID, DownloadInfo*> m_ByID; + QString m_OutputDirectory; std::set<int> m_RequestIDs; - QVector<int> 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<QString, int> m_DownloadFails; - bool m_ShowHidden; MOBase::IPluginGame const* m_ManagedGame; - - QTimer m_TimeoutTimer; -}; - -class ScopedDisableDirWatcher -{ -public: - ScopedDisableDirWatcher(DownloadManager* downloadManager); - ~ScopedDisableDirWatcher(); - -private: - DownloadManager* m_downloadManager; }; #endif // DOWNLOADMANAGER_H diff --git a/src/downloadstab.cpp b/src/downloadstab.cpp index 4f988bf0..ee0c250c 100644 --- a/src/downloadstab.cpp +++ b/src/downloadstab.cpp @@ -49,12 +49,17 @@ DownloadsTab::DownloadsTab(OrganizerCore& core, Ui::MainWindow* mwui) SLOT(removeDownload(int, bool))); connect(ui.list, SIGNAL(restoreDownload(int)), m_core.downloadManager(), SLOT(restoreDownload(int))); - connect(ui.list, SIGNAL(cancelDownload(int)), m_core.downloadManager(), - SLOT(cancelDownload(int))); - connect(ui.list, SIGNAL(pauseDownload(int)), m_core.downloadManager(), - SLOT(pauseDownload(int))); - connect(ui.list, &DownloadListView::resumeDownload, [&](int i) { - resumeDownload(i); + // The view reports actions by row; translate to DownloadID at the boundary. + connect(ui.list, &DownloadListView::cancelDownload, this, [this](int row) { + auto* dm = m_core.downloadManager(); + dm->cancelDownload(dm->downloadIDAtRow(row)); + }); + connect(ui.list, &DownloadListView::pauseDownload, this, [this](int row) { + auto* dm = m_core.downloadManager(); + dm->pauseDownload(dm->downloadIDAtRow(row)); + }); + connect(ui.list, &DownloadListView::resumeDownload, this, [this](int row) { + resumeDownload(row); }); } @@ -105,6 +110,7 @@ void DownloadsTab::queryInfos() void DownloadsTab::resumeDownload(int downloadIndex) { m_core.loggedInAction(ui.list, [this, downloadIndex] { - m_core.downloadManager()->resumeDownload(downloadIndex); + auto* dm = m_core.downloadManager(); + dm->resumeDownload(dm->downloadIDAtRow(downloadIndex)); }); } diff --git a/src/modlistviewactions.cpp b/src/modlistviewactions.cpp index 6b26a1e3..9f289678 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 a8fc67c2..dff3485e 100644 --- a/src/organizercore.cpp +++ b/src/organizercore.cpp @@ -866,7 +866,8 @@ OrganizerCore::doInstall(const QString& archivePath, GuessedValue<QString> 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); |
