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

Smarter Downloader::startDownload() #1066

Merged
merged 4 commits into from
Mar 20, 2024
Merged

Smarter Downloader::startDownload() #1066

merged 4 commits into from
Mar 20, 2024

Conversation

veloman-yunkan
Copy link
Collaborator

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.

Copy link

codecov bot commented Feb 27, 2024

Codecov Report

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

Project coverage is 40.93%. Comparing base (922c138) to head (3733e50).
Report is 69 commits behind head on main.

Files with missing lines Patch % Lines
src/downloader.cpp 0.00% 15 Missing ⚠️
Additional details and impacted files
@@            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              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mgautierfr
Copy link
Member

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

@mgautierfr
Copy link
Member

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

@veloman-yunkan
Copy link
Collaborator Author

@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.

@veloman-yunkan veloman-yunkan force-pushed the smarter_startDownload branch from cc566c3 to 3733e50 Compare March 20, 2024 07:26
@mgautierfr
Copy link
Member

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

@mgautierfr mgautierfr merged commit 8009edd into main Mar 20, 2024
15 of 16 checks passed
@mgautierfr mgautierfr deleted the smarter_startDownload branch March 20, 2024 09:52
@mgautierfr
Copy link
Member

Fixes kiwix/kiwix-desktop#1022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants