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

601 bookmarks initial libkiwix implementation #679

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

BPerlakiH
Copy link
Collaborator

@BPerlakiH BPerlakiH commented Mar 3, 2024

Initial attempt to use libkiwix's bookmark functionality.

Not to be merged!

After some battle with the interoperability on 3 levels swift / objective-c / c++, this is an initial implementation I could do to re-use the LibKiwix bookmark functionality.

There are several issues with this approach, and several things are missing compared to what we currently have via CoreData (on device SQL DB):

  • it is a lot of bridging between those languages. It is a headache to maintain such code, it's not "safe", and hard to follow
  • in order to implement this, and make it actually work, I had to look under the C++ source code to understand how it is working
  • there are a lot of intermediate steps taken and types to be created in order to create a bookmark
  • the current functionality is not persisting the bookmarks, meaning it is a memory only storage, so when the app is closed the bookmarks are lost
  • the kiwix::Bookmark is not supporting "html snippet", and not supporting thumbnail url (see the screenshot below)
  • I haven't found any built in way to add the "search bookmarks" functionality
  • We would need to write all the "reactive" part of the code, to make sure any bookmarks change is triggering the appropriate UI change as well (eg. bookmark added / removed), currently we even have animated tiles as they are added / removed, thanks to CoreData bindings.
Screenshot 2024-03-03 at 20 42 47

As you can see on the screenshot, in order to list the Bookmarks, we need the following minimum data:

  • url to the Article
  • title
  • snippet (the short description)
  • thumbnail url
    The above values are currently HTML parsed out from the Article, but the LibKiwix::Bookmark struct is not supporting these additional fields

At this stage I feel that going down this route we will add a lot of extra binding code, and we would need to re-implement most of the things the CoreData is offering out of the box.

@BPerlakiH BPerlakiH marked this pull request as ready for review March 3, 2024 19:51
@BPerlakiH BPerlakiH marked this pull request as draft March 3, 2024 19:51

func testAddingBookmark() {
let fileURLData = ZimFileService.getFileURLBookmarkData(for: testZimURL)!
try! ZimFileService.shared.open(fileURLBookmark: fileURLData)
Copy link
Collaborator Author

@BPerlakiH BPerlakiH Mar 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit cumbersome, we have a hash of stored Archive under ZimFileService that came from parsing things, using kiwix::library mostly (in OPDSParser), but we are not holding on to the library.
In order for the bookmarks to be successfully added, we do need to add them to the library itself (see here).
So we have a kind of dualism here, where the list of items are going through one temporary instance of library to be parsed and inserted into the DB, whereas in order to bookmark them we need another instance of library where the kiwix:Book had to be added, otherwise the added kiwix::Bookmark will be invalid.

book.update(zim::Archive([fileURL fileSystemRepresentation]));
} catch (std::exception e) { }
return book;
}
Copy link
Collaborator Author

@BPerlakiH BPerlakiH Mar 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a lot of legwork at the moment:

  • we need a kiwix::Book to create kiwix::Bookmark
  • we need a kiwix::Archive to create kiwix::Book

Whereas all we want is to store:

  • 2 strings (title, description) a thumbnail URL, keyed by the article URL, which is more or less this much code:
struct SwiftBookmark {
    let title: String
    let description: String
    let thumbnail: URL?
}

final class SwiftBookmarkManager {
    private var dict: [URL: SwiftBookmark] = [:]

    func add(bookmark: SwiftBookmark, withURL url: URL) {
        dict[url] = bookmark
    }
    func remove(withURL url: URL) {
        dict.removeValue(forKey: url)
    }
    func isBookmarked(withURL url: URL) -> Bool {
        dict[url] != nil
    }
}

This is to be compared with the 220 lines of code added in this PR...

- (BOOL) isBookmarked: (nonnull NSURL *) url inZIM: (nonnull NSUUID *) zimFileID {
std::string url_c = [url fileSystemRepresentation];
std::string fileID_c = [[[zimFileID UUIDString] lowercaseString] cStringUsingEncoding:NSUTF8StringEncoding];
const std::vector<kiwix::Bookmark> bookmarks = self.library->getBookmarks(true);
Copy link
Collaborator Author

@BPerlakiH BPerlakiH Mar 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this call can be used without the argument and it works equally good:
self.library->getBookmarks();
The tricky part is, which is not much documented, is that the kiwix::Book needs to be added to the library,
as on line 47.
If the Book is not added to the library, the self.library->getBookmarks(); won't return it, as it is an "invalid" bookmark..
The other solution is not to add the Book to the library and call getBookmarks(false).
Well... without looking into the C++ code it is not obvious...


XCTAssertTrue(bookmarks.isBookmarked(url: bookmarkURL))
}

Copy link
Collaborator Author

@BPerlakiH BPerlakiH Mar 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For comparison testing the pure swift implementation, does the same at the moment (or even more as it supports thumbnail URL and description):

    func testSwiftBookmars() {
        let bookmarks = SwiftBookmarkManager()
        XCTAssertFalse(bookmarks.isBookmarked(withURL: bookmarkURL))
        let bookmark = SwiftBookmark(title: testTitle, description: "test snippet", thumbnail: nil)
        bookmarks.add(bookmark: bookmark, withURL: bookmarkURL)
        XCTAssertTrue(bookmarks.isBookmarked(withURL: bookmarkURL))
    }

Plus currently the swift code is out-performing the libkiwix solution, mainly due to these extra bridging and casting (average: 0.000s vs. 0.013s)

@kelson42
Copy link
Contributor

kelson42 commented Mar 4, 2024

* it is a lot of bridging between those languages. It is a headache to maintain such code, it's not "safe", and hard to follow

That everything has to be bridged is normal. That this is not "safe", is not normal. Please do the necessary - like reporting problems at libkiwix if necessary - that things are "safe" (understandable and clean).

* in order to implement this, and make it actually work, I had to look under the C++ source code to understand how it is working

This is also not normal, please report things in issues at openzim/libkiwix.

* there are a lot of intermediate steps taken and types to be created in order to create a bookmark

Definitively and this is normal.

* the current functionality **is not persisting the bookmarks**, meaning it is a memory only storage, so when the app is closed the bookmarks are lost

Obviously it has to, and libkiwix offer primitive to save/load bookmarks to/from the filesystem.

* the `kiwix::Bookmark` is not supporting "html snippet", and not supporting thumbnail url (see the screenshot below)

Any argument to keep it? What is the value of such snippet?

* I haven't found any built in way to add the "search bookmarks" functionality

How does it technically work today? Do we have a dedicated DB with search index? Or just we go through all bookmarks?

* We would need to write all the "reactive" part of the code, to make sure any bookmarks change is triggering the appropriate UI change as well (eg. bookmark added / removed), currently we even have animated tiles as they are added / removed, thanks to CoreData bindings.

Anyway to avoid getting ride of these animations by moving to libkiwix?

Screenshot 2024-03-03 at 20 42 47

As you can see on the screenshot, in order to list the Bookmarks, we need the following minimum data:

* url to the Article

* title

* snippet (the short description)

* thumbnail url
  The above values are currently HTML parsed out from the Article, but the LibKiwix::Bookmark struct is not supporting these additional fields

Please open an issue for the two last items at kiwix/libkiwix which are currently not supported.

At this stage I feel that going down this route we will add a lot of extra binding code, and we would need to re-implement most of the things the CoreData is offering out of the box.

Binding code should not be a complicated thing, actually there is tools to do that more or less automatically. For the rest, what is exactly the difficulty (you have talked about this animation but what else?)

Base automatically changed from 601-bookmarks-intial-cleanup to main March 5, 2024 07:13
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.

2 participants