Merge lp:~mandel/ubuntu-download-manager/remove-db-pimpl into lp:ubuntu-download-manager

[Manuel de la Peña]

Remove the pimpl pattern from the DownloadsDb because that class will never be exposed to other projects.

Remove the pimpl pattern from the DownloadsDb because that class will never be exposed to other projects.

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Michał Karnicki (karni)]

256 + qDebug() << "open the db" << _db.open();
You probably should split the line to open the db and do the logging, because if that qDebug() ever gets commented out, the open() call at the end of the line will not be invoked.

        + QJsonDocument json = QJsonDocument::fromVariant(QVariant(metadata));
322 + return QString(json.toJson());
Personally I would name it jsonDoc, just because json.toJson() looks funny ;) Do as you see fit.

341 + _db.open();
You may wanna log that as well, like you do in line 256.

If the _db.open() call fails, is it safe to continue with execution, or should you return from the method?

Otherwise, looking good.

review: Needs Fixing

[lp:~mandel/ubuntu-download-manager/remove-db-pimpl]

197. By Manuel de la Peña on 2013-12-02

Updated according to reviews.

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Michał Karnicki (karni)]

Looks good.

review: Approve

=== modified file 'libubuntudownloadmanager/downloads/downloads_db.cpp'
--- libubuntudownloadmanager/downloads/downloads_db.cpp	2013-10-25 11:44:53 +0000
+++ libubuntudownloadmanager/downloads/downloads_db.cpp	2013-12-02 11:55:39 +0000
@@ -84,234 +84,191 @@
 
 namespace DownloadManager {
 
-class DownloadsDbPrivate {
-    Q_DECLARE_PUBLIC(DownloadsDb)
-
- public:
-    explicit DownloadsDbPrivate(DownloadsDb* parent)
-        : q_ptr(parent) {
-        _fileManager = new FileManager();
-        internalInit();
-    }
-
-    DownloadsDbPrivate(FileManager* fileManager, DownloadsDb* parent)
-        : _fileManager(fileManager),
-          q_ptr(parent) {
-        internalInit();
-    }
-
-    QSqlDatabase db() {
-        return _db;
-    }
-
-    QString filename() {
-        return _dbName;
-    }
-
-    bool dbExists() {
-        return _fileManager->exists(_dbName);
-    }
-
-    void internalInit() {
-
-        QString dataPath = QStandardPaths::writableLocation(
-            QStandardPaths::DataLocation);
-        QString path = dataPath + QDir::separator() + "ubuntu-download-manager";
-
-        bool wasCreated = QDir().mkpath(path);
-        if (!wasCreated) {
-            qCritical() << "Could not create the data path" << path;
-        }
-        _dbName = path + QDir::separator() + "downloads.db";
-        _db = QSqlDatabase::addDatabase("QSQLITE");
-        _db.setDatabaseName(_dbName);
-        qDebug() << "Db file is" << _dbName;
-    }
-
-    bool init() {
-        TRACE;
-        // create the required tables
-        qDebug() << "open the db" << _db.open();
-
-        _db.transaction();
-
-        // create the required tables and indexes
-        bool success = true;
-        QSqlQuery query;
-        success &= query.exec(SINGLE_DOWNLOAD_TABLE);
-        success &= query.exec(GROUP_DOWNLOAD_TABLE);
-        success &= query.exec(GROUP_DOWNLOAD_RELATION);
-
-        if (success)
-            _db.commit();
-        else
-            _db.rollback();
-        _db.close();
-        return success;
-    }
-
-    QString stateToString(Download::State state) {
-        switch (state) {
-            case Download::IDLE:
-                return IDLE_STRING;
-            case Download::START:
-                return START_STRING;
-            case Download::PAUSE:
-                return PAUSE_STRING;
-            case Download::RESUME:
-                return RESUME_STRING;
-            case Download::CANCEL:
-                return CANCEL_STRING;
-            case Download::FINISH:
-                return FINISH_STRING;
-            case Download::ERROR:
-                return ERROR_STRING;
-            default:
-                return IDLE_STRING;
-        }
-    }
-
-    Download::State stringToState(QString state) {
-        QString lowerState = state.toLower();
-
-        if (lowerState == IDLE_STRING)
-            return Download::IDLE;
-
-        if (lowerState == START_STRING)
-            return Download::START;
-
-        if (lowerState == PAUSE_STRING)
-            return Download::PAUSE;
-
-        if (lowerState == RESUME_STRING)
-            return Download::RESUME;
-
-        if (lowerState == CANCEL_STRING)
-            return Download::CANCEL;
-
-        if (lowerState == FINISH_STRING)
-            return Download::FINISH;
-
-        if (lowerState == ERROR_STRING)
-            return Download::ERROR;
-
-        // default case
-        return Download::IDLE;
-    }
-
-    QString metadataToString(const QVariantMap& metadata) {
-        QJsonDocument json = QJsonDocument::fromVariant(QVariant(metadata));
-        return QString(json.toJson());
-    }
-
-    QString headersToString(const QMap<QString, QString>& headers) {
-        QVariantMap headersVariant;
-        foreach(const QString& key, headers.keys()) {
-            headersVariant[key] = headers[key];
-        }
-        QJsonDocument json = QJsonDocument::fromVariant(
-            QVariant(headersVariant));
-        return QString(json.toJson());
-    }
-
-    bool storeSingleDownload(FileDownload* download) {
-        // decide if we store it as a new download or update an existing one
-        _db.open();
-
-        QSqlQuery query;
-        query.prepare(PRESENT_SINGLE_DOWNLOAD);
-        query.bindValue(":uuid", download->downloadId());
-
-        query.exec();
-        int rows = 0;
-        if (query.next())
-            rows = query.value(0).toInt();
-
-        if (rows > 0) {
-            qDebug() << "Update download";
-            query.prepare(UPDATE_SINGLE_DOWNLOAD);
-        } else {
-            qDebug() << "Insert download";
-            query.prepare(INSERT_SINGLE_DOWNLOAD);
-        }
-
-        query.bindValue(":uuid", download->downloadId());
-        query.bindValue(":url", download->url().toString());
-        query.bindValue(":dbus_path", download->path());
-        query.bindValue(":local_path", download->filePath());
-        query.bindValue(":hash", download->hash());
-        query.bindValue(":hash_algo",
-            HashAlgorithm::getHashAlgo(download->hashAlgorithm()));
-        query.bindValue(":state", stateToString(download->state()));
-        query.bindValue(":total_size",
-            QString::number(download->totalSize()));
-        query.bindValue(":throttle",
-            QString::number(download->throttle()));
-        query.bindValue(":metadata",
-            metadataToString(download->metadata()));
-        query.bindValue(":headers",
-            headersToString(download->headers()));
-
-        bool success = query.exec();
-        if (!success)
-            qDebug() << query.lastError();
-
-        _db.close();
-
-        return success;
-    }
-
- private:
-    QString _dbName;
-    FileManager* _fileManager;
-    QSqlDatabase _db;
-    DownloadsDb* q_ptr;
-};
-
-/*
- * PUBLIC IMPLEMENTATION
- */
-
-DownloadsDb::DownloadsDb(QObject *parent)
-    : QObject(parent),
-      d_ptr(new DownloadsDbPrivate(this)) {
+DownloadsDb::DownloadsDb(QObject* parent)
+    : QObject(parent) {
+    _fileManager = new FileManager();
+    internalInit();
 }
 
-DownloadsDb::DownloadsDb(FileManager* fileManager, QObject *parent)
+DownloadsDb::DownloadsDb(FileManager* fileManager, QObject* parent)
     : QObject(parent),
-      d_ptr(new DownloadsDbPrivate(fileManager, this)) {
+      _fileManager(fileManager) {
+    internalInit();
 }
 
 QSqlDatabase
 DownloadsDb::db() {
-    Q_D(DownloadsDb);
-    return d->db();
+    return _db;
 }
 
 QString
 DownloadsDb::filename() {
-    Q_D(DownloadsDb);
-    return d->filename();
+    return _dbName;
 }
 
 bool
 DownloadsDb::dbExists() {
-    Q_D(DownloadsDb);
-    return d->dbExists();
+    return _fileManager->exists(_dbName);
+}
+
+void
+DownloadsDb::internalInit() {
+    QString dataPath = QStandardPaths::writableLocation(
+        QStandardPaths::DataLocation);
+    QString path = dataPath + QDir::separator() + "ubuntu-download-manager";
+
+    bool wasCreated = QDir().mkpath(path);
+    if (!wasCreated) {
+        qCritical() << "Could not create the data path" << path;
+    }
+    _dbName = path + QDir::separator() + "downloads.db";
+    _db = QSqlDatabase::addDatabase("QSQLITE");
+    _db.setDatabaseName(_dbName);
+    qDebug() << "Db file is" << _dbName;
 }
 
 bool
 DownloadsDb::init() {
-    Q_D(DownloadsDb);
-    return d->init();
+    TRACE;
+    // create the required tables
+    bool opened = _db.open();
+    if (!opened) {
+        qCritical() << _db.lastError();
+        return false;
+    }
+
+    _db.transaction();
+
+    // create the required tables and indexes
+    bool success = true;
+    QSqlQuery query;
+    success &= query.exec(SINGLE_DOWNLOAD_TABLE);
+    success &= query.exec(GROUP_DOWNLOAD_TABLE);
+    success &= query.exec(GROUP_DOWNLOAD_RELATION);
+
+    if (success)
+        _db.commit();
+    else
+        _db.rollback();
+    _db.close();
+    return success;
+}
+
+QString
+DownloadsDb::stateToString(Download::State state) {
+    switch (state) {
+        case Download::IDLE:
+            return IDLE_STRING;
+        case Download::START:
+            return START_STRING;
+        case Download::PAUSE:
+            return PAUSE_STRING;
+        case Download::RESUME:
+            return RESUME_STRING;
+        case Download::CANCEL:
+            return CANCEL_STRING;
+        case Download::FINISH:
+            return FINISH_STRING;
+        case Download::ERROR:
+            return ERROR_STRING;
+        default:
+            return IDLE_STRING;
+    }
+}
+
+Download::State
+DownloadsDb::stringToState(QString state) {
+    QString lowerState = state.toLower();
+    if (lowerState == IDLE_STRING)
+        return Download::IDLE;
+    if (lowerState == START_STRING)
+        return Download::START;
+    if (lowerState == PAUSE_STRING)
+        return Download::PAUSE;
+    if (lowerState == RESUME_STRING)
+        return Download::RESUME;
+    if (lowerState == CANCEL_STRING)
+        return Download::CANCEL;
+    if (lowerState == FINISH_STRING)
+        return Download::FINISH;
+    if (lowerState == ERROR_STRING)
+        return Download::ERROR;
+
+    // default case
+    return Download::IDLE;
+}
+
+QString
+DownloadsDb::metadataToString(const QVariantMap& metadata) {
+    QJsonDocument jsonDoc = QJsonDocument::fromVariant(QVariant(metadata));
+    return QString(jsonDoc.toJson());
+}
+
+QString
+DownloadsDb::headersToString(const QMap<QString, QString>& headers) {
+    QVariantMap headersVariant;
+    foreach(const QString& key, headers.keys()) {
+        headersVariant[key] = headers[key];
+    }
+    QJsonDocument jsonDoc = QJsonDocument::fromVariant(
+        QVariant(headersVariant));
+    return QString(jsonDoc.toJson());
 }
 
 bool
 DownloadsDb::storeSingleDownload(FileDownload* download) {
-    Q_D(DownloadsDb);
-    return d->storeSingleDownload(download);
+    // decide if we store it as a new download or update an existing one
+    bool opened = _db.open();
+
+    if (!opened) {
+        qCritical() << _db.lastError();
+        return false;
+    }
+
+    QSqlQuery query;
+    query.prepare(PRESENT_SINGLE_DOWNLOAD);
+    query.bindValue(":uuid", download->downloadId());
+
+    query.exec();
+    int rows = 0;
+    if (query.next())
+        rows = query.value(0).toInt();
+
+    if (rows > 0) {
+        qDebug() << "Update download";
+        query.prepare(UPDATE_SINGLE_DOWNLOAD);
+    } else {
+        qDebug() << "Insert download";
+        query.prepare(INSERT_SINGLE_DOWNLOAD);
+    }
+
+    query.bindValue(":uuid", download->downloadId());
+    query.bindValue(":url", download->url().toString());
+    query.bindValue(":dbus_path", download->path());
+    query.bindValue(":local_path", download->filePath());
+    query.bindValue(":hash", download->hash());
+    query.bindValue(":hash_algo",
+        HashAlgorithm::getHashAlgo(download->hashAlgorithm()));
+    query.bindValue(":state", stateToString(download->state()));
+    query.bindValue(":total_size",
+        QString::number(download->totalSize()));
+    query.bindValue(":throttle",
+        QString::number(download->throttle()));
+    query.bindValue(":metadata",
+        metadataToString(download->metadata()));
+    query.bindValue(":headers",
+        headersToString(download->headers()));
+
+    bool success = query.exec();
+    if (!success)
+        qDebug() << query.lastError();
+
+    _db.close();
+
+    return success;
 }
 
+
 }  // DownloadManager
 
 }  // Ubuntu

=== modified file 'libubuntudownloadmanager/downloads/downloads_db.h'
--- libubuntudownloadmanager/downloads/downloads_db.h	2013-10-25 11:44:53 +0000
+++ libubuntudownloadmanager/downloads/downloads_db.h	2013-12-02 11:55:39 +0000
@@ -28,10 +28,8 @@
 
 namespace DownloadManager {
 
-class DownloadsDbPrivate;
 class DownloadsDb : public QObject {
     Q_OBJECT
-    Q_DECLARE_PRIVATE(DownloadsDb)
 
  public:
     explicit DownloadsDb(QObject *parent = 0);
@@ -44,8 +42,16 @@
     bool storeSingleDownload(FileDownload* download);
 
  private:
-    // use pimpl so that we can mantains ABI compatibility
-    DownloadsDbPrivate* d_ptr;
+    QString headersToString(const QMap<QString, QString>& headers);
+    void internalInit();
+    QString metadataToString(const QVariantMap& metadata);
+    QString stateToString(Download::State state);
+    Download::State stringToState(QString state);
+
+ private:
+    QString _dbName;
+    FileManager* _fileManager;
+    QSqlDatabase _db;
 };
 
 }  // DownloadManager