search

kiwix / libkiwix

Common code base for all Kiwix ports
https://download.kiwix.org/release/libkiwix/
GNU General Public License v3.0
112 stars 54 forks source link

Smarter Downloader::startDownload() #1066

Closed veloman-yunkan closed 3 months ago

veloman-yunkan commented 4 months ago

This PR is an alternative to #1052. Similar to the latter it addresses the user-observable problems behind #1050 without fixing that issue the way it was formulated.

The shared property of #1052 and this PR is that they try to solve the bugs without enhancing the libkiwix API. The difference is that this PR tries to make Downloader::startDownload() smarter, whereas #1052 deprived it of the slightest smartness that it tried to exercise.

Now having tried both approaches I think that we better follow the dumb path. Although we can close the seemingly small gap in downloadCanBeReused() (marked with an XXX comment) my feeling is that trying to optimize for a theoretical situation that can hardly happen in practice is not justified.

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 0% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 40.93%. Comparing base (922c138) to head (3733e50).

Files Patch % Lines
src/downloader.cpp 0.00% 15 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1066 +/- ## ========================================== - Coverage 41.07% 40.93% -0.14% ========================================== Files 58 58 Lines 4190 4204 +14 Branches 2304 2307 +3 ========================================== Hits 1721 1721 - Misses 1001 1015 +14 Partials 1468 1468 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mgautierfr commented 4 months ago

It is a technical approval. Maybe we need a extra step of discussion if this is a better solution than #1052 (or not)

mgautierfr commented 3 months ago

@veloman-yunkan Do we agree to merge this solution ?

veloman-yunkan commented 3 months ago

@veloman-yunkan Do we agree to merge this solution ?

I think that it may be the best way to proceed. Although my intuition tells me that we may eventually run into other (more subtle) issues connected with this approach, I don't see why we should try to absolutely avoid them, since we can easily switch to the dumb solution at any time.

mgautierfr commented 3 months ago

Let's go with this solution then. If we face some bug with this, we will see then what to do.

mgautierfr commented 3 months ago

Fixes https://github.com/kiwix/kiwix-desktop/issues/1022