Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Jul 28, 2024

Conversation

ShaopengLin
Copy link
Collaborator

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
Copy link
Collaborator Author

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

@kelson42 kelson42 added this to the 2.4.0 milestone Jun 17, 2024
@ShaopengLin ShaopengLin force-pushed the Issue#1073-file-not-found-page branch from 9c39cc2 to 58ac238 Compare June 17, 2024 17:01
Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

resources/i18n/en.json Outdated Show resolved Hide resolved
resources/i18n/en.json Outdated Show resolved Hide resolved
src/readinglistbar.cpp Outdated Show resolved Hide resolved
src/readinglistbar.cpp Outdated Show resolved Hide resolved
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())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible for a bookmark to lack a bookId?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@veloman-yunkan
Copy link
Collaborator

veloman-yunkan commented Jun 20, 2024

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.

@ShaopengLin 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 ShaopengLin force-pushed the Issue#1073-file-not-found-page branch from dac9002 to 2639e5d Compare June 21, 2024 15:01
Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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).
  2. Listing all bookmarks (including invalid ones)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are the two nested <div>s needed?

src/webview.cpp Outdated
Comment on lines 183 to 193
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) {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

src/tabbar.h Outdated
Comment on lines 15 to 28
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}}; };
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. simply save all the books that we have seen in updateFromDir, no matter if we will be removing them.
  2. only remove a saved book when all traces such as opened tab and bookmarks of that book is removed.

@ShaopengLin 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
Copy link
Collaborator Author

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

src/kiwixapp.cpp Outdated
Comment on lines 596 to 626
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);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

@ShaopengLin Any feedback?

@ShaopengLin
Copy link
Collaborator Author

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
Copy link
Collaborator Author

@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
Copy link
Collaborator

@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
Copy link
Collaborator

@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
Copy link
Collaborator Author

@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
Copy link
Collaborator

@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
Copy link
Collaborator Author

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 veloman-yunkan force-pushed the directory_monitoring_improvements branch from 000e918 to 32277ba Compare July 26, 2024 08:53
@veloman-yunkan
Copy link
Collaborator

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 ShaopengLin force-pushed the Issue#1073-file-not-found-page branch from 37146c7 to 32277ba Compare July 27, 2024 00:47
@ShaopengLin ShaopengLin reopened this Jul 27, 2024
@ShaopengLin
Copy link
Collaborator Author

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

  1. 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.

  2. 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!

Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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.

  2. 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

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

@kelson42 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
@kelson42
Copy link
Collaborator

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

Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function should be introduced in the next commit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 184 to 192
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 */ }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@ShaopengLin ShaopengLin force-pushed the Issue#1073-file-not-found-page branch from e38ba4f to 660e9ea Compare July 28, 2024 19:38
@kelson42 kelson42 merged commit 319f9e5 into main Jul 28, 2024
4 checks passed
@kelson42 kelson42 deleted the Issue#1073-file-not-found-page branch July 28, 2024 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: DONE
Development

Successfully merging this pull request may close these issues.

Remember current tab from previous user session
3 participants