Merge lp:~mandel/ubuntu-download-manager/improve-file-logging into lp:ubuntu-download-manager

[Manuel de la Peña]

Ensure that the error code from a fail file removal is logged.

Ensure that the error code from a fail file removal is logged.

[Diego Sarmentero (diegosarmentero)]

+1

review: Approve

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Mike McCracken (mikemc)]

Thanks for adding the logging, no thanks for throwing in a bunch of unrelated nullptr changes. (And not even in a separate revision!)

Mostly kidding, thanks for doing this so quickly.

review: Approve

[Manuel de la Peña (mandel)]

> Thanks for adding the logging, no thanks for throwing in a bunch of unrelated nullptr changes.
> (And not even in a separate revision!)

Sorry!!! I wanted to remove NULL for nullptr because is more C++

> Mostly kidding, thanks for doing this so quickly.

But nevertheless right.

=== modified file 'ubuntu-download-manager-priv/downloads/file_download.cpp'
--- ubuntu-download-manager-priv/downloads/file_download.cpp	2013-12-20 22:57:15 +0000
+++ ubuntu-download-manager-priv/downloads/file_download.cpp	2014-01-15 11:01:16 +0000
@@ -84,7 +84,7 @@
 }
 
 FileDownload::~FileDownload() {
-    if (_currentData != NULL) {
+    if (_currentData != nullptr) {
         _currentData->close();
     }
     delete _currentData;
@@ -95,13 +95,13 @@
 FileDownload::cancelDownload() {
     TRACE << _url;
 
-    if (_reply != NULL) {
+    if (_reply != nullptr) {
         // disconnect so that we do not get useless signals
         // and remove the reply
         disconnectFromReplySignals();
         _reply->abort();
         _reply->deleteLater();
-        _reply = NULL;
+        _reply = nullptr;
     }
 
     // remove current data and metadata
@@ -114,7 +114,7 @@
 FileDownload::pauseDownload() {
     TRACE << _url;
 
-    if (_reply == NULL) {
+    if (_reply == nullptr) {
         // cannot pause because is not running
         qDebug() << "Cannot pause download because reply is NULL";
         qDebug() << "EMIT paused(false)";
@@ -135,7 +135,7 @@
         emit paused(false);
     } else {
         _reply->deleteLater();
-        _reply = NULL;
+        _reply = nullptr;
         qDebug() << "EMIT paused(true)";
         _downloading = false;
         emit paused(true);
@@ -146,7 +146,7 @@
 FileDownload::resumeDownload() {
     qDebug() << __PRETTY_FUNCTION__ << _url;
 
-    if (_reply != NULL) {
+    if (_reply != nullptr) {
         // cannot resume because it is already running
         qDebug() << "Cannot resume download because reply != NULL";
         qDebug() << "EMIT resumed(false)";
@@ -177,7 +177,7 @@
 FileDownload::startDownload() {
     TRACE << _url;
 
-    if (_reply != NULL) {
+    if (_reply != nullptr) {
         // the download was already started, lets say that we did it
         qDebug() << "Cannot start download because reply != NULL";
         qDebug() << "EMIT started(false)";
@@ -208,7 +208,7 @@
 
 qulonglong
 FileDownload::progress() {
-    return (_currentData == NULL) ? 0 : _currentData->size();
+    return (_currentData == nullptr) ? 0 : _currentData->size();
 }
 
 qulonglong
@@ -220,7 +220,7 @@
 FileDownload::setThrottle(qulonglong speed) {
     TRACE << _url;
     Download::setThrottle(speed);
-    if (_reply != NULL)
+    if (_reply != nullptr)
         _reply->setReadBufferSize(speed);
 }
 
@@ -336,7 +336,7 @@
 
     // clean the reply
     _reply->deleteLater();
-    _reply = NULL;
+    _reply = nullptr;
 }
 
 void
@@ -359,9 +359,7 @@
     TRACE << exitCode << exitStatus;
     if (exitCode == 0 && exitStatus == QProcess::NormalExit) {
         // remove the file since we are done with it
-        bool success = QFile::remove(_filePath);
-        if (!success)
-            qWarning() << "Error removing" << _filePath;
+        cleanUpCurrentData();
         setState(Download::FINISH);
         qDebug() << "EMIT finished" << filePath();
         emit finished(filePath());
@@ -385,7 +383,7 @@
 
     // if no longer online yet we have a reply (that is, we are trying
     // to get data from the missing connection) we pause
-    if (!_connected && _reply != NULL) {
+    if (!_connected && _reply != nullptr) {
         pauseDownload();
         // set it to be downloading even when pause download sets it
         // to false
@@ -415,7 +413,7 @@
 
 void
 FileDownload::connectToReplySignals() {
-    if (_reply != NULL) {
+    if (_reply != nullptr) {
         connect(_reply, &NetworkReply::downloadProgress,
             this, &FileDownload::onDownloadProgress);
         connect(_reply, &NetworkReply::error,
@@ -429,7 +427,7 @@
 
 void
 FileDownload::disconnectFromReplySignals() {
-    if (_reply != NULL) {
+    if (_reply != nullptr) {
         disconnect(_reply, &NetworkReply::downloadProgress,
             this, &FileDownload::onDownloadProgress);
         disconnect(_reply, &NetworkReply::error,
@@ -488,20 +486,26 @@
 
 void
 FileDownload::cleanUpCurrentData() {
-    bool success;
-    QString fileName;
-    if (_currentData) {
-        // delete the current data, we did cancel.
-        fileName = _currentData->fileName();
+    bool success = true; 
+    QFile::FileError error = QFile::NoError;
+    if (_currentData != nullptr) {
         success = _currentData->remove();
+    
+        if (!success)
+            error = _currentData->error();
+    
         _currentData->deleteLater();
-        _currentData = NULL;
+        _currentData = nullptr;
+    } else {
+        QScopedPointer<QFile> tempFile(new QFile(_filePath));
+        success = tempFile->remove();
         if (!success)
-            qWarning() << "Error removing" << fileName;
+            error = tempFile->error();
     }
-    success = QFile::remove(_filePath);
+    
     if (!success)
-        qWarning() << "Error removing" << _filePath;
+        qWarning() << "Error " << error <<
+            "removing file with path" << _filePath;
 }
 
 QNetworkRequest
@@ -527,7 +531,7 @@
     TRACE << error;
     disconnectFromReplySignals();
     _reply->deleteLater();
-    _reply = NULL;
+    _reply = nullptr;
     cleanUpCurrentData();
     Download::emitError(error);
 }