Download files from direct URLs via a command line parameter #1873

eddoursul · 2023-09-14T23:02:55Z

Adds the ability to use MO2's download manager to download files via direct URLs or temporary URLs from S3-compatible storages.

Usage:
~~ModOrganizer.exe "https://eddoursul.win/Cyberware%20TTW%20Patch.7z"~~
ModOrganizer.exe download "https://eddoursul.win/Cyberware%20TTW%20Patch.7z"

…compatible temporary URLs

Holt59

I made some minor comments on the review but I don't like the way this is currently done. I think downloading file should be a distinct command, like modorganizer.exe download https://xxx. This is how nxm links should be handled (but are not for historical reasons).

Holt59 · 2023-09-15T07:15:20Z

src/commandline.cpp

@@ -187,6 +187,8 @@ std::optional<int> CommandLine::process(const std::wstring& line)
        // not a shortcut, try a link
        if (isNxmLink(qs)) {
          m_nxmLink = qs;
+        } else if (qs.startsWith("https://", Qt::CaseInsensitive) || qs.startsWith("http://", Qt::CaseInsensitive)) {


I don't think http:// download links should be allowed.

It is about what user supplies to MO2. Links posted on the internet may begin with http: (this happens), when in fact, the link would be immediately redirected to https. If you want user to see a warning message, manually fix the link and run again, sure, I can add that.

I understand why you would do that but that's a very bad idea, especially knowing what's possible to do with mods. If people still post http:// links on their website, then that's the website faults, MO2 should not have to deal with that, we're in 2023.

I would simply not handle the link. Users are unlikely to run this command manually, so the warning would only be seen by the external tools that would dispatch link, in which case the check could be made there and MO2 would just have to return an error code.

Holt59 · 2023-09-15T07:16:41Z

src/downloadmanager.cpp

@@ -440,7 +455,10 @@ bool DownloadManager::addDownload(const QStringList &URLs, QString gameName,
  h2Conf.setStreamReceiveWindowSize(16777215);
  QNetworkRequest request(preferredUrl);
  request.setHeader(QNetworkRequest::UserAgentHeader, m_NexusInterface->getAccessManager()->userAgent());
+  request.setAttribute(QNetworkRequest::CacheSaveControlAttribute, false);


Any reason for these two added attributes?

The same reason they are used in Nexus downloads https://github.com/ModOrganizer2/modorganizer/blob/qt6/src/nexusinterface.cpp#L821 . Spent half a day pulling my hair out on this.

By default, MO2 caches repeating requests, especially with small files. When this happens, the finished() signal is sent immediately, causing downloadFinished to fire before downloading a single byte. Then the check for size==0 kicks in here https://github.com/ModOrganizer2/modorganizer/blob/qt6/src/downloadmanager.cpp#L2100 , and the download gets marked as failed without showing a single message, because messages don't cover this case.

These two lines disable cache for the request.

Holt59 · 2023-09-15T07:17:24Z

src/downloadmanager.cpp

@@ -580,7 +598,10 @@ void DownloadManager::startDownload(QNetworkReply *reply, DownloadInfo *newDownl
      newDownload->m_State != STATE_READY &&
      newDownload->m_State != STATE_FETCHINGMODINFO &&
      reply->isFinished()) {
-      downloadFinished(indexByInfo(newDownload));
+      int index = indexByInfo(newDownload);
+      if (index >= 0) {


The needs for this should be investigated, although probably not relevant for this PR, but maybe add a TODO comment.

If the fix to downloadFinished (below) is accepted, this may be redundant (I need to re-check). If it's not accepted, downloadFinished may fire with invalid -1 value, causing the no download index warning.

Holt59 · 2023-09-15T07:17:42Z

src/downloadmanager.cpp

@@ -916,7 +937,7 @@ void DownloadManager::resumeDownloadInt(int index)

  // Check for finished download;
  if (info->m_TotalSize <= info->m_Output.size() && info->m_Reply != nullptr
-      && info->m_Reply->isOpen() && info->m_Reply->isFinished() && info->m_State != STATE_ERROR) {
+      && info->m_Reply->isFinished() && info->m_State != STATE_ERROR) {


Any reason for this change?

Correct, this is actually a leftover from investigating the cache issue, I used resumeDownloadInt
as a crutch. When MO2 is closed and file exists, resumeDownloadInt would skip this block, because connection is not open at that point, and append the same contents to the file again, doubling its size.
Since resumeDownloadInt is irrelevant to this PR, I have reverted this change.

Correction - this is unrelated to the cache issue, happens with disabled cache as well, I brought the change back.

Holt59 · 2023-09-15T07:18:29Z

src/downloadmanager.cpp

@@ -2076,10 +2097,14 @@ void DownloadManager::nxmRequestFailed(QString gameName, int modID, int fileID,
 void DownloadManager::downloadFinished(int index)
 {
  DownloadInfo *info;
-  if (index)
+  if (index > 0)


Any reason for this change (and the one below)?

downloadFinished is used in two ways:

as a function, accepting an integer argument,

as a signal callback.

Currently, downloadFinished has a bug, when it does not run for the first download in the list (index 0), because of the logical mistake here 0==false https://github.com/ModOrganizer2/modorganizer/blob/qt6/src/downloadmanager.cpp#L2079

When it runs with downloadFinished(0), if (index) skips execution and jumps to detecting the signal sender, which yields nothing, because it is not a signal call. As a result, the first file may never see the finishing routine, causing cascade of errors and discrepancy in interface and actual status. In case of the proposed command, this looks like a duplicating paused file, when user tries to redownload the first file.

The first idea was to add if (index >= 0), but then the signal callback info = findDownload(this->sender(), &index) never fires, because during the callback index is also 0.

So I implemented detection of the first row as a fallback after all paths are exhausted.

I expected opposition on this fix and added the additional check in startDownload, catching possible downloadFinished(-1) call, which is an invalid value, causing the no download index warning. If the fix is accepted, the startDownload change probably becomes redundant.

Holt59 · 2023-09-15T07:18:44Z

src/moapplication.cpp

@@ -405,6 +405,9 @@ void MOApplication::externalMessage(const QString& message)
  } else if (isNxmLink(message)) {
    MessageDialog::showMessage(tr("Download started"), qApp->activeWindow(), false);
    m_core->downloadRequestedNXM(message);
+  } else if (message.startsWith("https://", Qt::CaseInsensitive) || message.startsWith("http://", Qt::CaseInsensitive)) {


Same as above, I don't think http:// links should be accepted.

Al12rs · 2023-09-15T08:46:21Z

I agree with Holt, this should be behind a dedicated download command.

Holt59 · 2023-09-15T11:50:47Z

I've changed the target branch to master since qt6 should no longer be used but apparently @Silarn made commits to qt6 that are not in master so this should be fixed before this can be merged (and this will probably need to be rebased).

This reverts commit dfa9800.

# Conflicts: # src/aboutdialog.cpp # src/aboutdialog.h # src/commandline.cpp # src/commandline.h # src/directoryrefresher.cpp # src/downloadmanager.cpp # src/organizercore.cpp

src/commandline.cpp

eddoursul added 2 commits September 15, 2023 00:50

Download files via a command line parameter

6228644

Use a GET parameter to predetermine Content-Disposition header in S3-…

f0494dc

…compatible temporary URLs

Holt59 requested changes Sep 15, 2023

View reviewed changes

Reverted resumeDownloadInt

dfa9800

Holt59 changed the base branch from qt6 to master September 15, 2023 11:47

eddoursul added 3 commits September 15, 2023 13:52

Revert "Reverted resumeDownloadInt"

2821606

This reverts commit dfa9800.

Moved downloading to a dedicated command

c1f41f2

Enforce HTTPS download links

72727c7

eddoursul requested a review from Holt59 September 15, 2023 12:57

eddoursul added 4 commits September 15, 2023 15:58

Merge branch 'master' into qt6

a6d950b

# Conflicts: # src/aboutdialog.cpp # src/aboutdialog.h # src/commandline.cpp # src/commandline.h # src/directoryrefresher.cpp # src/downloadmanager.cpp # src/organizercore.cpp

Return numeric error codes

cf45dfd

Return numeric error codes

8908df4

clang-format pass

1c6ad48

Holt59 reviewed Sep 18, 2023

View reviewed changes

src/commandline.cpp Show resolved Hide resolved

Merge branch 'master' into qt6

1ffcc6f

eddoursul requested a review from Holt59 September 18, 2023 08:58

Holt59 merged commit 59b5b1c into ModOrganizer2:master Sep 18, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Download files from direct URLs via a command line parameter #1873

Download files from direct URLs via a command line parameter #1873

eddoursul commented Sep 14, 2023 •

edited

Loading

Holt59 left a comment

Holt59 Sep 15, 2023

vensko Sep 15, 2023 •

edited

Loading

Holt59 Sep 15, 2023

Holt59 Sep 15, 2023

vensko Sep 15, 2023 •

edited

Loading

Holt59 Sep 15, 2023

vensko Sep 15, 2023 •

edited

Loading

Holt59 Sep 15, 2023

vensko Sep 15, 2023 •

edited

Loading

vensko Sep 15, 2023

Holt59 Sep 15, 2023

vensko Sep 15, 2023 •

edited

Loading

Holt59 Sep 15, 2023

Al12rs commented Sep 15, 2023

Holt59 commented Sep 15, 2023

Download files from direct URLs via a command line parameter #1873

Download files from direct URLs via a command line parameter #1873

Conversation

eddoursul commented Sep 14, 2023 • edited Loading

Holt59 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vensko Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vensko Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vensko Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vensko Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vensko Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Al12rs commented Sep 15, 2023

Holt59 commented Sep 15, 2023

eddoursul commented Sep 14, 2023 •

edited

Loading

vensko Sep 15, 2023 •

edited

Loading

vensko Sep 15, 2023 •

edited

Loading

vensko Sep 15, 2023 •

edited

Loading

vensko Sep 15, 2023 •

edited

Loading

vensko Sep 15, 2023 •

edited

Loading