-
-
Notifications
You must be signed in to change notification settings - Fork 497
Fixed Hosted Books don't update on Application #3218
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
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #3218 +/- ##
=============================================
- Coverage 49.19% 49.15% -0.05%
- Complexity 1071 1075 +4
=============================================
Files 285 285
Lines 10330 10365 +35
Branches 1378 1382 +4
=============================================
+ Hits 5082 5095 +13
- Misses 4432 4451 +19
- Partials 816 819 +3
☔ View full report in Codecov by Sentry. |
|
@MohitMaliDeveloper #3219 should be implemented, this was the solution, not this. I can not figure out why you did this although i have proposed #3219 (a new ticket was not necessary as we already have a bug ticket). |
hi @kelson42 , I have created separate ticket because there was changes on the UI so just to make sure we have direct reference to changes. To me its more sort of like new feature. |
|
Solutions placed in #3219 |
|
OK for me, @gouri-panda This is on you now for the revirw. |
233c07f to
f26a3a8
Compare
|
@MohitMaliFtechiz #3219 has already close. Why are you mentioning that it closes that issue? |
|
@gouri-panda ticket chaos, forget about it. |
|
@kelson42 Ok |
gouri-panda
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MohitMaliDeveloper The update items still don't update after the server starts and the server crashes if the app goes in the background or switches to another application.
@gouri-panda ,
|
No, it is completely different. In this case, the if the app goes to the background and if refresh the page after sometime it stops it crashes.
Mobile Network.
No, I was not connected to WIFI. But if you face that problem also that isn't idol case. We should work on that too. Also, I found a small memory leak. It's happening because we are forgetting to unbind some services in the ZimHostFragment. |
hi @gouri-panda , thanks
WIFI wifi.mp4Mobile Network Screenrecorder-2023-02-06-15-26-46-0.mp4 |
@gouri-panda , can you please provide the logs? |
|
@gouri-panda any feedback? |
|
@MohitMaliDeveloper @kelson42 Sorry for the late reply. Here are the logs and video. LogsRecord_2023-02-13-14-17-38.mp4 |
|
hi @gouri-panda , this is not the memory leak, it's We can avoid this by using |
|
@MohitMaliFtechiz But it still crashes the App. Have you tried successfully reproduce the crash? |
@gouri-panda , I'm unable to reproduce this crash. it's working fine in my device (I have checked on multiple devices like in Samsung , Vivo , Redmi). |
|
@MohitMaliFtechiz Ok . I'll send the video of a crash in slack. |
|
@gouri-panda Here too we need a new review pass. PR is mandatory for 3.8 and we want to release. |
|
@MohitMaliFtechiz Can you please rebase and fix conflicts? |
d626ad7 to
17717e9
Compare
|
@MohitMaliFtechiz Why this PR is in draft? |
|
@kelson42 TestCases have failed on API level 33 twice after merging a PR, that's why I have marked this PR as draft. I have not tested this PR yet why the test cases are falling after merging another PR. |
|
@MohitMaliFtechiz OK, please complete once current issues around custom apps are fixed |
1dcc2bc to
4d9c5d2
Compare
|
@kelson42, @gouri-panda Test cases are fixed, now this PR is ready for review. |
…nd unselect zim files when server already started
* Created a new class to match the how many checkbox is checked in the recyclerview, it will also help to test this type of functionality in future.
* When the application goes in the background from `ZimHostFragment`, `KiwixReaderFragment/ZimHostFragment` without starting the Service then the `ReadAloudService`, and `HotspotService` variables are not used and GC try to clear those objects but we are not clearing those objects, that's why memory leak is happening. So now we have free those objects if they are not in use.
* We have made improvements to `ZimHostFragmentTest`. We added the `ACCESS_FINE_LOCATION` permission, which is required for running this test case on real devices. Additionally, we have enhanced the permission array and removed unnecessary permissions from the test case.
…it has the `SYSTEM_ALERT_WINDOW` permission which is not grantable through code on Android 13, so we have removed this permission from our `ZimHostFragment` test as well as from our other test cases since we had added this to fix the test cases because without this permission test cases were not launching on API level 21, and now our minimum SDK version is 24 so this permission is no longer needed.
* In the `ZimHostFragment`, there were occasional test failures due to specific conditions. When reattempting the test, it failed to detect the 'WiFi connection detected' dialog because the server was already running. To resolve this issue, we have improved our test case. Now, we first check if the server is already running. If it is, we close the server before running the test case. * In previous test failures within the `ZimHostFragment`, there were instances where the zim file was unselected, causing our test case to fail to locate the required views. To mitigate this, we now check whether the zim file is selected. If it's not selected, we first select the zim file before running the test case. * n the `LocalLibraryFragment` test, there were cases where it was unable to locate the 'file_management_no_files' view due to variations in the order of test cases. This occurred because a zim file was sometimes present in the `LocalLibrary`. To address this, we now check for the presence of any zim files in the `LocalLibrary` and delete them before running our test case.
|
@gouri-panda Any update? |
|
Merging... without review :( |
gouri-panda
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MohitMaliFtechiz Looks good to me! Thanks for the tests. I can't produce the error that i was talking about. Unfortunately, i can't find the commit that fixed.
@gouri-panda The issue was on the server side, after adapting the latest |
|
@MohitMaliFtechiz Ok. Thanks! |
Fixes #2537
Fixes #3219
Whats the issue
We are not updating
Adapter itemafter server is started.How I fix this
I have added a new feature to this. when the server is already started then the checkbox will be showing there(which previously we hidden from the user) and the user can select and unselect the zim file there. It will automatically update on the
Kiwix server(Basically if the server already started then it will restart the server with an updated zim file list).Added UI test cases for testing this functionality, we have bypassed this test case on API level 24 since wifi and hotspot is not available on below Api Level 25 as mentioned in the official docs https://developer.android.com/studio/run/emulator-wifi

Fixed memory leak in
ZimHostFragmentandCoreReaderFragment. When the application goes in the background from ZimHostFragment, KiwixReaderFragment/ZimHostFragment without starting the Service then theReadAloudService, andHotspotServicevariables are not used and GC try to clear those objects but we are not clearing those objects, that's why memory leak is happening. So now we have free those objects if they are not in use.Fixed
ZimHostFragmentfailing on API level 33 (Android 13) because it has theSYSTEM_ALERT_WINDOWpermission which is not grantable through code on Android 13, so we have removed this permission from ourZimHostFragmenttest as well as from our other test cases since we had added this to fix the test cases in Fixed Bookmark toggle is not ON for saved bookmarks #3473 because without this permission test cases were not launching on API level 21, and now our minimum SDK version is 24 so this permission is no longer needed.ZimHostFragment, there were occasional test failures due to specific conditions. When reattempting the test, it failed to detect the 'WiFi connection detected' dialog because the server was already running. To resolve this issue, we have improved our test case. Now, we first check if the server is already running. If it is, we close the server before running the test case.ZimHostFragment, there were instances where the zim file was unselected, causing our test case to fail to locate the required views. To mitigate this, we now check whether the zim file is selected. If it's not selected, we first select the zim file before running the test case.LocalLibraryFragmenttest, there were cases where it was unable to locate the 'file_management_no_files' view due to variations in the order of test cases. This occurred because a zim file was sometimes present in theLocalLibrary. To address this, we now check for the presence of any zim files in theLocalLibraryand delete them before running our test case.