Use libkiwix to store bookmarks #3653

MohitMaliFtechiz · 2024-01-04T11:05:00Z

Now we are saving the bookmark in libkiwix instead of ObjectBox. Libkiwix allows us to save bookmarks in a file with the help of the Library, and we can save the library too in a file, the library contains the contains ZIM file paths and favicons in it so we are saving both library/bookmarks in a file. We can read these saved files via the Manager class to retrieve the saved bookmarks/library content.
We are saving both these files (library.xml, bookmark.xml) inside the Bookmarks folder of our app-specific directory.
For migrating the previous bookmarks from ObjectBox to Libkiwix we have created ObjectBoxToLibkiwixMigrator class which handles the migration part. We have not deleted the previous data from the ObjectBox yet to prevent any data loss.
Implemented proper error handling and showing logs if anything goes wrong while saving or migrating the data for better debugging.
Writing the bookmarks in the file and migration are performed on the background thread to avoid affecting user experience.
Added test cases for Saving bookmarks and migration process
To understand the small things that we did in this PR we have added necessary comments on each method what and why they are doing.

codecov · 2024-01-04T11:31:28Z

Codecov Report

Attention: 78 lines in your changes are missing coverage. Please review.

Comparison is base (d733ee8) 51.05% compared to head (8199480) 51.29%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
...rg/kiwix/kiwixmobile/core/dao/LibkiwixBookmarks.kt	65.62%	25 Missing and 19 partials ⚠️
...le/core/data/remote/ObjectBoxToLibkiwixMigrator.kt	54.83%	11 Missing and 3 partials ⚠️
.../kiwix/kiwixmobile/core/main/CoreReaderFragment.kt	45.45%	2 Missing and 4 partials ⚠️
...rg/kiwix/kiwixmobile/core/main/CoreMainActivity.kt	0.00%	4 Missing and 1 partial ⚠️
...java/org/kiwix/kiwixmobile/core/data/Repository.kt	50.00%	3 Missing ⚠️
...kiwix/kiwixmobile/core/page/viewmodel/PageState.kt	0.00%	3 Missing ⚠️
...wix/kiwixmobile/core/main/MainRepositoryActions.kt	0.00%	0 Missing and 1 partial ⚠️
...core/page/bookmark/adapter/LibkiwixBookmarkItem.kt	97.50%	0 Missing and 1 partial ⚠️
...bile/core/page/bookmark/viewmodel/BookmarkState.kt	66.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #3653      +/-   ##
============================================
+ Coverage     51.05%   51.29%   +0.24%     
- Complexity     1177     1218      +41     
============================================
  Files           285      288       +3     
  Lines         10622    10841     +219     
  Branches       1429     1451      +22     
============================================
+ Hits           5423     5561     +138     
- Misses         4277     4332      +55     
- Partials        922      948      +26

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

kelson42 · 2024-01-12T07:13:55Z

@MohitMaliFtechiz What is the status here? I don‘t understand why such a simple thing like handling bookamrks takes so mich time?!

MohitMaliFtechiz · 2024-01-12T07:53:03Z

@kelson42 Migrating bookmarks is working correctly now with low-end devices as well(at least for 10K bookmarks), I was improving the large data migration in lower devices so that it will migrate the bookmarks easily on the lower devices, The main issue is there are some random test failures on CI after these change. But locally all the test cases are passing, so I am finding the actual cause of these failures.

kelson42 · 2024-01-12T08:13:45Z

@MohitMaliFtechiz What do you mean with "large data"? bookmarks are small amount of data, how is that possible that migrating them somehow is a performance problem?

MohitMaliFtechiz · 2024-01-12T08:30:05Z

What do you mean with "large data"? bookmarks are small amount of data,

@kelson42 I mean a large amount of Bookmarks, assume there are 10K bookmarks in the ObjectBox and we are migrating them to libkiwix at that time lower-end devices were crashing due to we were writing the data into the library and there were some local variables (archive, bookmark) which is not disposed of after there use so they are in the memory.

We have improved our migration and data loading process from libkiwix to avoid this kind of error.

kelson42 · 2024-01-12T08:48:09Z

@MohitMaliDeveloper There is not reason that migration 10K bookmarks takes more than a few seconds. And BTW, do you know anybody with 10K bookmarks even in its browser? I have also no clue what you mean with "some local variables (archive, bookmark")?! Please be precise and extrem clear in what you write.

MohitMaliFtechiz · 2024-01-14T07:14:03Z

@kelson42 My explanation was short to understand properly what was going on. In the migrating process, I have tested it with different scenarios to make sure everything will work fine in every scenario. e.g. It will not affect the performance of the application when there many numbers of bookmarks are available in the libkiwix. Since now we do not have any database(so that we can filter the data to show) we are directly saving the bookmarks in a file via libkiwix and we have to retrieve the full bookmarks data from libkiwix and perform the filtration on that data for current books bookmark, so when I was performing this operation with 512 bookmarks on Android 8 it was giving me Local reference table overflow so I was making sure everything works perfectly without any error in every scenario.

And BTW, do you know anybody with 10K bookmarks even in its browser?

I don't know any person who has the 10K bookmarks, but being a developer we should be ready for every scenario.

I have also no clue what you mean with "some local variables (archive, bookmark")?! Please be precise and extrem clear in what you write.

I mean, during the bookmark migration, we were creating archive/bookmark objects for saving them to the library. However, these objects were not being garbage collected due to being inside a loop. Consequently, these local variables were retained in memory. Similar issues arose when retrieving bookmarks from libkiwix for display. I resolved these errors, and the fixes took only a few hours. The primary challenges, though, were encountered in the test cases. Since we lacked direct access to libkiwix functions in our unit test cases, we needed wrapper classes for access. Unfortunately, there were issues with accessing the Book constructor in the wrapper, rendering this approach ineffective. Consequently, I transferred all migration tests to instrumentation test cases to execute all migration operations on the UI. The UI test takes longer to implement than the unit test case because we conduct instrumentation test cases on various devices. This ensures that these tests do not interfere with other test cases running concurrently on the Continuous Integration (CI) system.

MohitMaliFtechiz · 2024-01-16T13:56:15Z

@gouri-panda, @kelson42 This PR is ready for review as now there is nothing left to implement on the Android side only an issue is occurring with libkiwix for that I have opened a ticket on the libkiwix kiwix/java-libkiwix#80. I am keeping this PR in the draft due to the error of libkiwix on Android 24.

kelson42 · 2024-01-26T06:36:12Z

@MohitMaliFtechiz We should review and merge this ticket independently of kiwix/java-libkiwix#80

MohitMaliFtechiz · 2024-01-26T07:35:39Z

@kelson42 It is working fine with API level 26(Android 8) but I am concerned about lower devices if someone has bookmarks more than 512 it will crash the application.

kelson42 · 2024-01-26T16:26:50Z

@kelson42 It is working fine with API level 26(Android 8) but I am concerned about lower devices if someone has bookmarks more than 512 it will crash the application.

I don't think that anybody has more than 500 bookmarks, please put in review if this is the only known problem.

MohitMaliFtechiz · 2024-01-29T07:11:33Z

@gouri-panda This PR is ready for review.

kelson42 · 2024-02-03T15:30:17Z

@MohitMaliFtechiz Please rebase and fix conflict
@gouri-panda We really need your review to get a chance to release 3.10.0 soon.

gouri-panda

@kelson42 I've reviewed and tested this PR so far it looks good but i think i should review this again for edge cases since this is a big and important PR i may've missed something. Give me another day to review this properly.

gouri-panda

@MohitMaliDeveloper Thanks for the robust tests! LGTM!

kelson42 · 2024-02-06T17:43:30Z

@MohitMaliFtechiz Can you please rebase (a last time)

* We introduced a Flowable list in LibkiwixBookmarks to observe live changes on the UI. Previously, the UI was not updating after deleting bookmarks on the BookmarkScreen, and toggling bookmarks in the reader screen was also not reflecting the changes. To address this, we created a Flowable list and are now observing it whenever bookmarks are added or removed in the library.

…wixBookmarks`.

…st cases

…save the `library`, which contains information about books, their file paths, and favicons, into a file. * We now save this library information into a file named `library.txt` and subsequently read from it to retrieve file paths and favicons. * The test cases have been refactored to accommodate this new functionality. * The `ObjectBoxToLibkiwiMigrator` code has also been enhanced. With this change, we now save books in the library for their favicon and zimFilePath, resulting in a refactor and improvement of this class's functionality and its associated test cases. * The process of writing bookmarks and library data to file has been enhanced. Now, this is performed asynchronously in a background thread to mitigate potential data loss. * Additionally, several other improvements have been made throughout the codebase.

… since we don't need these classes as saving/deleting functionality will be tested in java-libkiwix. * Added instrumentation test case for testing the UI part with libkiwix bookmark functionality.

…etrieve the saved bookmarks. * Enhanced the `isBookMarkExist` method, addressing a bug that prevented the addition of new bookmarks for the same file. The method has been refactored for improved functionality. * In the debug version, added informative logs to provide developers with insights into the bookmark-saving functionality.

…e current book has been removed because adding the book to the library is unnecessary, as we have not saved any bookmarks yet. * We now write bookmarks and the library on the main thread instead of saving them in the background thread to prevent any data loss.

…d logs before writing the library in file.

…he favicon from it, we are now speeding up the process by directly taking the favicon from the book, as the issue at kiwix/java-libkiwix#73 has been resolved.

* Removed unused imports.

…id impacting application performance.

* Improved the instrumentation test cases. * Improved the migration test case.

* Refined test cases to thoroughly assess the migration process. * Enhanced the migration logic to effectively manage large datasets during migration. * Optimized the Bookmark functionality to minimize unnecessary data loading on libkiwix. Retrieving data from libkiwix is now performed only when there is a change in the bookmark, reducing redundant data fetches. Otherwise, the previously loaded data is returned to minimize unnecessary resource access.

* Enhanced the process of adding books to the library to prevent unnecessary data loading from libkiwix. * Released the memory occupied by bookmarks and archives to resolve potential issues when running migrations on lower-end devices.

* Re-enabled test case for migrating bookmarks more then 512 to test it properly on API level 24.

MohitMaliFtechiz · 2024-02-07T06:20:00Z

@kelson42 Done.

kelson42 · 2024-02-07T06:51:40Z

@MohitMaliDeveloper Thx. I have look very briefly to this big PR. Here is what I'm missing:

A proper PR description with the usual stuff (features of the PR, global approach, impact for both future devs and users)
The critical part here is the migration part, I see no class to handle the migration (with a name appropriate). The migration should be fully isolated of the rest so its easy to remove at some point.

MohitMaliFtechiz · 2024-02-07T09:59:51Z

@kelson42 We have the ObjectBoxToLibkiwixMigrator class which handles the migration. Also, I have updated the PR description with the necessary details. Let me know if anything I am missing something.

MohitMaliFtechiz marked this pull request as draft January 4, 2024 11:05

MohitMaliFtechiz force-pushed the Fix#3109 branch from 533e081 to db6601d Compare January 9, 2024 06:08

MohitMaliFtechiz marked this pull request as ready for review January 10, 2024 13:37

MohitMaliFtechiz requested a review from gouri-panda January 10, 2024 13:37

MohitMaliFtechiz marked this pull request as draft January 11, 2024 06:43

MohitMaliFtechiz force-pushed the Fix#3109 branch from b32db28 to 1b2e5dd Compare January 11, 2024 06:43

MohitMaliFtechiz force-pushed the Fix#3109 branch 2 times, most recently from 25d241e to 9746fc2 Compare January 15, 2024 18:28

MohitMaliFtechiz force-pushed the Fix#3109 branch from 09bbfae to ed4eb35 Compare January 29, 2024 07:10

MohitMaliFtechiz marked this pull request as ready for review January 29, 2024 07:10

kelson42 force-pushed the Fix#3109 branch from ed4eb35 to 8d714e1 Compare January 30, 2024 18:12

MohitMaliFtechiz force-pushed the Fix#3109 branch from 8d714e1 to 0a31d8f Compare February 5, 2024 09:06

gouri-panda reviewed Feb 5, 2024

View reviewed changes

kelson42 force-pushed the Fix#3109 branch from 0a31d8f to 1e467fd Compare February 5, 2024 20:02

gouri-panda approved these changes Feb 6, 2024

View reviewed changes

Created ObjectBox to libkiwix migrator for Bookmarks.

4fe6d11

MohitMaliDeveloper and others added 24 commits February 7, 2024 11:32

Fixed detekt errors

7c23207

Refactored unit coverage test cases for testing bookmarks with `Libki…

429502a

…wixBookmarks`.

Fixed application crash while running application on the emulators.

3d7a4af

Improved LibkiwixBookTest and added new ObjectBoxToLibkiwixMigratorTe…

07e40ee

…st cases

Removed the unnecessary wrapper classes of libkiwix from the codebase…

a78c456

… since we don't need these classes as saving/deleting functionality will be tested in java-libkiwix. * Added instrumentation test case for testing the UI part with libkiwix bookmark functionality.

Fixed, compilation error

1f3cfd6

Added logs while reading reading the bookmarks/library data, and adde…

aaa9cc3

…d logs before writing the library in file.

Fixed path and improved the logs

d821d5a

Instead of creating an archive object for every book and retrieving t…

0d48e23

…he favicon from it, we are now speeding up the process by directly taking the favicon from the book, as the issue at kiwix/java-libkiwix#73 has been resolved.

Removed unnecessary comments

ecc0d4b

Storing bookmarks/library inside our app-specific directory

8666675

Getting zimFilePath efficiently for LibkiwixBookmarkItem.

60966b9

* Removed unused imports.

Writing bookmarks and library data to a file in the background to avo…

e866dd2

…id impacting application performance.

Refactored the test cases.

f45e321

* Improved the instrumentation test cases. * Improved the migration test case.

Enhanced the migration process to handle exceptions more effectively.

efd8caf

Fixed the failing LibkiwixBookmarkTest.

50f92be

* Enhanced the process of adding books to the library to prevent unnecessary data loading from libkiwix. * Released the memory occupied by bookmarks and archives to resolve potential issues when running migrations on lower-end devices.

Fixed the failing LibkiwixBookmarkTest on Android 33 and 30.

ed2a9e0

Fixed Migration test was failing on the API level 24.

1460e0d

Upgraded java-libkiwix to 2.0.0

8199480

* Re-enabled test case for migrating bookmarks more then 512 to test it properly on API level 24.

MohitMaliFtechiz force-pushed the Fix#3109 branch from 10eda26 to 8199480 Compare February 7, 2024 06:19

kelson42 merged commit 1f07863 into main Feb 7, 2024
9 of 10 checks passed

kelson42 deleted the Fix#3109 branch February 7, 2024 10:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use libkiwix to store bookmarks #3653

Use libkiwix to store bookmarks #3653

MohitMaliFtechiz commented Jan 4, 2024 •

edited

Loading

codecov bot commented Jan 4, 2024 •

edited

Loading

kelson42 commented Jan 12, 2024

MohitMaliFtechiz commented Jan 12, 2024

kelson42 commented Jan 12, 2024 •

edited

Loading

MohitMaliFtechiz commented Jan 12, 2024

kelson42 commented Jan 12, 2024 •

edited

Loading

MohitMaliFtechiz commented Jan 14, 2024

MohitMaliFtechiz commented Jan 16, 2024

kelson42 commented Jan 26, 2024

MohitMaliFtechiz commented Jan 26, 2024

kelson42 commented Jan 26, 2024

MohitMaliFtechiz commented Jan 29, 2024

kelson42 commented Feb 3, 2024

gouri-panda left a comment

gouri-panda left a comment

kelson42 commented Feb 6, 2024

MohitMaliFtechiz commented Feb 7, 2024

kelson42 commented Feb 7, 2024

MohitMaliFtechiz commented Feb 7, 2024

Use libkiwix to store bookmarks #3653

Use libkiwix to store bookmarks #3653

Conversation

MohitMaliFtechiz commented Jan 4, 2024 • edited Loading

codecov bot commented Jan 4, 2024 • edited Loading

Codecov Report

kelson42 commented Jan 12, 2024

MohitMaliFtechiz commented Jan 12, 2024

kelson42 commented Jan 12, 2024 • edited Loading

MohitMaliFtechiz commented Jan 12, 2024

kelson42 commented Jan 12, 2024 • edited Loading

MohitMaliFtechiz commented Jan 14, 2024

MohitMaliFtechiz commented Jan 16, 2024

kelson42 commented Jan 26, 2024

MohitMaliFtechiz commented Jan 26, 2024

kelson42 commented Jan 26, 2024

MohitMaliFtechiz commented Jan 29, 2024

kelson42 commented Feb 3, 2024

gouri-panda left a comment

Choose a reason for hiding this comment

gouri-panda left a comment

Choose a reason for hiding this comment

kelson42 commented Feb 6, 2024

MohitMaliFtechiz commented Feb 7, 2024

kelson42 commented Feb 7, 2024

MohitMaliFtechiz commented Feb 7, 2024

MohitMaliFtechiz commented Jan 4, 2024 •

edited

Loading

codecov bot commented Jan 4, 2024 •

edited

Loading

kelson42 commented Jan 12, 2024 •

edited

Loading

kelson42 commented Jan 12, 2024 •

edited

Loading