Added Custom File-Not-Found page for Tabs #1132

ShaopengLin · 2024-06-16T22:25:02Z

Fix #1073
Follow-up from #1120.

Changes:

Custom file-not-found page when tabs have Urls that point to non-existing Zim files.
Now displays the bookmarks of the non-existing Zim files. Previously they were marked as invalid and not added.

ShaopengLin · 2024-06-16T22:26:13Z

Let's review this after #1120 is merged. The reopening functionality will be available after we rebase this.

veloman-yunkan

Why were the changes related to a more informative File-Not-Found error page (reporting the ZIM path) lost?

resources/i18n/en.json

src/readinglistbar.cpp

veloman-yunkan · 2024-06-20T12:16:19Z

src/readinglistbar.cpp

        std::shared_ptr<zim::Archive> archive;
        const QString& title = QString::fromStdString(bookmark.getTitle());
        try {
            archive = library->getArchive(QString::fromStdString(bookmark.getBookId()));
        } catch (std::out_of_range& e) {
+            /* There can be bookmarks whose information is lost. */
+            if (!bookmark.getBookId().empty())


Is it possible for a bookmark to lack a bookId?

hmm, somehow I have a bunch of entries that don't have zimIds. It might be a bug but I am not sure how to reproduce.

Let's try not to hide any invalid bookmarks. But we can display them in a special way.

Note that if you add all bookmarks to the reading list, you won't need the extra idx parameter in ReadingListBar::addItem()

ShaopengLin · 2024-06-20T15:28:22Z

Why were the changes related to a more informative File-Not-Found error page (reporting the ZIM path) lost?

@veloman-yunkan sorry I am not exactly sure what this means? I thought I removed everything about zim path and only did a custom error page.

veloman-yunkan · 2024-06-20T16:25:29Z

Why were the changes related to a more informative File-Not-Found error page (reporting the ZIM path) lost?

@veloman-yunkan sorry I am not exactly sure what this means? I thought I removed everything about zim path and only did a custom error page.

I thought that this PR should contain all changes that were removed from #1120 when we decided to split it into two parts.

ShaopengLin · 2024-06-20T16:29:37Z

I was looking at kelson's comment and thought that you meant we are going go for that. Should I still add the path? #1120 (comment)

ShaopengLin · 2024-06-20T16:31:05Z

If we were to add the missing paths we will have to go back to that original complicated solution.

veloman-yunkan · 2024-06-20T20:08:23Z

If we were to add the missing paths we will have to go back to that original complicated solution.

And that is exactly why I chose to split the PR into two parts - so that the complicated solution doesn't block merging the fix for the original issue while we keep working around problems that crop up as we pursue a secondary improvement.

veloman-yunkan

I think that we should again split this PR into two:

Improvement of the missing ZIM file error page. The main challenge with this one will be to resolve the issues that we faced just before the previous split (related to the side effects of directory monitoring).
Listing all bookmarks (including invalid ones)

veloman-yunkan · 2024-06-25T07:35:31Z

src/urlschemehandler.cpp

+                                            const QString &zimId)
+{
+    QBuffer *buffer = new QBuffer;
+    QString contentHtml = "<section><div><div>"


Why are the two nested <div>s needed?

veloman-yunkan · 2024-06-25T07:53:00Z

src/webview.cpp

    try {
-        std::string favicon, _mimetype;
-        auto item = archive->getIllustrationItem(48);
-        favicon = item.getData();
-        _mimetype = item.getMimetype();
-        QPixmap pixmap;
-        pixmap.loadFromData((const uchar*)favicon.data(), favicon.size());
-        m_icon = QIcon(pixmap);
+        m_icon = app->getLibrary()->getArchiveIcon(archive);
        emit iconChanged(m_icon);
    } catch (zim::EntryNotFound& e) {}


I think the fact that iconChanged() is emitted only if an icon is successfully retrieved is a bug. Suppose that the tab is switched from a ZIM file containing a valid icon to a ZIM file missing a valid icon. Then the icon of the previous ZIM file will keep being displayed on the tab. Thus emit iconChanged(m_icon); should be moved outside the try/catch block. But then you can move the try/catch block inside the Library::getArchiveIcon() method. Even more, I think that you can change its first parameter from std::shared_ptr<zim::Archive> to a bookId and move more code into that method, allowing it to throw if the ZIM archive cannot be obtained, otherwise it should return a (possibly invalid) QIcon object.

veloman-yunkan · 2024-06-25T08:02:00Z

src/readinglistbar.cpp

        std::shared_ptr<zim::Archive> archive;
        const QString& title = QString::fromStdString(bookmark.getTitle());
        try {
            archive = library->getArchive(QString::fromStdString(bookmark.getBookId()));
        } catch (std::out_of_range& e) {
+            /* There can be bookmarks whose information is lost. */
+            if (!bookmark.getBookId().empty())


Let's try not to hide any invalid bookmarks. But we can display them in a special way.

Note that if you add all bookmarks to the reading list, you won't need the extra idx parameter in ReadingListBar::addItem()

veloman-yunkan · 2024-06-25T08:08:00Z

src/tabbar.h

+struct BookInfo 
+{
+    QString name = "N/A";
+    QString path = "N/A";
+
+    BookInfo() {};
+    BookInfo(const QString &name, const QString &path)
+        : name{name}, path{path} {};
+    BookInfo(const QJsonObject &json)
+        : name{json.value("name").toString()},
+          path{json.value("path").toString()} {};
+
+    QJsonObject toJson() const { return {{"name", name}, {"path", path}}; };
+};


When you were referring to the complicated solution I thought that you meant the last version (that had some issues because of directory monitoring) before we decided to split the PR. Sorry.

Since we cannot change the behavior of directory monitoring, we can:

simply save all the books that we have seen in updateFromDir, no matter if we will be removing them.

only remove a saved book when all traces such as opened tab and bookmarks of that book is removed.

ShaopengLin · 2024-06-27T06:27:08Z

@veloman-yunkan This solution is a lot simpler than the last one. If you have any other suggestions I am open to try.

veloman-yunkan · 2024-07-04T14:32:25Z

src/kiwixapp.cpp

+void KiwixApp::addRemovedZimBookInfo(const QList<kiwix::Book> &books)
+{
+    QMutexLocker locker(&m_updateBookInfoMutex);
+    auto bookInfoMap = mp_session->value("removedZimBookInfo", QVariantMap{}).toMap();
+    for (auto& book : books)
+    {
+        auto id = QString::fromStdString(book.getId());
+        auto name = QString::fromStdString(book.getName());
+        auto path = QString::fromStdString(book.getPath());
+        bookInfoMap[id] = QJsonObject{{"name", name}, {"path", path}};
+    }
+    mp_session->setValue("removedZimBookInfo", bookInfoMap);
+}
+
+/**
+ * @brief Removes only the book infos that has no trace left in the app.
+ * 
+ */
+void KiwixApp::cleanupRemovedZimBookInfo()
+{
+    QMutexLocker locker(&m_updateBookInfoMutex);
+    auto bookInfoMap = mp_session->value("removedZimBookInfo", QVariantMap{}).toMap();
+    auto existingZimIds = getTabWidget()->getTabZimIds();
+
+    for (const auto& zimId : bookInfoMap.keys())
+    {
+        if (!existingZimIds.contains(zimId))
+            bookInfoMap.remove(zimId);
+    }
+    mp_session->setValue("removedZimBookInfo", bookInfoMap);
+}


To me something seems wrong about saving this information in the session. Please give me some time to come up with a better solution or admit that none exists.

kelson42 · 2024-07-13T11:49:42Z

@ShaopengLin Any feedback?

ShaopengLin · 2024-07-13T12:18:33Z

To me something seems wrong about saving this information in the session. Please give me some time to come up with a better solution or admit that none exists.

We are a little stuck on how to properly design the solution to this problem completely without changing the behaviour of the directory monitoring. I believe veloman is busy with the other issue and hasn't touched on this yet.

@veloman-yunkan @kelson42 This has been stuck for a while. We don't have an easy path to save those information otherwise, since the directory monitoring completely removes the record of zims if it disappears.

I believe a clean solution would likely need us to either create a new file like library.xml to retain the missing zim entries, or change libkiwix to save this information.

ShaopengLin · 2024-07-19T02:56:02Z

@veloman-yunkan This has been stuck for too long. If storing this information in settings or a file doesn't make sense, we can store it in the URL paths. What do you think?

Otherwise, we should simply do what Kelson said, have a simple error message with Zim Id, and then move on to have the bookmark portion finished. Having the bookmark feature I believe is an equally important user experience.

kelson42 · 2024-07-19T05:52:43Z

@ShaopengLin I would like to intervene here as things don't move well and maybe not even in the right direction.

First of all, to me, all the work around bookmarks in that PR is unclear: what is tried to be achieved and what is the chosen approach. One reason is that there is no proper issue describing the problem and therefore no formal validation about both problem and solution approach.

Regarding the topic of "invalid" bookmarks, to me this is not a problem because:

Bookmarks refering to a ZIM which is not in the local library should never be displayed
All the bookmark handling should be done in the libkiwix (not in Kiwix Desktop)

For all these reasons, please:

Keep only the first part of your PR here (about missing page error)
Open an issue describing clearly the problem around the bookmark so we can discuss it independently of any PR

veloman-yunkan · 2024-07-19T07:24:06Z

@veloman-yunkan This has been stuck for too long. If storing this information in settings or a file doesn't make sense, we can store it in the URL paths. What do you think?

@ShaopengLin I still think that the approach outlined in #1120 (comment) is the best. A proper implementation of it would require moving the directory monitoring functionality from Library to ContentManager - the former doesn't have any knowledge of a book being opened in a tab while the latter does. Then ContentManager will be in charge of deciding how to handle a disappeared ZIM file - if it is of special interest to the user (e.g. was open in a tab, was bookmarked, etc) then it will be preserved in the library but marked as missing, otherwise it will be simply dropped from the library.

If you think it's too complex or get stuck while pursuing that solution please let me know and I will implement it.

ShaopengLin · 2024-07-20T02:55:56Z

@veloman-yunkan Kelson has mentioned this before:

There are many valid reasons why the directory monitoring can remove a file from the library and all of them are external to Kiwix. We have no control on this. The rest must follow.

Doesn't this violate our solution to keep the book in library.xml? This was the reason I was not going with the solution from #1120 (comment) for this PR in the first place.

veloman-yunkan · 2024-07-22T09:30:46Z

@veloman-yunkan Kelson has mentioned this before:

There are many valid reasons why the directory monitoring can remove a file from the library and all of them are external to Kiwix. We have no control on this. The rest must follow.

Doesn't this violate our solution to keep the book in library.xml? This was the reason I was not going with the solution from #1120 (comment) for this PR in the first place.

If we don't consider the case of library.xml being edited manually (or otherwise externally to kiwix-desktop), then the contents of library.xml is fully under the control of kiwix-desktop and it can manage the contents thereof with respect to directory monitoring any way we like.

I propose that I implement the said change in library monitoring as a separate PR whereupon you can rebase and test your PR on top of my branch.

ShaopengLin · 2024-07-26T08:10:06Z

Please separate the change addressing the subtlety with directory monitoring into a separate commit. Then this PR will be complete.

I assume you meant the setupDirectoryMonitoring subtlety

veloman-yunkan · 2024-07-26T09:02:26Z

Please separate the change addressing the subtlety with directory monitoring into a separate commit. Then this PR will be complete.

I assume you meant the setupDirectoryMonitoring subtlety

No, I meant everything related to directory monitoring. Suppose that you implement the more informative file-not-found error page with directory monitoring disabled. Then you find out that turning directory monitoring on leads to certain issues. I want those issues to be addressed in a separate commit (with a proper explanation).

ShaopengLin · 2024-07-27T02:19:11Z

@veloman-yunkan
There are two situations where external modification to the library due to directory monitoring is problematic:

If the library file(s) are removed or modified, our code for file-not-found page can fault due to the library losing book information. This is, however already mitigated with the try/catch when we are unable to find the book.
Existing functions may rely on the previous monitoring behaviour to validate whether the zim file physically exist. The getValidBookById resolves this issue. This function retains the previous behaviour of getBookById, and should be used if we want to get a book that is physically present. Could you do a sanity check on the getBookById calls I left out or replaced to make sure I didn't make any misconception of the intentions of getBookById? Thanks!

veloman-yunkan

@veloman-yunkan There are two situations where external modification to the library due to directory monitoring is problematic:

If the library file(s) are removed or modified, our code for file-not-found page can fault due to the library losing book information. This is, however already mitigated with the try/catch when we are unable to find the book.

Existing functions may rely on the previous monitoring behaviour to validate whether the zim file physically exist. The getValidBookById resolves this issue. This function retains the previous behaviour of getBookById, and should be used if we want to get a book that is physically present. Could you do a sanity check on the getBookById calls I left out or replaced to make sure I didn't make any misconception of the intentions of getBookById? Thanks!

Let's not let this PR's scope drift. The purpose of this PR is to improve the information displayed in the file-not-found error page.

Existing functionality does not rely on previous behaviour of directory monitoring. Directory monitoring was added relatively recently and not all of its side effects were fully understood and addressed. Let's not go after potential issues that have been introduced due to directory monitoring in the past. Just make sure that your enhancement

works in the absence of directory monitoring in the first place and
also works when directory monitoring is enabled (changes targeting this part should come in a separate commit).

kelson42 · 2024-07-28T10:46:49Z

#1149 has been merged, so if I get it right, after a rebase it should be possible tom omplete this PR!?

veloman-yunkan

Almost there. Please fix the last two comments and also format your commit messages to meet the commit message guidelines

veloman-yunkan · 2024-07-28T19:04:15Z

src/tabbar.h

@@ -50,6 +50,7 @@ class TabBar : public QTabBar
    void openFindInPageBar();
    void closeTabsByZimId(const QString &id);
    QStringList getTabUrls() const;
+    QStringList getTabZimIds() const;


This function should be introduced in the next commit.

veloman-yunkan · 2024-07-28T19:09:14Z

src/urlschemehandler.cpp

-    auto& book = KiwixApp::instance()->getLibrary()->getBookById(zimId);
-    QString path = QString::fromStdString(book.getPath());
-    QString name = QString::fromStdString(book.getName());
+    QString path = "N/A", name = "N/A";
+    try 
+    {
+        auto& book = KiwixApp::instance()->getLibrary()->getBookById(zimId);
+        path = QString::fromStdString(book.getPath());
+        name = QString::fromStdString(book.getName());
+    }
+    catch (...) { /* Blank */ }
+


I think that this change is unrelated to directory monitoring - it can be merged into the previous commit.

Page is displayed when zim file is missing.

kelson42 added this to the 2.4.0 milestone Jun 17, 2024

ShaopengLin force-pushed the Issue#1073-file-not-found-page branch from 9c39cc2 to 58ac238 Compare June 17, 2024 17:01

ShaopengLin requested a review from veloman-yunkan June 17, 2024 17:10

veloman-yunkan requested changes Jun 20, 2024

View reviewed changes

ShaopengLin mentioned this pull request Jun 21, 2024

Implemented Save Page #1134

Merged

ShaopengLin force-pushed the Issue#1073-file-not-found-page branch 2 times, most recently from c482195 to dac9002 Compare June 21, 2024 06:04

ShaopengLin requested a review from veloman-yunkan June 21, 2024 06:20

ShaopengLin force-pushed the Issue#1073-file-not-found-page branch from dac9002 to 2639e5d Compare June 21, 2024 15:01

veloman-yunkan requested changes Jun 25, 2024

View reviewed changes

ShaopengLin force-pushed the Issue#1073-file-not-found-page branch 3 times, most recently from 992fd2b to bf13512 Compare June 27, 2024 06:23

ShaopengLin requested a review from veloman-yunkan June 27, 2024 06:26

veloman-yunkan reviewed Jul 4, 2024

View reviewed changes

veloman-yunkan mentioned this pull request Jul 22, 2024

Directory monitoring improvements #1149

Merged

ShaopengLin force-pushed the Issue#1073-file-not-found-page branch from 68440c5 to 37146c7 Compare July 26, 2024 08:05

ShaopengLin requested a review from veloman-yunkan July 26, 2024 08:08

veloman-yunkan force-pushed the directory_monitoring_improvements branch from 000e918 to 32277ba Compare July 26, 2024 08:53

ShaopengLin closed this Jul 27, 2024

ShaopengLin force-pushed the Issue#1073-file-not-found-page branch from 37146c7 to 32277ba Compare July 27, 2024 00:47

ShaopengLin reopened this Jul 27, 2024

veloman-yunkan reviewed Jul 28, 2024

View reviewed changes

kelson42 force-pushed the directory_monitoring_improvements branch from 32277ba to deb91e7 Compare July 28, 2024 10:31

Base automatically changed from directory_monitoring_improvements to main July 28, 2024 10:45

ShaopengLin force-pushed the Issue#1073-file-not-found-page branch from c358223 to 9b56ced Compare July 28, 2024 16:53

ShaopengLin closed this Jul 28, 2024

ShaopengLin force-pushed the Issue#1073-file-not-found-page branch from 9b56ced to 6ff18a8 Compare July 28, 2024 16:57

ShaopengLin reopened this Jul 28, 2024

ShaopengLin requested a review from veloman-yunkan July 28, 2024 17:12

veloman-yunkan requested changes Jul 28, 2024

View reviewed changes

ShaopengLin force-pushed the Issue#1073-file-not-found-page branch from e38ba4f to 660e9ea Compare July 28, 2024 19:38

ShaopengLin added 3 commits July 28, 2024 15:38

Added custom file-not-found page

368fafb

Page is displayed when zim file is missing.

File-not-found tab now displays zim name and path

17702f8

Directory monitoring does not removes zims opened

34a4a9c

ShaopengLin force-pushed the Issue#1073-file-not-found-page branch from 660e9ea to 34a4a9c Compare July 28, 2024 19:41

ShaopengLin requested a review from veloman-yunkan July 28, 2024 19:42

veloman-yunkan approved these changes Jul 28, 2024

View reviewed changes

kelson42 merged commit 319f9e5 into main Jul 28, 2024
4 checks passed

kelson42 deleted the Issue#1073-file-not-found-page branch July 28, 2024 20:37

veloman-yunkan mentioned this pull request Aug 19, 2024

Handling of invalid ZIM files #1174

Merged

kelson42 mentioned this pull request Sep 29, 2024

Add ipv6 support to local server #1221

Merged

Added Custom File-Not-Found page for Tabs #1132

Added Custom File-Not-Found page for Tabs #1132

Conversation

ShaopengLin commented Jun 16, 2024

ShaopengLin commented Jun 16, 2024

veloman-yunkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaopengLin commented Jun 20, 2024

veloman-yunkan commented Jun 20, 2024

ShaopengLin commented Jun 20, 2024

ShaopengLin commented Jun 20, 2024

veloman-yunkan commented Jun 20, 2024 • edited Loading

veloman-yunkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaopengLin commented Jun 27, 2024

Choose a reason for hiding this comment

kelson42 commented Jul 13, 2024

ShaopengLin commented Jul 13, 2024

ShaopengLin commented Jul 19, 2024

kelson42 commented Jul 19, 2024

veloman-yunkan commented Jul 19, 2024

ShaopengLin commented Jul 20, 2024

veloman-yunkan commented Jul 22, 2024

ShaopengLin commented Jul 26, 2024

veloman-yunkan commented Jul 26, 2024

ShaopengLin commented Jul 27, 2024

veloman-yunkan left a comment

Choose a reason for hiding this comment

kelson42 commented Jul 28, 2024

veloman-yunkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veloman-yunkan commented Jun 20, 2024 •

edited

Loading