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

Possible to turn on mode buffer-local? #74

Closed
akashpal-21 opened this issue Dec 23, 2023 · 13 comments
Closed

Possible to turn on mode buffer-local? #74

akashpal-21 opened this issue Dec 23, 2023 · 13 comments

Comments

@akashpal-21
Copy link

akashpal-21 commented Dec 23, 2023

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
Copy link
Owner

nobiot commented Dec 24, 2023

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
Copy link
Owner

nobiot commented Dec 24, 2023

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
Copy link
Owner

nobiot commented Dec 28, 2023

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

nobiot added a commit that referenced this issue Dec 30, 2023
Function `org-remark-highlights-load' is set to `after-revert-hook'.
Before this fix, it was wrongly set as a global hook. This caused the
hook to call the function wrongly for buffers that do not have
`org-remark-mode` enabled -- it is a local minor mode.
@nobiot
Copy link
Owner

nobiot commented Dec 30, 2023

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 call revert-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
Copy link
Owner

nobiot commented Dec 30, 2023

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

@akashpal-21
Copy link
Author

akashpal-21 commented Dec 31, 2023

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
Copy link
Owner

nobiot commented Jan 4, 2024

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
Copy link
Owner

nobiot commented Apr 21, 2024

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
Copy link
Owner

nobiot commented Apr 21, 2024

And your solution is pasted in full in the diff in this comment?
#74 (comment)

@nobiot
Copy link
Owner

nobiot commented Apr 21, 2024

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

@akashpal-21
Copy link
Author

akashpal-21 commented Jun 28, 2024

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
Copy link
Owner

nobiot commented Jun 28, 2024

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

@akashpal-21
Copy link
Author

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?

nobiot added a commit that referenced this issue Jun 29, 2024
Refer to issues #82 #74.

If buffer in question is opened directly instead of using find-file --
during new client frame creation - this leaks to ALL open buffers if
org-remark-mode is activated by using local evaluation, for example:

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

The solution is to replace the use of `window-state-change-functions'
hook with the local post-command-hook.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants