From 4b773f4c1d130d1acb2f177edaff8c6deb2af26c Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Thu, 22 Feb 2024 17:18:31 +0400 Subject: [PATCH 01/19] Deleted a dubious piece of code In theory, a book recorded in a local library may have only its url defined (and no path to a ZIM file). Such a book qualifies as downloadable too. That's why the call to `ContentManager::getRemoteOrLocalBook()` (which may return a "local" book) perfectly makes sense in `ContentManager::downloadBook()`. However, it doesn't then make sense to raise an error if the returned book turns out to be a "local" one. It would be more logical to request only for a remote book and report an error if that operation fails. --- src/contentmanager.cpp | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/contentmanager.cpp b/src/contentmanager.cpp index d5fb8f757..8c11b8a90 100644 --- a/src/contentmanager.cpp +++ b/src/contentmanager.cpp @@ -505,15 +505,10 @@ void ContentManager::downloadBook(const QString &id) throwDownloadUnavailableError(); const auto& book = getRemoteOrLocalBook(id); + auto downloadPath = KiwixApp::instance()->getSettingsManager()->getDownloadDir(); checkEnoughStorageAvailable(book, downloadPath); - auto booksList = mp_library->getBookIds(); - for (auto b : booksList) { - if (b.toStdString() == book.getId()) - throwDownloadUnavailableError(); // but why??? - } - std::shared_ptr download; try { std::pair downloadDir("dir", downloadPath.toStdString()); From 6ba6ad0ee8c384a938b74893a4b4b236482f0d3f Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Fri, 9 Feb 2024 18:01:07 +0400 Subject: [PATCH 02/19] ContentManager::startDownload() --- src/contentmanager.cpp | 24 ++++++++++++++++-------- src/contentmanager.h | 1 + 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/src/contentmanager.cpp b/src/contentmanager.cpp index 8c11b8a90..a895aac9a 100644 --- a/src/contentmanager.cpp +++ b/src/contentmanager.cpp @@ -499,6 +499,19 @@ 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> 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) @@ -506,18 +519,13 @@ void ContentManager::downloadBook(const QString &id) const auto& book = getRemoteOrLocalBook(id); - auto downloadPath = KiwixApp::instance()->getSettingsManager()->getDownloadDir(); - checkEnoughStorageAvailable(book, downloadPath); - - std::shared_ptr download; + std::string downloadId; try { - std::pair downloadDir("dir", downloadPath.toStdString()); - const std::vector> options = { downloadDir }; - download = mp_downloader->startDownload(book.getUrl(), options); + downloadId = startDownload(book); } catch (std::exception& e) { throwDownloadUnavailableError(); } - downloadStarted(book, download->getDid()); + downloadStarted(book, downloadId); } void ContentManager::eraseBookFilesFromComputer(const QString dirPath, const QString fileName, const bool moveToTrash) diff --git a/src/contentmanager.h b/src/contentmanager.h index 4f581c32d..b19a80ae1 100644 --- a/src/contentmanager.h +++ b/src/contentmanager.h @@ -85,6 +85,7 @@ public slots: // the remote or local library (in that order). const kiwix::Book& getRemoteOrLocalBook(const QString &id); + std::string startDownload(const kiwix::Book& book); void downloadStarted(const kiwix::Book& book, const std::string& downloadId); void downloadCancelled(QString bookId); void downloadCompleted(QString bookId, QString path); From b53042edd5aebf52efa6de91efa52a1da50316e1 Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Fri, 9 Feb 2024 18:54:14 +0400 Subject: [PATCH 03/19] Got rid of ContentManagerModel::startDownload() Inlined ContentManagerModel::startDownload() into ContentManager::downloadBook(). --- src/contentmanager.cpp | 19 ++++++++++++++++++- src/contentmanagermodel.cpp | 18 ------------------ src/contentmanagermodel.h | 5 +---- 3 files changed, 19 insertions(+), 23 deletions(-) diff --git a/src/contentmanager.cpp b/src/contentmanager.cpp index a895aac9a..29c621a9d 100644 --- a/src/contentmanager.cpp +++ b/src/contentmanager.cpp @@ -476,12 +476,29 @@ ContentManager::DownloadInfo ContentManager::updateDownloadInfos(QString bookId, return result; } +namespace +{ + +std::shared_ptr getSharedPointer(RowNode* ptr) +{ + return std::static_pointer_cast(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(index.internalPointer())); + const auto newDownload = std::make_shared(); + m_downloads[id] = newDownload; + node->setDownloadState(newDownload); + QTimer *timer = newDownload->getDownloadUpdateTimer(); + connect(timer, &QTimer::timeout, [=]() { + managerModel->updateDownload(id); + }); } catch ( const ContentManagerError& err ) { diff --git a/src/contentmanagermodel.cpp b/src/contentmanagermodel.cpp index 15f655d34..846f91e26 100644 --- a/src/contentmanagermodel.cpp +++ b/src/contentmanagermodel.cpp @@ -245,24 +245,6 @@ void ContentManagerModel::updateImage(QString bookId, QString url, QByteArray im emit dataChanged(index, index); } -std::shared_ptr getSharedPointer(RowNode* ptr) -{ - return std::static_pointer_cast(ptr->shared_from_this()); -} - -void ContentManagerModel::startDownload(QModelIndex index) -{ - auto node = getSharedPointer(static_cast(index.internalPointer())); - const auto bookId = node->getBookId(); - const auto newDownload = std::make_shared(); - 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); diff --git a/src/contentmanagermodel.h b/src/contentmanagermodel.h index b9196af74..9d78d4d0d 100644 --- a/src/contentmanagermodel.h +++ b/src/contentmanagermodel.h @@ -47,18 +47,15 @@ class ContentManagerModel : public QAbstractItemModel 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 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 rootNode; From 087881d51079da395321fb13d2e782e43e465b8a Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Fri, 1 Mar 2024 20:29:07 +0400 Subject: [PATCH 04/19] Fixed download progress update for the first row --- src/contentmanagermodel.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/contentmanagermodel.cpp b/src/contentmanagermodel.cpp index 846f91e26..a2f95db5c 100644 --- a/src/contentmanagermodel.cpp +++ b/src/contentmanagermodel.cpp @@ -272,8 +272,7 @@ void ContentManagerModel::updateDownload(QString bookId) 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); + const QModelIndex newIndex = this->index(row, 5); emit dataChanged(newIndex, newIndex); } } From 4842742e77a3fe836c34d24ece4d7712f83681d4 Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Fri, 9 Feb 2024 20:25:37 +0400 Subject: [PATCH 05/19] ContentManagerModel::removeDownload() --- src/contentmanager.cpp | 2 +- src/contentmanagermodel.cpp | 25 ++++++++++++------------- src/contentmanagermodel.h | 2 +- 3 files changed, 14 insertions(+), 15 deletions(-) diff --git a/src/contentmanager.cpp b/src/contentmanager.cpp index 29c621a9d..93f084602 100644 --- a/src/contentmanager.cpp +++ b/src/contentmanager.cpp @@ -652,7 +652,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); }); } diff --git a/src/contentmanagermodel.cpp b/src/contentmanagermodel.cpp index a2f95db5c..961041490 100644 --- a/src/contentmanagermodel.cpp +++ b/src/contentmanagermodel.cpp @@ -262,15 +262,8 @@ void ContentManagerModel::updateDownload(QString bookId) 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(*rootNode->child(row)); - rowNode.setDownloadState(nullptr); - } - } - - if ( it != bookIdToRowMap.constEnd() ) { + removeDownload(bookId); + } else if ( it != bookIdToRowMap.constEnd() ) { const size_t row = it.value(); const QModelIndex newIndex = this->index(row, 5); emit dataChanged(newIndex, newIndex); @@ -287,11 +280,17 @@ void ContentManagerModel::resumeDownload(QModelIndex index) emit dataChanged(index, index); } -void ContentManagerModel::cancelDownload(QModelIndex index) +void ContentManagerModel::removeDownload(QString bookId) { - auto node = static_cast(index.internalPointer()); - node->setDownloadState(nullptr); - m_downloads.remove(node->getBookId()); + m_downloads.remove(bookId); + const auto it = bookIdToRowMap.constFind(bookId); + if ( it == bookIdToRowMap.constEnd() ) + return; + + const size_t row = it.value(); + auto& node = static_cast(*rootNode->child(row)); + node.setDownloadState(nullptr); + const QModelIndex index = this->index(row, 5); emit dataChanged(index, index); } diff --git a/src/contentmanagermodel.h b/src/contentmanagermodel.h index 9d78d4d0d..bfc0e52db 100644 --- a/src/contentmanagermodel.h +++ b/src/contentmanagermodel.h @@ -49,7 +49,7 @@ public slots: void updateImage(QString bookId, QString url, QByteArray imageData); void pauseDownload(QModelIndex index); void resumeDownload(QModelIndex index); - void cancelDownload(QModelIndex index); + void removeDownload(QString bookId); void updateDownload(QString bookId); protected: // functions From d3c4361d60b9f99f445c4c4cc142ad3eb87bb972 Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Fri, 9 Feb 2024 20:32:19 +0400 Subject: [PATCH 06/19] Moved download deregistration to ContentManager --- src/contentmanager.cpp | 2 ++ src/contentmanagermodel.cpp | 1 - 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/contentmanager.cpp b/src/contentmanager.cpp index 93f084602..ffcd1918c 100644 --- a/src/contentmanager.cpp +++ b/src/contentmanager.cpp @@ -666,6 +666,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 diff --git a/src/contentmanagermodel.cpp b/src/contentmanagermodel.cpp index 961041490..4c80d3dcc 100644 --- a/src/contentmanagermodel.cpp +++ b/src/contentmanagermodel.cpp @@ -282,7 +282,6 @@ void ContentManagerModel::resumeDownload(QModelIndex index) void ContentManagerModel::removeDownload(QString bookId) { - m_downloads.remove(bookId); const auto it = bookIdToRowMap.constFind(bookId); if ( it == bookIdToRowMap.constEnd() ) return; From 582dce0cedd1f1136498cb4c9d73262fe1b54f96 Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Sat, 10 Feb 2024 15:51:10 +0400 Subject: [PATCH 07/19] ContentManager::updateDownload() --- src/contentmanager.cpp | 9 ++++++++- src/contentmanager.h | 1 + 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/contentmanager.cpp b/src/contentmanager.cpp index ffcd1918c..474bf9a0e 100644 --- a/src/contentmanager.cpp +++ b/src/contentmanager.cpp @@ -476,6 +476,13 @@ ContentManager::DownloadInfo ContentManager::updateDownloadInfos(QString bookId, return result; } +void ContentManager::updateDownload(QString bookId) +{ + // This calls ContentManager::updateDownloadInfos() in a convoluted way + // and also has some other side-effects + managerModel->updateDownload(bookId); +} + namespace { @@ -497,7 +504,7 @@ void ContentManager::downloadBook(const QString &id, QModelIndex index) node->setDownloadState(newDownload); QTimer *timer = newDownload->getDownloadUpdateTimer(); connect(timer, &QTimer::timeout, [=]() { - managerModel->updateDownload(id); + this->updateDownload(id); }); } catch ( const ContentManagerError& err ) diff --git a/src/contentmanager.h b/src/contentmanager.h index b19a80ae1..8741f4c0b 100644 --- a/src/contentmanager.h +++ b/src/contentmanager.h @@ -86,6 +86,7 @@ public slots: const kiwix::Book& getRemoteOrLocalBook(const QString &id); std::string startDownload(const kiwix::Book& book); + void updateDownload(QString bookId); void downloadStarted(const kiwix::Book& book, const std::string& downloadId); void downloadCancelled(QString bookId); void downloadCompleted(QString bookId, QString path); From 4ab2a89c03a6ead593f16437593f909ff3686e67 Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Sat, 10 Feb 2024 16:19:45 +0400 Subject: [PATCH 08/19] Deleted an unused function --- src/rownode.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/rownode.h b/src/rownode.h index 85cbe615d..4198e98d2 100644 --- a/src/rownode.h +++ b/src/rownode.h @@ -19,7 +19,6 @@ class DownloadState public: DownloadState(); - bool isDownloading() const { return m_downloadUpdateTimer.get() != nullptr; } DownloadInfo getDownloadInfo() const { return m_downloadInfo; } QTimer* getDownloadUpdateTimer() const { return m_downloadUpdateTimer.get(); } void pause(); From 8e3a783700e8973ee3b994e4c76df4d7478bf62e Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Sat, 10 Feb 2024 16:28:13 +0400 Subject: [PATCH 09/19] Got rid of the timer in DownloadState Now there is a single timer in ContentManager shared by all pending downloads. Temporarily, all active downloads are updated on every firing of the timer and this happens in the main thread. This will be changed later. The main objective of this change is to get rid of the timer in DownloadState so that it can be converted to a dumb struct set by ContentManager. --- src/contentmanager.cpp | 24 +++++++++++++++++------- src/contentmanager.h | 2 ++ src/rownode.cpp | 11 ----------- src/rownode.h | 4 ---- 4 files changed, 19 insertions(+), 22 deletions(-) diff --git a/src/contentmanager.cpp b/src/contentmanager.cpp index 474bf9a0e..15a2b16c2 100644 --- a/src/contentmanager.cpp +++ b/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() @@ -478,9 +482,19 @@ ContentManager::DownloadInfo ContentManager::updateDownloadInfos(QString bookId, void ContentManager::updateDownload(QString bookId) { - // This calls ContentManager::updateDownloadInfos() in a convoluted way - // and also has some other side-effects - managerModel->updateDownload(bookId); + const auto downloadState = m_downloads.value(bookId); + if ( downloadState && !downloadState->getDownloadInfo().paused ) { + // This calls ContentManager::updateDownloadInfos() in a convoluted way + // and also has some other side-effects + managerModel->updateDownload(bookId); + } +} + +void ContentManager::updateDownloads() +{ + for ( const auto& bookId : m_downloads.keys() ) { + updateDownload(bookId); + } } namespace @@ -502,10 +516,6 @@ void ContentManager::downloadBook(const QString &id, QModelIndex index) const auto newDownload = std::make_shared(); m_downloads[id] = newDownload; node->setDownloadState(newDownload); - QTimer *timer = newDownload->getDownloadUpdateTimer(); - connect(timer, &QTimer::timeout, [=]() { - this->updateDownload(id); - }); } catch ( const ContentManagerError& err ) { diff --git a/src/contentmanager.h b/src/contentmanager.h index 8741f4c0b..edce66295 100644 --- a/src/contentmanager.h +++ b/src/contentmanager.h @@ -71,6 +71,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(); @@ -97,6 +98,7 @@ public slots: 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; diff --git a/src/rownode.cpp b/src/rownode.cpp index 2559b992c..b2ad6110d 100644 --- a/src/rownode.cpp +++ b/src/rownode.cpp @@ -10,8 +10,6 @@ DownloadState::DownloadState() : m_downloadInfo({0, "", "", false}) { - m_downloadUpdateTimer.reset(new QTimer); - m_downloadUpdateTimer->start(1000); } namespace @@ -37,13 +35,6 @@ bool DownloadState::update(QString id) { 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; } @@ -60,13 +51,11 @@ bool DownloadState::update(QString id) void DownloadState::pause() { m_downloadInfo.paused = true; - m_downloadUpdateTimer->stop(); } void DownloadState::resume() { m_downloadInfo.paused = false; - m_downloadUpdateTimer->start(1000); } diff --git a/src/rownode.h b/src/rownode.h index 4198e98d2..62d915001 100644 --- a/src/rownode.h +++ b/src/rownode.h @@ -20,15 +20,11 @@ class DownloadState DownloadState(); DownloadInfo getDownloadInfo() const { return m_downloadInfo; } - QTimer* getDownloadUpdateTimer() const { return m_downloadUpdateTimer.get(); } void pause(); void resume(); bool update(QString id); protected: - // This is non-NULL only for a pending (even if paused) download - std::unique_ptr m_downloadUpdateTimer; - DownloadInfo m_downloadInfo; }; From a6359af1ab1ba100478c846a9ca9e53b3d788ea6 Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Sun, 11 Feb 2024 15:10:27 +0400 Subject: [PATCH 10/19] Merged ::DownloadInfo into DownloadState --- src/contentmanager.cpp | 4 ++-- src/contentmanager.h | 1 - src/contentmanagerdelegate.cpp | 7 +++---- src/rownode.cpp | 13 ++++--------- src/rownode.h | 16 ++++------------ 5 files changed, 13 insertions(+), 28 deletions(-) diff --git a/src/contentmanager.cpp b/src/contentmanager.cpp index 15a2b16c2..297da377a 100644 --- a/src/contentmanager.cpp +++ b/src/contentmanager.cpp @@ -163,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); @@ -483,7 +483,7 @@ ContentManager::DownloadInfo ContentManager::updateDownloadInfos(QString bookId, void ContentManager::updateDownload(QString bookId) { const auto downloadState = m_downloads.value(bookId); - if ( downloadState && !downloadState->getDownloadInfo().paused ) { + if ( downloadState && !downloadState->paused ) { // This calls ContentManager::updateDownloadInfos() in a convoluted way // and also has some other side-effects managerModel->updateDownload(bookId); diff --git a/src/contentmanager.h b/src/contentmanager.h index edce66295..c441a6ee6 100644 --- a/src/contentmanager.h +++ b/src/contentmanager.h @@ -20,7 +20,6 @@ class ContentManager : public QObject typedef ContentManagerModel::BookInfo BookInfo; typedef ContentManagerModel::BookInfoList BookInfoList; - // XXX: potentional source of confusion with ::DownloadInfo from rownode.h typedef QMap DownloadInfo; public: // functions diff --git a/src/contentmanagerdelegate.cpp b/src/contentmanagerdelegate.cpp index 8944026ce..7a9eddbd2 100644 --- a/src/contentmanagerdelegate.cpp +++ b/src/contentmanagerdelegate.cpp @@ -103,7 +103,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(); @@ -178,8 +178,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, downloadInfo); + showDownloadProgress(painter, r, *downloadState); } else { baseButton->style()->drawControl( QStyle::CE_PushButton, &button, painter, baseButton.data()); @@ -245,7 +244,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 { diff --git a/src/rownode.cpp b/src/rownode.cpp index b2ad6110d..cea54b28a 100644 --- a/src/rownode.cpp +++ b/src/rownode.cpp @@ -7,11 +7,6 @@ // DowloadState //////////////////////////////////////////////////////////////////////////////// -DownloadState::DownloadState() - : m_downloadInfo({0, "", "", false}) -{ -} - namespace { @@ -35,7 +30,7 @@ bool DownloadState::update(QString id) { auto downloadInfos = KiwixApp::instance()->getContentManager()->updateDownloadInfos(id, {"status", "completedLength", "totalLength", "downloadSpeed"}); if (!downloadInfos["status"].isValid()) { - m_downloadInfo = {0, "", "", false}; + *this = {0, "", "", false}; return false; } @@ -44,18 +39,18 @@ bool DownloadState::update(QString id) percent = QString::number(percent, 'g', 3).toDouble(); auto completedLength = convertToUnits(downloadInfos["completedLength"].toString()); auto downloadSpeed = convertToUnits(downloadInfos["downloadSpeed"].toString()) + "/s"; - m_downloadInfo = {percent, completedLength, downloadSpeed, false}; + *this = {percent, completedLength, downloadSpeed, false}; return true; } void DownloadState::pause() { - m_downloadInfo.paused = true; + this->paused = true; } void DownloadState::resume() { - m_downloadInfo.paused = false; + this->paused = false; } diff --git a/src/rownode.h b/src/rownode.h index 62d915001..cea055f15 100644 --- a/src/rownode.h +++ b/src/rownode.h @@ -6,26 +6,18 @@ #include #include "kiwix/book.h" -struct DownloadInfo +class DownloadState { - double progress; +public: + double progress = 0; QString completedLength; QString downloadSpeed; - bool paused; -}; + bool paused = false; -class DownloadState -{ public: - DownloadState(); - - DownloadInfo getDownloadInfo() const { return m_downloadInfo; } void pause(); void resume(); bool update(QString id); - -protected: - DownloadInfo m_downloadInfo; }; class RowNode : public Node From b288f287f2ef478c1b6aa87b0d5f14654d78fd52 Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Sun, 11 Feb 2024 15:21:25 +0400 Subject: [PATCH 11/19] Made DownloadState::update() a setter DownloadState::update() no longer calls ContentManager::updateDownloadInfos() --- src/contentmanager.cpp | 4 ++-- src/contentmanager.h | 2 -- src/contentmanagermodel.cpp | 5 +++-- src/rownode.cpp | 3 +-- src/rownode.h | 4 +++- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/contentmanager.cpp b/src/contentmanager.cpp index 297da377a..622a6ff0c 100644 --- a/src/contentmanager.cpp +++ b/src/contentmanager.cpp @@ -437,7 +437,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) { @@ -464,7 +464,7 @@ ContentManager::DownloadInfo ContentManager::getDownloadInfo(QString bookId, con return values; } -ContentManager::DownloadInfo ContentManager::updateDownloadInfos(QString bookId, QStringList keys) +DownloadInfo ContentManager::updateDownloadInfos(QString bookId, QStringList keys) { if ( !keys.contains("status") ) keys.append("status"); if ( !keys.contains("path") ) keys.append("path"); diff --git a/src/contentmanager.h b/src/contentmanager.h index c441a6ee6..d128c2256 100644 --- a/src/contentmanager.h +++ b/src/contentmanager.h @@ -20,8 +20,6 @@ class ContentManager : public QObject typedef ContentManagerModel::BookInfo BookInfo; typedef ContentManagerModel::BookInfoList BookInfoList; - typedef QMap DownloadInfo; - public: // functions explicit ContentManager(Library* library, kiwix::Downloader *downloader, QObject *parent = nullptr); virtual ~ContentManager() {} diff --git a/src/contentmanagermodel.cpp b/src/contentmanagermodel.cpp index 4c80d3dcc..e39c54741 100644 --- a/src/contentmanagermodel.cpp +++ b/src/contentmanagermodel.cpp @@ -252,9 +252,10 @@ void ContentManagerModel::updateDownload(QString bookId) if ( ! download ) return; - const bool downloadStillValid = download->update(bookId); + const auto downloadInfos = KiwixApp::instance()->getContentManager()->updateDownloadInfos(bookId, {"status", "completedLength", "totalLength", "downloadSpeed"}); + const bool downloadStillValid = download->update(downloadInfos); - // The download->update() call above may result in + // The ContentManager::updateDownloadInfos() 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. diff --git a/src/rownode.cpp b/src/rownode.cpp index cea54b28a..2c989687a 100644 --- a/src/rownode.cpp +++ b/src/rownode.cpp @@ -26,9 +26,8 @@ QString convertToUnits(QString size) } // unnamed namespace -bool DownloadState::update(QString id) +bool DownloadState::update(const DownloadInfo& downloadInfos) { - auto downloadInfos = KiwixApp::instance()->getContentManager()->updateDownloadInfos(id, {"status", "completedLength", "totalLength", "downloadSpeed"}); if (!downloadInfos["status"].isValid()) { *this = {0, "", "", false}; return false; diff --git a/src/rownode.h b/src/rownode.h index cea055f15..db51b5774 100644 --- a/src/rownode.h +++ b/src/rownode.h @@ -6,6 +6,8 @@ #include #include "kiwix/book.h" +typedef QMap DownloadInfo; + class DownloadState { public: @@ -17,7 +19,7 @@ class DownloadState public: void pause(); void resume(); - bool update(QString id); + bool update(const DownloadInfo& info); }; class RowNode : public Node From b112c1b34cf34cfada88879879abf0a5339c42d2 Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Sun, 11 Feb 2024 15:46:48 +0400 Subject: [PATCH 12/19] Bugfix: completed downloads are removed The commit "Moved download deregistration to ContentManager" introduced a bug - completed downloads were not properly deregistered. Note that a simpler fix by adding a single `m_downloads.remove(bookId);` line in `ContentManager::downloadCompleted()` didn't work (which means that that function is not always called upon download completion). I am not going to investigate that issue now, hoping that it will be automatically resolved once the redesign of download management is finished. --- src/contentmanager.cpp | 4 +++- src/contentmanagermodel.cpp | 6 ++++-- src/contentmanagermodel.h | 2 +- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/contentmanager.cpp b/src/contentmanager.cpp index 622a6ff0c..c31d2e68b 100644 --- a/src/contentmanager.cpp +++ b/src/contentmanager.cpp @@ -486,7 +486,9 @@ void ContentManager::updateDownload(QString bookId) if ( downloadState && !downloadState->paused ) { // This calls ContentManager::updateDownloadInfos() in a convoluted way // and also has some other side-effects - managerModel->updateDownload(bookId); + if ( ! managerModel->updateDownload(bookId) ) { + m_downloads.remove(bookId); + } } } diff --git a/src/contentmanagermodel.cpp b/src/contentmanagermodel.cpp index e39c54741..4dfc6de26 100644 --- a/src/contentmanagermodel.cpp +++ b/src/contentmanagermodel.cpp @@ -245,12 +245,12 @@ void ContentManagerModel::updateImage(QString bookId, QString url, QByteArray im emit dataChanged(index, index); } -void ContentManagerModel::updateDownload(QString bookId) +bool ContentManagerModel::updateDownload(QString bookId) { const auto download = m_downloads.value(bookId); if ( ! download ) - return; + return true; const auto downloadInfos = KiwixApp::instance()->getContentManager()->updateDownloadInfos(bookId, {"status", "completedLength", "totalLength", "downloadSpeed"}); const bool downloadStillValid = download->update(downloadInfos); @@ -269,8 +269,10 @@ void ContentManagerModel::updateDownload(QString bookId) const QModelIndex newIndex = this->index(row, 5); emit dataChanged(newIndex, newIndex); } + return downloadStillValid; } + void ContentManagerModel::pauseDownload(QModelIndex index) { emit dataChanged(index, index); diff --git a/src/contentmanagermodel.h b/src/contentmanagermodel.h index bfc0e52db..f7f67f972 100644 --- a/src/contentmanagermodel.h +++ b/src/contentmanagermodel.h @@ -50,7 +50,7 @@ public slots: void pauseDownload(QModelIndex index); void resumeDownload(QModelIndex index); void removeDownload(QString bookId); - void updateDownload(QString bookId); + bool updateDownload(QString bookId); protected: // functions bool canFetchMore(const QModelIndex &parent) const override; From d089dced0568744bb8f3abf779409d2740fd5d8b Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Sun, 11 Feb 2024 16:13:08 +0400 Subject: [PATCH 13/19] Eliminated indirect call to ContentManager::updateDownloadInfos() Now ContentManager::updateDownloadInfos() is called directly by ContentManager (rather than indirectly via ContentManagerModel). --- src/contentmanager.cpp | 9 ++++++--- src/contentmanagermodel.cpp | 20 ++------------------ src/contentmanagermodel.h | 2 +- 3 files changed, 9 insertions(+), 22 deletions(-) diff --git a/src/contentmanager.cpp b/src/contentmanager.cpp index c31d2e68b..3de973603 100644 --- a/src/contentmanager.cpp +++ b/src/contentmanager.cpp @@ -484,10 +484,13 @@ void ContentManager::updateDownload(QString bookId) { const auto downloadState = m_downloads.value(bookId); if ( downloadState && !downloadState->paused ) { - // This calls ContentManager::updateDownloadInfos() in a convoluted way - // and also has some other side-effects - if ( ! managerModel->updateDownload(bookId) ) { + const auto downloadInfo = updateDownloadInfos(bookId, {"status", "completedLength", "totalLength", "downloadSpeed"}); + const bool downloadStillValid = downloadState->update(downloadInfo); + if ( ! downloadStillValid ) { m_downloads.remove(bookId); + managerModel->removeDownload(bookId); + } else { + managerModel->updateDownload(bookId); } } } diff --git a/src/contentmanagermodel.cpp b/src/contentmanagermodel.cpp index 4dfc6de26..8428263c1 100644 --- a/src/contentmanagermodel.cpp +++ b/src/contentmanagermodel.cpp @@ -245,31 +245,15 @@ void ContentManagerModel::updateImage(QString bookId, QString url, QByteArray im emit dataChanged(index, index); } -bool ContentManagerModel::updateDownload(QString bookId) +void ContentManagerModel::updateDownload(QString bookId) { - const auto download = m_downloads.value(bookId); - - if ( ! download ) - return true; - - const auto downloadInfos = KiwixApp::instance()->getContentManager()->updateDownloadInfos(bookId, {"status", "completedLength", "totalLength", "downloadSpeed"}); - const bool downloadStillValid = download->update(downloadInfos); - - // The ContentManager::updateDownloadInfos() 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 ) { - removeDownload(bookId); - } else if ( it != bookIdToRowMap.constEnd() ) { + if ( it != bookIdToRowMap.constEnd() ) { const size_t row = it.value(); const QModelIndex newIndex = this->index(row, 5); emit dataChanged(newIndex, newIndex); } - return downloadStillValid; } diff --git a/src/contentmanagermodel.h b/src/contentmanagermodel.h index f7f67f972..bfc0e52db 100644 --- a/src/contentmanagermodel.h +++ b/src/contentmanagermodel.h @@ -50,7 +50,7 @@ public slots: void pauseDownload(QModelIndex index); void resumeDownload(QModelIndex index); void removeDownload(QString bookId); - bool updateDownload(QString bookId); + void updateDownload(QString bookId); protected: // functions bool canFetchMore(const QModelIndex &parent) const override; From 6f781a8e0ec3a5b594f2145d3362a22714cf31fe Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Sun, 11 Feb 2024 16:25:04 +0400 Subject: [PATCH 14/19] Got rid of ContentManager::updateDownloadInfo() ... by inlining it into ContentManager::updateDownload() --- src/contentmanager.cpp | 25 ++++++++----------------- src/contentmanager.h | 1 - 2 files changed, 8 insertions(+), 18 deletions(-) diff --git a/src/contentmanager.cpp b/src/contentmanager.cpp index 3de973603..0f6c6bba8 100644 --- a/src/contentmanager.cpp +++ b/src/contentmanager.cpp @@ -464,27 +464,18 @@ DownloadInfo ContentManager::getDownloadInfo(QString bookId, const QStringList & return values; } -DownloadInfo ContentManager::updateDownloadInfos(QString bookId, QStringList keys) -{ - if ( !keys.contains("status") ) keys.append("status"); - if ( !keys.contains("path") ) keys.append("path"); - - const DownloadInfo result = getDownloadInfo(bookId, keys); - - if ( result.isEmpty() ) { - downloadCancelled(bookId); - } else if ( result["status"] == "completed" ) { - downloadCompleted(bookId, result["path"].toString()); - } - - return result; -} - void ContentManager::updateDownload(QString bookId) { const auto downloadState = m_downloads.value(bookId); if ( downloadState && !downloadState->paused ) { - const auto downloadInfo = updateDownloadInfos(bookId, {"status", "completedLength", "totalLength", "downloadSpeed"}); + const auto downloadInfo = getDownloadInfo(bookId, {"status", "completedLength", "totalLength", "downloadSpeed", "path"}); + + if ( downloadInfo.isEmpty() ) { + downloadCancelled(bookId); + } else if ( downloadInfo["status"] == "completed" ) { + downloadCompleted(bookId, downloadInfo["path"].toString()); + } + const bool downloadStillValid = downloadState->update(downloadInfo); if ( ! downloadStillValid ) { m_downloads.remove(bookId); diff --git a/src/contentmanager.h b/src/contentmanager.h index d128c2256..585b1817f 100644 --- a/src/contentmanager.h +++ b/src/contentmanager.h @@ -49,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(); From 4153765b7bb7537d2c649f2de194bab02948696f Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Sun, 11 Feb 2024 16:46:56 +0400 Subject: [PATCH 15/19] ContentManagerModel only reads from m_downloads --- src/contentmanagermodel.cpp | 2 +- src/contentmanagermodel.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/contentmanagermodel.cpp b/src/contentmanagermodel.cpp index 8428263c1..534953985 100644 --- a/src/contentmanagermodel.cpp +++ b/src/contentmanagermodel.cpp @@ -7,7 +7,7 @@ #include "kiwixapp.h" #include -ContentManagerModel::ContentManagerModel(Downloads* downloads, QObject *parent) +ContentManagerModel::ContentManagerModel(const Downloads* downloads, QObject *parent) : QAbstractItemModel(parent) , m_downloads(*downloads) { diff --git a/src/contentmanagermodel.h b/src/contentmanagermodel.h index bfc0e52db..30905b94e 100644 --- a/src/contentmanagermodel.h +++ b/src/contentmanagermodel.h @@ -25,7 +25,7 @@ class ContentManagerModel : public QAbstractItemModel typedef QMap> 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; @@ -63,7 +63,7 @@ public slots: ThumbnailDownloader td; QMap bookIdToRowMap; QMap iconMap; - Downloads& m_downloads; + const Downloads& m_downloads; }; #endif // CONTENTMANAGERMODEL_H From defe52a2460939350fcabc420f5749fe5382f9f1 Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Sun, 11 Feb 2024 20:07:53 +0400 Subject: [PATCH 16/19] ContentManager::removeDownload() --- src/contentmanager.cpp | 9 +++++++-- src/contentmanager.h | 1 + 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/contentmanager.cpp b/src/contentmanager.cpp index 0f6c6bba8..d63e172d1 100644 --- a/src/contentmanager.cpp +++ b/src/contentmanager.cpp @@ -410,6 +410,12 @@ 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) { kiwix::Book bCopy(mp_library->getBookById(bookId)); @@ -478,8 +484,7 @@ void ContentManager::updateDownload(QString bookId) const bool downloadStillValid = downloadState->update(downloadInfo); if ( ! downloadStillValid ) { - m_downloads.remove(bookId); - managerModel->removeDownload(bookId); + removeDownload(bookId); } else { managerModel->updateDownload(bookId); } diff --git a/src/contentmanager.h b/src/contentmanager.h index 585b1817f..62c063c0d 100644 --- a/src/contentmanager.h +++ b/src/contentmanager.h @@ -84,6 +84,7 @@ public slots: 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); From e0f79bf9f211705622499167fe28bb4181a6c93a Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Sun, 11 Feb 2024 20:11:51 +0400 Subject: [PATCH 17/19] More elegant ContentManager::updateDownload() --- src/contentmanager.cpp | 8 +++----- src/rownode.cpp | 5 ----- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/src/contentmanager.cpp b/src/contentmanager.cpp index d63e172d1..bee053a59 100644 --- a/src/contentmanager.cpp +++ b/src/contentmanager.cpp @@ -418,6 +418,7 @@ void ContentManager::removeDownload(QString bookId) void ContentManager::downloadCancelled(QString bookId) { + removeDownload(bookId); kiwix::Book bCopy(mp_library->getBookById(bookId)); bCopy.setDownloadId(""); mp_library->getKiwixLibrary()->addOrUpdateBook(bCopy); @@ -427,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(""); @@ -480,12 +482,8 @@ void ContentManager::updateDownload(QString bookId) downloadCancelled(bookId); } else if ( downloadInfo["status"] == "completed" ) { downloadCompleted(bookId, downloadInfo["path"].toString()); - } - - const bool downloadStillValid = downloadState->update(downloadInfo); - if ( ! downloadStillValid ) { - removeDownload(bookId); } else { + downloadState->update(downloadInfo); managerModel->updateDownload(bookId); } } diff --git a/src/rownode.cpp b/src/rownode.cpp index 2c989687a..e4d0cd740 100644 --- a/src/rownode.cpp +++ b/src/rownode.cpp @@ -28,11 +28,6 @@ QString convertToUnits(QString size) bool DownloadState::update(const DownloadInfo& downloadInfos) { - if (!downloadInfos["status"].isValid()) { - *this = {0, "", "", false}; - return false; - } - double percent = downloadInfos["completedLength"].toDouble() / downloadInfos["totalLength"].toDouble(); percent *= 100; percent = QString::number(percent, 'g', 3).toDouble(); From b434833edc844ddbaa5b7f4b25f66169168d0d60 Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Sun, 11 Feb 2024 20:21:50 +0400 Subject: [PATCH 18/19] Dropped an obsolete return value --- src/rownode.cpp | 3 +-- src/rownode.h | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/rownode.cpp b/src/rownode.cpp index e4d0cd740..6a0cf12ff 100644 --- a/src/rownode.cpp +++ b/src/rownode.cpp @@ -26,7 +26,7 @@ QString convertToUnits(QString size) } // unnamed namespace -bool DownloadState::update(const DownloadInfo& downloadInfos) +void DownloadState::update(const DownloadInfo& downloadInfos) { double percent = downloadInfos["completedLength"].toDouble() / downloadInfos["totalLength"].toDouble(); percent *= 100; @@ -34,7 +34,6 @@ bool DownloadState::update(const DownloadInfo& downloadInfos) auto completedLength = convertToUnits(downloadInfos["completedLength"].toString()); auto downloadSpeed = convertToUnits(downloadInfos["downloadSpeed"].toString()) + "/s"; *this = {percent, completedLength, downloadSpeed, false}; - return true; } void DownloadState::pause() diff --git a/src/rownode.h b/src/rownode.h index db51b5774..f98680613 100644 --- a/src/rownode.h +++ b/src/rownode.h @@ -19,7 +19,7 @@ class DownloadState public: void pause(); void resume(); - bool update(const DownloadInfo& info); + void update(const DownloadInfo& info); }; class RowNode : public Node From 5d28f8a332146a77336a40862e0b3991bb175d3e Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Mon, 12 Feb 2024 16:48:19 +0400 Subject: [PATCH 19/19] Changed the parameter type --- src/rownode.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/rownode.cpp b/src/rownode.cpp index 6a0cf12ff..57ce5ac4a 100644 --- a/src/rownode.cpp +++ b/src/rownode.cpp @@ -10,11 +10,10 @@ 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++; @@ -31,8 +30,8 @@ void DownloadState::update(const DownloadInfo& downloadInfos) double percent = downloadInfos["completedLength"].toDouble() / downloadInfos["totalLength"].toDouble(); percent *= 100; percent = QString::number(percent, 'g', 3).toDouble(); - auto completedLength = convertToUnits(downloadInfos["completedLength"].toString()); - auto downloadSpeed = convertToUnits(downloadInfos["downloadSpeed"].toString()) + "/s"; + auto completedLength = convertToUnits(downloadInfos["completedLength"].toDouble()); + auto downloadSpeed = convertToUnits(downloadInfos["downloadSpeed"].toDouble()) + "/s"; *this = {percent, completedLength, downloadSpeed, false}; }