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

A couple of bugfixes in Download management #1024

Merged
merged 21 commits into from
Dec 14, 2023
Merged

Conversation

veloman-yunkan
Copy link
Collaborator

Fixes #1021
Fixes #1023

This PR also contains some attempts at cleaning up the code, however the conclusion is that a major overhaul is required.

Overall it looks like there are many more bugs related to Download management, and the proper way to eliminate them all is to significantly simplify the code architecture through a more direct usage of Kiwix::Download.

So far the details of the host serving the OPDS catalog were hardcoded.
In order to debug issues related to large downloads it helps if one
can use one's own server. Hence this enhancement.
The static function RowNode::createNode() was moved into
ContentManagerModel as is.
Download update timers are no-longer managed separately in
ContentManagerModel. Instead the timer associated with each active
download is owned by the respective RowNode object.
Now DownloadState is an optional element of the state of RowNode
and is set in it only in case of an active (even if paused) download
associated with the that entry.
This is needed for preserving download state across filtering and
similar operations that currently work by fully rebuilding the
ContentManagerModel.
This fixes the bugs related to pending download statuses being discarded
by filtering and similar operations that result in full update of the
ContentManagerModel.

Instead this commit leads to a new bug - completion of a download
is not always properly detected for very small (and thus instantaneous)
downloads. This is caused by library update events triggered by ZIM
directory monitoring (which notifies about a new ZIM file appearing
in the download directory) interfering with the download status update
signal.
It's more logical to check for the conditions of a disappeared download
first.
This fix is rather a temporary and ugly hack, since the current mess
with how download states are being maintained (in the presence of
multiple sources of their updates) asks for a major clean-up.
The diff is simpler if whitespace changes are ignored.
@mgautierfr
Copy link
Member

Nice changes !

Overall it looks like there are many more bugs related to Download management, and the proper way to eliminate them all is to significantly simplify the code architecture through a more direct usage of Kiwix::Download.

I strongly agree. I would go even further. I think we could probably drop the whole ContentManager.

At the time, The library view was implemented in html/js and it was discussing with the c++ part through a QWebChannel. The ContentManager was the C++ endpoint to pass value in the channel. But now #946 is merged, we don't need it anymore. Node can directly call the library to get the information. Even more, we can have the Library itself implement QAbstractItemModel and directly have the view displaying the Library. And if we go with the idea of the implementation of a RemoteLibrary class (#957 (comment)) we could also make it implement QAbstractItemModel and simply display the remote library too.

@mgautierfr mgautierfr merged commit 2d49d79 into main Dec 14, 2023
4 checks passed
@mgautierfr mgautierfr deleted the download_bugfixes branch December 14, 2023 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants