search

kiwix / kiwix-desktop

Kiwix for Windows and GNU/Linux desktops
https://download.kiwix.org/release/kiwix-desktop/
GNU General Public License v3.0
776 stars 101 forks source link

Handling of invalid ZIM files #1174

Closed veloman-yunkan closed 3 months ago

veloman-yunkan commented 3 months ago

Fixes #1128

When trying to display resources from an invalid ZIM file that fail to be retrieved due to problems with the ZIM data, now an HTML error page is returned instead. Note that the resource being accessed may in fact be an image, a CSS or a javascript file and the user may not see the error (though the appearance of the webpage trying to load and use that resource will be affected to various degrees).

To test the fix use the data from https://tmp.kiwix.org/corrupted_zim_crash/ and override the library.xml and kiwix-desktop.session files in your ~/.local/share/kiwix directory with those found in the attached ZIP archive. After kiwix-desktop is started it will contain two tabs:

  1. Home page of the ZIM file - 4 books ("Les misérables Tome I: Fantine", "Histoire de France 1440-1465 (Volume 7/19)", "Anciennes loix des François, conservées dans les coutumes angloises, recueillies par Littleton, Vol. I", and "Lexicon Latinum : $b Universae phraseologiae corpus congestum etc.") will have their thumbnails replaced with a placeholder image because of the inability to load the image from the ZIM file (an HTML error page is instead served). As you can see it is a very weak indication that something is wrong with the ZIM file
  2. The second tab refers to one of those images directly and the error is obvious
veloman-yunkan commented 3 months ago

Rebased on main and addressed review comments of @kelson42 and @ShaopengLin in new commits. @mgautierfr has identified a good point that has to be answered before we merge this PR or decide to change its implementation.

veloman-yunkan commented 3 months ago

@kelson42 I think that you missed the question that had to be answered before merging the PR.

kelson42 commented 3 months ago

@veloman-yunkan mist, was sure there was only one indeed.