From 9a754715f774c527fec4374f106b5588854b4492 Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Thu, 2 May 2024 17:35:29 +0200 Subject: [PATCH 1/7] In some situations, short is better than long --- src/contentmanagerdelegate.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/contentmanagerdelegate.cpp b/src/contentmanagerdelegate.cpp index 72fc437..c49e397 100644 --- a/src/contentmanagerdelegate.cpp +++ b/src/contentmanagerdelegate.cpp @@ -249,22 +249,23 @@ void ContentManagerDelegate::handleLastColumnClicked(const QModelIndex& index, Q int x = r.left(); int w = r.width(); + ContentManager& contentMgr = *KiwixApp::instance()->getContentManager(); if (const auto downloadState = node->getDownloadState()) { if (downloadState->paused) { if (clickX < (x + w/2)) { - KiwixApp::instance()->getContentManager()->cancelBook(id); + contentMgr.cancelBook(id); } else { - KiwixApp::instance()->getContentManager()->resumeBook(id, index); + contentMgr.resumeBook(id, index); } } else { - KiwixApp::instance()->getContentManager()->pauseBook(id, index); + contentMgr.pauseBook(id, index); } } else { try { const auto book = KiwixApp::instance()->getLibrary()->getBookById(id); - KiwixApp::instance()->getContentManager()->openBook(id); + contentMgr.openBook(id); } catch (std::out_of_range& e) { - KiwixApp::instance()->getContentManager()->downloadBook(id, index); + contentMgr.downloadBook(id, index); } } } From b7fa28b8112eb6859db23234be29d5dce10217c2 Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Thu, 2 May 2024 17:51:04 +0200 Subject: [PATCH 2/7] Better nesting & indentation --- src/contentmanagerdelegate.cpp | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/src/contentmanagerdelegate.cpp b/src/contentmanagerdelegate.cpp index c49e397..a880065 100644 --- a/src/contentmanagerdelegate.cpp +++ b/src/contentmanagerdelegate.cpp @@ -251,23 +251,21 @@ void ContentManagerDelegate::handleLastColumnClicked(const QModelIndex& index, Q ContentManager& contentMgr = *KiwixApp::instance()->getContentManager(); if (const auto downloadState = node->getDownloadState()) { - if (downloadState->paused) { - if (clickX < (x + w/2)) { - contentMgr.cancelBook(id); - } else { - contentMgr.resumeBook(id, index); - } - } else { + if ( !downloadState->paused ) { contentMgr.pauseBook(id, index); + } else if (clickX < (x + w/2)) { + contentMgr.cancelBook(id); + } else { + contentMgr.resumeBook(id, index); } } else { - try { - const auto book = KiwixApp::instance()->getLibrary()->getBookById(id); - contentMgr.openBook(id); - } catch (std::out_of_range& e) { - contentMgr.downloadBook(id, index); - } - } + try { + const auto book = KiwixApp::instance()->getLibrary()->getBookById(id); + contentMgr.openBook(id); + } catch (std::out_of_range& e) { + contentMgr.downloadBook(id, index); + } + } } QSize ContentManagerDelegate::sizeHint(const QStyleOptionViewItem &option, const QModelIndex &index) const From 7bef51073114fd895adb3525bfcdcec78c048613 Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Fri, 3 May 2024 16:23:10 +0200 Subject: [PATCH 3/7] Enter ContentManager::BookState --- src/contentmanager.cpp | 52 +++++++++++++++++++++++++++++++++++------- src/contentmanager.h | 38 ++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 8 deletions(-) diff --git a/src/contentmanager.cpp b/src/contentmanager.cpp index 2e44125..d8d037b 100644 --- a/src/contentmanager.cpp +++ b/src/contentmanager.cpp @@ -390,6 +390,19 @@ QVariant getBookAttribute(const kiwix::Book& b, const QString& a) return QVariant(); } +ContentManager::BookState getStateOfLocalBook(const kiwix::Book& book) +{ + if ( !book.isPathValid() ) { + return ContentManager::BookState::ERROR_MISSING_ZIM_FILE; + } + + // XXX: When a book is detected to be corrupted, information about that + // XXX: has to be recorded somewhere so that we can return + // XXX: ERROR_CORRUPTED_ZIM_FILE here + + return ContentManager::BookState::AVAILABLE_LOCALLY_AND_HEALTHY; +} + } // unnamed namespace ContentManager::BookInfo ContentManager::getBookInfos(QString id, const QStringList &keys) @@ -420,17 +433,40 @@ ContentManager::BookInfo ContentManager::getBookInfos(QString id, const QStringL return values; } +ContentManager::BookState ContentManager::getBookState(QString bookId) +{ + if ( const auto downloadState = m_downloads.value(bookId) ) { + return downloadState->paused + ? BookState::DOWNLOAD_PAUSED + : BookState::DOWNLOADING; + // TODO: a download may be in error state + } + + try { + const kiwix::Book& b = mp_library->getBookById(bookId); + return b.getDownloadId().empty() + ? getStateOfLocalBook(b) + : BookState::DOWNLOADING; + } catch (...) {} + + try { + QMutexLocker locker(&remoteLibraryLocker); + const kiwix::Book& b = mp_remoteLibrary->getBookById(bookId.toStdString()); + return !b.getUrl().empty() + ? BookState::AVAILABLE_ONLINE + : BookState::METADATA_ONLY; + } catch (...) {} + + return BookState::INVALID; +} + void ContentManager::openBookWithIndex(const QModelIndex &index) { - try { - auto bookNode = static_cast(index.internalPointer()); - const QString bookId = bookNode->getBookId(); - // throws std::out_of_range if the book isn't available in local library - const kiwix::Book& book = mp_library->getBookById(bookId); - if ( !book.getDownloadId().empty() ) - return; + auto bookNode = static_cast(index.internalPointer()); + const QString bookId = bookNode->getBookId(); + if ( getBookState(bookId) == BookState::AVAILABLE_LOCALLY_AND_HEALTHY ) { openBook(bookId); - } catch (std::out_of_range &e) {} + } } void ContentManager::openBook(const QString &id) diff --git a/src/contentmanager.h b/src/contentmanager.h index 69a2ad6..cf1869c 100644 --- a/src/contentmanager.h +++ b/src/contentmanager.h @@ -20,6 +20,43 @@ public: // types typedef ContentManagerModel::BookInfo BookInfo; typedef ContentManagerModel::BookInfoList BookInfoList; + enum class BookState + { + // Nothing known about a book with that id + INVALID, + + // Only (some) metadata is available for the book, however neither a + // ZIM-file nor a URL is associated with it. + METADATA_ONLY, + + // No ZIM file is associated with the book but a URL is provided. + AVAILABLE_ONLINE, + + // The book is being downloaded. + DOWNLOADING, + + // The book started downloading, but the download is currently paused. + DOWNLOAD_PAUSED, + + // The book started downloading but the download was stopped due to + // errors. + DOWNLOAD_ERROR, + + // A valid ZIM file path is associated with the book and no evidence + // about any issues with that ZIM file has so far been obtained. + AVAILABLE_LOCALLY_AND_HEALTHY, + + // A ZIM file path is associated with the book but no such file seems + // to exist (may be caused by missing read permissions for the directory + // containing the ZIM file). + ERROR_MISSING_ZIM_FILE, + + // A ZIM file is associated with the book but it cannot be opened + // due to issues with its content. + ERROR_CORRUPTED_ZIM_FILE + }; + + public: // functions explicit ContentManager(Library* library, kiwix::Downloader *downloader, QObject *parent = nullptr); virtual ~ContentManager(); @@ -49,6 +86,7 @@ signals: public slots: QStringList getTranslations(const QStringList &keys); BookInfo getBookInfos(QString id, const QStringList &keys); + BookState getBookState(QString id); void openBook(const QString& id); void downloadBook(const QString& id); void downloadBook(const QString& id, QModelIndex index); From d35fc1ff0631d144c9a629d78c3c3ecfe3b09254 Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Fri, 3 May 2024 16:23:33 +0200 Subject: [PATCH 4/7] Rewrote context menu setup using book states --- src/contentmanager.cpp | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/src/contentmanager.cpp b/src/contentmanager.cpp index d8d037b..71f9d9e 100644 --- a/src/contentmanager.cpp +++ b/src/contentmanager.cpp @@ -218,15 +218,19 @@ void ContentManager::onCustomContextMenu(const QPoint &point) QAction menuCancelBook(gt("cancel-download"), this); QAction menuOpenFolder(gt("open-folder"), this); - if (const auto download = bookNode->getDownloadState()) { - if (download->paused) { - contextMenu.addAction(&menuResumeBook); - } else { - contextMenu.addAction(&menuPauseBook); - } + switch ( getBookState(id) ) { + case BookState::DOWNLOAD_PAUSED: + contextMenu.addAction(&menuResumeBook); contextMenu.addAction(&menuCancelBook); - } else { - try { + break; + + case BookState::DOWNLOADING: + contextMenu.addAction(&menuPauseBook); + contextMenu.addAction(&menuCancelBook); + break; + + case BookState::AVAILABLE_LOCALLY_AND_HEALTHY: + { const auto book = mp_library->getBookById(id); auto bookPath = QString::fromStdString(book.getPath()); contextMenu.addAction(&menuOpenBook); @@ -235,9 +239,14 @@ void ContentManager::onCustomContextMenu(const QPoint &point) connect(&menuOpenFolder, &QAction::triggered, [=]() { openFileLocation(bookPath, mp_view); }); - } catch (...) { - contextMenu.addAction(&menuDownloadBook); + break; } + + case BookState::AVAILABLE_ONLINE: + contextMenu.addAction(&menuDownloadBook); + break; + + default: break; } connect(&menuDeleteBook, &QAction::triggered, [=]() { From 7761c6f7b37f7c5689ac989063aee3a177b1d8e3 Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Fri, 3 May 2024 16:31:08 +0200 Subject: [PATCH 5/7] Context menu takes error states into account --- src/contentmanager.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/contentmanager.cpp b/src/contentmanager.cpp index 71f9d9e..2634a3f 100644 --- a/src/contentmanager.cpp +++ b/src/contentmanager.cpp @@ -218,7 +218,8 @@ void ContentManager::onCustomContextMenu(const QPoint &point) QAction menuCancelBook(gt("cancel-download"), this); QAction menuOpenFolder(gt("open-folder"), this); - switch ( getBookState(id) ) { + const auto bookState = getBookState(id); + switch ( bookState ) { case BookState::DOWNLOAD_PAUSED: contextMenu.addAction(&menuResumeBook); contextMenu.addAction(&menuCancelBook); @@ -230,10 +231,14 @@ void ContentManager::onCustomContextMenu(const QPoint &point) break; case BookState::AVAILABLE_LOCALLY_AND_HEALTHY: + case BookState::ERROR_MISSING_ZIM_FILE: + case BookState::ERROR_CORRUPTED_ZIM_FILE: { const auto book = mp_library->getBookById(id); auto bookPath = QString::fromStdString(book.getPath()); - contextMenu.addAction(&menuOpenBook); + if ( bookState == BookState::AVAILABLE_LOCALLY_AND_HEALTHY ) { + contextMenu.addAction(&menuOpenBook); + } contextMenu.addAction(&menuDeleteBook); contextMenu.addAction(&menuOpenFolder); connect(&menuOpenFolder, &QAction::triggered, [=]() { From b6fbfb6104a013ffdc7ddb21ed39ed7540e0988d Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Fri, 3 May 2024 16:08:42 +0200 Subject: [PATCH 6/7] Rewrote last column display using book states --- src/contentmanagerdelegate.cpp | 57 +++++++++++++++++++--------------- src/contentmanagerdelegate.h | 4 +++ 2 files changed, 36 insertions(+), 25 deletions(-) diff --git a/src/contentmanagerdelegate.cpp b/src/contentmanagerdelegate.cpp index a880065..e2afbd1 100644 --- a/src/contentmanagerdelegate.cpp +++ b/src/contentmanagerdelegate.cpp @@ -146,17 +146,39 @@ void showDownloadProgress(QPainter *painter, QRect box, const DownloadState& dow createArc(painter, startAngle, spanAngle, rectangle, pen); } -void ContentManagerDelegate::paint(QPainter *painter, const QStyleOptionViewItem &option, const QModelIndex &index) const +void ContentManagerDelegate::paintButton(QPainter *p, const QRect &r, QString t) const { QStyleOptionButton button; - QRect r = option.rect; - int x,y,w,h; - x = r.left(); - y = r.top(); - w = r.width(); - h = r.height(); - button.rect = QRect(x,y,w,h); + button.rect = r; button.state = QStyle::State_Enabled; + button.text = t; + baseButton->style()->drawControl( QStyle::CE_PushButton, &button, p, baseButton.data()); +} + +void ContentManagerDelegate::paintBookState(QPainter *p, QRect r, const QModelIndex &index) const +{ + const auto node = static_cast(index.internalPointer()); + const auto id = node->getBookId(); + switch ( KiwixApp::instance()->getContentManager()->getBookState(id) ) { + case ContentManager::BookState::AVAILABLE_LOCALLY_AND_HEALTHY: + return paintButton(p, r, gt("open")); + + case ContentManager::BookState::AVAILABLE_ONLINE: + return paintButton(p, r, gt("download")); + + case ContentManager::BookState::DOWNLOADING: + case ContentManager::BookState::DOWNLOAD_PAUSED: + case ContentManager::BookState::DOWNLOAD_ERROR: + return showDownloadProgress(p, r, *node->getDownloadState()); + + default: + return; + } +} + +void ContentManagerDelegate::paint(QPainter *painter, const QStyleOptionViewItem &option, const QModelIndex &index) const +{ + QRect r = option.rect; if (index.parent().isValid()) { // additional info QRect nRect = r; @@ -165,24 +187,9 @@ void ContentManagerDelegate::paint(QPainter *painter, const QStyleOptionViewItem painter->drawText(nRect, Qt::AlignLeft | Qt::AlignVCenter, index.data(Qt::UserRole+1).toString()); return; } - auto node = static_cast(index.internalPointer()); - try { - const auto id = node->getBookId(); - const auto book = KiwixApp::instance()->getLibrary()->getBookById(id); - if ( book.getDownloadId().empty() ) { - button.text = gt("open"); - } - } catch (std::out_of_range& e) { - button.text = gt("download"); - } QStyleOptionViewItem eOpt = option; if (index.column() == 5) { - if (const auto downloadState = node->getDownloadState()) { - showDownloadProgress(painter, r, *downloadState); - } - else { - baseButton->style()->drawControl( QStyle::CE_PushButton, &button, painter, baseButton.data()); - } + paintBookState(painter, option.rect, index); return; } if (index.column() == 0) { @@ -192,7 +199,7 @@ void ContentManagerDelegate::paint(QPainter *painter, const QStyleOptionViewItem QPixmap pix; pix.loadFromData(iconData); QIcon icon(pix); - icon.paint(painter, QRect(x+10, y+10, 30, 50)); + icon.paint(painter, QRect(r.left()+10, r.top()+10, 30, 50)); return; } if (index.column() == 1) { diff --git a/src/contentmanagerdelegate.h b/src/contentmanagerdelegate.h index 5821f53..d69cd7f 100644 --- a/src/contentmanagerdelegate.h +++ b/src/contentmanagerdelegate.h @@ -15,6 +15,10 @@ public: bool editorEvent(QEvent *event, QAbstractItemModel *model, const QStyleOptionViewItem &option, const QModelIndex &index) override; QSize sizeHint(const QStyleOptionViewItem &option, const QModelIndex &index) const override; +private: // functions + void paintBookState(QPainter *p, QRect r, const QModelIndex &index) const; + void paintButton(QPainter *p, const QRect &r, QString t) const; + private: QScopedPointer baseButton; QByteArray placeholderIcon; From 0ecb142ca62f46323769a0ff9807fa1af47d6df9 Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Fri, 3 May 2024 15:41:01 +0200 Subject: [PATCH 7/7] Rewrote handling of last column clicks using book states --- src/contentmanagerdelegate.cpp | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/src/contentmanagerdelegate.cpp b/src/contentmanagerdelegate.cpp index e2afbd1..40acde8 100644 --- a/src/contentmanagerdelegate.cpp +++ b/src/contentmanagerdelegate.cpp @@ -257,21 +257,23 @@ void ContentManagerDelegate::handleLastColumnClicked(const QModelIndex& index, Q int w = r.width(); ContentManager& contentMgr = *KiwixApp::instance()->getContentManager(); - if (const auto downloadState = node->getDownloadState()) { - if ( !downloadState->paused ) { - contentMgr.pauseBook(id, index); - } else if (clickX < (x + w/2)) { - contentMgr.cancelBook(id); - } else { - contentMgr.resumeBook(id, index); - } - } else { - try { - const auto book = KiwixApp::instance()->getLibrary()->getBookById(id); - contentMgr.openBook(id); - } catch (std::out_of_range& e) { - contentMgr.downloadBook(id, index); - } + switch ( contentMgr.getBookState(id) ) { + case ContentManager::BookState::AVAILABLE_LOCALLY_AND_HEALTHY: + return contentMgr.openBook(id); + + case ContentManager::BookState::AVAILABLE_ONLINE: + return contentMgr.downloadBook(id, index); + + case ContentManager::BookState::DOWNLOADING: + return contentMgr.pauseBook(id, index); + + case ContentManager::BookState::DOWNLOAD_PAUSED: + return clickX < (x + w/2) + ? contentMgr.cancelBook(id) + : contentMgr.resumeBook(id, index); + + default: + return; } }