Add check for fileName in eraseBookFilesFromComputer

[juuz0]

Fix #809

[juuz0]

This change adds the check which should fix the problem reported but I don't understand why is there a need to add * in ContentManager::eraseBook, this line:
QString fileName = QString::fromStdString(kiwix::getLastPathElement(book.getPath())) + "*";

[kelson42]

@juuz0 Thank you for the super quick patch. I suspect this is needed to delete all files in case of a splitted ZIM file.

[kelson42]

@juuz0 The bug to me is more that under certain circunstances filename is empty string! Why this is the case? I suspect this is because the download has failed, but in that case we should think about what is the best place to handle it? For the moment, I believe it would be better to just not call eraseBookFilesFromComputer in case of book.getPath() returning nothing.

[juuz0]

The bug to me is more that under certain circunstances filename is empty string! Why this is the case?

Not a clear idea but as you mentioned probably because the download has failed and this book is not added in library then?

For the moment, I believe it would be better to just not call eraseBookFilesFromComputer in case of book.getPath() returning nothing.

I considered this but eraseBookFilesFromComputer is also used in cancelBook so to have a safeguard at both places, I went with this.
As an alternative, I feel just not calling ContentManager::eraseBook on a download fail should work without any other changes because for failed downloads, book.getPath() (assumption) will always return an empty string?
Changing this in _contentManager.js:

else if (d.status == "error") {
      clearInterval(downloadUpdaters[id]);
      Vue.delete(app.downloads, id);
      alert("Error: download failed.");
      contentManager.eraseBook(id); <------ remove this
      return;
    }

[kelson42]

The bug to me is more that under certain circunstances filename is empty string! Why this is the case?

Not a clear idea but as you mentioned probably because the download has failed and this book is not added in library then?

We can leave it like it is then, if this seems legit to call

For the moment, I believe it would be better to just not call eraseBookFilesFromComputer in case of book.getPath() returning nothing.

I considered this but eraseBookFilesFromComputer is also used in cancelBook so to have a safeguard at both places, I went with this. As an alternative, I feel just not calling ContentManager::eraseBook on a download fail should work without any other changes because for failed downloads, book.getPath() (assumption) will always return an empty string? Changing this in _contentManager.js:

else if (d.status == "error") {
      clearInterval(downloadUpdaters[id]);
      Vue.delete(app.downloads, id);
      alert("Error: download failed.");
      contentManager.eraseBook(id); <------ remove this
      return;
    }

There is quite a few things to say here IMO:

• First your download can have error after having started to save data on the filesystem, in that case you have to clean it.
• cancelBook and eraseBook are in a way quite similar and share similarities. Not sure we have the best code factoring here...
• Neither the best method naming...
• eraseBookFilesFromComputer() is a tool, you should let him do his job... if something does not use it right it not its fault. The safeguard is anyway pretty weak, think about *.* for example.
• If the download has an error (so download is not completed), I don't understand anyway why eraseBook() is called and not cancelBook()!

Actually quite a few things are NOK to me. If all my remarks bother you ;) then just leave it, will try to propose an other PR later this week.

[WildLynxDev]

I think doing file deletions with * is a bad thing.
There is so many ways it could go rogue, not only with empty book name.
Any software that is not a file manager should remove own files only by exact name. There were a lot of stories about uninstallers clearing disk C because of some directory walking algorithm gone nuts.

[mgautierfr]

book.getPath() (assumption) will always return an empty string?

The path of the book is set once the download is finished (https://github.com/kiwix/kiwix-desktop/blob/master/src/contentmanager.cpp#L165). Before that, the book is a copy of what is the remote library and, obviously, it hasn't a (local) path.
So yes, for a failed or canceled download, getPath() is always empty.

We probably have to:

• The the path of the book as soon as we start the download.
• The the path valid (https://github.com/kiwix/libkiwix/blob/master/include/book.h#L111) when the download succeed.
• Use the signature I propose for eraseBookFilesFromComputer to make the method take a Book reference.
• Add a check in the method to be sur that book.getPath() is not empty.
• Do not use * globing. We have to remove the path of the book. If there is something else to remove (meta4 file ?) we know it and we must try to remove it only.
• For split zim file, we may try to compose what would be the filenames (.zimaa, .zimab, ...) and search for them. But I'm not sure about this. kiwix-desktop don't use split files, so if the zim file is split, it is a zim file added by the user, and I'm in favor of not remove it (Deleting a ZIM file should move it to the trash can #479 (comment))

[mgautierfr]

It is probably to eraseBookFilesFromComputer to split the bookPath and erase book properly.

The signature of the method should probably be:
bool eraseBookFilesFromComputer(const Book& book)

[mgautierfr]

This fixed issue is really important.
I prefer a proper solution to this PR, but it does the work.
I will merge this PR as it is and make a 2.2.1 release.
Please @juuz0, continue to work on the issue to have a better fix.

[juuz0]

👍 I will follow all inputs above