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

org-noter, nov epub-reader support initial stage #54

Closed sati-bodhi closed 1 year ago

sati-bodhi commented 1 year ago

@nobiot, I have forked the repo and refactored part of the code base to incorporate most of the recommended changes in our discussion #49 about nov epub-reader support with org-noter.

All except for the last bit that has to do with how new notes are added for epub files.

I've tested out on my system and everything seems to be working as expected.

I'd like to see if anything breaks on your side before amending the org-remark-highlight-add-note for better epub support.

nobiot commented 1 year ago

@sati-bodhi I appreciate your efforts. Thank you for putting the time.

In order not to waste your time and efforts, I must note a couple of things first.

  1. Please work on the dev/round-off branch. You have reported that you do not have access to it. How about simply doing something like this below? Unless it is a GitHub default feature, I'm not aware of allowing/disallowing users from certain branches.

    git clone https://github.com/nobiot/org-remark -b dev/round-off --depth 1 org-remark

    This branch is 14 commits ahead and I personally treat it as the main development branch; merging with the main is coming in a matter of days. I would not like you or me to spend time working on the merge conflict.

  2. Please try to discuss an overall design approach first before committing in an implementation

    It's great to see implemented code, and it would be a great way of communication if the purpose is to facilitate concrete discussions on the design. At the moment, I cannot accept this PR as is. I will make comments in the code review. There are some critical issues that prevent me from taking up your proposals.

    It is not my intention to discourage you from contributing to the project -- it's indeed great to see someone is so interested in supporting nov.el. If you agree, we should discuss an overall approach first.

Design for nov.el support

My initial hunch is that it should be a separate file as an extension, much like what I have done with eww with org-remark-eww.el.

I am also interested in supporting highlights for a line of the text file (and some other location pointers). Org-roam currently only supports a range between "start" and "end" points. This is inspired by W3C's Web Annotation's "fragment selector" for the text file, which has "Line Position". This might be applicable to epub and nov.el, perhaps. But I am not sure.

I guess there will be more things we might need to agree on. But this should do for now. I will now move to code review comments.

nobiot commented 1 year ago

Motivation for the PR

Fundamentally, why would you like Org-remark to work with Org-noter?

I do not really use Org-noter but as an outsider I would assume most of the features you are proposing are already available with it. What is the benefit for you if Org-noter and Org-remark work in tandem like this?

sati-bodhi commented 1 year ago

Motivation for the PR

Fundamentally, why would like Org-remark to work with Org-noter?

I do not really use Org-noter but as an outsider I would assume most of the features you are proposing are already available with it. What is the benefit for you if Org-noter and Org-remark work in tandem like this?

As I've mentioned briefly before, org-noter does not provide highlight-comment functionality like org-remark does. This functionality is covered by the wonderful pdf-tools for PDFs, but not for EPUBs; at least not with nov.el. (I have not tried out the other ebook reader because it lacks documentation.)

I use org-noter to write summaries and/or analyses across large swaths of ebook texts; a convenient tool like org-remark would help me paraphrase, make passing observations and/or concise responses to specific phrases or wordings in the text that can be used to build my analyses in org-noter.

To be sure, org-noter does provide an add-precise-note function that allows us to select a single point on screen to add a note. But it falls short of highlighting the word or phrase of interest, which is visually more eye-catching and easy to work with.

Screenshot from 2023-01-09 17-50-09

sati-bodhi commented 1 year ago
  1. You have reported that you do not have access to it. How about simply doing something like this below?

Thanks for the instruction. This is very helpful. Pardon me for being clumsy with git. This is the first time I am submitting a formal PR to a project.

  1. Please try to discuss an overall design approach first before committing in an implementation

Yes, a discussion of overall design would be good to have.

My initial hunch is that it should be a separate file as an extension

I believe my present setup has resolved about 80% of the issues related to nov support. If the highlights getting messed up issue #50 can be resolved, just make org-remark add notes differently for epub files and I'll be satisfied.

Since epub-reader support in my case would definitely require the use of org-noter, nov and (perhaps also) org-roam, if you are principled against having dependencies for the main module, I can support working on a separate file - but I need some sort of guidance as to how to do it properly.

I am also interested in supporting highlights for a line of the text file (and some other location pointers).

Web annotation is a different creature altogether, in my view. But I do believe some of the functions in that area can be shared with epub, since they are essentially the same thing under the hood.

nobiot commented 1 year ago

Thank you for your thoughtful reply. Just as a quick response, let me comment on this one point. I will come back for the other points.

I can support you. As you clarified your motivation for me, I think nov.el support will be a good addition.

My initial hunch is that it should be a separate file as an extension

I believe my present setup has resolved about 80% of the issues related to nov support. If the highlights getting messed up issue #50 can be resolved, just make org-remark add notes differently for epub files and I'll be satisfied.

Since epub-reader support in my case would definitely require the use of org-noter, nov and (perhaps also) org-roam, if you are principled against having dependencies for the main module, I can support working on a separate file - but I need some sort of guidance as to how to do it properly.

I have quickly tweaked your implementation and done below that does not require org-noter. It requires the latest commit 8004555 on dev/round-off branch, but the benefit is that:

;; compatibility with org-noter
(defun org-remark-get-epub-source ()
  "Returns the path of the epub source from which the present session is initiated."
  (when (eq major-mode 'nov-mode)
    (concat
     (file-name-nondirectory nov-file-name)
     "/"
     (file-name-base (cdr (aref nov-documents nov-documents-index))))))

(defun org-remark-load-after-render ()
  (org-remark-highlights-load))

(add-hook 'org-remark-source-find-file-name-functions #'org-remark-get-epub-source)
(add-hook 'nov-post-html-render-hook #'org-remark-load-after-render)

It's most likely me unfamiliar with org-noter but it requires a new frame dedicated to an org-noter "session" -- I feel that it's cumbersome.

Do you need org-remark to work together with org-noter like in your implementation? What would be the benefits for you?

This can be a basis for a "separate file" extension. You should be able to use it in combination with org-noter if your workflow requires it (to be verified).

nobiot commented 1 year ago
;; compatibility with org-noter
(defun org-remark-get-epub-source ()
  "Returns the path of the epub source from which the present session is initiated."
  (when (eq major-mode 'nov-mode)
    (concat
     (file-name-nondirectory nov-file-name)
     "/"
     (file-name-base (cdr (aref nov-documents nov-documents-index))))))

(defun org-remark-load-after-render ()
  (org-remark-highlights-load))

(add-hook 'org-remark-source-find-file-name-functions #'org-remark-get-epub-source)
(add-hook 'nov-post-html-render-hook #'org-remark-load-after-render)

Simplified it and made it lexical-binding.

;; -*- lexical-binding: t; -*-

;; compatibility with org-noter
(defun org-remark-get-epub-source ()
  "Returns the path of the epub source from which the present session is initiated."
  (when (eq major-mode 'nov-mode)
    (concat
     (file-name-nondirectory nov-file-name)
     "/"
     (file-name-base (cdr (aref nov-documents nov-documents-index))))))

(add-hook 'org-remark-source-find-file-name-functions #'org-remark-get-epub-source)
(add-hook 'nov-post-html-render-hook #'org-remark-highlights-load)
sati-bodhi commented 1 year ago

It's most likely me unfamiliar with org-noter but it requires a new frame dedicated to an org-noter "session" -- I feel that it's cumbersome.

Yes - this looks good. Less dependency is always better.

nobiot commented 1 year ago
;; -*- lexical-binding: t; -*-

;; compatibility with org-noter
(defun org-remark-get-epub-source ()
  "Returns the path of the epub source from which the present session is initiated."
  (when (eq major-mode 'nov-mode)
    (concat
     (file-name-nondirectory nov-file-name)
     "/"
     (file-name-base (cdr (aref nov-documents nov-documents-index))))))

(add-hook 'org-remark-source-find-file-name-functions #'org-remark-get-epub-source)
(add-hook 'nov-post-html-render-hook #'org-remark-highlights-load)

As I noted in #53, I have added support for the nov: link type. You can see the updated version of the source in the the new file named org-remark-nov.el in the new branch ded/nov.el.

sati-bodhi commented 1 year ago

@nobiot, I'd like to share some of my thoughts about the epub extension.

I am looking at org-remark-highlight-save from the dev/round-off branch.

(defun org-remark-highlight-save (filename beg end props &optional title)
  "Save a single HIGHLIGHT in the marginal notes file.

Return the highlight's data properties list (TODO refer to ...).

FILENAME is the name of source file with which the marginal notes
buffer is associated.  When the source buffer does not visit a
file (e.g. a website), it is the source buffer's file name
equivalent, such as the URL.

BEG and END are the range of the highlight being saved.  It
is the highlight overlay's start and end.

PROPS are the highlight overlay's properties.  Not all the
properties will be added as headline properties.  Refer to
`org-remark-notes-set-properties'.

For the first highlight of the source buffer, this function will
create a new H1 headline for it at the bottom of the marginal
notes buffer with TITLE as its headline text.

When called for a new highlight that is unsaved in the marginal
notes file, this function will create a new H2 headline with the
highlighted text as its headline text at the end of the H1
headline for the source buffer.

If a headline with the same ID already exists, update its
position and properties named \"org-remark-*\" and CATEGORY from
the highlight overlay.  For update, the headline text will be
kept intact, because the user might have changed it to their
needs.

This function will also add a normal file link as property
\"org-remark-link\" of the H2 headline entry, pointing back to
the source file with search option \"::line-number\", or for
non-file sources, calls `run-hook-with-args-until-success' for
`org-remark-highlight-link-to-source-functions' with FILENAME as
the argument.

ORGID can be passed to this function.  If user option
`org-remark-use-org-id' is non-nil, this function will add an
Org-ID link in the body text of the headline, linking back to the
source with using ORGID.

When the current source buffer is not set up for sync with notes,
this function calls `org-remark-notes-setup' to prepare the notes
buffer for automatic sync."
  (let* ((filename (org-remark-source-get-file-name filename))
         (id (plist-get props 'org-remark-id))
         (text (org-with-wide-buffer (buffer-substring-no-properties beg end)))
         (notes-buf (find-file-noselect (org-remark-notes-get-file-name)))
         (source-buf (current-buffer))
         (line-num (org-current-line beg))
         (orgid (org-remark-highlight-get-org-id beg))
         (link (if buffer-file-name
                   (concat "[[file:" filename
                           (when line-num (format "::%d" line-num)) "]]")
                 (run-hook-with-args-until-success
                  'org-remark-highlight-link-to-source-functions filename)))
         (notes-props))
    ;;; Set up notes buffer for sync for the source buffer
    (unless org-remark-source-setup-done
      (org-remark-notes-setup notes-buf source-buf))
    (with-current-buffer notes-buf
      (when (featurep 'org-remark-convert-legacy) (org-remark-convert-legacy-data))
      ;;`org-with-wide-buffer' is a macro that should work for non-Org file
      (org-with-wide-buffer
       (let ((file-headline
              (or (org-find-property
                   org-remark-prop-source-file filename)
                  (progn
                    ;; If file-headline does not exist, create one at the bottom
                    (goto-char (point-max))
                    ;; Ensure to be in the beginning of line to add a new headline
                    (when (eolp) (open-line 1) (forward-line 1) (beginning-of-line))
                    (insert (concat "* " title "\n"))
                    (org-set-property org-remark-prop-source-file filename)
                    (org-up-heading-safe) (point))))
             (id-headline (org-find-property org-remark-prop-id id)))
         ;; Add org-remark-link with updated line-num as a property
         (when link (plist-put props "org-remark-link" link))
         (if id-headline
             (progn
               (goto-char id-headline)
               ;; Update the existing headline and position properties
               ;; Don't update the headline text when it already exists
               ;; Let the user decide how to manage the headlines
               ;; (org-edit-headline text)
               (org-remark-notes-set-properties beg end props))
           ;; No headline with the marginal notes ID property. Create a new one
           ;; at the end of the file's entry
           (goto-char file-headline)
           (org-narrow-to-subtree)
           (goto-char (point-max))
           ;; Ensure to be in the beginning of line to add a new headline
           (when (eolp) (open-line 1) (forward-line 1) (beginning-of-line))
           ;; 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-body)))))
      notes-props)))

As before, I believe this section:

(org-with-wide-buffer
       (let ((file-headline
              (or (org-find-property
                   org-remark-prop-source-file filename)
                  (progn
                    ;; If file-headline does not exist, create one at the bottom
                    (goto-char (point-max))
                    ;; Ensure to be in the beginning of line to add a new headline
                    (when (eolp) (open-line 1) (forward-line 1) (beginning-of-line))
                    (insert (concat "* " title "\n"))
                    (org-set-property org-remark-prop-source-file filename)
                    (org-up-heading-safe) (point))))
             (id-headline (org-find-property org-remark-prop-id id)))
         ;; Add org-remark-link with updated line-num as a property
         (when link (plist-put props "org-remark-link" link))
         (if id-headline
             (progn
               (goto-char id-headline)
               ;; Update the existing headline and position properties
               ;; Don't update the headline text when it already exists
               ;; Let the user decide how to manage the headlines
               ;; (org-edit-headline text)
               (org-remark-notes-set-properties beg end props))
           ;; No headline with the marginal notes ID property. Create a new one
           ;; at the end of the file's entry
           (goto-char file-headline)
           (org-narrow-to-subtree)
           (goto-char (point-max))
           ;; Ensure to be in the beginning of line to add a new headline
           (when (eolp) (open-line 1) (forward-line 1) (beginning-of-line))
           ;; 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-body)))))

can be factored out into a separate add-notes function, which can subsequently be rewritten to accommodate the epub scenario.

My original thinking was simply to substitute the two add-notes function one for the other, based on different contexts under which org-remark is activated.

I've noted your preference for hooks, etc. in place of a simple if/else conditional. This should make epub-specific functionalities more flexible and extensible, which is a good thing.

However, I don't think epub support should merit a re-writing of the entire highlight-save function itself. So how the refactoring issue sits with the new dev/round-off update, as well as how other contextualizing strategies can be implemented to serve the purpose should be further deliberated and decided upon.

sati-bodhi commented 1 year ago

The new update looks good, by the way. No issues so far.

nobiot commented 1 year ago

However, I don't think epub support should merit a re-writing of the entire highlight-save function itself. So how the refactoring issue sits with the new dev/round-off update, as well as how other contextualizing strategies can be implemented to serve the purpose should be further deliberated and decided upon.

Thanks for your thoughts. I appreciate the fact that the discussion is, to me, more focused now than before.

I am open to refactoring; I have been meaning to do exactly that for -highlight-save. Before considering it, though...

What would you like to see for "epub support"? Is it the flexibility of headline levels that you mentioned before (where was it?), like this below?

* Book
** Chapter (or indexed "document" in `nov.el`)
*** Highlight/Notes
nobiot commented 1 year ago

Would I be too bold to suggest that we could perhaps these headline levels be an user option?

For H1, this line:

                    (insert (concat "* " title "\n"))

For H2 this line:

 (insert (concat "** " text "\n"))

They are currently hard-coded to be H1 (*) and H2 (**) but perhaps they can be any consecutive levels -- e.g. H1 & H2 (default), H2 & H3, H3 & H4, etc. as the user chooses.

sati-bodhi commented 1 year ago

This may be beyond the scope of the current project, but I see a lot of potential - as far as epub-reader support is concerned.

The issue is this: before my discovery of org-remark, I add highlights to my epub library primarily through Calibre's E-book Viewer. These are not easily accessible for consolidation in org-noter at present.

Again, pdfs are "more equal" than epubs in this case because org-noter has a create-skeleton function that can extract annotations out from the pdfs through pdf-tools. org-remark can now serve as an annotating device with nov; but notes previously taken in Calibre remain out of reach.

Annotations for a single Calibre Library are collectively stored in metadata.db, which can be accessed with sqlite3.

Here's the fun part: the calibredb.el project has already built up a whole ecosystem to access and process data stored in that database from Emacs.

This means, with a little bit of effort, we might just be able to integrate the two systems together to build an epub note-taking system on Emacs which is at least as good as the present pdf support.

Notes taken in Calibre can be imported into org-remark and accessed with nov; vice-versa. How cool would that be.

This also fits nicely with your intuition of having a separate module for epub support and integration.

nobiot commented 1 year ago

Annotations for a single Calibre Library are collectively stored in metadata.db, which can be accessed with sqlite3.

I appreciate your enthusiasm :) Not within the scope of the core feature set but it might be an interesting avenue for further extension.

Could I suggest that you raise a new issue on it to discuss further? No immediate work can be expected, at least of me. But calibredb.el looks to use Org mode’s derived mode for editing an annotation. At the moment, I use save-buffer to save the notes buffer (assuming it’s a file) but perhaps it does not need to be; which means… you might be able to directly work off an annotation buffer stored in calibredb? (Not sure if this is tenable).

sati-bodhi commented 1 year ago

What would you like to see for "epub support"? Is it the flexibility of headline levels that you mentioned before (where was it?)

Over here.

* Short Title of the Book
::PROPERTIES::
:org-remark-file: amazing_book.epub
::END::

** Chapter 10
::PROPERTIES::
:org-remark-file: amazing_book.epub/Chapter10
::END::

*** Good point there!
:PROPERTIES:
:org-remark-file: amazing_book.epub/Chapter10
:org-remark-beg: 16268
:org-remark-end: 16292
:org-remark-id: 2831c7f4
:org-remark-label: nil
:END:

Notes should be added at this level. 

Consider adding a `:org-remark-file:` property to these as well, so that they do not get lost in the crowd. 

Notes should be added at least H3 or beyond because each note must follow its nov-document or book chapter, and each chapter must follow the book title.

Two levels of checks would have to be performed in this case, instead of just one to make sure (1) the title node exists and (2) the chapter node exists before a new epub note is added.

Would I be too bold to suggest that we could perhaps these headline levels be an user option?

There are in fact some cases in which I would prefer a certain note to be at H4 ,or even H5 rather than H3 so that the relationship between ideas can be clearer.

But such cases are few and I don't think org-remark should support adding epub notes at levels other than H3 to avoid confusion due to too much freedom and flexibility.

It is trivial to adjust the level of a heading in org-mode for these exceptions anyway (just add a * to the bullet). I am not comfortable with the present default setting because I have to adjust heading level for virtually every new note I make in the ebook.

nobiot commented 1 year ago

I have done refactoring of the -highlight-save function including its signature/API. And implemented the facility to dynamically define headline levels per major-mode so that nov-mode source buffers can have 3 levels notes where the default is 2 levels just like now. See org-remark.el in dev/nov.el branch. It still has test/sample coding but overall it seems to work.

I'll come back tomorrow or later to document/explain/discuss with you further.

And... it's still work-in-progress and I am not married to the design yet; if there are better ways, I'm open to change it.

sati-bodhi commented 1 year ago

This should (probably) apply to regular notes as well (in cases, for example, where fill-paragraph is used:

When we add a new highlight note, if that highlight happens to span across two consecutive lines, the part of the text that is on the second line tends to be cut off from the main text body and inserted as part of the note content under the properties drawer.

Say I have [some 
text] highlighted like this. 

I would get:

** some
:PROPERTIES:
:org-remark-beg: 2604
:org-remark-end: 2679
:org-remark-id: c2097d20
:org-remark-label: nil
:END:
text

In my new note. 

You need to pre-process the text variable with (replace-regexp-in-string "\n" " " text) before passing it to (insert (concat "** " text "\n")) in highlight-save.

nobiot commented 1 year ago

You need to pre-process the text variable with (replace-regexp-in-string "\n" " " text) before passing it to (insert (concat "** " text "\n")) in highlight-save.

Good catch. I’ll have a look next chance I have

nobiot commented 1 year ago

Thank you

nobiot commented 1 year ago

image

Following on from yesterday's update, I have brought it to where you can automatically insert H1 and H2 for nov-mode.

The key logic is here.

;; excerpt from org-remark.el in dev/nov.el
  (let ((notes-props)
        (notes-headline-functions
         (cdr (or (assoc major-mode org-remark-notes-headline-functions)
                  (assoc 'default org-remark-notes-headline-functions)))))
    (with-current-buffer notes-buf
      (org-with-wide-buffer
        (dolist (pair notes-headline-functions)
          (let ((level (car pair))
                (fn (cdr pair)))
            (goto-char (funcall fn level source-buf notes-buf))
            (org-narrow-to-subtree)))

For default, outside nov-mode buffers, we would add just an H1 for the source file and H2 for highlights in it (no change of behaviour). This is defined by this list variable, which the logic above iterates through.

;; excerpt from org-remark.el in dev/nov.el
(defvar org-remark-notes-headline-functions
  '((default . ((1 . org-remark-highlight-add-source-headline-maybe)))))

I hope it is easy to see what it is doing. It's a list of cons cells and each cons cell is of this form:

(MODE . ((LEVEL1 . FUNCTION1)
         (LEVEL2 . FUNCTION2)
         ...
         (LEVELN . FUNCTIONN)))

MODE can be 'default, which is the default behaviour.
FUNCTION is called with LEVEL NOTES-BUF SOURCE-BUF as arguments

For nov.el, the extension would add the mode-specific element to the list like this to add H1 and H2. The heading text and properties are inserted by the function assigned to each headline level (also defined in the nov.el extension file).

;; excerpt from org-remark-nov.el in dev/nov.el
(add-to-list 'org-remark-notes-headline-functions
  '(nov-mode . ((1 . org-remark-nov-highlight-add-book-headline-maybe)
                (2 . org-remark-highlight-add-source-headline-maybe))))

It seems to work so far. What do you think? Again, still WIP and I will be happy to change the design if there is a better alternative.

On a side note, I have changed the naming convention of these functions from -highlight-save to -highlight-add, taking the hint from your proposal. They don't save the buffer to file; they just add or update headlines.

sati-bodhi commented 1 year ago

For default, outside nov-mode buffers, we would add just an H1 for the source file and H2 for highlights in it (no change of behaviour). This is defined by this list variable, which the logic above iterates through.

;; excerpt from org-remark.el in dev/nov.el
(defvar org-remark-notes-headline-functions
  '((default . ((1 . org-remark-highlight-add-source-headline-maybe)))))

For nov.el, the extension would add the mode-specific element to the list like this to add H1 and H2. The heading text and properties are inserted by the function assigned to each headline level (also defined in the nov.el extension file).

;; excerpt from org-remark-nov.el in dev/nov.el
(add-to-list 'org-remark-notes-headline-functions
  '(nov-mode . ((1 . org-remark-nov-highlight-add-book-headline-maybe)
                (2 . org-remark-highlight-add-source-headline-maybe))))

I think the above design looks clean and clear enough if you are already familiar with org-mode and elisp. But it could look daunting and confusing to a newbie who is trying to configure the headline functions for herself.

Perhaps this level of abstraction (at headline level) is not needed and we could simply use:

(defvar org-remark-notes-new-note-functions-maybe
 '(default . #'org-remark-highlight-new-regular-note))

(add-to-list 'org-remark-notes-new-note-functions-maybe
'(nov-mode . #'org-remark-nov-highlight-new-epub-note))

You could still use your headline-functions within these new-note functions if that level of control and abstraction is needed.

What I would like to see is a clear-cut new-note processing logic for each use case scenario with each new-note-function and not be bothered with the different ways a new headline can be processed and added.

sati-bodhi commented 1 year ago

A separate point is this:

With the present design, the following idiom keeps cropping up:

            (goto-char (point-max))
            ;; Ensure to be in the beginning of line to add a new headline
            (when (eolp) (open-line 1) (forward-line 1) (beginning-of-line))
            (insert-char (string-to-char "*") level)  ;; set headline level
            (insert (concat " " title "\n"))  ;; set headline caption
            (org-set-property org-remark-prop-source-file source-name)  ;; set properties

I would probably use a wrapper like this to abstract out the detail:

(defun org-remark-new-headline-maybe (level caption property value)
"Set new org-remark-highlight-note headline with properties."
  (goto-char (point-max))
  ;; Ensure to be in the beginning of line to add a new headline
  (when (eolp) (open-line 1) (forward-line 1) (beginning-of-line))
  (insert-char (string-to-char "*") level)   ;; set headline level
  (insert (concat " " caption "\n"))           ;; set headline caption
  (org-set-property property value) ;; set properties; * leave out of wrapper function if not universally applicable. *
  )

I see org-remark-notes-set-properties is used in place of org-set-property, with a different set of arguments, in the highlight-add-or-update-highlight-headline function. We could either leave it out of the wrapper or find some other way to accommodate the different use-cases .

sati-bodhi commented 1 year ago

I think the algorithm for processing a new note is largely similar and essentially the same at note level for both contexts (as reflected by your re-use of org-remark-highlight-add-source-headline-maybe in the nov-mode setting):

  1. Look for parent node.
  2. If parent node is present, add new subheading under the parent node.
  3. If parent note is not present, add parent node together with new subheading under the parent node.

I suppose this is your rationale for having a universal highlight-add-or-update-highlight-headline function not tied to the context settings, but I am not so sure - especially how this is supposed to work hand-in-hand with your add-headline functions, with their separately defined (insert (concat (insert-char (string-to-char "*") level) " " text "\n")) code.

The above processing logic can, in fact, be used to process both H1->H2 as well as H2->H3 scenarios. My intuition would be to pack that logic into a processing-template function and feed different new-node functions into it.

Haven't really tried out your code to see how it works, so the above are just some observations that need further testing to see if they can and should be implemented.

sati-bodhi commented 1 year ago

Overall, I am happy to go with your current design if it works without issues. Thank you for your enthusiastic support!

nobiot commented 1 year ago

Thank you for your enthusiastic support!

Thank for your engagement; I believe the enthusiasm so far has been mutual :)

These are good points and suggestions.

I think the above design looks clean and clear enough if you are already familiar with org-mode and elisp. But it could look daunting and confusing to a newbie who is trying to configure the headline functions for herself.

I would probably use a wrapper like this to abstract out the detail:

(defun org-remark-new-headline-maybe (level caption property value)

Just very quickly, a couple of points:

  1. I will be spending some time this week and weekend to wrap up the dev/round-off branch to merge it with main. I will come back to dev/nov.el after this -- hopefully next week (or earlier if things work out well).

  2. With the point of "daunting" configuration for different headline levels, I am considering of providing a minor-mode, so end users can put (org-remark-nov-mode +1) in their configuration -- the detail of the defvar is exposed only to extension developers and skilled tinkerers.

  3. I will consider your suggestions on further refactoring with defvar org-remark-notes-new-note-functions-maybe and org-remark-new-headline-maybe

  4. I have done a quick implementation locally on my PC for adjusting the positions for the highlights displacement caused by the <table> tag -- It seems to work, so I will push it to dev/nov.el branch for you to try out (not happening today, but hopefully soon)

Now... In the meantime, could I suggest that you consider doing the FSF paperwork, so that your code can be directly merged -- so far I get suggestions from you and I get to put the code in. But it might be better if you had the freedom to contribute the code to the project.

As noted in the README, the FSF paperwork is required if you contribute more than 15 lines of code. Here Org mode's website gives you how to proceed with the paperwork: https://orgmode.org/contribute.html#copyright

If you want to contribute more than 15 lines of code to Org mode you will need to assign the copyright for your contributions to the FSF so that they can be included in GNU Emacs. To start the copyright assignment process fill out this form and email it to assign@gnu.org.

sati-bodhi commented 1 year ago

As noted in the README, the FSF paperwork is required if you contribute more than 15 lines of code. Here Org mode's website gives you how to proceed with the paperwork: https://orgmode.org/contribute.html#copyright

I've sent FSF my application. Waiting for their reply.

nobiot commented 1 year ago

Time to move :)