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

Fixed: Some EPUB files are not downloading. #3738

Merged
merged 5 commits into from
Jun 7, 2024
Merged

Fixed: Some EPUB files are not downloading. #3738

merged 5 commits into from
Jun 7, 2024

Conversation

MohitMaliFtechiz
Copy link
Collaborator

@MohitMaliFtechiz MohitMaliFtechiz commented Mar 8, 2024

Fixes #3737
Fixes #3453

  • The issue was epub fileName containing the colon ":" in it, that is not supported by most of the fileSystem, that's why it was not creating the file in fileSystem and we were not able to download the epub file.
  • So to fix this we have improved our getDecodedFileName method which returns the fileName of the epub file, here we are removing the colon from fileName if any contains. For this change, we have added the test cases as well for our getDecodedFileName function to test it properly.
  • We also refined our downloadFileFromUrl method. Previously, the generateSequence function was used to create new files with underscores and incremented numbers, anticipating multiple attempts to save the same file. However, since we now save files only once in our storage, this feature is no longer utilized. This enhancement is detailed in issue Fix same picture and pdf saved multiple times #2879.
  • Added epub query in our manifest to properly open epub files in the external application.
Before Fix After Fix
EPubNotOpening.mp4
EPUBOpeningIssueFixed.mp4
screen-20240606-142648.mp4

@MohitMaliFtechiz MohitMaliFtechiz marked this pull request as draft March 8, 2024 11:02
Copy link

codecov bot commented Mar 8, 2024

Codecov Report

Attention: Patch coverage is 17.24138% with 48 lines in your changes missing coverage. Please review.

Project coverage is 53.78%. Comparing base (94feb77) to head (da88313).
Report is 21 commits behind head on main.

Current head da88313 differs from pull request most recent head 13fb613

Please upload reports for the commit 13fb613 to get more accurate results.

Files Patch % Lines
...le/core/utils/dialog/UnsupportedMimeTypeHandler.kt 14.63% 35 Missing ⚠️
...kiwix/kiwixmobile/core/utils/dialog/KiwixDialog.kt 0.00% 6 Missing ⚠️
.../kiwix/kiwixmobile/core/main/CoreReaderFragment.kt 0.00% 3 Missing ⚠️
...g/kiwix/kiwixmobile/core/main/CoreWebViewClient.kt 50.00% 2 Missing ⚠️
...rg/kiwix/kiwixmobile/core/utils/files/FileUtils.kt 33.33% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #3738      +/-   ##
============================================
- Coverage     53.86%   53.78%   -0.08%     
- Complexity     1376     1377       +1     
============================================
  Files           298      299       +1     
  Lines         11308    11333      +25     
  Branches       1495     1495              
============================================
+ Hits           6091     6096       +5     
- Misses         4226     4246      +20     
  Partials        991      991              

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

Copy link
Contributor

@Sagar0-0 Sagar0-0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If any EPUB reader is not present in the device(which is not in most devices by default), then where the Open button will navigate, or what will it popup?

@MohitMaliFtechiz
Copy link
Collaborator Author

@Sagar0-0 It will show the toast error message if no PDF/Epub Reader application is installed on the mobile device.

@kelson42
Copy link
Collaborator

@MohitMaliDeveloper CI Failed again!!!

1 similar comment
@kelson42
Copy link
Collaborator

@MohitMaliDeveloper CI Failed again!!!

@MohitMaliFtechiz
Copy link
Collaborator Author

@kelson42 CI looks stable after merging #3785. CI is green on this PR #3785 as well as on the main branch. Also, CI is green in #3788.

* The issue was epub fileName containing the colon ":" in it that is not supported by most of fileSystem, that's why it is not creating the file in fileSystem and we are not able to download the epub file.
* So to fix this we have improved our `getDecodedFileName` method which returns the fileName of the epub file, here we are removing the colon from fileName if any contains. For this change we have added the test cases as well for our `getDecodedFileName` function to properly test it.
* We also refined our downloadFileFromUrl method. Previously, the generateSequence function was used to create new files with underscores and incremented numbers, anticipating multiple attempts to save the same file. However, since we now save files only once in our storage, this feature is no longer utilized. This enhancement is detailed in issue #2879.
* Added epub query in our manifest to properly open epub files in external application.
* This was requested in #3453 and this PR is related to the Epub opening issue so we are introducing this here.
…e scenarios e.g. "Download Failed", "Download Successfull", "When no reader application is installed in device", "File opens in external reader application", "When user clicks on NO Thanks button".
* Improved the dialog's message and title.
@kelson42 kelson42 merged commit f17e3f9 into main Jun 7, 2024
8 checks passed
@kelson42 kelson42 deleted the Fix#3737 branch June 7, 2024 04:41
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.

Some EPUB files are not downloading. Can't open EPUB from Project Gutenberg in external app
4 participants