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: Opening the zim file with kiwix fails to open(not showing the copy/move dialog). #4006

Merged
merged 17 commits into from
Oct 1, 2024

Conversation

MohitMaliFtechiz
Copy link
Collaborator

@MohitMaliFtechiz MohitMaliFtechiz commented Sep 23, 2024

Fixes #3941
Fixes #3997

  • The issue occurred because the logic for opening ZIM files was inside the KiwixReaderFragment, and if the user was on any other screen while using the application, this code was not triggered. As a result, the copy/move dialog was not shown. To resolve this, we moved the functionality to the KiwixMainActivity.
  • Fixed: ZIM file not moving/Copying on Android 14 tablet.
    • When opening the ZIM file via file picker or deep linking, we were obtaining the file path using the getZimFileFromUri() method to perform the renaming for the move functionality. However, on Android 14 tablets, the URI was not resolved correctly by this method, leading to a "ZIM file not found" error(This is a separate issue In Samsung tablet of Android 14, downloaded files can not be opened via file picker or deep linking. #4008).
    • To fix this issue, we removed the dependency on the getZimFileFromUri() method in the Play Store variant, as future Android versions may handle URIs differently. We refactored our code so that the copy/move functionality works independently of this method and now works directly with Android's provided URI.

Reference APK https://drive.google.com/file/d/16uwwoKWdF00rwhvAvR5musunKse4bBwD/view?usp=sharing https://drive.google.com/file/d/16uwwoKWdF00rwhvAvR5musunKse4bBwD/view?usp=sharing you can try this APK and confirms the behaviours of this functionality.

Copy link

codecov bot commented Sep 23, 2024

Codecov Report

Attention: Patch coverage is 62.16216% with 14 lines in your changes missing coverage. Please review.

Project coverage is 58.13%. Comparing base (a959978) to head (05eecc2).
Report is 18 commits behind head on main.

Files with missing lines Patch % Lines
...va/org/kiwix/kiwixmobile/main/KiwixMainActivity.kt 73.68% 3 Missing and 2 partials ⚠️
...le/nav/destination/library/LocalLibraryFragment.kt 58.33% 1 Missing and 4 partials ⚠️
...ile/nav/destination/library/CopyMoveFileHandler.kt 33.33% 1 Missing and 3 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #4006      +/-   ##
============================================
+ Coverage     57.86%   58.13%   +0.27%     
+ Complexity     1602     1599       -3     
============================================
  Files           314      314              
  Lines         13109    13083      -26     
  Branches       1666     1659       -7     
============================================
+ Hits           7585     7606      +21     
+ Misses         4417     4373      -44     
+ Partials       1107     1104       -3     
Flag Coverage Δ
58.13% <62.16%> (+0.27%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@kelson42
Copy link
Collaborator

@MohitMaliFtechiz It works! But why my tablet behaves differently than other android 14?

@MohitMaliFtechiz
Copy link
Collaborator Author

@MohitMaliFtechiz It works! But why my tablet behave differently than other Android 14?

@kelson42 Tablet Uris are different than normal phones. I have checked with another real tablet and found the same issue there. These download URIs are different in tablets to make them compatible with scoped storage.

In tablets:
For example, if I open a zim file directly from the opera browser then it returns the URI like this: content://com.opera.browser.DownloadProvider/downloads/beer.stackexchange.com_en_all_2023-05.zim?hash=w4A3vMuc7l1FPQwk23rmbgvfPGJfhcj5FujW37NApdA%3D&mt=application%2Foctet-stream which opera gives with its own file provider.

When I directly open the downloaded zim file from the file manager it gives us this URI: content://com.android.providers.downloads.documents/document/msf%3A1000000057.

In phones:

Chrome Uri: content://media/external/downloads/2825

File Manager Uri: content://com.android.providers.downloads.documents/100059.

@kelson42
Copy link
Collaborator

kelson42 commented Sep 25, 2024

@MohitMaliFtechiz Please test this then for both tablet and phone within the CI. This very super basic should 100% work.

3 scenarios:

  • From Browser
  • From file navigator
  • Using the "+"

In mean obviously: testing in CI

…copy/move dialog).

* The issue occurred because the logic for opening ZIM files was inside the KiwixReaderFragment, and if the user was on any other screen while using the application, this code was not triggered. As a result, the copy/move dialog was not shown. To resolve this, we moved the functionality to the KiwixMainActivity.
* When opening the ZIM file via file picker or deep linking, we were obtaining the file path using the getZimFileFromUri() method to perform the renaming for the move functionality. However, on Android 14 tablets, the URI was not resolved correctly by this method, leading to a "ZIM file not found" error.
* To fix this issue, we removed the dependency on the `getZimFileFromUri()` method in the Play Store variant, as future Android versions may handle URIs differently. We refactored our code so that the copy/move functionality works independently of this method and now works directly with Android's provided URI.
* Improved the test case to remove the file from the emulator to free up device storage, as we do not have access to this file after running the test case, and it will remain in storage.
…y do not require much memory. As a result, we downgraded the RAM for these specific CI environments.
… would sometimes shut down when running multiple test cases in sequence. Previously, we were executing all tests within a single job, which significantly increased the CI execution time. Additionally, there were occasional "insufficient storage" errors in the emulator when reaching the final stages of the CI.
@MohitMaliFtechiz MohitMaliFtechiz marked this pull request as ready for review October 1, 2024 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants