Fix Q(Quick)WebEngineDownloadItem::setDownloadDirectory()

Keep the custom file name if the calling order of setDownloadDirectory()
and setDownloadFileName() changes.

Also do not emit patchChanged signal twice if setDownloadDirectory()
changes the uniquifier of the file name.

Add TempDir for qml auto tests what uses QTemporaryDir() to create
temporary directory for downloads.

See https://cgit.kde.org/messagelib.git/commit/?id=2c113dcb155b11bf2c0af3c85544962485784b26
for details.

Fixes: QTBUG-80566
Change-Id: Ia76f263558eaf55cb297700407948523788c6229
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>

8 files changed, 163 insertions, 21 deletions

diff --git a/src/webengine/api/qquickwebenginedownloaditem.cpp b/src/webengine/api/qquickwebenginedownloaditem.cpp
index 6abd89910..767ada58f 100644
--- a/src/webengine/api/qquickwebenginedownloaditem.cpp
+++ b/src/webengine/api/qquickwebenginedownloaditem.cpp
@@ -112,6 +112,7 @@ QQuickWebEngineDownloadItemPrivate::QQuickWebEngineDownloadItemPrivate(QQuickWeb
     , downloadPaused(false)
     , view(nullptr)
     , downloadUrl(url)
+    , isCustomFileName(false)
 {
 }
 
@@ -511,25 +512,31 @@ void QQuickWebEngineDownloadItem::setDownloadDirectory(const QString &directory)
         return;
     }
 
+    bool isPathChanged = false;
     QString changeDirectory = d->downloadDirectory;
     if (!directory.isEmpty() && changeDirectory != directory) {
         changeDirectory = directory;
 
         if (d->downloadDirectory != changeDirectory) {
             d->downloadDirectory = changeDirectory;
-            Q_EMIT pathChanged();
             Q_EMIT downloadDirectoryChanged();
+            isPathChanged = true;
         }
 
-        QString newFileName = QFileInfo(d->profile->d_ptr->profileAdapter()->determineDownloadPath(d->downloadDirectory,
-                                                                                                   d->suggestedFileName,
-                                                                                                   d->startTime)).fileName();
-        if (d->downloadFileName != newFileName) {
-            d->downloadFileName = newFileName;
-            Q_EMIT pathChanged();
-            Q_EMIT downloadFileNameChanged();
+        if (!d->isCustomFileName) {
+            QString newFileName = QFileInfo(d->profile->d_ptr->profileAdapter()->determineDownloadPath(d->downloadDirectory,
+                                                                                                       d->suggestedFileName,
+                                                                                                       d->startTime)).fileName();
+            if (d->downloadFileName != newFileName) {
+                d->downloadFileName = newFileName;
+                Q_EMIT downloadFileNameChanged();
+                isPathChanged = true;
+            }
         }
     }
+
+    if (isPathChanged)
+        Q_EMIT pathChanged();
 }
 
 /*!
@@ -561,8 +568,9 @@ void QQuickWebEngineDownloadItem::setDownloadFileName(const QString &fileName)
 
     if (d->downloadFileName != fileName && !fileName.isEmpty()) {
         d->downloadFileName = fileName;
-        Q_EMIT pathChanged();
+        d->isCustomFileName = true;
         Q_EMIT downloadFileNameChanged();
+        Q_EMIT pathChanged();
     }
 }
 
diff --git a/src/webengine/api/qquickwebenginedownloaditem_p.h b/src/webengine/api/qquickwebenginedownloaditem_p.h
index beb359622..e1b1b9040 100644
--- a/src/webengine/api/qquickwebenginedownloaditem_p.h
+++ b/src/webengine/api/qquickwebenginedownloaditem_p.h
@@ -169,6 +169,7 @@ public:
     void setDownloadDirectory(const QString &directory);
     QString downloadFileName() const;
     void setDownloadFileName(const QString &fileName);
+    bool isCustomFileName;
 
 Q_SIGNALS:
     void stateChanged();
diff --git a/src/webengine/api/qquickwebenginedownloaditem_p_p.h b/src/webengine/api/qquickwebenginedownloaditem_p_p.h
index 1be6434ec..acd7fe806 100644
--- a/src/webengine/api/qquickwebenginedownloaditem_p_p.h
+++ b/src/webengine/api/qquickwebenginedownloaditem_p_p.h
@@ -87,6 +87,7 @@ public:
     QString suggestedFileName;
     QString downloadDirectory;
     QString downloadFileName;
+    bool isCustomFileName;
 
     void update(const QtWebEngineCore::ProfileAdapterClient::DownloadItemInfo &info);
     void updateState(QQuickWebEngineDownloadItem::DownloadState newState);
diff --git a/src/webenginewidgets/api/qwebenginedownloaditem.cpp b/src/webenginewidgets/api/qwebenginedownloaditem.cpp
index fd7d90704..7366dbf59 100644
--- a/src/webenginewidgets/api/qwebenginedownloaditem.cpp
+++ b/src/webenginewidgets/api/qwebenginedownloaditem.cpp
@@ -170,6 +170,7 @@ QWebEngineDownloadItemPrivate::QWebEngineDownloadItemPrivate(QWebEngineProfilePr
     , interruptReason(QWebEngineDownloadItem::NoReason)
     , downloadUrl(url)
     , downloadPaused(false)
+    , isCustomFileName(false)
     , totalBytes(-1)
     , receivedBytes(0)
     , page(0)
@@ -594,14 +595,15 @@ void QWebEngineDownloadItem::setDownloadDirectory(const QString &directory)
     if (d->downloadState != QWebEngineDownloadItem::DownloadRequested) {
         qWarning("Setting the download directory is not allowed after the download has been accepted.");
         return;
-     }
+    }
 
     if (!directory.isEmpty() && d->downloadDirectory != directory)
         d->downloadDirectory = directory;
 
-    d->downloadFileName = QFileInfo(d->profile->profileAdapter()->determineDownloadPath(d->downloadDirectory,
-                                                                                        d->suggestedFileName,
-                                                                                        d->startTime)).fileName();
+    if (!d->isCustomFileName)
+        d->downloadFileName = QFileInfo(d->profile->profileAdapter()->determineDownloadPath(d->downloadDirectory,
+                                                                                            d->suggestedFileName,
+                                                                                            d->startTime)).fileName();
 }
 
 /*!
@@ -634,8 +636,10 @@ void QWebEngineDownloadItem::setDownloadFileName(const QString &fileName)
         return;
     }
 
-    if (!fileName.isEmpty())
+    if (!fileName.isEmpty()) {
         d->downloadFileName = fileName;
+        d->isCustomFileName = true;
+    }
 }
 
 /*!
diff --git a/src/webenginewidgets/api/qwebenginedownloaditem_p.h b/src/webenginewidgets/api/qwebenginedownloaditem_p.h
index 034684a00..04c6fadcc 100644
--- a/src/webenginewidgets/api/qwebenginedownloaditem_p.h
+++ b/src/webenginewidgets/api/qwebenginedownloaditem_p.h
@@ -82,6 +82,7 @@ public:
     QString suggestedFileName;
     QString downloadDirectory;
     QString downloadFileName;
+    bool isCustomFileName;
 
     qint64 totalBytes;
     qint64 receivedBytes;
diff --git a/tests/auto/quick/qmltests/data/tst_download.qml b/tests/auto/quick/qmltests/data/tst_download.qml
index e049f3621..1b1750dd8 100644
--- a/tests/auto/quick/qmltests/data/tst_download.qml
+++ b/tests/auto/quick/qmltests/data/tst_download.qml
@@ -30,6 +30,7 @@ import QtQuick 2.0
 import QtTest 1.0
 import QtWebEngine 1.10
 import Qt.labs.platform 1.0
+import Test.util 1.0
 
 TestWebEngineView {
     id: webEngineView
@@ -51,6 +52,9 @@ TestWebEngineView {
     property int downloadDirectoryChanged: 0
     property int downloadFileNameChanged: 0
     property int downloadPathChanged: 0
+    property bool setDirectoryFirst: false
+
+    TempDir { id: tempDir }
 
     function urlToPath(url) {
         var path = url.toString()
@@ -87,7 +91,7 @@ TestWebEngineView {
         id: testDownloadProfile
 
         onDownloadRequested: {
-            testDownloadProfile.downloadPath = urlToPath(StandardPaths.writableLocation(StandardPaths.TempLocation))
+            testDownloadProfile.downloadPath = tempDir.path()
             downloadState.push(download.state)
             downloadItemConnections.target = download
             if (cancelDownload) {
@@ -99,8 +103,15 @@ TestWebEngineView {
                     download.path = testDownloadProfile.downloadPath + downloadedSetPath
                     downloadedPath = download.path
                 } else {
-                    download.downloadDirectory = downloadDirectory.length != 0 ? testDownloadProfile.downloadPath + downloadDirectory : testDownloadProfile.downloadPath
-                    download.downloadFileName = downloadFileName.length != 0 ? downloadFileName : "testfile.zip"
+                    if (setDirectoryFirst && downloadDirectory.length != 0)
+                        download.downloadDirectory = testDownloadProfile.downloadPath + downloadDirectory
+
+                    if (downloadFileName.length != 0)
+                        download.downloadFileName = downloadFileName
+
+                    if (!setDirectoryFirst && downloadDirectory.length != 0)
+                        download.downloadDirectory = testDownloadProfile.downloadPath + downloadDirectory
+
                     downloadedPath = download.downloadDirectory + download.downloadFileName
                 }
 
@@ -133,10 +144,12 @@ TestWebEngineView {
             downloadFileName = ""
             downloadedPath = ""
             downloadedSetPath = ""
+            setDirectoryFirst = false
         }
 
         function test_downloadRequest() {
             compare(downLoadRequestedSpy.count, 0)
+            downloadDirectory = "/test_downloadRequest/";
             webEngineView.url = Qt.resolvedUrl("download.zip")
             downLoadRequestedSpy.wait()
             compare(downLoadRequestedSpy.count, 1)
@@ -148,6 +161,7 @@ TestWebEngineView {
 
         function test_totalFileLength() {
             compare(downLoadRequestedSpy.count, 0)
+            downloadDirectory = "/test_totalFileLength/";
             webEngineView.url = Qt.resolvedUrl("download.zip")
             downLoadRequestedSpy.wait()
             compare(downLoadRequestedSpy.count, 1)
@@ -159,6 +173,7 @@ TestWebEngineView {
 
         function test_downloadSucceeded() {
             compare(downLoadRequestedSpy.count, 0)
+            downloadDirectory = "/test_downloadSucceeded/";
             webEngineView.url = Qt.resolvedUrl("download.zip")
             downLoadRequestedSpy.wait()
             compare(downLoadRequestedSpy.count, 1)
@@ -196,11 +211,19 @@ TestWebEngineView {
             compare(testDownloadProfile.downloadPath, downloadPath);
         }
 
-        function test_downloadToDirectoryWithFileName() {
+        function test_downloadToDirectoryWithFileName_data() {
+            return [
+                   { tag: "setDirectoryFirst", setDirectoryFirst: true },
+                   { tag: "setFileNameFirst", setDirectoryFirst: false },
+            ];
+        }
+
+        function test_downloadToDirectoryWithFileName(row) {
             compare(downLoadRequestedSpy.count, 0);
             compare(downloadDirectoryChanged, 0);
             compare(downloadFileNameChanged, 0);
-            downloadDirectory = "/test/";
+            setDirectoryFirst = row.setDirectoryFirst;
+            downloadDirectory = "/test_downloadToDirectoryWithFileName/";
             downloadFileName = "test.zip";
             webEngineView.url = Qt.resolvedUrl("download.zip");
             downLoadRequestedSpy.wait();
@@ -219,11 +242,82 @@ TestWebEngineView {
             verify(!downloadInterruptReason);
         }
 
+        function test_downloadToDirectoryWithSuggestedFileName() {
+            // Download file to a custom download directory with suggested file name.
+            compare(downLoadRequestedSpy.count, 0);
+            compare(downloadDirectoryChanged, 0);
+            compare(downloadFileNameChanged, 0);
+            downloadDirectory = "/test_downloadToDirectoryWithSuggestedFileName/";
+            webEngineView.url = Qt.resolvedUrl("download.zip");
+            downLoadRequestedSpy.wait();
+            compare(downLoadRequestedSpy.count, 1);
+            compare(downloadUrl, webEngineView.url);
+            compare(suggestedFileName, "download.zip");
+            compare(downloadState[0], WebEngineDownloadItem.DownloadRequested);
+            tryCompare(downloadState, "1", WebEngineDownloadItem.DownloadInProgress);
+            compare(downloadedPath, testDownloadProfile.downloadPath + downloadDirectory + "download.zip");
+            compare(downloadDirectoryChanged, 1);
+            compare(downloadFileNameChanged, 0);
+            compare(downloadPathChanged, 1);
+            downloadFinishedSpy.wait();
+            compare(totalBytes, receivedBytes);
+            tryCompare(downloadState, "2", WebEngineDownloadItem.DownloadCompleted);
+            verify(!downloadInterruptReason);
+
+            // Download the same file to another directory with suggested file name.
+            // The downloadFileNameChanged signal should not be emitted.
+            downLoadRequestedSpy.clear();
+            compare(downLoadRequestedSpy.count, 0);
+            downloadDirectoryChanged = 0;
+            downloadFileNameChanged = 0;
+            downloadPathChanged = 0;
+            downloadDirectory = "/test_downloadToDirectoryWithSuggestedFileName1/";
+            webEngineView.url = Qt.resolvedUrl("download.zip");
+            downLoadRequestedSpy.wait();
+            compare(downLoadRequestedSpy.count, 1);
+            compare(downloadUrl, webEngineView.url);
+            compare(suggestedFileName, "download.zip");
+            compare(downloadState[0], WebEngineDownloadItem.DownloadRequested);
+            tryCompare(downloadState, "1", WebEngineDownloadItem.DownloadInProgress);
+            compare(downloadedPath, testDownloadProfile.downloadPath + downloadDirectory + "download.zip");
+            compare(downloadDirectoryChanged, 1);
+            compare(downloadFileNameChanged, 0);
+            compare(downloadPathChanged, 1);
+            downloadFinishedSpy.wait();
+            compare(totalBytes, receivedBytes);
+            tryCompare(downloadState, "2", WebEngineDownloadItem.DownloadCompleted);
+            verify(!downloadInterruptReason);
+
+            // Download same file to same directory second time -> file name should be unified.
+            // The downloadFileNameChanged signal should be emitted.
+            downLoadRequestedSpy.clear();
+            compare(downLoadRequestedSpy.count, 0);
+            downloadDirectoryChanged = 0;
+            downloadFileNameChanged = 0;
+            downloadPathChanged = 0;
+            downloadDirectory = "/test_downloadToDirectoryWithSuggestedFileName1/";
+            webEngineView.url = Qt.resolvedUrl("download.zip");
+            downLoadRequestedSpy.wait();
+            compare(downLoadRequestedSpy.count, 1);
+            compare(downloadUrl, webEngineView.url);
+            compare(suggestedFileName, "download.zip");
+            compare(downloadState[0], WebEngineDownloadItem.DownloadRequested);
+            tryCompare(downloadState, "1", WebEngineDownloadItem.DownloadInProgress);
+            compare(downloadedPath, testDownloadProfile.downloadPath + downloadDirectory + "download (1).zip");
+            compare(downloadDirectoryChanged, 1);
+            compare(downloadFileNameChanged, 1);
+            compare(downloadPathChanged, 1);
+            downloadFinishedSpy.wait();
+            compare(totalBytes, receivedBytes);
+            tryCompare(downloadState, "2", WebEngineDownloadItem.DownloadCompleted);
+            verify(!downloadInterruptReason);
+}
+
         function test_downloadWithSetPath() {
             compare(downLoadRequestedSpy.count, 0);
             compare(downloadDirectoryChanged, 0);
             compare(downloadFileNameChanged, 0);
-            downloadedSetPath = "/test/test.zip";
+            downloadedSetPath = "/test_downloadWithSetPath/test.zip";
             webEngineView.url = Qt.resolvedUrl("download.zip");
             downLoadRequestedSpy.wait();
             compare(downLoadRequestedSpy.count, 1);
diff --git a/tests/auto/quick/qmltests/tst_qmltests.cpp b/tests/auto/quick/qmltests/tst_qmltests.cpp
index ba7a992db..d70a43895 100644
--- a/tests/auto/quick/qmltests/tst_qmltests.cpp
+++ b/tests/auto/quick/qmltests/tst_qmltests.cpp
@@ -27,8 +27,10 @@
 ****************************************************************************/
 
 #include <QtCore/QScopedPointer>
+#include <QTemporaryDir>
 #include <QtQuickTest/quicktest.h>
 #include <QtWebEngine/QQuickWebEngineProfile>
+#include <QQmlEngine>
 #include "qt_webengine_quicktest.h"
 
 #if defined(Q_OS_LINUX) && defined(QT_DEBUG)
@@ -95,6 +97,19 @@ static void sigSegvHandler(int signum)
 }
 #endif
 
+class TempDir : public QObject {
+    Q_OBJECT
+
+public:
+    Q_INVOKABLE QString path() {
+        Q_ASSERT(tempDir.isValid());
+        return tempDir.isValid() ? tempDir.path() : QString();
+    }
+
+private:
+    QTemporaryDir tempDir;
+};
+
 int main(int argc, char **argv)
 {
 #if defined(Q_OS_LINUX) && defined(QT_DEBUG)
@@ -127,9 +142,12 @@ int main(int argc, char **argv)
     }
     QtWebEngine::initialize();
     QQuickWebEngineProfile::defaultProfile()->setOffTheRecord(true);
+    qmlRegisterType<TempDir>("Test.util", 1, 0, "TempDir");
 
     QTEST_SET_MAIN_SOURCE_PATH
 
     int i = quick_test_main(argc, argv, "qmltests", QUICK_TEST_SOURCE_DIR);
     return i;
 }
+
+#include "tst_qmltests.moc"
diff --git a/tests/auto/widgets/qwebenginedownloaditem/tst_qwebenginedownloaditem.cpp b/tests/auto/widgets/qwebenginedownloaditem/tst_qwebenginedownloaditem.cpp
index bbcef2226..55d8ac6e8 100644
--- a/tests/auto/widgets/qwebenginedownloaditem/tst_qwebenginedownloaditem.cpp
+++ b/tests/auto/widgets/qwebenginedownloaditem/tst_qwebenginedownloaditem.cpp
@@ -80,6 +80,7 @@ private Q_SLOTS:
 #if QT_DEPRECATED_SINCE(5, 14)
     void downloadPathValidation();
 #endif
+    void downloadToDirectoryWithFileName_data();
     void downloadToDirectoryWithFileName();
 
 private:
@@ -1271,8 +1272,17 @@ void tst_QWebEngineDownloadItem::downloadPathValidation()
 }
 #endif
 
+void tst_QWebEngineDownloadItem::downloadToDirectoryWithFileName_data()
+{
+    QTest::addColumn<bool>("setDirectoryFirst");
+
+    QTest::newRow("setDirectoryFirst") << true;
+    QTest::newRow("setFileNameFirst") << false;
+}
+
 void tst_QWebEngineDownloadItem::downloadToDirectoryWithFileName()
 {
+    QFETCH(bool, setDirectoryFirst);
     QString downloadDirectory;
     QString downloadFileName;
     QString downloadedFilePath;
@@ -1302,7 +1312,7 @@ void tst_QWebEngineDownloadItem::downloadToDirectoryWithFileName()
     // Set up profile and download handler
     ScopedConnection sc2 = connect(m_profile, &QWebEngineProfile::downloadRequested, [&](QWebEngineDownloadItem *item) {
 
-        if (!downloadDirectory.isEmpty()) {
+        if (!downloadDirectory.isEmpty() && setDirectoryFirst) {
             item->setDownloadDirectory(downloadDirectory);
             QCOMPARE(item->downloadDirectory(), downloadDirectory);
         }
@@ -1312,6 +1322,11 @@ void tst_QWebEngineDownloadItem::downloadToDirectoryWithFileName()
             QCOMPARE(item->downloadFileName(), downloadFileName);
         }
 
+        if (!downloadDirectory.isEmpty() && !setDirectoryFirst) {
+            item->setDownloadDirectory(downloadDirectory);
+            QCOMPARE(item->downloadDirectory(), downloadDirectory);
+        }
+
         QCOMPARE(item->path(), QDir(item->downloadDirectory()).filePath(item->downloadFileName()));
         item->accept();