Merge lp:~mandel/ubuntu-download-manager/better-filenames into lp:ubuntu-download-manager

[Manuel de la Peña]

Check if the file is present and use a more human redable filename that does not break extensions.

Check if the file is present and use a more human redable filename that does not break extensions.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:212
http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-ci/276/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-amd64-ci/165
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-armhf-ci/165

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-download-manager-ci/276/rebuild

review: Approve (continuous-integration)

[Diego Sarmentero (diegosarmentero)]

+1

review: Approve

[Alejandro J. Cura (alecu)]

Please move everything after this comment into a separate function called "uniquePath" or something like that:
> // check if the file exists, if it does lets append the uuid to it
Also change that comment since this is not using UUIDs any longer, but numbers instead.

> firstPart = finalPath.left(finalPath.size() - secondPart.size())
Please replace this with fileInfo.baseName, and probably take it out of that if, and remove that else.

review: Needs Fixing

[Manuel de la Peña (mandel)]

> Please move everything after this comment into a separate function called "uniquePath" or something like that:
> // check if the file exists, if it does lets append the uuid to it
> Also change that comment since this is not using UUIDs any longer, but numbers instead.

Sure, it is not hard to do.

>> firstPart = finalPath.left(finalPath.size() - secondPart.size())
> Please replace this with fileInfo.baseName, and probably take it out of that if, and remove that else.

Sorry but baseName does not do what you expect or what is correct. In this case we are dealing with absolute paths and per docuemtnation of Qt http://qt-project.org/doc/qt-4.8/qfileinfo.html#baseName basename does not return the entire full path of the file before the first '.'

Example:

 QFileInfo fi("/tmp/archive.tar.gz");
 QString base = fi.baseName(); // base = "archive"

Nor completeBaseName does it:

 QFileInfo fi("/tmp/archive.tar.gz");
 QString base = fi.completeBaseName(); // base = "archive.tar"

Nor fileName:

 QFileInfo fi("/tmp/archive.tar.gz");
 QString name = fi.fileName(); // name = "archive.tar.gz"

So the best way to deal with it is the one currently present in the MP.

[lp:~mandel/ubuntu-download-manager/better-filenames]

213. By Manuel de la Peña on 2014-01-16

Made changes according to the reviews.

214. By Manuel de la Peña on 2014-01-16

Remove not needed else.

[Alejandro J. Cura (alecu)]

Looks good, +1

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:214
http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-ci/278/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-amd64-ci/167
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-armhf-ci/167

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-download-manager-ci/278/rebuild

review: Approve (continuous-integration)

=== modified file 'ubuntu-download-manager-priv/downloads/file_download.cpp'
--- ubuntu-download-manager-priv/downloads/file_download.cpp	2014-01-15 10:59:14 +0000
+++ ubuntu-download-manager-priv/downloads/file_download.cpp	2014-01-16 15:24:26 +0000
@@ -474,16 +474,44 @@
         }
     } else {
         finalPath = rootPath() + QDir::separator() + basename;
-
-        // check if the file exists, if it does lets append the uuid to it
         if (QFile::exists(finalPath)) {
-            finalPath += downloadId();
+            finalPath = uniqueFilePath(finalPath);
         }
+
     }
 
     return finalPath;
 }
 
+QString
+FileDownload::uniqueFilePath(QString path) {
+    QFileInfo fileInfo(path);
+
+    // Split the file into 2 parts - dot+extension, and everything else. For
+    // example, "path/file.tar.gz" becomes "path/file"+".tar.gz", while
+    // "path/file" (note lack of extension) becomes "path/file"+"".
+    auto secondPart = fileInfo.completeSuffix();
+    auto firstPart = path;
+
+    if (!secondPart.isEmpty()) {
+        secondPart = "." + secondPart;
+        firstPart = path.left(path.size() - secondPart.size());
+    } 
+
+    // Try with an ever-increasing number suffix, until we've reached a file
+    // that does not yet exist.
+    for (int ii = 1; ; ii++) {
+        // Construct the new file name by adding the unique number between the
+        // first and second part.
+        auto finalPath = QString("%1 (%2)%3").arg(firstPart).arg(ii).arg(secondPart);
+        // If no file exists with the new name, return it.
+        if (!QFile::exists(finalPath)) {
+            return finalPath;
+        }
+    }  // for
+    return path;
+}
+
 void
 FileDownload::cleanUpCurrentData() {
     bool success = true; 

=== modified file 'ubuntu-download-manager-priv/downloads/file_download.h'
--- ubuntu-download-manager-priv/downloads/file_download.h	2013-12-20 22:57:15 +0000
+++ ubuntu-download-manager-priv/downloads/file_download.h	2014-01-16 15:24:26 +0000
@@ -108,6 +108,7 @@
                            QProcess::ExitStatus exitStatus);
     void onOnlineStateChanged(bool);
     QString getSaveFileName();
+    QString uniqueFilePath(QString path);
 
  private:
     bool _downloading = false;

=== modified file 'ubuntu-download-manager-tests/downloads/test_download.cpp'
--- ubuntu-download-manager-tests/downloads/test_download.cpp	2013-12-20 22:57:15 +0000
+++ ubuntu-download-manager-tests/downloads/test_download.cpp	2014-01-16 15:24:26 +0000
@@ -1694,6 +1694,53 @@
 }
 
 void
+TestDownload::testDownloadPresentSeveralFiles_data() {
+    QTest::addColumn<int>("count");
+
+    QTest::newRow("One") << 1;
+    QTest::newRow("Some") << 3;
+    QTest::newRow("Several") << 10; 
+    QTest::newRow("Plenti") << 100;
+}
+
+void
+TestDownload::testDownloadPresentSeveralFiles() {
+    QFETCH(int, count);
+
+    QScopedPointer<FileDownload> download(new FileDownload(_id, _path, true,
+        _rootPath, _url, _metadata, _headers));
+
+    QString filePath = download->filePath();
+
+    QScopedPointer<QFile> file(new QFile(filePath));
+    file->open(QIODevice::ReadWrite | QFile::Append);
+    file->write("data data data!");
+    file->close();
+
+    QFileInfo fileInfo(filePath);
+    auto suffix = "." + fileInfo.completeSuffix();
+    auto prefix = filePath.left(filePath.size() - suffix.size());
+
+    // write the rest of the files
+    for(int index=1; index < count; index++) {
+        auto otherPath = QString("%1 (%2)%3").arg(prefix).arg(index).arg(suffix);
+        QScopedPointer<QFile> otherFile(new QFile(otherPath));
+        otherFile->open(QIODevice::ReadWrite | QFile::Append);
+        otherFile->write("data data data!");
+        otherFile->close();
+    }
+
+    QScopedPointer<FileDownload> other(new FileDownload(_id, _path, true,
+        _rootPath, _url, _metadata, _headers));
+
+    QVERIFY(filePath != other->filePath());
+    if (count > 0) {
+        auto expectedPath = QString("%1 (%2)%3").arg(prefix).arg(count).arg(suffix);
+        QCOMPARE(expectedPath, other->filePath());
+    }
+}
+
+void
 TestDownload::testProcessingJustOnce() {
     QCryptographicHash::Algorithm algorithm = HashAlgorithm::getHashAlgo(_algo);
     QByteArray data(300, 't');

=== modified file 'ubuntu-download-manager-tests/downloads/test_download.h'
--- ubuntu-download-manager-tests/downloads/test_download.h	2013-12-20 22:57:15 +0000
+++ ubuntu-download-manager-tests/downloads/test_download.h	2014-01-16 15:24:26 +0000
@@ -137,6 +137,8 @@
 
     // filename tests
     void testDownloadPresent();
+    void testDownloadPresentSeveralFiles_data();
+    void testDownloadPresentSeveralFiles();
 
     // processing signal tests
     void testProcessingJustOnce();