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: Online library cannot be retrieved in the Play Store variant. #3994

Merged
merged 16 commits into from
Sep 19, 2024

Conversation

MohitMaliFtechiz
Copy link
Collaborator

@MohitMaliFtechiz MohitMaliFtechiz commented Sep 12, 2024

Fixes #3991

  • The issue arose after upgrading the Gradle version of our project (which introduced Android 14). The new Gradle version includes an updated minify tool that excludes some SimpleXML attributes from the code. As a result, this error occurred. To resolve this, we added specific rules to the ProGuard file to ensure these attributes are retained in the release variant.
  • Added test cases to the CI for testing the minified version of the application to prevent this type of error from happening in the future.
  • Improved the DownloadTest to handle cases where pausing the download may show a complete pause message, such as 'Pause: Waiting for Retry,' which previously caused the test case to fail. The test case has been updated to accommodate this behavior.
  • Fixed the NoSucMethodError in UI test cases see. It was due to AGP silently downgrade to tracing:1.0.0. It is mentioned in the official github repo
09-13 12:28:47.887  4720  4768 E AndroidXTracer: java.lang.NoSuchMethodError: No static method forceEnableAppTracing()V in class Landroidx/tracing/Trace; or its super classes (declaration of 'androidx.tracing.Trace' appears in /data/app/org.kiwix.kiwixmobile-1/base.apk)
  • Improved the fetching and downloading the online library on low bandwidth. Since the online library file is 19MB large and on slow bandwidth devices it takes more time to fetch that file. Our current read timeout is 60 seconds which is not enough for reading this file on slow-bandwidth devices(After reaching the timeout it cancels the ongoing request). So we have increased it to 180 seconds.
java.io.InterruptedIOException: timeout
Caused by: okhttp3.internal.http2.StreamResetException: stream was reset: CANCEL
  • Refactored our code to properly show the progress of downloading the online library.
  • Improved the library loading process: previously, fetching the online library involved making two requests, which not only took more time to get a response from the server but also used twice the bandwidth. To address this, we have refactored our code to ensure that only one request is made at a time.
  • Upgraded the retrofit, and interceptor dependencies to the latest version.
screen-20240918-163719.mp4

Screenshot from 2024-09-18 22-03-27

Any suggestions/improvements are welcome for the UI to show the progress.

* The issue arose after upgrading the Gradle version of our project (which introduced Android 14). The new Gradle version includes an updated minify tool that excludes SimpleXML attributes from the code. As a result, this error occurred. To resolve this, we added specific rules to the ProGuard file to ensure these attributes are retained in the release variant.
@MohitMaliFtechiz MohitMaliFtechiz marked this pull request as draft September 12, 2024 10:55
Copy link

codecov bot commented Sep 12, 2024

Codecov Report

Attention: Patch coverage is 75.19380% with 32 lines in your changes missing coverage. Please review.

Project coverage is 57.94%. Comparing base (e3844e5) to head (b3cff85).
Report is 17 commits behind head on main.

Files with missing lines Patch % Lines
...e/nav/destination/library/OnlineLibraryFragment.kt 62.85% 7 Missing and 6 partials ⚠️
...kiwix/kiwixmobile/zimManager/ZimManageViewModel.kt 81.81% 7 Missing and 5 partials ⚠️
...wixmobile/core/data/remote/ProgressResponseBody.kt 75.00% 2 Missing and 2 partials ⚠️
...ixmobile/zimManager/AppProgressListenerProvider.kt 81.81% 1 Missing and 1 partial ⚠️
...wix/kiwixmobile/core/utils/SharedPreferenceUtil.kt 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #3994      +/-   ##
============================================
+ Coverage     57.61%   57.94%   +0.32%     
- Complexity     1594     1603       +9     
============================================
  Files           312      314       +2     
  Lines         12984    13092     +108     
  Branches       1651     1663      +12     
============================================
+ Hits           7481     7586     +105     
+ Misses         4409     4399      -10     
- Partials       1094     1107      +13     

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

…plication to prevent this type of error from happening in the future.
…may show a complete pause message, such as 'Pause: Waiting for Retry,' which previously caused the test case to fail. The test case has been updated to accommodate this behavior.
… forceEnableAppTracing` error in UI test cases.
* Refactored the viewModel code to show the downloading updates, e.g. content is fetching, downloading the online content, etc.
…rary.

* Also, disabled the refresh layout so avoid unnecessary calls when a request is already in progress.
…idth.

* Since the online library file is 19MB large and on slow bandwidth devices it takes more time to fetch that file. Our current read timeout is 60 seconds which is not enough for reading this file on slow bandwidth devices. So we have increased it to 180 seconds.
…rogress.

* Added a head request to get the library length and showing the progress on behalf of that length.
* Improved the library loading process: previously, fetching the online library involved making two requests, which not only took more time to get a response from the server but also used twice the bandwidth. To address this, we have refactored our code to ensure that only one request is made at a time.
* Upgraded the retrofit, and interceptor dependencies to latest version.
@kelson42
Copy link
Collaborator

kelson42 commented Sep 18, 2024

@MohitMaliDeveloper From a user perspective this looks really good, here a few points:

  • We should be sure that this sync does not happen in mobile data only without clearly accepted by the user (should already be the case)
  • Please ensure that we can not start such a process twice at a time (should already be the case). Please confirm.
  • Do we have scenario where this sync with the online scenario happen in the background (so not triggered by a user action)? If "yes" does it continues to work fine?
  • Does this sync continues to work fine if we put the app in background or leave this view by switching back to the content for example?
  • Can you put the pourcentage on the new line at the bottom please (so we don't have the comment message and the pourcentage on the same line)?
  • CI is not passing!!!

@MohitMaliFtechiz
Copy link
Collaborator Author

We should be sure that this sync does not happen in mobile data only without clearly accepted by the user (should already be the case)

@kelson42 Yes, the sync does not happen on mobile data if the user does not accepted it.

Please ensure that we can not start such a process twice at a time (should already be the case). Please confirm.

@kelson42 It was happening, but I have placed a fix for it in this PR.

Do we have scenario where this sync with the online scenario happen in the background (so not triggered by a user action)? If "yes" does it continues to work fine?

Yes, we have this scenario and it is working fine. I have tested it.

Does this sync continues to work fine if we put the app in background or leave this view by switching back to the content for example?

yes, the sync works in these scenarios.

Can you put the pourcentage on the new line at the bottom please (so we don't have the comment message and the pourcentage on the same line)?

@kelson42 I have made the changes as per your request.

CI is not passing!!!

Yes. the unit coverage was failing due to our changes. I have placed a fix for those test failures.

@kelson42
Copy link
Collaborator

@MohitMaliFtechiz wenn no wifi is available the download process should not start at all!

@MohitMaliFtechiz
Copy link
Collaborator Author

@MohitMaliFtechiz wenn no wifi is available the download process should not start at all!

@kelson42 The downloading/sync process only starts on the mobile network when the user confirms to sync/download via the mobile network. Downloading/Sync will not start when no wifi is available and the user does not agree to download/sync content via the mobile network.

* Hiding the "No internet connection" text when user turn on the internet and previous this text is showing.
@MohitMaliFtechiz MohitMaliFtechiz marked this pull request as ready for review September 19, 2024 09:00
@kelson42
Copy link
Collaborator

@MohitMaliDeveloper Thank you. I will merge this PR and please release in testflight.

@kelson42 kelson42 merged commit 007e12c into main Sep 19, 2024
11 checks passed
@kelson42 kelson42 deleted the Fixes#3991 branch September 19, 2024 09:43
@MohitMaliFtechiz
Copy link
Collaborator Author

MohitMaliFtechiz commented Sep 19, 2024

@kelson42 The application has been released in internal testing: https://github.com/kiwix/kiwix-android/actions/runs/10938877209/job/30367933099.

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.

Online library can not be retrieved
3 participants