Merge pull request #1024 from kiwix/download_bugfixes

A couple of bugfixes in Download management

Author: mgautierfr

resources/i18n/en.json

@@ -137,7 +137,7 @@
     "monitor-directory-tooltip":"All ZIM files in this directory will be automatically added to the library.",
     "next-tab":"Move to next tab",
     "previous-tab":"Move to previous tab",
-    "cancel-download": "Cancel Download",
+    "cancel-download": "Cancel download",
     "cancel-download-text": "Are you sure you want to cancel the download of <b>{{ZIM}}</b>?",
     "delete-book": "Delete book",
     "delete-book-text": "Are you sure you want to delete <b>{{ZIM}}</b>?",

src/contentmanager.cpp

@@ -77,10 +77,10 @@ ContentManager::ContentManager(Library* library, kiwix::Downloader* downloader,
     setLanguages();
 }
 
-QList<QMap<QString, QVariant>> ContentManager::getBooksList()
+ContentManager::BookInfoList ContentManager::getBooksList()
 {
     const auto bookIds = getBookIds();
-    QList<QMap<QString, QVariant>> bookList;
+    BookInfoList bookList;
     QStringList keys = {"title", "tags", "date", "id", "size", "description", "faviconUrl"};
     QIcon bookIcon;
     for (auto bookId : bookIds) {
@@ -107,8 +107,8 @@ void ContentManager::onCustomContextMenu(const QPoint &point)
     QAction menuCancelBook(gt("cancel-download"), this);
     QAction menuOpenFolder(gt("open-folder"), this);
 
-    if (bookNode->isDownloading()) {
-        if (bookNode->getDownloadInfo().paused) {
+    if (const auto download = bookNode->getDownloadState()) {
+        if (download->getDownloadInfo().paused) {
             contextMenu.addAction(&menuResumeBook);
         } else {
             contextMenu.addAction(&menuPauseBook);
@@ -216,9 +216,9 @@ void ContentManager::setLanguages()
 }
 
 #define ADD_V(KEY, METH) {if(key==KEY) values.insert(key, QString::fromStdString((b->METH())));}
-QMap<QString, QVariant> ContentManager::getBookInfos(QString id, const QStringList &keys)
+ContentManager::BookInfo ContentManager::getBookInfos(QString id, const QStringList &keys)
 {
-    QMap<QString, QVariant> values;
+    BookInfo values;
     const kiwix::Book* b = [=]()->const kiwix::Book* {
         try {
             return &mp_library->getBookById(id);
@@ -681,13 +681,13 @@ void ContentManager::updateLibrary() {
     } catch (std::runtime_error&) {}
 }
 
-#define CATALOG_URL "library.kiwix.org"
 void ContentManager::updateRemoteLibrary(const QString& content) {
     QtConcurrent::run([=]() {
         QMutexLocker locker(&remoteLibraryLocker);
         mp_remoteLibrary = kiwix::Library::create();
         kiwix::Manager manager(mp_remoteLibrary);
-        manager.readOpds(content.toStdString(), CATALOG_URL);
+        const auto catalogUrl = m_remoteLibraryManager.getCatalogHost();
+        manager.readOpds(content.toStdString(), catalogUrl.toStdString());
         emit(this->booksChanged());
         emit(this->pendingRequest(false));
     });

src/contentmanager.h

@@ -17,9 +17,13 @@ class ContentManager : public QObject
     Q_PROPERTY(QStringList downloadIds READ getDownloadIds NOTIFY downloadsChanged)
     Q_PROPERTY(bool isLocal MEMBER m_local READ isLocal WRITE setLocal NOTIFY localChanged)
 
-public:
+public: // types
     typedef QList<QPair<QString, QString>> LanguageList;
     typedef QList<QPair<QString, QString>> FilterList;
+    typedef ContentManagerModel::BookInfo     BookInfo;
+    typedef ContentManagerModel::BookInfoList BookInfoList;
+
+public: // functions
     explicit ContentManager(Library* library, kiwix::Downloader *downloader, QObject *parent = nullptr);
     virtual ~ContentManager() {}
 
@@ -51,7 +55,7 @@ class ContentManager : public QObject
 
     QStringList getBookIds();
     void eraseBookFilesFromComputer(const QString dirPath, const QString filename, const bool moveToTrash);
-    QList<QMap<QString, QVariant>> getBooksList();
+    BookInfoList getBooksList();
     ContentManagerModel *managerModel;
     QMutex remoteLibraryLocker;
     void setCategories();
@@ -71,7 +75,7 @@ class ContentManager : public QObject
 
 public slots:
     QStringList getTranslations(const QStringList &keys);
-    QMap<QString, QVariant> getBookInfos(QString id, const QStringList &keys);
+    BookInfo getBookInfos(QString id, const QStringList &keys);
     void openBook(const QString& id);
     QMap<QString, QVariant> updateDownloadInfos(QString id, const QStringList& keys);
     QString downloadBook(const QString& id);

src/contentmanagerdelegate.cpp

@@ -177,8 +177,8 @@ void ContentManagerDelegate::paint(QPainter *painter, const QStyleOptionViewItem
     }
     QStyleOptionViewItem eOpt = option;
     if (index.column() == 5) {
-        if (node->isDownloading()) {
-            auto downloadInfo = node->getDownloadInfo();
+        if (const auto downloadState = node->getDownloadState()) {
+            auto downloadInfo = downloadState->getDownloadInfo();
             showDownloadProgress(painter, r, downloadInfo);
         }
         else {
@@ -244,8 +244,8 @@ void ContentManagerDelegate::handleLastColumnClicked(const QModelIndex& index, Q
     int x = r.left();
     int w = r.width();
 
-    if (node->isDownloading()) {
-        if (node->getDownloadInfo().paused) {
+    if (const auto downloadState = node->getDownloadState()) {
+        if (downloadState->getDownloadInfo().paused) {
             if (clickX < (x + w/2)) {
                 KiwixApp::instance()->getContentManager()->cancelBook(id, index);
             } else {

src/contentmanagermodel.cpp

@@ -5,6 +5,7 @@
 #include <zim/error.h>
 #include <zim/item.h>
 #include "kiwixapp.h"
+#include <kiwix/tools.h>
 
 ContentManagerModel::ContentManagerModel(QObject *parent)
     : QAbstractItemModel(parent)
@@ -101,33 +102,60 @@ QVariant ContentManagerModel::headerData(int section, Qt::Orientation orientatio
     }
 }
 
-void ContentManagerModel::setBooksData(const QList<QMap<QString, QVariant>>& data)
+void ContentManagerModel::setBooksData(const BookInfoList& data)
 {
     m_data = data;
     rootNode = std::shared_ptr<RowNode>(new RowNode({tr("Icon"), tr("Name"), tr("Date"), tr("Size"), tr("Content Type"), tr("Download")}, "", std::weak_ptr<RowNode>()));
     setupNodes();
     emit dataChanged(QModelIndex(), QModelIndex());
 }
 
-QString convertToUnits(QString size)
+std::shared_ptr<RowNode> ContentManagerModel::createNode(BookInfo bookItem, QMap<QString, QByteArray> iconMap) const
 {
-    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++;
+    auto faviconUrl = "https://" + bookItem["faviconUrl"].toString();
+    QString id = bookItem["id"].toString();
+    QByteArray bookIcon;
+    try {
+        auto book = KiwixApp::instance()->getLibrary()->getBookById(id);
+        std::string favicon;
+        auto item = book.getIllustration(48);
+        favicon = item->getData();
+        bookIcon = QByteArray::fromRawData(reinterpret_cast<const char*>(favicon.data()), favicon.size());
+        bookIcon.detach(); // deep copy
+    } catch (...) {
+        if (iconMap.contains(faviconUrl)) {
+            bookIcon = iconMap[faviconUrl];
+        }
     }
-
-    const auto preciseBytes = QString::number(bytes, 'g', 3);
-    return preciseBytes + " " + units[unitIndex];
+    std::weak_ptr<RowNode> weakRoot = rootNode;
+    auto rowNodePtr = std::shared_ptr<RowNode>(new
+                                    RowNode({bookIcon, bookItem["title"],
+                                   bookItem["date"],
+                                   QString::fromStdString(kiwix::beautifyFileSize(bookItem["size"].toULongLong())),
+                                   bookItem["tags"]
+                                   }, id, weakRoot));
+    std::weak_ptr<RowNode> weakRowNodePtr = rowNodePtr;
+    const auto descNodePtr = std::make_shared<DescriptionNode>(DescriptionNode(bookItem["description"].toString(), weakRowNodePtr));
+
+    rowNodePtr->appendChild(descNodePtr);
+    return rowNodePtr;
 }
 
 void ContentManagerModel::setupNodes()
 {
     beginResetModel();
+    bookIdToRowMap.clear();
     for (auto bookItem : m_data) {
-        rootNode->appendChild(RowNode::createNode(bookItem, iconMap, rootNode));
+        const auto rowNode = createNode(bookItem, iconMap);
+
+        // Restore download state during model updates (filtering, etc)
+        const auto downloadIter = m_downloads.constFind(rowNode->getBookId());
+        if ( downloadIter != m_downloads.constEnd() ) {
+            rowNode->setDownloadState(downloadIter.value());
+        }
+
+        bookIdToRowMap[bookItem["id"].toString()] = rootNode->childCount();
+        rootNode->appendChild(rowNode);
     }
     endResetModel();
 }
@@ -222,58 +250,68 @@ std::shared_ptr<RowNode> getSharedPointer(RowNode* ptr)
 void ContentManagerModel::startDownload(QModelIndex index)
 {
     auto node = getSharedPointer(static_cast<RowNode*>(index.internalPointer()));
-    node->setIsDownloading(true);
-    auto id = node->getBookId();
-    QTimer *timer = new QTimer(this);
+    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, [=]() {
-        auto downloadInfos = KiwixApp::instance()->getContentManager()->updateDownloadInfos(id, {"status", "completedLength", "totalLength", "downloadSpeed"});
-        double percent = (double) downloadInfos["completedLength"].toInt() / downloadInfos["totalLength"].toInt();
-        percent *= 100;
-        percent = QString::number(percent, 'g', 3).toDouble();
-        auto completedLength = convertToUnits(downloadInfos["completedLength"].toString());
-        auto downloadSpeed = convertToUnits(downloadInfos["downloadSpeed"].toString()) + "/s";
-        node->setDownloadInfo({percent, completedLength, downloadSpeed});
-        if (!downloadInfos["status"].isValid()) {
-            node->setIsDownloading(false);
-            timer->stop();
-            timer->deleteLater();
-        }
-        emit dataChanged(index, index);
+        updateDownload(bookId);
     });
-    timer->start(1000);
-    timers[id] = timer;
+}
+
+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, rootNodeIndex);
+        emit dataChanged(newIndex, newIndex);
+    }
 }
 
 void ContentManagerModel::pauseDownload(QModelIndex index)
 {
     auto node = static_cast<RowNode*>(index.internalPointer());
-    auto id = node->getBookId();
-    auto prevDownloadInfo = node->getDownloadInfo();
-    prevDownloadInfo.paused = true;
-    node->setDownloadInfo(prevDownloadInfo);
-    timers[id]->stop();
+    node->getDownloadState()->pause();
     emit dataChanged(index, index);
 }
 
 void ContentManagerModel::resumeDownload(QModelIndex index)
 {
     auto node = static_cast<RowNode*>(index.internalPointer());
-    auto id = node->getBookId();
-    auto prevDownloadInfo = node->getDownloadInfo();
-    prevDownloadInfo.paused = false;
-    node->setDownloadInfo(prevDownloadInfo);
-    timers[id]->start(1000);
+    node->getDownloadState()->resume();
     emit dataChanged(index, index);
 }
 
 void ContentManagerModel::cancelDownload(QModelIndex index)
 {
     auto node = static_cast<RowNode*>(index.internalPointer());
-    auto id = node->getBookId();
-    node->setIsDownloading(false);
-    node->setDownloadInfo({0, "", "", false});
-    timers[id]->stop();
-    timers[id]->deleteLater();
+    node->setDownloadState(nullptr);
+    m_downloads.remove(node->getBookId());
     emit dataChanged(index, index);
 }
 

src/contentmanagermodel.h

@@ -6,6 +6,7 @@
 #include <QVariant>
 #include <QIcon>
 #include "thumbnaildownloader.h"
+#include "rownode.h"
 #include <memory>
 
 class RowNode;
@@ -16,7 +17,11 @@ class ContentManagerModel : public QAbstractItemModel
 {
     Q_OBJECT
 
-public:
+public: // types
+    typedef QMap<QString, QVariant> BookInfo;
+    typedef QList<BookInfo>         BookInfoList;
+
+public: // functions
     explicit ContentManagerModel(QObject *parent = nullptr);
     ~ContentManagerModel();
 
@@ -29,30 +34,36 @@ class ContentManagerModel : public QAbstractItemModel
     QModelIndex parent(const QModelIndex &index) const override;
     int rowCount(const QModelIndex &parent = QModelIndex()) const override;
     int columnCount(const QModelIndex &parent = QModelIndex()) const override;
-    void setBooksData(const QList<QMap<QString, QVariant>>& data);
+    void setBooksData(const BookInfoList& data);
     void setupNodes();
     bool hasChildren(const QModelIndex &parent) const override;
     void sort(int column, Qt::SortOrder order = Qt::AscendingOrder) override;
     void refreshIcons();
 
+    std::shared_ptr<RowNode> createNode(BookInfo bookItem, QMap<QString, QByteArray> iconMap) const;
+
 public slots:
     void updateImage(QModelIndex index, QString url, QByteArray imageData);
     void startDownload(QModelIndex index);
     void pauseDownload(QModelIndex index);
     void resumeDownload(QModelIndex index);
     void cancelDownload(QModelIndex index);
 
-protected:
+protected: // functions
     bool canFetchMore(const QModelIndex &parent) const override;
     void fetchMore(const QModelIndex &parent) override;
 
-private:
-    QList<QMap<QString, QVariant>> m_data;
+private: // functions
+    void updateDownload(QString bookId);
+
+private: // data
+    BookInfoList m_data;
     std::shared_ptr<RowNode> rootNode;
     int zimCount = 0;
     ThumbnailDownloader td;
+    QMap<QString, size_t> bookIdToRowMap;
     QMap<QString, QByteArray> iconMap;
-    QMap<QString, QTimer*> timers;
+    QMap<QString, std::shared_ptr<DownloadState>> m_downloads;
 };
 
 #endif // CONTENTMANAGERMODEL_H