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: Application crashes when opening the random article. #4162

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MohitMaliFtechiz
Copy link
Collaborator

@MohitMaliFtechiz MohitMaliFtechiz commented Jan 6, 2025

Fixes #4161

  • The issue occurred in the Play Store variant, specifically with the copy/move functionality for ZIM files. After copying or moving a ZIM file, the application attempts to open it in the reader. However, if the application tries to open a split ZIM file with only one chunk, it throws an error indicating that the file is not a valid ZIM file (which is expected, as not all ZIM chunks have been copied/moved). When this error occurs, the menu buttons remain enabled, allowing the user to interact with them. If the user then attempts to open a random article, the application enters an infinite loop and crashes. To resolve this, we now disable the menu controls whenever an error occurs while opening a ZIM file, preventing the user from using them in such scenarios.

Before fix

screen-20250108-154820.mp4

After fix

screen-20250108-152814.mp4
  • Added a retry mechanism to attempt fetching the random article twice in case of an internal error in libzim to prevent going into an infinite loop.
  • After two failed attempts, an error message is displayed to the user if the random article is not found.
  • Previously, when a ZIM file was in the process of opening, the reopenBook method was used to hide the "Open Library" button if it was visible. However, this method also enabled the menu buttons. If an error occurred while loading the ZIM file, the controls remained enabled, which was not desirable. Now, the "Open Library" button is hidden, and the menu buttons are shown only after the ZIM file has successfully loaded. If an error occurs during loading, an error message is displayed (as it was previously), and the UI is updated accordingly.

Copy link

codecov bot commented Jan 6, 2025

Codecov Report

Attention: Patch coverage is 7.14286% with 13 lines in your changes missing coverage. Please review.

Project coverage is 57.02%. Comparing base (c069a12) to head (61dca7e).

Files with missing lines Patch % Lines
.../kiwix/kiwixmobile/core/main/CoreReaderFragment.kt 7.14% 12 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #4162      +/-   ##
============================================
+ Coverage     56.91%   57.02%   +0.11%     
- Complexity     1531     1536       +5     
============================================
  Files           313      313              
  Lines         13421    13433      +12     
  Branches       1670     1673       +3     
============================================
+ Hits           7638     7660      +22     
+ Misses         4633     4615      -18     
- Partials       1150     1158       +8     

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

@kelson42
Copy link
Collaborator

kelson42 commented Jan 6, 2025

@MohitMaliFtechiz Why the reader handle should be or is null? If there is a valid scenario, then we should have a generoc function trsting this and this should be builtin in the ZIM reader class at all places where needed. I don't understand why such a basic thing is done at such a high level!?

@MohitMaliFtechiz
Copy link
Collaborator Author

MohitMaliFtechiz commented Jan 6, 2025

@MohitMaliFtechiz Why the reader handle should be or is null?

@kelson42 No it will not be null if there is a ZIM file opened in the reader. The controls are disabled until the ZIM file is not loaded into the webView. Actually, the error occurs when we are copying/moving the split ZIM file so after copying/moving the ZIM file it tries to open the ZIM file in the reader but currently, it is corrupted since all ZIM chunks are not copied and the reader showing invalid ZIm file. But at this moment, menu controls are enabled for this ZIM and the user tries to open the random article, so in this condition, the reader was null, and openRandomArticle was going into an infinite loop. I have placed a fix for it and updated the videos on the PR description.

@MohitMaliFtechiz MohitMaliFtechiz marked this pull request as draft January 8, 2025 09:41
@MohitMaliFtechiz MohitMaliFtechiz changed the title Fixed: infinite loop and improve error handling for fetching random article. Fixed: Application crashes when opening the random article. Jan 8, 2025
@MohitMaliFtechiz MohitMaliFtechiz marked this pull request as ready for review January 8, 2025 10:27
@MohitMaliFtechiz
Copy link
Collaborator Author

The Ci is failing due to #4169.

@kelson42
Copy link
Collaborator

But at this moment, menu controls are enabled for this ZIM and the user tries to open the random article,

This should not happen. Either we have a valid ZIM file and the controls are enabled OR there is a problem at loading and the controls stay disabled and the standart loading error message is displayed.

…rticle.

* Fixed an issue where the app would enter an infinite loop when the zimFileReader was null, causing a crash.
* Improved the condition to check if zimFileReader is null and show a proper error message to the user without retrying.
* Added a retry mechanism to attempt fetching the random article twice in case of an internal error in libzim.
* After two failed attempts, an error message is displayed to the user if the random article is not found.
… to crash.

* The issue occurred in the Play Store variant, specifically with the copy/move functionality for ZIM files. After copying or moving a ZIM file, the application attempts to open it in the reader. However, if the application tries to open a split ZIM file with only one chunk, it throws an error indicating that the file is not a valid ZIM file (which is expected, as not all ZIM chunks have been copied/moved). When this error occurs, the menu buttons remain enabled, allowing the user to interact with them. If the user then attempts to open a random article, the application enters an infinite loop and crashes. To resolve this, we now disable the menu controls whenever an error occurs while opening a ZIM file, preventing the user from using them in such scenarios.
* Previously, when a ZIM file was in the process of opening, the reopenBook method was used to hide the "Open Library" button if it was visible. However, this method also enabled the menu buttons. If an error occurred while loading the ZIM file, the controls remained enabled, which was not desirable. Now, the "Open Library" button is hidden, and the menu buttons are shown only after the ZIM file has successfully loaded. If an error occurs during loading, an error message is displayed (as it was previously), and the UI is updated accordingly.
@MohitMaliFtechiz
Copy link
Collaborator Author

This should not happen. Either we have a valid ZIM file and the controls are enabled OR there is a problem at loading and the controls stay disabled and the standart loading error message is displayed.

yes, now, it is working like this. I have also improved the enabling of menu buttons in the last commit and updated the PR description for that change.

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.

Application crashes on openRandomArticle.
3 participants