Merge pull request #1045 from kiwix/refactoring

src/contentmanager.cpp

@@ -126,6 +126,10 @@ ContentManager::ContentManager(Library* library, kiwix::Downloader* downloader,
     connect(&m_remoteLibraryManager, &OpdsRequestManager::categoriesReceived, this, &ContentManager::updateCategories);
     setCategories();
     setLanguages();
+
+    m_downloadUpdateTimer.start(1000);
+    connect(&m_downloadUpdateTimer, &QTimer::timeout,
+            this, &ContentManager::updateDownloads);
 }
 
 ContentManager::BookInfoList ContentManager::getBooksList()
@@ -159,7 +163,7 @@ void ContentManager::onCustomContextMenu(const QPoint &point)
     QAction menuOpenFolder(gt("open-folder"), this);
 
     if (const auto download = bookNode->getDownloadState()) {
-        if (download->getDownloadInfo().paused) {
+        if (download->paused) {
             contextMenu.addAction(&menuResumeBook);
         } else {
             contextMenu.addAction(&menuPauseBook);
@@ -406,8 +410,15 @@ void ContentManager::downloadStarted(const kiwix::Book& book, const std::string&
     emit(oneBookChanged(QString::fromStdString(book.getId())));
 }
 
+void ContentManager::removeDownload(QString bookId)
+{
+    m_downloads.remove(bookId);
+    managerModel->removeDownload(bookId);
+}
+
 void ContentManager::downloadCancelled(QString bookId)
 {
+    removeDownload(bookId);
     kiwix::Book bCopy(mp_library->getBookById(bookId));
     bCopy.setDownloadId("");
     mp_library->getKiwixLibrary()->addOrUpdateBook(bCopy);
@@ -417,6 +428,7 @@ void ContentManager::downloadCancelled(QString bookId)
 
 void ContentManager::downloadCompleted(QString bookId, QString path)
 {
+    removeDownload(bookId);
     kiwix::Book bCopy(mp_library->getBookById(bookId));
     bCopy.setPath(QDir::toNativeSeparators(path).toStdString());
     bCopy.setDownloadId("");
@@ -433,7 +445,7 @@ void ContentManager::downloadCompleted(QString bookId, QString path)
     }
 }
 
-ContentManager::DownloadInfo ContentManager::getDownloadInfo(QString bookId, const QStringList &keys) const
+DownloadInfo ContentManager::getDownloadInfo(QString bookId, const QStringList &keys) const
 {
     DownloadInfo values;
     if (!mp_downloader) {
@@ -460,28 +472,49 @@ ContentManager::DownloadInfo ContentManager::getDownloadInfo(QString bookId, con
     return values;
 }
 
-ContentManager::DownloadInfo ContentManager::updateDownloadInfos(QString bookId, QStringList keys)
+void ContentManager::updateDownload(QString bookId)
 {
-    if ( !keys.contains("status") ) keys.append("status");
+    const auto downloadState = m_downloads.value(bookId);
-    if ( !keys.contains("path")   ) keys.append("path");
+    if ( downloadState && !downloadState->paused ) {
+        const auto downloadInfo = getDownloadInfo(bookId, {"status", "completedLength", "totalLength", "downloadSpeed", "path"});
 
-    const DownloadInfo result = getDownloadInfo(bookId, keys);
+        if ( downloadInfo.isEmpty() ) {
-
+            downloadCancelled(bookId);
-    if ( result.isEmpty() ) {
+        } else if ( downloadInfo["status"] == "completed" ) {
-        downloadCancelled(bookId);
+            downloadCompleted(bookId, downloadInfo["path"].toString());
-    } else if ( result["status"] == "completed" ) {
+        } else {
-        downloadCompleted(bookId, result["path"].toString());
+            downloadState->update(downloadInfo);
+            managerModel->updateDownload(bookId);
+        }
     }
-
-    return result;
 }
 
+void ContentManager::updateDownloads()
+{
+    for ( const auto& bookId : m_downloads.keys() ) {
+        updateDownload(bookId);
+    }
+}
+
+namespace
+{
+
+std::shared_ptr<RowNode> getSharedPointer(RowNode* ptr)
+{
+    return std::static_pointer_cast<RowNode>(ptr->shared_from_this());
+}
+
+} // unnamed namespace
+
 void ContentManager::downloadBook(const QString &id, QModelIndex index)
 {
     try
     {
         downloadBook(id);
-        emit managerModel->startDownload(index);
+        auto node = getSharedPointer(static_cast<RowNode*>(index.internalPointer()));
+        const auto newDownload = std::make_shared<DownloadState>();
+        m_downloads[id] = newDownload;
+        node->setDownloadState(newDownload);
     }
     catch ( const ContentManagerError& err )
     {
@@ -499,30 +532,33 @@ const kiwix::Book& ContentManager::getRemoteOrLocalBook(const QString &id)
     }
 }
 
+std::string ContentManager::startDownload(const kiwix::Book& book)
+{
+    auto downloadPath = KiwixApp::instance()->getSettingsManager()->getDownloadDir();
+    checkEnoughStorageAvailable(book, downloadPath);
+
+    typedef std::vector<std::pair<std::string, std::string>> DownloadOptions;
+
+    const DownloadOptions downloadOptions{{"dir", downloadPath.toStdString()}};
+
+    const auto d = mp_downloader->startDownload(book.getUrl(), downloadOptions);
+    return d->getDid();
+}
+
 void ContentManager::downloadBook(const QString &id)
 {
     if (!mp_downloader)
         throwDownloadUnavailableError();
 
     const auto& book = getRemoteOrLocalBook(id);
-    auto downloadPath = KiwixApp::instance()->getSettingsManager()->getDownloadDir();
-    checkEnoughStorageAvailable(book, downloadPath);
 
-    auto booksList = mp_library->getBookIds();
+    std::string downloadId;
-    for (auto b : booksList) {
-        if (b.toStdString() == book.getId())
-            throwDownloadUnavailableError(); // but why???
-    }
-
-    std::shared_ptr<kiwix::Download> download;
     try {
-        std::pair<std::string, std::string> downloadDir("dir", downloadPath.toStdString());
+        downloadId = startDownload(book);
-        const std::vector<std::pair<std::string, std::string>> options = { downloadDir };
-        download = mp_downloader->startDownload(book.getUrl(), options);
     } catch (std::exception& e) {
         throwDownloadUnavailableError();
     }
-    downloadStarted(book, download->getDid());
+    downloadStarted(book, downloadId);
 }
 
 void ContentManager::eraseBookFilesFromComputer(const QString dirPath, const QString fileName, const bool moveToTrash)
@@ -632,7 +668,7 @@ void ContentManager::cancelBook(const QString& id, QModelIndex index)
     text = text.replace("{{ZIM}}", QString::fromStdString(mp_library->getBookById(id).getTitle()));
     showConfirmBox(gt("cancel-download"), text, mp_view, [=]() {
         cancelBook(id);
-        emit managerModel->cancelDownload(index);
+        emit managerModel->removeDownload(id);
     });
 }
 
@@ -646,6 +682,8 @@ void ContentManager::cancelBook(const QString& id)
     if (download->getStatus() != kiwix::Download::K_COMPLETE) {
         download->cancelDownload();
     }
+    m_downloads.remove(id);
+
     QString dirPath = QString::fromStdString(kiwix::removeLastPathElement(download->getPath()));
     QString filename = QString::fromStdString(kiwix::getLastPathElement(download->getPath())) + "*";
     // incompleted downloaded file should be perma deleted

src/contentmanager.h

@@ -20,9 +20,6 @@ public: // types
     typedef ContentManagerModel::BookInfo     BookInfo;
     typedef ContentManagerModel::BookInfoList BookInfoList;
 
-    // XXX: potentional source of confusion with ::DownloadInfo from rownode.h
-    typedef QMap<QString, QVariant> DownloadInfo;
-
 public: // functions
     explicit ContentManager(Library* library, kiwix::Downloader *downloader, QObject *parent = nullptr);
     virtual ~ContentManager() {}
@@ -52,7 +49,6 @@ public slots:
     QStringList getTranslations(const QStringList &keys);
     BookInfo getBookInfos(QString id, const QStringList &keys);
     void openBook(const QString& id);
-    DownloadInfo updateDownloadInfos(QString bookId, QStringList keys);
     void downloadBook(const QString& id);
     void downloadBook(const QString& id, QModelIndex index);
     void updateLibrary();
@@ -71,6 +67,7 @@ public slots:
     void cancelBook(const QString& id, QModelIndex index);
     void onCustomContextMenu(const QPoint &point);
     void openBookWithIndex(const QModelIndex& index);
+    void updateDownloads();
 
 private: // functions
     QStringList getBookIds();
@@ -85,6 +82,9 @@ private: // functions
     // the remote or local library (in that order).
     const kiwix::Book& getRemoteOrLocalBook(const QString &id);
 
+    std::string startDownload(const kiwix::Book& book);
+    void updateDownload(QString bookId);
+    void removeDownload(QString bookId);
     void downloadStarted(const kiwix::Book& book, const std::string& downloadId);
     void downloadCancelled(QString bookId);
     void downloadCompleted(QString bookId, QString path);
@@ -95,6 +95,7 @@ private: // data
     kiwix::LibraryPtr mp_remoteLibrary;
     kiwix::Downloader* mp_downloader;
     ContentManagerModel::Downloads m_downloads;
+    QTimer m_downloadUpdateTimer;
     OpdsRequestManager m_remoteLibraryManager;
     ContentManagerView* mp_view;
     bool m_local = true;

src/contentmanagerdelegate.cpp

@@ -104,7 +104,7 @@ void createDownloadStats(QPainter *painter, QRect box, QString downloadSpeed, QS
     painter->setFont(oldFont);
 }
 
-void showDownloadProgress(QPainter *painter, QRect box, DownloadInfo downloadInfo)
+void showDownloadProgress(QPainter *painter, QRect box, const DownloadState& downloadInfo)
 {
     int x,y,w,h;
     x = box.left();
@@ -179,8 +179,7 @@ void ContentManagerDelegate::paint(QPainter *painter, const QStyleOptionViewItem
     QStyleOptionViewItem eOpt = option;
     if (index.column() == 5) {
         if (const auto downloadState = node->getDownloadState()) {
-            auto downloadInfo = downloadState->getDownloadInfo();
+            showDownloadProgress(painter, r, *downloadState);
-            showDownloadProgress(painter, r, downloadInfo);
         }
         else {
             baseButton->style()->drawControl( QStyle::CE_PushButton, &button, painter, baseButton.data());
@@ -250,7 +249,7 @@ void ContentManagerDelegate::handleLastColumnClicked(const QModelIndex& index, Q
     int w = r.width();
 
     if (const auto downloadState = node->getDownloadState()) {
-        if (downloadState->getDownloadInfo().paused) {
+        if (downloadState->paused) {
             if (clickX < (x + w/2)) {
                 KiwixApp::instance()->getContentManager()->cancelBook(id, index);
             } else {

src/contentmanagermodel.cpp

@@ -7,7 +7,7 @@
 #include "kiwixapp.h"
 #include <kiwix/tools.h>
 
-ContentManagerModel::ContentManagerModel(Downloads* downloads, QObject *parent)
+ContentManagerModel::ContentManagerModel(const Downloads* downloads, QObject *parent)
     : QAbstractItemModel(parent)
     , m_downloads(*downloads)
 {
@@ -245,57 +245,18 @@ void ContentManagerModel::updateImage(QString bookId, QString url, QByteArray im
     emit dataChanged(index, index);
 }
 
-std::shared_ptr<RowNode> getSharedPointer(RowNode* ptr)
-{
-    return std::static_pointer_cast<RowNode>(ptr->shared_from_this());
-}
-
-void ContentManagerModel::startDownload(QModelIndex index)
-{
-    auto node = getSharedPointer(static_cast<RowNode*>(index.internalPointer()));
-    const auto bookId = node->getBookId();
-    const auto newDownload = std::make_shared<DownloadState>();
-    m_downloads[bookId] = newDownload;
-    node->setDownloadState(newDownload);
-    QTimer *timer = newDownload->getDownloadUpdateTimer();
-    connect(timer, &QTimer::timeout, this, [=]() {
-        updateDownload(bookId);
-    });
-}
-
 void ContentManagerModel::updateDownload(QString bookId)
 {
-    const auto download = m_downloads.value(bookId);
-
-    if ( ! download )
-        return;
-
-    const bool downloadStillValid = download->update(bookId);
-
-    // The download->update() call above may result in
-    // ContentManagerModel::setBooksData() being called (through a chain
-    // of signals), which in turn will rebuild bookIdToRowMap. Hence
-    // bookIdToRowMap access must happen after it.
-
     const auto it = bookIdToRowMap.constFind(bookId);
 
-    if ( ! downloadStillValid ) {
-        m_downloads.remove(bookId);
-        if ( it != bookIdToRowMap.constEnd() ) {
-            const size_t row = it.value();
-            RowNode& rowNode = static_cast<RowNode&>(*rootNode->child(row));
-            rowNode.setDownloadState(nullptr);
-        }
-    }
-
     if ( it != bookIdToRowMap.constEnd() ) {
         const size_t row = it.value();
-        const QModelIndex rootNodeIndex = this->index(0, 0);
+        const QModelIndex newIndex = this->index(row, 5);
-        const QModelIndex newIndex = this->index(row, 5, rootNodeIndex);
         emit dataChanged(newIndex, newIndex);
     }
 }
 
+
 void ContentManagerModel::pauseDownload(QModelIndex index)
 {
     emit dataChanged(index, index);
@@ -306,11 +267,16 @@ void ContentManagerModel::resumeDownload(QModelIndex index)
     emit dataChanged(index, index);
 }
 
-void ContentManagerModel::cancelDownload(QModelIndex index)
+void ContentManagerModel::removeDownload(QString bookId)
 {
-    auto node = static_cast<RowNode*>(index.internalPointer());
+    const auto it = bookIdToRowMap.constFind(bookId);
-    node->setDownloadState(nullptr);
+    if ( it == bookIdToRowMap.constEnd() )
-    m_downloads.remove(node->getBookId());
+        return;
+
+    const size_t row = it.value();
+    auto& node = static_cast<RowNode&>(*rootNode->child(row));
+    node.setDownloadState(nullptr);
+    const QModelIndex index = this->index(row, 5);
     emit dataChanged(index, index);
 }
 

src/contentmanagermodel.h

@@ -25,7 +25,7 @@ public: // types
     typedef QMap<QString, std::shared_ptr<DownloadState>> Downloads;
 
 public: // functions
-    ContentManagerModel(Downloads* downloads, QObject *parent = nullptr);
+    ContentManagerModel(const Downloads* downloads, QObject *parent = nullptr);
     ~ContentManagerModel();
 
     QVariant data(const QModelIndex &index, int role) const override;
@@ -47,18 +47,15 @@ public: // functions
 
 public slots:
     void updateImage(QString bookId, QString url, QByteArray imageData);
-    void startDownload(QModelIndex index);
     void pauseDownload(QModelIndex index);
     void resumeDownload(QModelIndex index);
-    void cancelDownload(QModelIndex index);
+    void removeDownload(QString bookId);
+    void updateDownload(QString bookId);
 
 protected: // functions
     bool canFetchMore(const QModelIndex &parent) const override;
     void fetchMore(const QModelIndex &parent) override;
 
-private: // functions
-    void updateDownload(QString bookId);
-
 private: // data
     BookInfoList m_data;
     std::shared_ptr<RowNode> rootNode;
@@ -66,7 +63,7 @@ private: // data
     ThumbnailDownloader td;
     QMap<QString, size_t> bookIdToRowMap;
     QMap<QString, QByteArray> iconMap;
-    Downloads& m_downloads;
+    const Downloads& m_downloads;
 };
 
 #endif // CONTENTMANAGERMODEL_H

src/rownode.cpp

@@ -7,21 +7,13 @@
 // DowloadState
 ////////////////////////////////////////////////////////////////////////////////
 
-DownloadState::DownloadState()
-    : m_downloadInfo({0, "", "", false})
-{
-    m_downloadUpdateTimer.reset(new QTimer);
-    m_downloadUpdateTimer->start(1000);
-}
-
 namespace
 {
 
-QString convertToUnits(QString size)
+QString convertToUnits(double bytes)
 {
     QStringList units = {"bytes", "KB", "MB", "GB", "TB", "PB", "EB"};
     int unitIndex = 0;
-    auto bytes = size.toDouble();
     while (bytes >= 1024 && unitIndex < units.size()) {
         bytes /= 1024;
         unitIndex++;
@@ -33,40 +25,24 @@ QString convertToUnits(QString size)
 
 } // unnamed namespace
 
-bool DownloadState::update(QString id)
+void DownloadState::update(const DownloadInfo& downloadInfos)
 {
-    auto downloadInfos = KiwixApp::instance()->getContentManager()->updateDownloadInfos(id, {"status", "completedLength", "totalLength", "downloadSpeed"});
-    if (!downloadInfos["status"].isValid()) {
-        m_downloadUpdateTimer->stop();
-
-        // Deleting the timer object immediately instead of via
-        // QObject::deleteLater() seems to be safe since it is not a recipient
-        // of any events that may be in the process of being delivered to it
-        // from another thread.
-        m_downloadUpdateTimer.reset();
-        m_downloadInfo = {0, "", "", false};
-        return false;
-    }
-
     double percent = downloadInfos["completedLength"].toDouble() / downloadInfos["totalLength"].toDouble();
     percent *= 100;
     percent = QString::number(percent, 'g', 3).toDouble();
-    auto completedLength = convertToUnits(downloadInfos["completedLength"].toString());
+    auto completedLength = convertToUnits(downloadInfos["completedLength"].toDouble());
-    auto downloadSpeed = convertToUnits(downloadInfos["downloadSpeed"].toString()) + "/s";
+    auto downloadSpeed = convertToUnits(downloadInfos["downloadSpeed"].toDouble()) + "/s";
-    m_downloadInfo = {percent, completedLength, downloadSpeed, false};
+    *this = {percent, completedLength, downloadSpeed, false};
-    return true;
 }
 
 void DownloadState::pause()
 {
-    m_downloadInfo.paused = true;
+    this->paused = true;
-    m_downloadUpdateTimer->stop();
 }
 
 void DownloadState::resume()
 {
-    m_downloadInfo.paused = false;
+    this->paused = false;
-    m_downloadUpdateTimer->start(1000);
 }
 
 

src/rownode.h

@@ -6,31 +6,20 @@
 #include <QIcon>
 #include "kiwix/book.h"
 
-struct DownloadInfo
+typedef QMap<QString, QVariant> DownloadInfo;
-{
-    double progress;
-    QString completedLength;
-    QString downloadSpeed;
-    bool paused;
-};
 
 class DownloadState
 {
 public:
-    DownloadState();
+    double progress = 0;
+    QString completedLength;
+    QString downloadSpeed;
+    bool paused = false;
 
-    bool isDownloading() const { return m_downloadUpdateTimer.get() != nullptr; }
+public:
-    DownloadInfo getDownloadInfo() const { return m_downloadInfo; }
-    QTimer* getDownloadUpdateTimer() const { return m_downloadUpdateTimer.get(); }
     void pause();
     void resume();
-    bool update(QString id);
+    void update(const DownloadInfo& info);
-
-protected:
-    // This is non-NULL only for a pending (even if paused) download
-    std::unique_ptr<QTimer> m_downloadUpdateTimer;
-
-    DownloadInfo m_downloadInfo;
 };
 
 class RowNode : public Node