Fix DownloadManager, improve some network code

src/downloadlist.h

@@ -88,7 +88,8 @@ public 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
+   * @param row the row that changed. This can be negative to update the whole view,
+   * but you have to call aboutToUpdate() before update(-1) and only for that case.
    **/
   void update(int row);
 

src/downloadmanager.cpp

@@ -30,10 +30,8 @@ along with Mod Organizer.  If not, see <http://www.gnu.org/licenses/>.
 #include "selectiondialog.h"
 #include "shared/util.h"
 #include "utility.h"
-#include <nxmurl.h>
 #include <report.h>
 #include <taskprogressmanager.h>
-#include <utility.h>
 
 #include <QCoreApplication>
 #include <QDesktopServices>
@@ -84,21 +82,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()) {
+  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)) {
@@ -339,7 +338,7 @@ void DownloadManager::refreshList()
     // avoid triggering other refreshes
     ScopedDisableDirWatcher scopedDirWatcher(this);
 
-    int downloadsBefore = m_ActiveDownloads.size();
+    // int downloadsBefore = m_ActiveDownloads.size(); // unused
 
     // remove finished downloads
     for (QVector<DownloadInfo*>::iterator iter = m_ActiveDownloads.begin();
@@ -447,7 +446,7 @@ void DownloadManager::refreshList()
 void DownloadManager::queryDownloadListInfo()
 {
   int incompleteCount = 0;
-  for (size_t i = 0; i < m_ActiveDownloads.size(); i++) {
+  for (qsizetype i = 0; i < m_ActiveDownloads.size(); i++) {
     if (isInfoIncomplete(i)) {
       incompleteCount++;
     }
@@ -464,7 +463,7 @@ void DownloadManager::queryDownloadListInfo()
     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++) {
+    for (qsizetype i = 0; i < m_ActiveDownloads.size(); i++) {
       if (isInfoIncomplete(i)) {
         queryInfoMd5(i, false);
       }
@@ -508,6 +507,8 @@ bool DownloadManager::addDownload(const QStringList& URLs, QString gameName, int
   QNetworkRequest request(preferredUrl);
   request.setHeader(QNetworkRequest::UserAgentHeader,
                     m_NexusInterface->getAccessManager()->userAgent());
+  request.setAttribute(QNetworkRequest::RedirectPolicyAttribute,
+                       QNetworkRequest::NoLessSafeRedirectPolicy);
   request.setAttribute(QNetworkRequest::CacheSaveControlAttribute, false);
   request.setAttribute(QNetworkRequest::CacheLoadControlAttribute,
                        QNetworkRequest::AlwaysNetwork);
@@ -567,7 +568,7 @@ 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) {
+  for (const auto& game : games) {
     MOBase::IPluginGame* gamePlugin = m_OrganizerCore->getGame(game);
     if (gamePlugin != nullptr &&
         gamePlugin->gameNexusName().compare(gameName, Qt::CaseInsensitive) == 0) {
@@ -576,7 +577,7 @@ void DownloadManager::removePending(QString gameName, int modID, int fileID)
     }
   }
   emit aboutToUpdate();
-  for (auto iter : m_PendingDownloads) {
+  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));
@@ -603,7 +604,7 @@ void DownloadManager::startDownload(QNetworkReply* reply, DownloadInfo* newDownl
   }
 
   newDownload->m_StartTime.start();
-  createMetaFile(newDownload);
+  // createMetaFile(newDownload); // only create meta file on finished download
 
   if (!newDownload->m_Output.open(mode)) {
     reportError(tr("failed to download %1: could not open output file: %2")
@@ -625,10 +626,9 @@ void DownloadManager::startDownload(QNetworkReply* reply, DownloadInfo* newDownl
     removePending(newDownload->m_FileInfo->gameName, newDownload->m_FileInfo->modID,
                   newDownload->m_FileInfo->fileID);
 
-    emit aboutToUpdate();
     m_ActiveDownloads.append(newDownload);
 
-    emit update(-1);
+    emit update(indexByInfo(newDownload));
     emit downloadAdded();
 
     if (QFile::exists(m_OutputDirectory + "/" + newDownload->m_FileName)) {
@@ -654,22 +654,22 @@ void DownloadManager::startDownload(QNetworkReply* reply, DownloadInfo* newDownl
         else
           setState(newDownload, STATE_DOWNLOADING);
       }
-    } else
-      connect(newDownload->m_Reply, SIGNAL(finished()), this, SLOT(downloadFinished()));
+    }
+  }
+  // always connect finished to avoid missing the finished signal in certain flows
+  connect(newDownload->m_Reply, SIGNAL(finished()), this, SLOT(downloadFinished()));
 
-    QCoreApplication::processEvents();
+  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;
+  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);
     }
-  } else
-    connect(newDownload->m_Reply, SIGNAL(finished()), this, SLOT(downloadFinished()));
+    return;
+  }
 }
 
 void DownloadManager::addNXMDownload(const QString& url)
@@ -680,7 +680,7 @@ void DownloadManager::addNXMDownload(const QString& url)
   MOBase::IPluginGame* foundGame = nullptr;
   validGames.append(m_ManagedGame->gameShortName());
   validGames.append(m_ManagedGame->validShortNames());
-  for (auto game : validGames) {
+  for (const auto& game : validGames) {
     MOBase::IPluginGame* gamePlugin = m_OrganizerCore->getGame(game);
 
     // some game plugins give names in validShortNames() that may refer to other
@@ -712,11 +712,11 @@ void DownloadManager::addNXMDownload(const QString& url)
     return;
   }
 
-  for (auto tuple : m_PendingDownloads) {
+  for (const 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())
@@ -1116,7 +1116,7 @@ void DownloadManager::queryInfo(int index)
     if (choices.size() == 1) {
       info->m_FileInfo->gameName = choices[0].first;
     } else {
-      for (auto choice : choices) {
+      for (const auto& choice : choices) {
         selection.addChoice(choice.first, choice.second, choice.first);
       }
       if (selection.exec() == QDialog::Accepted) {
@@ -1597,6 +1597,13 @@ QString DownloadManager::getFileNameFromNetworkReply(QNetworkReply* reply)
       return MOBase::sanitizeFileName(QString::fromUtf8(result.str(1).c_str()));
     }
   }
+  // fallback to url path
+  if (reply->url().isValid()) {
+    auto filename = QFileInfo(reply->url().path()).fileName();
+    std::regex exp("^.+\\..+$");
+    if (std::regex_search(filename.toStdString(), exp))
+      return MOBase::sanitizeFileName(filename);
+  }
 
   return QString();
 }
@@ -1677,7 +1684,6 @@ void DownloadManager::downloadProgress(qint64 bytesReceived, qint64 bytesTotal)
   try {
     DownloadInfo* info = findDownload(this->sender(), &index);
     if (info != nullptr) {
-      info->m_HasData = true;
       if (info->m_State == STATE_CANCELING) {
         setState(info, STATE_CANCELED);
       } else if (info->m_State == STATE_PAUSING) {
@@ -1922,7 +1928,7 @@ void DownloadManager::nxmFileInfoAvailable(QString gameName, int modID, int file
 
   QStringList games(m_ManagedGame->validShortNames());
   games += m_ManagedGame->gameShortName();
-  for (auto game : games) {
+  for (const auto& game : games) {
     MOBase::IPluginGame* gamePlugin = m_OrganizerCore->getGame(game);
     if (gamePlugin != nullptr &&
         gamePlugin->gameNexusName().compare(gameName, Qt::CaseInsensitive) == 0) {
@@ -1971,20 +1977,33 @@ bool ServerByPreference(const ServerList::container& preferredServers,
   return (a > b);
 }
 
+int DownloadManager::generateRandomDownloadID()
+{
+  QRandomGenerator* generator = QRandomGenerator::global();
+  auto currentDownloadSet     = std::set<int>();
+  for (const auto& download : m_ActiveDownloads) {
+    currentDownloadSet.insert(download->m_DownloadID);
+  }
+  int newID;
+  do {
+    newID = generator->generate();
+  } while (currentDownloadSet.contains(newID));
+  return newID;
+}
+
 int DownloadManager::startDownloadURLs(const QStringList& urls)
 {
   ModRepositoryFileInfo info;
   addDownload(urls, "", -1, -1, &info);
-  return m_ActiveDownloads.size() - 1;
+  return generateRandomDownloadID();
 }
 
 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 generateRandomDownloadID();
 }
 
 QString DownloadManager::downloadPath(int id)
@@ -2159,7 +2178,7 @@ void DownloadManager::nxmFileInfoFromMd5Available(QString gameName, QVariant use
   QString gameShortName = gameName;
   QStringList games(m_ManagedGame->validShortNames());
   games += m_ManagedGame->gameShortName();
-  for (auto game : games) {
+  for (const auto& game : games) {
     MOBase::IPluginGame* gamePlugin = m_OrganizerCore->getGame(game);
     if (gamePlugin != nullptr &&
         gamePlugin->gameNexusName().compare(gameName, Qt::CaseInsensitive) == 0) {
@@ -2242,20 +2261,21 @@ void DownloadManager::downloadFinished(int index)
     info = m_ActiveDownloads[index];
   else {
     info = findDownload(this->sender(), &index);
-    if (info == nullptr && index == 0) {
-      info = m_ActiveDownloads[index];
+    if (info == nullptr && index == 0 && !m_ActiveDownloads.isEmpty()) {
+      info = m_ActiveDownloads.last();
     }
   }
 
   if (info != nullptr) {
     QNetworkReply* reply = info->m_Reply;
-    QByteArray data;
-    if (reply->isOpen() && info->m_HasData) {
-      data = reply->readAll();
-      info->m_Output.write(data);
+    if (reply->isFinished()) {
+      writeData(info);
+      info->m_Output.close();
+      TaskProgressManager::instance().forgetMe(info->m_TaskProgressId);
+    } else {
+      emit showMessage(
+          tr("Download finished event received but download is not finished?"));
     }
-    info->m_Output.close();
-    TaskProgressManager::instance().forgetMe(info->m_TaskProgressId);
 
     bool error = false;
     if ((info->m_State != STATE_CANCELING) && (info->m_State != STATE_PAUSING)) {
@@ -2287,29 +2307,27 @@ void DownloadManager::downloadFinished(int index)
     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());
-      }
+      writeData(info);
       setState(info, STATE_PAUSED);
     }
 
+    bool cancelled = false;
     if (info->m_State == STATE_CANCELED || (info->m_Tries == 0 && error)) {
       emit aboutToUpdate();
       info->m_Output.remove();
       delete info;
+      cancelled = true;
       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,
@@ -2358,7 +2376,7 @@ void DownloadManager::downloadFinished(int index)
     reply->close();
     reply->deleteLater();
 
-    if ((info->m_Tries > 0) && error) {
+    if (!cancelled && (info->m_Tries > 0) && error) {
       --info->m_Tries;
       resumeDownloadInt(index);
     }
@@ -2429,20 +2447,26 @@ void DownloadManager::checkDownloadTimeout()
 
 void DownloadManager::writeData(DownloadInfo* info)
 {
-  if (info != nullptr) {
-    qint64 ret = info->m_Output.write(info->m_Reply->readAll());
-    if (ret < info->m_Reply->size()) {
+  if (info != nullptr && info->m_Reply != nullptr && info->m_Reply->isOpen() &&
+      info->m_Reply->bytesAvailable() > 0) {
+    QByteArray data = info->m_Reply->readAll();
+    if (data.isEmpty()) {
+      return;
+    }
+    qint64 ret = info->m_Output.write(data);
+    if (ret < data.size()) {
       QString fileName =
           info->m_FileName;  // m_FileName may be destroyed after setState
       setState(info, DownloadState::STATE_CANCELED);
 
-      log::error("Unable to write download \"{}\" to drive (return {})",
-                 info->m_FileName, ret);
+      log::error("Unable to write download \"{}\" to drive (wrote {} of {})",
+                 info->m_FileName, ret, data.size());
 
-      reportError(tr("Unable to write download to drive (return %1).\n"
-                     "Check the drive's available storage.\n\n"
-                     "Canceling download \"%2\"...")
+      reportError(tr("Unable to write download to drive (wrote %1).\n"
+                     "Check the drive's available storage (need %2).\n\n"
+                     "Canceling download \"%3\"...")
                       .arg(ret)
+                      .arg(data.size())
                       .arg(fileName));
     }
   }