nobiot / org-remark

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

Possible to turn on mode buffer-local? #74

Closed akashpal-21 closed 5 months ago

akashpal-21 commented 9 months ago

I only use this package for files explicitly - I add a prop line in the beginning of the file to evaluate the function that enables this mode. One problem with this is that - if a file in which this has been evaluated has been opened - any subsequent file I do revert C-x x g ; the package tries to open its note file even if the mode is disabled in the file. I think this is a bug

Secondly; this error pops up when i visit a file with org-remark-mode enabled:

Error during redisplay: (org-remark-highlights-delay-load #<window 3 on  Preview:test.org>) signaled (wrong-type-argument stringp nil)

The relevant function which causes the error is

 (defun org-remark-highlights-delay-load (window)
  "Delay load until WINDOW for current buffer is created."
  (when (windowp window)
    (remove-hook 'window-state-change-functions
                 #'org-remark-highlights-delay-load 'local)
    (org-remark-highlights-load)))

is it normal ?

nobiot commented 9 months ago

org-remark-mode is a buffer local minor mode, so yes.

Any reason why you don’t use org-remark-global-tracking-mode? If your notes file name (location) is custom, of the global tracking mode does not find an existing notes file, it does not turn on org-remark (that should be the design…). Did it not behave like this?

nobiot commented 9 months ago

I would also like to analyze what exactly happens. Do you mind providing me more detail of your set up and step-by-step procedure so that I cN reproduce the error?

Thank you 🙏

nobiot commented 9 months ago

@akashpal-21 Please take your time. No need to feel any pressure.

nobiot commented 9 months ago

Thank you for the detail. #74 should fix the issue where, in your example, remark0 creates a note buffer.

I could not replicate your second issue with this error:

Error during redisplay: (org-remark-highlights-delay-load #<window 3 on Preview:test.org>) signaled (wrong-type-argument stringp nil)

I am guessing it may be fixed by this patch too. Please let me know.


Two additional comments:

1. Use of File Local Variable

You use this syntax: #* _ eval: .... ; *_, and you callrevert-buffer` to get it to take effect.

When I call command add-file-local-variable, Emacs automatically calls the following. When I kill the Emacs, relaunch it, and visit remark1 file with this at the end of the file, I get the org-remark automatically added without revert-buffer. Perhaps this is something you would like to look into.

# Local Variables:
# eval: (org-remark-mode 1)
# End:

2. Pop-up to Ask for Read-Only Buffer

The read-only file pop-up seems to be cause by the fact the subdirectory remark1 (file-name-sans-extension) does not exist in the filesystem yet. Org-remark does not add a directory for you. Perhaps the custom code can be adjusted as following so that it can automatically add the subdirectory under resources. This way, you don't get the pop-up to confirm (tested on my end).

  (defun custom/org-remark-note-path ()
    (let ((dir (concat (file-name-directory (buffer-file-name))
                       "resources/"
                       (file-name-sans-extension (buffer-name))
                       "/")))
      (unless (file-exists-p dir) (make-directory dir t))
      (concat dir "notes.org")))
  (setq org-remark-notes-file-path #'custom/org-remark-note-path)
  (setq org-remark-source-file-name 'abbreviate-file-name))
nobiot commented 9 months ago

Thank you for raising this issue; it would have been impossible to notice this error in my code...

akashpal-21 commented 9 months ago

I managed to solve the highlight not loading when opening from the file manager directly too, for some reason the hook was not getting executed and therefore (org-remark-highlights-load) was not loading - which meant that tracking was turned off too, it was not only a visual bug.

I changed line no. 1656 from (if (not (get-buffer-window)) to (if (equal (current-buffer) (window-buffer))

This seems to have solved the bug in my case,

This is the full edit I made combined,

--- org-remark.el   2023-12-31 08:29:40.375868401 +0530
+++ org-remark.el.patched   2023-12-31 08:25:37.918113848 +0530
@@ -1640,7 +1640,7 @@

 (defun org-remark-highlights-delay-load (window)
   "Delay load until WINDOW for current buffer is created."
-  (when (windowp window)
+  (when (and (windowp window) org-remark-mode)
     (remove-hook 'window-state-change-functions
                  #'org-remark-highlights-delay-load 'local)
     (org-remark-highlights-load)))
@@ -1653,7 +1653,7 @@

 Non-nil value for UPDATE is passed for the notes-source sync
 process."
-  (if (not (get-buffer-window))
+  (if (equal (current-buffer) (window-buffer))
       (add-hook 'window-state-change-functions
                 #'org-remark-highlights-delay-load 95 'local)
     ;; Some major modes such as nov.el reuse the current buffer, deleting
nobiot commented 9 months ago

Thanks for the note. Please keep it open for now. I would like to see if I can replicate what you see. I hope to have some time in one of the weekends soon (hopefully in January).

nobiot commented 5 months ago

The bug I encountered is that if I open remark1 then do 'emacs-client --create-frame' and switch the frame, highlights will try to load on the buffer where org-remark-mode is not active. I tracked the problem to line 1657 (add-hook 'window-state-change-functions #'org-remark-highlights-delay-load 95 'local)

inside the function #'org-remark-highlights-load. I traced the function that is hooked just above and I modified line 1643 to also check whether org-remark-mode is enabled.

Is this the current issue you are reminding me of?

nobiot commented 5 months ago

And your solution is pasted in full in the diff in this comment? https://github.com/nobiot/org-remark/issues/74#issuecomment-1872656727

nobiot commented 5 months ago

Not annoyed at all. Just forgotten, apologies. Thanks for keeping me honest.

akashpal-21 commented 3 months ago

Hey @nobiot I have solved the problem - there is nothing wrong with these two functions, the problem all along was my custom/org-remark-note-path function - It was difficult to track it down since emacs doesn't generate the error on the point the function is failing but a general error on the hook - I will spare you a lengthy explanation, but only that I can no longer reproduce the problem

also

(defun custom/org-remark-note-path ()
    "Generate a path for storing notes in ./resources/buffer-name/notes.org.
     Create the directory structure if it doesn't exist."
    (when (buffer-file-name)
      (let ((directory-path 
         (concat
              (file-name-directory (buffer-file-name))
              "resources/"
              (file-name-sans-extension (buffer-name))
              "/")))

    (unless (file-exists-p directory-path)
      (make-directory directory-path t))

    (concat directory-path "notes.org"))))

(buffer-file-name) - this was failing - because the function call was being made from consult-preview instead of the buffer itself. This function would return nil on such circumstances then (file-name-directory would finally cause the error

Anyways - I cannot also replicate the window-state-change function leaking, I do not need to patch it - I do not know how it got solved, all that I cannot replicate this problem.

Sorry for the false alarm.

Best.

nobiot commented 3 months ago

Thank you for letting me know. I was going to come back to this issue. But... no need to come back then?

akashpal-21 commented 3 months ago

Thank you for letting me know. I was going to come back to this issue. But... no need to come back then?

I spoke too soon - it seems fundamentally window-state-change hook is buggy and should not be relied upon - i I have a better solution, instead of using window-state-change we can use the post-command-hook to delay the load operation.

(defun org-remark-highlights-delay-load ()
  "Delay load until WINDOW for current buffer is created."
  (when (get-buffer-window)
    (remove-hook 'post-command-hook
         #'org-remark-highlights-delay-load 'local)
    (org-remark-highlights-load)))

(defun org-remark-highlights-load (&optional update)
  "Visit notes file & load the saved highlights onto current buffer.
If there is no highlights or annotations for current buffer,
output a message in the echo.

Non-nil value for UPDATE is passed for the notes-source sync
process."
  (if (not (get-buffer-window))
      (add-hook 'post-command-hook
                #'org-remark-highlights-delay-load 95 'local)
          ....

I think such a delay mechanism would be more resilient.

This issue was for the error - we now know where the error source was - if youd not mind - can I open a new issue for the window-state-change-function leaking?