Download management fixes and improvements #1038

veloman-yunkan · 2024-02-12T15:37:49Z

Should fix #87, #1037

This PR contains a significant amount of changes related to download management and fixes various bugs discovered while performing those (refactoring) changes.

Download progress/status updates are performed in a separate worker thread
Thumbnails of remote books are downloaded lazily
Running and/or paused downloads are restored after kiwix-desktop is closed and started again
Fixed issues coming from the monitoring of the download directory interfering with download state lifecycle
Eliminated a few crash scenarios

mgautierfr

Glade you attack this part.
The content manager is a bit (to not say a lot) clumsy.

I don't know if you have the full history but here it is:

The view was first implemented using a webview. So we have to implement a ContentManager exchanging information with the webview using serializable data (hence all the keys passing)
Then we reimplement the view using Qt. But instead of recreating from scratch we "only" have reimplement the view using the ContentManager (and creating a ContentManagerModel as Qt views are base on model)

On the long run, I think we should drop all of this and directly have the view using Libraries and downloader (potentially making them implement the Model interface).

src/contentmanager.cpp

mgautierfr · 2024-02-19T14:34:37Z

src/contentmanager.cpp

-                              gt("download-storage-error-text"));
+    QStorageInfo storage(targetDir);
+    auto bytesAvailable = storage.bytesAvailable();
+    if (bytesAvailable == -1 || book.getSize() > (unsigned long long) bytesAvailable) {


This -1 test could be the root cause of #950

Most likely yes. During testing I trigger the "Not enough storage available" error by pointing the download directory to a non-existent directory.

src/contentmanager.cpp

veloman-yunkan · 2024-02-20T17:03:07Z

On the long run, I think we should drop all of this and directly have the view using Libraries and downloader (potentially making them implement the Model interface).

The role of ContentManagerModel in the current design doesn't make the proposed path look viable. It seems that we have to have a separate object (a model) for the filtered library.

veloman-yunkan · 2024-02-20T17:09:17Z

@mgautierfr Thanks for the review of this WIP PR. Since it keeps growing larger and starts involving more functionality than just download management I am most concerned about agreeing on the strategy for bringing these changes into the main branch - should we do it as several smaller PRs or one big PR is fine?

mgautierfr · 2024-02-21T10:47:27Z

should we do it as several smaller PRs or one big PR is fine?

Please do several smaller PRs. First commits of this PR, with a lot of Move code doing FOO, to "doFOO" methods can almost be a small PR in themselves.

veloman-yunkan · 2024-02-22T14:07:50Z

should we do it as several smaller PRs or one big PR is fine?

Please do several smaller PRs. First commits of this PR, with a lot of Move code doing FOO, to "doFOO" methods can almost be a small PR in themselves.

#1041 is ready. More to come.

veloman-yunkan · 2024-02-27T11:49:47Z

should we do it as several smaller PRs or one big PR is fine?

Please do several smaller PRs. First commits of this PR, with a lot of Move code doing FOO, to "doFOO" methods can almost be a small PR in themselves.

#1041 is ready. More to come.

#1045 has arrived.

Also got rid of `ContentManagerModel::setupNodes()` by expanding it into `ContentManagerModel::setBooksData()`.

Eliminated an instance of download removal handling where the conceptual actions of ContentManager::removeDownload() were performed without calling the latter.

Renamed one of the two overloads of ContentManager::cancelBook() to reallyCancelBook().

... by inlining them into the used ones

Note that this change drops the protection against accidentally removing all files in a book's directory. The risk of a such a destructive operation is still present if an invalid path is passed into `eraseBookFilesFromComputer()` but that will be addressed in a separate commit.

My experience is that `kiwix::Download::getPath()` may return an empty string (at least for a fresh download object and probably before Aria2 actually starts downloading and creates the target file). Passing such an empty string into `eraseBookFilesFromComputer()` might have disastrous consequences. Now that function should be safer.

... that can be called from a worker thread.

The failure of `ContentManager::getDownloadInfo()` is now reported via an exception instead of a special return value.

... in preparation for converting `ContentManagerModel::Downloads` into a thread-safe class.

... only one small step away from being thread-safe

Before this change `ContentManager::reallyCancelBook()` could throw (and thus result in a crash) if the confirmation to cancel the download was timed to perform the actual download cancellation on a completed download. This change fixes that problem. Besides it also fixes a subtle issue where `ContentManager::reallyCancelBook()` is called on a completed download and has the effect of removing the downloaded book from the local library. I considered preserving that dubious behaviour, but it couldn't be achieved in a simple and consistent fashion (because of the two ways an actually completed download can be seen by the `ContentManager::reallyCancelBook()` function).

New ZIM files discovered in monitored directories are not added to the library if they are known to be in the process of being downloaded by kiwix-desktop. That information is recorded in the library as follows. When a download is started the book is added to the local library with a fake path that has a special suffix (".beingdownloadedbykiwix") appended to the path of the ZIM file (previously, a book for which a download was initiated had only its download id set and the path was added only upon completion of the download). Note that the expected path of the ZIM file is derived from the download directory and the URL rather than obtained via `kiwix::Download::getPath()` since the latter seems to return an empty string until the download is actually started (it may stay queued if the count of active downloads exceeds the queue size), the file is created and `kiwix::Download::updateStatus()` is called after that which is too late for our purposes (we want the information about a new download to appear in the library before directory monitoring detects creation of the file). Known issues: - If kiwix-desktop is closed while a download is still in progress (either active or paused) then after reopening the app the information about the download state is not shown in the UI and attempting to download that file fails. However this problem is present before this commit too (though in a slightly different way).

Now the downloader paused/active state in UI is set by the updater thread. This will make it easier to restore downloads after closing and re-opening kiwix-desktop.

Downloads that were active during the application shutdown automatically resume. Known issues: - For paused downloads the progress information is lost (but is recovered when the download is resumed).

veloman-yunkan · 2024-04-05T09:09:52Z

LGTM. Please rebase/fixup before final approval.

Done. The description of the PR was updated.

veloman-yunkan requested a review from mgautierfr February 12, 2024 15:37

veloman-yunkan force-pushed the download_management_refactoring branch from 727c78a to 23d7c01 Compare February 13, 2024 14:00

Base automatically changed from more_reliable_thumbnail_loading to main February 14, 2024 11:00

veloman-yunkan force-pushed the download_management_refactoring branch 3 times, most recently from 8f914e2 to d4a698c Compare February 20, 2024 13:57

mgautierfr reviewed Feb 20, 2024

View reviewed changes

veloman-yunkan changed the base branch from main to refactoring February 22, 2024 13:59

veloman-yunkan mentioned this pull request Feb 22, 2024

Some cleanup of download management related code in ContentManager #1041

Merged

Base automatically changed from refactoring to main February 26, 2024 13:02

veloman-yunkan mentioned this pull request Feb 27, 2024

More cleanup of download management related code in ContentManager #1045

Merged

veloman-yunkan force-pushed the download_management_refactoring branch from 617991e to 59c2f0c Compare February 27, 2024 11:44

veloman-yunkan changed the base branch from main to refactoring February 27, 2024 11:44

veloman-yunkan force-pushed the refactoring branch from c03e0de to 5d28f8a Compare March 1, 2024 16:42

veloman-yunkan force-pushed the download_management_refactoring branch from 75e2949 to 5d667c6 Compare March 1, 2024 16:42

Base automatically changed from refactoring to main March 7, 2024 12:56

mgautierfr mentioned this pull request Mar 7, 2024

Give more information about the "Not enough space storage" message. #959

Closed

veloman-yunkan force-pushed the download_management_refactoring branch 3 times, most recently from 04b91fd to 5c00ef1 Compare March 7, 2024 16:33

veloman-yunkan changed the base branch from main to refactoring March 7, 2024 16:33

veloman-yunkan force-pushed the download_management_refactoring branch from 5c00ef1 to ef58bf9 Compare March 7, 2024 16:40

veloman-yunkan force-pushed the refactoring branch from 3d192c6 to cb3bc65 Compare March 7, 2024 16:40

veloman-yunkan mentioned this pull request Mar 7, 2024

Light refactoring in ContentManager and ContentManagerModel #1049

Merged

Base automatically changed from refactoring to main March 8, 2024 14:48

veloman-yunkan added 20 commits April 5, 2024 12:14

Got rid of ContentManagerModel::m_data

3c6c155

Also got rid of `ContentManagerModel::setupNodes()` by expanding it into `ContentManagerModel::setBooksData()`.

Enforced ContentManager::removeDownload()'s monopoly

595973b

Eliminated an instance of download removal handling where the conceptual actions of ContentManager::removeDownload() were performed without calling the latter.

De-overloaded ContentManager::cancelBook()

c6d892a

Renamed one of the two overloads of ContentManager::cancelBook() to reallyCancelBook().

Dropped an unused parameter

8073593

Got rid of two unused public slots

c8bd038

... by inlining them into the used ones

ContentManagerModel::triggerDataUpdateAt()

8969d9a

Preparing to make updateDownload() a slot

32e3c74

... that can be called from a worker thread.

ContentManager::updateDownload() became a slot

b6a8691

ContentManager::updateDownloads() runs in a separate thread

5340bc4

ContentManager::getDownloadInfo() may throw

de296ed

The failure of `ContentManager::getDownloadInfo()` is now reported via an exception instead of a special return value.

Reduced the in-use API of ContentManagerModel::Downloads

728a8a4

... in preparation for converting `ContentManagerModel::Downloads` into a thread-safe class.

ContentManagerModel::Downloads became a class

c042532

... only one small step away from being thread-safe

Thread-safe ContentManagerModel::Downloads

52b568d

Safer ContentManager::pauseBook()

fa418a6

Download paused state in UI set by updater thread

550604b

Now the downloader paused/active state in UI is set by the updater thread. This will make it easier to restore downloads after closing and re-opening kiwix-desktop.

Incomplete downloads are restored on startup

54881f7

Downloads that were active during the application shutdown automatically resume. Known issues: - For paused downloads the progress information is lost (but is recovered when the download is resumed).

veloman-yunkan force-pushed the download_management_refactoring branch from e0c8a18 to 54881f7 Compare April 5, 2024 08:44

veloman-yunkan changed the title ~~[WIP] Download management refactoring~~ Download management improvements Apr 5, 2024

veloman-yunkan changed the title ~~Download management improvements~~ Download management fixes and improvements Apr 5, 2024

veloman-yunkan marked this pull request as ready for review April 5, 2024 09:08

veloman-yunkan requested a review from mgautierfr April 5, 2024 09:08

kelson42 mentioned this pull request Apr 6, 2024

Windows download hang #919

Closed

kelson42 merged commit 2794699 into main Apr 6, 2024
4 checks passed

kelson42 deleted the download_management_refactoring branch April 6, 2024 08:15

kelson42 mentioned this pull request Apr 6, 2024

Windows 11 - Kiwix desktop "not responding" freezing #1037

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Download management fixes and improvements #1038

Download management fixes and improvements #1038

veloman-yunkan commented Feb 12, 2024 •

edited

Loading

mgautierfr left a comment

mgautierfr Feb 19, 2024

veloman-yunkan Feb 20, 2024

veloman-yunkan commented Feb 20, 2024

veloman-yunkan commented Feb 20, 2024

mgautierfr commented Feb 21, 2024

veloman-yunkan commented Feb 22, 2024

veloman-yunkan commented Feb 27, 2024

veloman-yunkan commented Apr 5, 2024

Download management fixes and improvements #1038

Download management fixes and improvements #1038

Conversation

veloman-yunkan commented Feb 12, 2024 • edited Loading

mgautierfr left a comment

Choose a reason for hiding this comment

mgautierfr Feb 19, 2024

Choose a reason for hiding this comment

veloman-yunkan Feb 20, 2024

Choose a reason for hiding this comment

veloman-yunkan commented Feb 20, 2024

veloman-yunkan commented Feb 20, 2024

mgautierfr commented Feb 21, 2024

veloman-yunkan commented Feb 22, 2024

veloman-yunkan commented Feb 27, 2024

veloman-yunkan commented Apr 5, 2024

veloman-yunkan commented Feb 12, 2024 •

edited

Loading