nobiot / org-remark

Highlight & annotate text, EWW, Info, and EPUB
https://nobiot.github.io/org-remark/
GNU General Public License v3.0
436 stars 22 forks source link

`nov` epub-reader and `org-noter` support #49

Closed sati-bodhi closed 1 year ago

sati-bodhi commented 1 year ago

Regarding @darcamo's nov epub-reader support request above, I find:

(defun org-remark-source-find-file-name ()
  "Assumes that we are currently in the source buffer.
Returns the filename for the source buffer.  We use this filename
to identify the source buffer in all operations related to
marginal notes."
  (let ((filename (or buffer-file-name
                      (run-hook-with-args-until-success
                       'org-remark-source-find-file-name-functions))))
    filename))

It seems to me org-remark-source-find-file-name-functions can be customized to make use of, say, in-built org-noter functions that return the epub source file referenced by the org-noter--session.

Here is my preliminary setup:

  ;; compatibility with org-noter
  (defun org-noter-get-epub-source ()
    "Returns the path of the epub source from which the present session is initiated."
    (org-noter--session-property-text org-noter--session))

  (setq org-remark-source-find-file-name-functions '(org-noter-get-epub-source))

I was able to highlight and add notes to epub files viewed in nov with this simple setup.

Only issue is, the notes file, marginalia.org in this case is stored at the temporary location where the session.10e8598b3bc0292941167275239838173700000033310232 instance lives - ~/.emacs.d/.local/cache - by default.

Tried setting org-remark-notes-file-name to an absolute path, but wasn't able to change this behavior.

Any leads?

Originally posted by @sati-bodhi in https://github.com/nobiot/org-remark/issues/21#issuecomment-1374314220

sati-bodhi commented 1 year ago

The rationale for the above feature/pull-request is this:

  1. nov loads each buffer from a temp file with a randomized identifier for each session. On top of that, it doesn't provide a function to trace back to the epub source file. So working with nov alone does not fit our purposes in this case.
  2. org-noter does have a insert-precise-note function, which allows the user to select a point in the document to insert a note. A red arrow appears on screen to indicate the note location (at line level, at best) when the relevant note is accessed. This is visually and substantially different from the highlight with comments functionality that org-remark provides. It certainly adds value to the present epub-reader setup, as far as note-taking is concerned.
nobiot commented 1 year ago

Hi @sati-bodhi thank you for this. I will be flying soon, so I will look more carefully when I have landed and arrived back home.

Just real quick on this question:

Tried setting org-remark-notes-file-name to an absolute path, but wasn't able to change this behavior.

Any leads?

org-remark-notes-file-name can be either a string or a function. I think for your case setting a (custom) function will solve the issue. Let me know if this does not work or does not make sense; I will come back.

sati-bodhi commented 1 year ago

Only issue is, the notes file, marginalia.org in this case is stored at the temporary location where the session.10e8598b3bc0292941167275239838173700000033310232 instance lives - ~/.emacs.d/.local/cache - by default.

After some further research, I realize the cache directory in question above is actually the user-emacs-directory variable defined in subr.el. Since things like org-roam.db and projectile.projects live there, apparently, it is not exactly a "temporary directory" as I deem it to be.

Yet, by virtue of it being a cache location for user-specific settings, I am concerned about its persistence in situations such as when a fresh install of doom-emacs is needed.

Most files in that directory can be built from scratch during a fresh install. Not quite for a personal reading notes file like marginalia.org.

nobiot commented 1 year ago

I think we just missed each other. Does my suggestion above work for you?

nobiot commented 1 year ago

I think we just missed each other. Does my suggestion above work for you?

sati-bodhi commented 1 year ago

Does my suggestion above work for you?

@nobiot, not sure what sort of custom function you had in mind, but this works for me.

  (setq org-remark-notes-file-name (expand-file-name "marginalia.org" org-roam-directory))

Now I can have marginalia.org where I want it to be. But curiously, org-remark-notes-file-name-function still returns "/home/sati/.emacs.d/.local/cache/marginalia.org".

(defun org-remark-notes-file-name-function ()
  "Return a marginal notes file name for the current buffer.

This is the default function for the customizing variable
`org-remark-notes-file-name' for its function option.

When the current buffer is visiting a file, the name of marginal
notes file will be \"FILE-notes.org\", adding \"-notes.org\" as a
suffix to the file name without the extension."
  (if buffer-file-name
      (concat (file-name-sans-extension
               (file-name-nondirectory (org-remark-source-find-file-name)))
              "-notes.org")
    ;; If buffer is not visiting a file, a default file name.  If this
    ;; file name is not suitable, either override the function or set
    ;; the user option to a custom function.
    (expand-file-name "marginalia.org" user-emacs-directory)))

I would suggest changing user-emacs-directory above to a more convenient and logical location for notes, like org-directory or org-roam-directory.

Thanks for the excellent package, by the way ^_^

sati-bodhi commented 1 year ago

not sure what sort of custom function you had in mind, but this works for me

Oh, I think I get it now.

org-remark-notes-file-name-function is the default function used to generate the file names (and file paths) of org-remark notes file(s), specific to each buffer where the notes are taken.

We can customize this function to get our notes where we want.

sati-bodhi commented 1 year ago

It seems to me org-remark-source-find-file-name-functions can be customized to make use of, say, in-built org-noter functions that return the epub source file referenced by the org-noter--session.

On a side-note, org-remark-source-find-file-name-functions is originally not defined. You should probably defvar it somewhere in your package and get users to extend it.

sati-bodhi commented 1 year ago

One issue with the setup above is this: org-remark interferes with the org-noter--create-session process and prevents it from launching the nov epub-reader when global-tracking-mode is set to t.

Debugger entered--Lisp error: (wrong-type-argument org-noter--session nil)
  signal(wrong-type-argument (org-noter--session nil))
  (or (progn (and (memq (type-of org-noter--session) cl-struct-org-noter--session-tags) t)) (signal 'wrong-type-argument (list 'org-noter--session org-noter--session)))
  (progn (or (progn (and (memq (type-of org-noter--session) cl-struct-org-noter--session-tags) t)) (signal 'wrong-type-argument (list 'org-noter--session org-noter--session))) (aref org-noter--session 10))
  org-noter-get-epub-source()
  org-remark-source-find-file-name()
  org-remark-highlights-load()
  org-remark-mode(1)
  org-remark-auto-on()
  run-hooks(find-file-hook)
  #<subr after-find-file>(nil t nil nil nil)
  apply(#<subr after-find-file> (nil t))
  (progn (fset #'sit-for #'ignore) (apply fn args))
  (unwind-protect (progn (fset #'sit-for #'ignore) (apply fn args)) (fset #'sit-for old))
  (let* ((old (symbol-function #'sit-for))) (unwind-protect (progn (fset #'sit-for #'ignore) (apply fn args)) (fset #'sit-for old)))
  doom--shut-up-autosave-a(#<subr after-find-file> nil t)
  apply(doom--shut-up-autosave-a #<subr after-find-file> (nil t))
  #f(advice-wrapper :around #<subr after-find-file> doom--shut-up-autosave-a)(nil t)
  apply(#f(advice-wrapper :around #<subr after-find-file> doom--shut-up-autosave-a) (nil t))
  #f(advice-wrapper :before #f(advice-wrapper :around #<subr after-find-file> doom--shut-up-autosave-a) chain-doom-first-buffer-hook-to-find-file-hook-h ((depth . -101)))(nil t)
  apply(#f(advice-wrapper :before #f(advice-wrapper :around #<subr after-find-file> doom--shut-up-autosave-a) chain-doom-first-buffer-hook-to-find-file-hook-h ((depth . -101))) (nil t))
  after-find-file(nil t)
  find-file-noselect-1(#<buffer 印順-1971-成佛之道.epub<3>> "~/Calibre Library/Yin Shun/Cheng Fo Zhi Dao (111)/..." nil nil "~/Calibre Library/Yin Shun/Cheng Fo Zhi Dao (111)/..." (37356917 2049))
  find-file-noselect("~/Calibre Library/Yin Shun/Cheng Fo Zhi Dao (111)/...")
  org-noter--create-session((headline (:raw-value "成佛之道" :begin 125 :end 284 :pre-blank 0 :contents-begin 132 :contents-end 284 :robust-begin nil :robust-end nil :level 1 :priority nil :tags nil :todo-keyword nil :todo-type nil :post-blank 0 :footnote-section-p nil :archivedp nil :commentedp nil :post-affiliated 125 :CUSTOM_ID "印順_成佛道_1971" :TYPE "book" :AUTHOR "印順" :NOTER_DOCUMENT "~/Calibre Library/Yin Shun/Cheng Fo Zhi Dao (111)/..." :title "成佛之道" :mode first-section :granularity greater-element ...) (section (:begin 132 :end 284 :contents-begin 132 :contents-end 284 :robust-begin 132 :robust-end 282 :post-blank 0 :post-affiliated 132 :mode section :granularity greater-element :parent #1) (property-drawer (:begin 132 :end 284 :contents-begin 145 :contents-end 278 :post-blank 0 :post-affiliated 132 :mode planning :granularity greater-element :parent #4)))) "~/Calibre Library/Yin Shun/Cheng Fo Zhi Dao (111)/..." "/home/sati/org/roam/reading_notes/印順_成佛道_1971.org")
  org-noter(nil)
  funcall-interactively(org-noter nil)
  command-execute(org-noter)
sati-bodhi commented 1 year ago

Another issue is that org-remark does not differentiate notes at document level. This causes notes created for different chapters of an ebook to be lumped together on the current chapter displayed on buffer.

Is there a way to make use of the ids in noter-documents alist to create a more specific identifier for each chapter?

Does the identifier have to be a working file path?

sati-bodhi commented 1 year ago

Another issue is that org-remark does not differentiate notes at document level. This causes notes created for different chapters of an ebook to be lumped together on the current chapter displayed on buffer.

Is there a way to make use of the ids in noter-documents alist to create a more specific identifier for each chapter?

Does the identifier have to be a working file path?

This setup works perfectly for me.

  (defun org-noter-get-epub-source ()
    "Returns the path of the epub source from which the present session is initiated."
    (if org-noter--session
        (concat
         (org-noter--session-property-text org-noter--session)
         "/"
         (file-name-base (cdr (aref nov-documents nov-documents-index))))
      nil))

As discussed above, the alist nov-documents is used to provide the file-base-name of the document, which is appended to the epub filepath.

The document identifier given by car is not used because file-base-name is typically more meaningful and easy to identify.

sati-bodhi commented 1 year ago

Related to the setup proposed above would be trivial stuff such as heading level for each new note and the way epub notes are organized. This would be slightly different from one we would expect from a regular org-mode buffer because new notes are added at chapter level. For example:

* Book Title
** Chapter 10
::PROPERTIES::
:org-remark-file: amazing_book.epub/Chapter10
::END::
*** Good point there!
Notes should be added at this level. 
nobiot commented 1 year ago

It is good that you seem to be solving your problems on your own.

I do not use Nov.el or Org-noter, so it is not easy to fully understand the remaining issue.

With the current design, the notes file would need to have a fixed format. The example of the book and Chapter 10 should look like this:

* Chapter 10
::PROPERTIES::
:org-remark-file: amazing_book.epub/Chapter10
::END::
** Good point there!
Notes should be added at this level. 

H1 should contain org-remark-file. Its value should be a file name or "file name equivalent" that uniquely locate the source; e.g. URL. In this case, amazong_book.epub/Chapter10 looks to meet the criteria.

H2 headlines are each highlight within the "file".

sati-bodhi commented 1 year ago

With the current design, the notes file would need to have a fixed format.

I suppose this is just an issue of how new notes are created and how flexible you code them to be. For my use-case, after some manual adjustment, I can view and edit my notes under H3 heading just fine, so there should be no "design limitation" issue in the sense that notes only work when org-remark-file is in H1.

If we can make epub notes the exception, things should be fine.

sati-bodhi commented 1 year ago

I am looking at this section of the org-remark-highlight-save function:

           ;; Create a headline
           ;; Add a properties
           (insert (concat "** " text "\n"))
           (org-remark-notes-set-properties beg end props)
           (when (and orgid org-remark-use-org-id)
             (insert (concat "[[id:" orgid "]" "[" title "]]"))))
         (setq notes-props (list :body (org-remark-notes-get-text)))))

I think all we have to do is to add an if-else condition to check if FILENAME is an epub file. Customize create headline to do things differently for the case of epub files.

Sections such as these in org-remark-highlight-save can probably be re-written as sub-functions to keep things clean and tidy.

nobiot commented 1 year ago

to add an if-else condition to check if FILENAME is an epub file

Please don't use if-else conditions for a single file extension. We should find an alternative. e.g. hooks or polymorphism via cl-defmethod, etc.

org-remark-highlight-save can probably be re-written as sub-functions to keep things clean and tidy.

I agree. However, we would need to conduct thorough regression tests; this function is a fundamental piece of Org-remark. I have just been making major changes via dev/round-off branch after I have made a mistake of prematurely merging changes to main and broke other people's stable workflow. I would like to avoid these incidents.

Perhaps we should start with creating automatic unit tests... This should not prevent you or me from moving forward with refactoring, but something to keep in mind.

nobiot commented 1 year ago

nov.el support added and released as part of version 1.2.1. Closing this issue. Thank you for your great collaboration, @sati-bodhi. Without you, the feature would not have been added.