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

Html / Epub reader implementation #836

Open
wants to merge 53 commits into
base: master
Choose a base branch
from

Conversation

michalrentka
Copy link
Contributor

No description provided.

@jarbus
Copy link

jarbus commented Jan 29, 2024

I've literally been stalking this repo since last November because I've been so excited for epub support in Zotero haha, so great to see this PR

Copy link
Contributor

@mvasilak mvasilak left a comment

Choose a reason for hiding this comment

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

Some first comments. More will follow.

scripts/bundle_reader.sh Outdated Show resolved Hide resolved
Zotero/Scenes/Detail/DetailCoordinator.swift Outdated Show resolved Hide resolved
Zotero/Models/Files.swift Show resolved Hide resolved
@chenggiant
Copy link

Looking forward to the epub and html support!

@haidar47x
Copy link

Glad to see it being implemented. :)

@mvasilak
Copy link
Contributor

mvasilak commented Oct 15, 2024

@michalrentka some comments from testing ePUB attachments.

General

  • When I download an attachment to open, rarely, it opens empty and I have to close the reader and open it again, for the document to appear. Could there be a potential race issue when downloading, copying to temporary folder, and loading the document?
  • For images in the document, e.g. a cover, I can hold-tap to get a context menu, which is nice, unless we think it may cause some confusion.
Wuthering Heights
  • I can even select text on an image, which may be confusing if a user tries to annotate it (like I tried 😅).
Wuthering Heights

Annotations

  • Creating an annotation may create 2 annotations with some text overlapping.
Screenshot 2024-10-15 at 15 10 23
  • Creating further annotations, may not be rendered, although text is selected, and you get an error like:

[ERROR] Zotero(+0000001): HtmlEpubDocumentViewController: navigating to 6NREE26N failed - Error Domain=WKErrorDomain Code=4 "A JavaScript exception occurred" UserInfo={WKJavaScriptExceptionLineNumber=31037, WKJavaScriptExceptionMessage=Error: Unsupported FragmentSelector.conformsTo: undefined, WKJavaScriptExceptionColumnNumber=103, WKJavaScriptExceptionSourceURL=file:///.../view.js, NSLocalizedDescription=A JavaScript exception occurred}. [(155) HtmlEpubDocumentViewController.selectInDocument(key:); com.apple.main-thread]

  • Closing and re-opening the document after, in both iOS & macOS, will show a blank screen, as it seems corrupted. (No errors are logged in iOS log when re-opening)
  • Trying to delete an annotation from the sidebar in this state, logs this error (although the annotation is deleted):

[ERROR] Zotero(+0000000): HtmlEpubDocumentViewController: updating document failed - Error Domain=WKErrorDomain Code=4 "A JavaScript exception occurred" UserInfo={WKJavaScriptExceptionLineNumber=31037, WKJavaScriptExceptionMessage=Error: Unsupported FragmentSelector.conformsTo: undefined, WKJavaScriptExceptionColumnNumber=103, WKJavaScriptExceptionSourceURL=file:///.../view.js, NSLocalizedDescription=A JavaScript exception occurred}. [(167) HtmlEpubDocumentViewController.updateView(modifications:insertions:deletions:); com.apple.main-thread]

  • Deleting all the annotations (only possible in iOS), eliminates the corrupted state, and re-opening the document displays it as expected.

Rendering

  • It’s not always the same in macOS and iOS, which makes sense, but I may see empty pages in iOS, that I don’t see in macOS.
  • Decreasing reader height, e.g. by moving the toolbar to the top, may hide some of the bottom content.
  • Changing reader size in any way, e.g. by rotating, will render annotation rects in the wrong place.
Screenshot 2024-10-15 at 15 14 32

Navigation

  • It would be nice to have a table of contents in the sidebar.
  • It would be nice to have page controls in the toolbar.
  • Tapping a link would be nice to create a back button.

@mvasilak
Copy link
Contributor

@michalrentka some comments from testing snapshot attachments.

General

  • Taping an external link doesn't open it in a the browser or an in-app safari view controller. In macOS it is opened in browser.
  • Snapshots with existing annotations, perhaps created in previous versions of the macOS client, seem to have breaking changes, resulting in annotations not showing in iOS and logging errors even for selecting an annotation tool. I'm not sure if this will be an issue when we release the feature for iOS.

Annotations

Creating annotations from text selection is not very accurate, and could result in:

  • Missing characters from start or end.
  • Adding more characters than selected.
  • Drawing an annotation that e.g. underlines more than the annotation's text, usually a whole paragraph.
  • Creating more than one annotations at the same time.

These issues seem to stem from the inconsistency of the text selection box and the annotation being drawn, as if two separate text selections are considered in the end. Could be related to web view configuration.

Check the following example:

snapshots-in-ios.mov

@michalrentka
Copy link
Contributor Author

  • For images in the document, e.g. a cover, I can hold-tap to get a context menu, which is nice, unless we think it may cause some confusion.
Wuthering Heights * I can even select text on an image, which may be confusing if a user tries to annotate it (like I tried 😅). Wuthering Heights

@dstillman what do you think about this? In my opinion this is actually fine as it's a feature that iOS provides. Yes it might be confusing when you try to select text and annotate it and you don't notice that you're actually selecting text in an image (so you don't have any option to annotate), but I guess that's on you. It should be possible to disable this functionality if you think it's counterproductive though.

@michalrentka
Copy link
Contributor Author

@mvasilak you can try to re-test. Hopefully all or at least most issues should be fixed now. Some were caused by me incorrectly passing the data to reader, others were in the reader itself. I left image handling as is for now.

@mvasilak
Copy link
Contributor

mvasilak commented Nov 6, 2024

@michalrentka things look improved. Some new observations:

both ePUBS & snapshots

  • onSetSelectionPopup is ignored if an annotation tool is selected, is this intended? This breaks muscle memory learned from PDF reader.
  • Page number label in annotation sidebar cells/popovers, as well as page edit actions, should be omitted, same as in desktop.
  • Annotation tools remains open after toolbar has been closed.

snapshots only

  • Selecting an annotation with sidebar open doesn’t scroll to its cell.
  • Annotations that cover more than the visible screen, e.g. created in desktop by selecting text and scrolling, may not always show a popover, but still may cause a dismissal event, which w/o presented popover will dismiis the reader.

@michalrentka michalrentka force-pushed the reader branch 2 times, most recently from aab23bc to 513da43 Compare November 29, 2024 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants