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
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
AppStore/screenshots
CoreKiwix.xcframework
*.zim
!Tests/TestResources/test_small.zim

# Xcode
#
Expand Down
22 changes: 22 additions & 0 deletions Model/LibBookmarks/LibBookmark.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
//
// LibBookmark.h
// Kiwix

#import <Foundation/Foundation.h>

NS_ASSUME_NONNULL_BEGIN

@interface LibBookmark : NSObject

@property std::string zimID_c;
@property std::string url_c;
@property std::string title_c;

- (nonnull instancetype) init: (nonnull NSURL *) url inZIM: (nonnull NSUUID *) zimFileID withTitle: (nonnull NSString *) title;

//- (const kiwix::Bookmark&) bridged;
- (kiwix::Book) book;

NS_ASSUME_NONNULL_END

@end
39 changes: 39 additions & 0 deletions Model/LibBookmarks/LibBookmark.mm
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
//
// LibBookmark.m
// Kiwix

#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdocumentation"
#include "kiwix/bookmark.h"
#include "kiwix/book.h"
#include "zim/archive.h"
#pragma clang diagnostic pop

#import <Foundation/Foundation.h>
#import "LibBookmark.h"
#import "ZimFileService.h"

@interface LibBookmark()
@property (nonatomic, strong) NSURL *_Nonnull url;
@property (nonatomic, strong) NSUUID *_Nonnull zimFileID;
@end

@implementation LibBookmark

- (instancetype) init: (NSURL *) url inZIM: (NSUUID *) zimFileID withTitle: (nonnull NSString *) title{
self = [super init];
if (self) {
self.url = url;
self.zimFileID = zimFileID;
self.zimID_c = [[[zimFileID UUIDString] lowercaseString] cStringUsingEncoding:NSUTF8StringEncoding];
self.url_c = [url fileSystemRepresentation];
self.title_c = [title cStringUsingEncoding:NSUTF8StringEncoding];
}
return self;
}

- (kiwix::Book) book {
return [ZimFileService.sharedInstance getBookBy: self.zimFileID];
}

@end
42 changes: 42 additions & 0 deletions Model/LibBookmarks/LibBookmarks.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
//
// LibBookmarks.swift
// Kiwix

import Foundation

struct LibBookmarks {

static let shared = LibBookmarks()
private let bridge = LibBookmarksBridge()

private init() {}

func isBookmarked(url: URL) -> Bool {
guard let zimID = UUID(fromZimURL: url) else { return false }
return bridge.__isBookmarked(url, inZIM: zimID)
}

func addBookmark(_ bookmark: LibBookmark) {
bridge.__add(bookmark)
}

func removeBookmark(_ bookmark: LibBookmark) {
bridge.__remove(bookmark)
}
}

extension LibBookmark {
convenience init?(withUrl url: URL, withTitle title: String) {
guard let zimFileID = UUID(fromZimURL: url) else { return nil }
self.init(url, inZIM: zimFileID, withTitle: title)
}
}

extension UUID {
init?(fromZimURL url: URL) {
guard let uuid = UUID(uuidString: url.host ?? "") else {
return nil
}
self = uuid
}
}
17 changes: 17 additions & 0 deletions Model/LibBookmarks/LibBookmarksBridge.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
//
// LibBookmarksBridge.h
// Kiwix

#import <Foundation/Foundation.h>
#import "kiwix/library.h"
#import "kiwix/bookmark.h"
#import "LibBookmark.h"

@interface LibBookmarksBridge : NSObject

- (nonnull instancetype) init;
- (BOOL) isBookmarked: (nonnull NSURL *) url inZIM: (nonnull NSUUID *) zimFileID NS_REFINED_FOR_SWIFT;
- (void) add: (nonnull LibBookmark *) bookmark NS_REFINED_FOR_SWIFT;
- (void) remove: (nonnull LibBookmark *) bookmark NS_REFINED_FOR_SWIFT;

@end
56 changes: 56 additions & 0 deletions Model/LibBookmarks/LibBookmarksBridge.mm
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
//
// LibBookmarksBridge.mm
// Kiwix

#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdocumentation"
#include "kiwix/library.h"
#include "kiwix/bookmark.h"
#pragma clang diagnostic pop

#import "LibBookmarksBridge.h"

# pragma mark - Private properties
@interface LibBookmarksBridge ()

@property kiwix::LibraryPtr library;

@end


# pragma mark - Implementation

@implementation LibBookmarksBridge

- (instancetype _Nonnull)init {
self = [super init];
if (self) {
self.library = kiwix::Library::create();
}
return self;
}

- (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...

for (kiwix::Bookmark bookmark: bookmarks) {
if (bookmark.getUrl() == url_c && bookmark.getBookId() == fileID_c) {
return true;
}
}
return false;
}

- (void) add: (LibBookmark *) bookmark {
kiwix::Book book = [bookmark book];
self.library->addBook(book);
kiwix::Bookmark bridgedBookmark = kiwix::Bookmark(book, bookmark.url_c, bookmark.title_c);
self.library->addBookmark(bridgedBookmark);
}

- (void) remove: (LibBookmark *) bookmark {
self.library->removeBookmark(bookmark.zimID_c, bookmark.url_c);
}

@end
2 changes: 2 additions & 0 deletions Model/ZimFileService/ZimFileService.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#import <Foundation/Foundation.h>
#import "ZimFileMetaData.h"
#import "kiwix/book.h"

@interface ZimFileService : NSObject

Expand All @@ -23,6 +24,7 @@
- (void)close:(NSUUID *_Nonnull)zimFileID NS_REFINED_FOR_SWIFT;
- (NSArray *_Nonnull)getReaderIdentifiers NS_REFINED_FOR_SWIFT;
- (nonnull void *) getArchives;
- (kiwix::Book) getBookBy: (nonnull NSUUID*) fileZimID NS_REFINED_FOR_SWIFT;

# pragma mark - Metadata

Expand Down
10 changes: 10 additions & 0 deletions Model/ZimFileService/ZimFileService.mm
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,16 @@ + (ZimFileMetaData *)getMetaDataWithFileURL:(NSURL *)url {
return metaData;
}

- (kiwix::Book) getBookBy:(nonnull NSUUID *) fileZimID {
kiwix::Book book = kiwix::Book();
try {
NSURL *fileURL = [ZimFileService.sharedInstance getFileURL: fileZimID];
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...



# pragma mark - URL Handling

- (NSURL *)getFileURL:(NSUUID *)zimFileID {
Expand Down
2 changes: 2 additions & 0 deletions Support/Kiwix-Bridging-Header.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
#import "ZimFileMetaData.h"
#import "SearchOperation.h"
#import "SearchResult.h"
#import "LibBookMarksBridge.h"
#import "LibBookmark.h"


NS_INLINE NSException * _Nullable objCTryBlock(void(^_Nonnull tryBlock)(void)) {
Expand Down
30 changes: 30 additions & 0 deletions Tests/LibBookmarkTests.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
//
// LibBookmarkTests.swift
// UnitTests

import XCTest
@testable import Kiwix

final class LibBookmarkTests: XCTestCase {

private let testZimURL: URL = Bundle(for: LibBookmarkTests.self).url(forResource: "test_small", withExtension: "zim")!
private let testTitle: String = "Dionysius of Halicarnassus On Literary Composition"
private let bookmarkURL = URL(string: "kiwix://FF7D59DE-0FD8-F486-09FA-C57904646707/A/Dionysius%20of%20Halicarnassus%20On%20Literary%20Composition.50212.html")!

func testIfZimFileExists() throws {
XCTAssertNotNil(testZimURL)
}

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.

let bookmarks = LibBookmarks.shared
XCTAssertFalse(bookmarks.isBookmarked(url: bookmarkURL))

let bookmark = LibBookmark(withUrl: bookmarkURL, withTitle: testTitle)!
bookmarks.addBookmark(bookmark)

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)

}
Binary file added Tests/TestResources/test_small.zim
Binary file not shown.