Skip to content
This repository was archived by the owner on Nov 26, 2020. It is now read-only.

Add bookmarks #328

Closed
wants to merge 4 commits into from
Closed

Add bookmarks #328

wants to merge 4 commits into from

Conversation

omaralbeik
Copy link

Hi guys 👋

We're trying to add bookmarking capability to FolioReaderKit

Here is what I did so far

  1. Add a new allowBookmarking and localizedBookmarkTitle to FolioReaderConfig
  2. Add new bookmark button to the navigation bar
  3. Created a new Bookmark model (copied most of the code from the Highlight model) 😅
  4. Create a new BookmarkList UItableViewController
  5. Add a new Bookmarks segment next to Contents and Highlights

What I was not able to do:

  1. Get the current fragment visible on screen so I can forward the user back when tapping on a bookmark in the BookmarkList view controller
  2. Set the navigation button's isHighlighted to true or false based on whether the currently visible page was bookmarked or not

@hebertialmeida your help is highly appreciated, it would be nice to see Bookmark capability in FolioReaderKit soon 💯

@omaralbeik
Copy link
Author

cc @soyhanbeyazit

@hebertialmeida hebertialmeida self-assigned this Mar 27, 2018
@omaralbeik
Copy link
Author

Any update on this @hebertialmeida :)

@hebertialmeida
Copy link
Member

Hey @omaralbeik, I haven't time to review that yet, but I will soon, I am on vacation right now... Sorry.

rodrigorsa
rodrigorsa reviewed Apr 3, 2018
View reviewed changes
Source/FolioReaderBookmarkList.swift Outdated
override func viewDidLoad() {
super.viewDidLoad()

self.tableView.register(UITableViewCell.self, forCellReuseIdentifier: kReuseCellIdentifier)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you remove all unnecessary self?

Source/FolioReaderBookmarkList.swift Outdated
super.viewDidLoad()

self.tableView.register(UITableViewCell.self, forCellReuseIdentifier: kReuseCellIdentifier)
self.tableView.separatorInset = UIEdgeInsets.zero
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you remove UIEdgeInsets?
tableView.separatorInset = .zero

Source/FolioReaderBookmarkList.swift Outdated

// MARK: - Table view data source

override func numberOfSections(in tableView: UITableView) -> Int {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could remove this method

Source/FolioReaderBookmarkList.swift Outdated

override func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
let cell = tableView.dequeueReusableCell(withIdentifier: kReuseCellIdentifier, for: indexPath)
cell.backgroundColor = UIColor.clear
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you remove UIColor?
cell.backgroundColor = .clear

Source/FolioReaderBookmarkList.swift Outdated
let cell = tableView.dequeueReusableCell(withIdentifier: kReuseCellIdentifier, for: indexPath)
cell.backgroundColor = UIColor.clear

let bookmark = bookmarks[(indexPath as NSIndexPath).row]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why (indexPath as NSIndexPath)?

Source/Models/Bookmark+Helper.swift
let realm = try Realm(configuration: readerConfig.realmConfiguration)
bookmarks = realm.objects(Bookmark.self).filter(predicate).toArray(Bookmark.self)
return (bookmarks ?? [])
} catch let error as NSError {
Copy link
Contributor

Choose a reason for hiding this comment

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

here too let error as NSError

Source/Models/Bookmark+Helper.swift
do {
let realm = try Realm(configuration: readerConfig.realmConfiguration)
bookmarks = realm.objects(Bookmark.self).filter(predicate).toArray(Bookmark.self)
return (bookmarks ?? [])
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra parenthesis (...)

Source/Models/Bookmark+Helper.swift Outdated
do {
let realm = try Realm(configuration: readerConfig.realmConfiguration)
bookmarks = realm.objects(Bookmark.self).toArray(Bookmark.self)
return (bookmarks ?? [])
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra parenthesis (...)

Source/Models/Bookmark+Helper.swift Outdated
let realm = try Realm(configuration: readerConfig.realmConfiguration)
bookmarks = realm.objects(Bookmark.self).toArray(Bookmark.self)
return (bookmarks ?? [])
} catch let error as NSError {
Copy link
Contributor

Choose a reason for hiding this comment

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

here too let error as NSError

Source/Models/Bookmark.swift

/// A Bookmark object
open class Bookmark: Object {
@objc open dynamic var bookId: String!
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you using String!? Could it be optional? If not, you shouldn't use !

@omaralbeik
Copy link
Author

Hey @rodrigorsa, thank you for your review, I updated the submission with fixes to some points you mentioned above.

  • removed extra selfs, I'm not a fan of stating it everywhere as well, I added it to follow the same coding style of the project - look at FolioReaderHighlightList :)
  • as of the NSError, it is required because the Completion object expects NSError

I'd recommend adding SwiftLint to catch this kind of warnings, however, I'm afraid there are tons of changes required as the current code base follows a different code style

@hebertialmeida
Copy link
Member

This PR is really good, the only problem that I have not merged yet is the same problem with #334.

Basically, we need to solve locator in the epub, that is the biggest problem we have right now, most of the bugs are highlight related and that being said anything we add will just increase bug reports.

Do you have experience with epub locators like CFI? So you might help us solve this problem before merging that.

If we merge and people start using it we will have to probably implement some migration solution because the highlights format probably will change.

@shalinipd
Copy link

@hebertialmeida @rodrigorsa Is this is being merged in the Repository as We are also looking for this (Bookmark) feature.
I have one question - Do Book search feature available in the Repository.

@shubhankarbhavsar
Copy link

Hi @hebertialmeida ,

This library is good but has some bugs related to highlights. Especially when trying to highlight multiple paragraphs at once.

@omaralbeik Have you completed the bookmark feature? I require this feature for one of my projects. Still working on my own implementation for it.

@shalinipd
Copy link

Anyone is ware that library have Book Search feature or not?

@ynpl
Copy link

ynpl commented Jun 12, 2019

When can this library implement the bookmark function?

@omaralbeik omaralbeik closed this Jun 16, 2019
@omaralbeik
Copy link
Author

Sorry, I can't find the time to work on this feature anymore 😅

@shalinipd
Copy link

Hi @hebertialmeida : We are facing issue while highlighting multiple paragraphs at once. Can you please provide solution for the same.if anyone else has resolved it please provide the solution.

@shubhankarbhavsar
Copy link

Hi @shalinipd : I had faced the same issue. I end up integrating rangy library in iOS to resolve this and that gives me an advantage as both platforms iOS and Android now have the same approach for Highlight.

@shalinipd
Copy link

shalinipd commented Jul 17, 2019 via email

@shalinipd
Copy link

@shubhankarbhavsar If you can share the code base that will be helpful.

@shubhankarbhavsar
Copy link

@shalinipd Unfortunately I do not have the code but I can help and guide.

@shubhankarbhavsar
Copy link

Hey ..thanks for replying.Can you give us the link of library as we are searching the same for iOS and have not found.
…
On Wed, Jul 17, 2019 at 12:49 PM shubhankarbhavsar @.***> wrote: Hi @shalinipd https://github.com/shalinipd : I had faced the same issue. I end up integrating rangy library in iOS to resolve this and that gives me an advantage as both platforms iOS and Android now have the same approach for Highlight. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#328?email_source=notifications&email_token=ABWPL7646R2DTZHUXZ5C7OTP73BY7A5CNFSM4EXL7FF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2DI62A#issuecomment-512135016>, or mute the thread https://github.com/notifications/unsubscribe-auth/ABWPL7ZIVUX4FKKEZ2KTSX3P73BY7ANCNFSM4EXL7FFQ .

Please use rangy js files from FolioReader Android project.

@shalinipd
Copy link

shalinipd commented Jul 31, 2019 via email

@shalinipd
Copy link

Hi @hebertialmeida ,@shubhankarbhavsar We were able to integrate the rangy lib and successfully see the highlight by using "highlighselection" method but how to persist it so that redirection can be achieved.Can you please guide us on the same.

@shubhankarbhavsar
Copy link

@shalinipd You can follow a similar approach as Highlight works here. As you said you've already achieved the highlighting part which means there is only one step left which is to return parameters from the highlight function. Highlighter's highlightSelection() method returns the highlightRangy string and you can prepare a custom JSON object by adding this in the parameter list and return it from the highlight function. After that, you can save this JSON object in any database like realm or core data.

@shalinipd
Copy link

shalinipd commented Aug 7, 2019 via email

@shubhankarbhavsar
Copy link

@shalinipd
This is what I'm explaining in the last message.

var highlightRangy = this.highlighter.highlightSelection(color, null, highlightId);
var params = { content: rangeString, rangy: highlightRangy, color: color, id: highlightId };
return JSON.stringify(params);

Expected Rangy string returned in highlightRangy param should be like this: 69$79$1$highlight-yellow$

Please note that you must use the rangy files that are attached in FolioReader Android Project.

@shalinipd
Copy link

shalinipd commented Aug 26, 2019 via email

@shubhankarbhavsar
Copy link

Hi Shalini,

Congratulations on the success. I've not faced issue while highlighting table contents. I'll require code access to understand the issue. If possible please share something to look into.

@shalinipd
Copy link

shalinipd commented Aug 27, 2019 via email

@shalinipd
Copy link

shalinipd commented Aug 29, 2019 via email

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rodrigorsa rodrigorsa rodrigorsa left review comments

Assignees

@hebertialmeida hebertialmeida

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@omaralbeik @hebertialmeida @shalinipd @shubhankarbhavsar @ynpl @rodrigorsa