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

Error when inserting link or precise link #93

Closed
uliw opened this issue Mar 23, 2023 · 20 comments · Fixed by #94
Closed

Error when inserting link or precise link #93

uliw opened this issue Mar 23, 2023 · 20 comments · Fixed by #94

Comments

@uliw
Copy link

uliw commented Mar 23, 2023

possibly related to earlier reports in Windows, but here it happens on Linux

When inserting a precise link I get (wrong-number-of-arguments #<subr org-noter-pdftools--doc-goto-location> 3)
Inserting a regular link works as long as it is on a single line, but fails when more than one line is selected with

org-noter-pdf--pretty-print-location-for-title: Wrong type argument: listp, #s(org-noter-pdftools--location "pdf:~/user/papers_to_read/burke-2018-sulfur-isotop-river.pdf" 7 0.0 "annot-7-35" nil "pdf:~/user/papers_to_read/burke-2018-sulfur-isotop-river.pdf::7++0.00;;annot-7-35")

Inserting with C-i Tab works however.

This happens with GNU Emacs 28.2 (build 1, x86_64-suse-linux-gnu, GTK+ Version 3.24.35, cairo version 1.17.6) and

  • org-noter-pdftools 20220320.300
  • org-pdftools 20220320.301
  • org-noter 20230315.111
  • pdf-tool 20230322.1541

Fresh install running with -q on minimal init file (attached)
settings-min.zip

@bigodel
Copy link

bigodel commented Mar 24, 2023

possibly related to earlier reports in Windows, but here it happens on Linux

actually i think this is related to the recent changes made by the new maintainer of the package. it now lives under this repo.

When inserting a precise link I get (wrong-number-of-arguments #<subr org-noter-pdftools--doc-goto-location> 3) Inserting a regular link works as long as it is on a single line, but fails when more than one line is selected with

i think this is related to this particular change, where now the ...goto-location functions require an additional window argument which isn't being used. you could simply rewrite org-noter-pdftools--doc-goto-location with a third unused argument, the function definition would look like

(defun org-noter-pdftools--doc-goto-location (mode location &optional _window)
  ...)

and that should solve the problem for now.

i believe some other changes are due given org-noter seems to have gone through a substancial refactoring with this new maintainer.

@uliw
Copy link
Author

uliw commented Mar 24, 2023

If I understand you correctly, you mean to patch the existing function. Doing so results in

org-noter--get-precise-info: Wrong number of arguments: (lambda (mode) "Get precise info from MODE." (if (eq mode 'pdf-view-mode) (progn (let ((org-pdftools-free-pointer-icon org-noter-pdftools-free-pointer-icon) (org-pdftools-free-pointer-color org-noter-pdftools-free-pointer-color) (org-pdftools-free-pointer-opacity org-noter-pdftools-free-pointer-opacity) (org-pdftools-markup-pointer-color org-noter-pdftools-markup-pointer-color) (org-pdftools-markup-pointer-opacity org-noter-pdftools-markup-pointer-opacity) (org-pdftools-markup-pointer-function org-noter-pdftools-markup-pointer-function)) (org-noter-pdftools--parse-link (org-pdftools-get-link)))))), 2

@2e0byo
Copy link

2e0byo commented Mar 24, 2023

@uliw for the time being you may just wish to install the old org-noter, either with straight or manually.

@uliw
Copy link
Author

uliw commented Mar 24, 2023

ok. thanks

@bigodel
Copy link

bigodel commented Mar 24, 2023

If I understand you correctly, you mean to patch the existing function.

not really patch but actually override it entirely. a better approach, now that i think of it, would be to advise it (which is what i have done and it seemed to have fixed it, but i haven't tested inserting links) like so:

(advice-add #'org-noter-pdftools--doc-goto-location :around 
            (lambda (orig-fn mode location _window)
              (apply orig-fn mode location)))

but other problems still may arise, so @2e0byo's solution is more robust indeed. i ended up just removing the package altogether since i wasn't really making much use of it.

@uliw
Copy link
Author

uliw commented Mar 24, 2023

I tried this, but I still get the same error message

@petermao
Copy link

petermao commented Mar 26, 2023

Try this:
change Line 224 to
(defun org-noter-pdftools--get-precise-info (mode &optional _window)...

in addition to the fix from @bigodel above:
change Line 198 to
(defun org-noter-pdftools--doc-goto-location (mode location &optional _window) ...

not sure what else this needs, as I'm not a user of this pkg, but those are the changes I see it needs so far.

btw, window is used in the org-noter pdf module, but not in nov and dvju. We made the change to pass it in so that in the running of org-noter, the relevant window is not repeatedly calculated.

@rdiaz02
Copy link
Contributor

rdiaz02 commented Mar 26, 2023

@uliw for the time being you may just wish to install the old org-noter, either with straight or manually.

@uliw: In case it is simpler, note that it might actually be enough to just install org-noter from MELPA- stable, so as to use version 1.4.1. I have also experienced some related problems (see org-noter/org-noter#6 and org-noter/org-noter#8) and installing 1.4.1 solved them.

@rdiaz02
Copy link
Contributor

rdiaz02 commented Mar 27, 2023

I am not sure what this suggested. So this is what I did:

Try this: change Line 224 to (defun org-noter-pdftools--get-precise-info (mode &optional _window)...

Done

in addition to the fix from @bigodel above:

In addition to? I do not understand: the suggestion to change line 198 (which I followed ---see below), if I understand correctly, changes code that the advice suggestion in #93 (comment) would modify via advice. So they seem to do the same. Does, I did not implement @bigodel.

change Line 198 to (defun org-noter-pdftools--doc-goto-location (mode location &optional _window) ...

Done (i.e., I changed the code in line 198).

not sure what else this needs, as I'm not a user of this pkg, but those are the changes I see it needs so far.

btw, window is used in the org-noter pdf module, but not in nov and dvju. We made the change to pass it in so that in the running of org-noter, the relevant window is not repeatedly calculated.

After doing this, it still fails if the area marked spans more than one line. In other words

  1. Issuing i , org-noter-insert-note, fails with
listp: Wrong type argument: 
listp, #s(org-noter-pdftools--location 
"pdf:/home/ramon/Downloads/test-annot-1.pdf" 
1 0.0 "annot-1-34" nil "pdf:/home/ramon/Downloads/test-annot-1.pdf::1++0.00;;annot-1-34")
  1. Issuing M-i, org-noter-insert-precise-note, fails with
listp: Wrong type argument: l
istp, #s(org-noter-pdftools--location 
"pdf:/home/ramon/Downloads/test-annot-1.pdf" 
1 0.0 "annot-1-35" nil "pdf:/home/ramon/Downloads/test-annot-1.pdf::1++0.00;;annot-1-35")
  1. Issuing C-M-i, org-noter-insert-precise-note-toggle-no-questions, fails with
listp: Wrong type argument: 
listp, #s(org-noter-pdftools--location 
"pdf:/home/ramon/Downloads/test-annot-1.pdf" 
1 0.0 "annot-1-36" nil "pdf:/home/ramon/Downloads/test-annot-1.pdf::1++0.00;;annot-1-36")

All of the above three commands work fine (and seem to do the exact same thing) if we only mark one line.

@petermao
Copy link

@rdiaz02, can you M-x toggle-debug-on-error and dump the stack trace here? I'm not yet sure why a multiple line selection causes a problem for this package. It works in org-noter, but I realize that doesn't help you if your workflow includes this package!

One problem I do see is due to the change we made from 1-D to 2-D precise notes. A precise location is now (<page> <vertical pos> . <horizontal pos>), whereas it was formerly just (<page> . <vertical pos>).

For example, this line will likely fail:

(format "%.2f" (cdr location))))

The probable fix is to change the cdr to cadr, so you pick up the vertical position instead of a cons pair of (<vertical> . <horizontal>).

@petermao
Copy link

another one that needs to be changed to cadr location.

(cdr location)

I find no other cdr location's in this repo.

@rdiaz02
Copy link
Contributor

rdiaz02 commented Mar 30, 2023

I made those two changes, and I M-x toggle-debug-on-error.

This is what I get when issuing i on two or more lines:

Debugger entered--Lisp error: (wrong-type-argument listp #s(org-noter-pdftools--location :path "pdf:/home/ramon/Downloads/test-annot-5.pdf" :page 1 :height 0.0 :annot-id "annot-1-19" :search-string nil :original-property "pdf:/home/ramon/Downloads/test-annot-5.pdf::1++0.0..."))
  cdr(#s(org-noter-pdftools--location :path "pdf:/home/ramon/Downloads/test-annot-5.pdf" :page 1 :height 0.0 :annot-id "annot-1-19" :search-string nil :original-property "pdf:/home/ramon/Downloads/test-annot-5.pdf::1++0.0..."))
  (listp (cdr location))
  (if (listp (cdr location)) (car (cdr location)) (cdr location))
  org-noter--get-location-top(#s(org-noter-pdftools--location :path "pdf:/home/ramon/Downloads/test-annot-5.pdf" :page 1 :height 0.0 :annot-id "annot-1-19" :search-string nil :original-property "pdf:/home/ramon/Downloads/test-annot-5.pdf::1++0.0..."))
  (let ((mode (progn (or (progn (and (memq ... cl-struct-org-noter--session-tags) t)) (signal 'wrong-type-argument (list 'org-noter--session session))) (aref session 7))) (vpos (org-noter--get-location-top location)) (hpos (org-noter--get-location-left location)) (vtxt "") (htxt "") pagelabel) (cond ((eq mode 'pdf-view-mode) (if (> hpos 0) (setq htxt (format " H: %d%%" (round (* 100 hpos))))) (if (or (> vpos 0) (> hpos 0)) (setq vtxt (format " V: %d%%" (round (* 100 vpos))))) (select-window (org-noter--get-doc-window)) (setq pagelabel (pdf-view-current-pagelabel)) (select-window (org-noter--get-notes-window)) (format "%s%s%s" pagelabel vtxt htxt)) ((eq mode 'doc-view-mode) (org-noter-pdf--pretty-print-location location))))
  (progn (let ((mode (progn (or (progn (and ... t)) (signal 'wrong-type-argument (list ... session))) (aref session 7))) (vpos (org-noter--get-location-top location)) (hpos (org-noter--get-location-left location)) (vtxt "") (htxt "") pagelabel) (cond ((eq mode 'pdf-view-mode) (if (> hpos 0) (setq htxt (format " H: %d%%" (round ...)))) (if (or (> vpos 0) (> hpos 0)) (setq vtxt (format " V: %d%%" (round ...)))) (select-window (org-noter--get-doc-window)) (setq pagelabel (pdf-view-current-pagelabel)) (select-window (org-noter--get-notes-window)) (format "%s%s%s" pagelabel vtxt htxt)) ((eq mode 'doc-view-mode) (org-noter-pdf--pretty-print-location location)))))
  (progn (progn (let ((mode (progn (or (progn ...) (signal ... ...)) (aref session 7))) (vpos (org-noter--get-location-top location)) (hpos (org-noter--get-location-left location)) (vtxt "") (htxt "") pagelabel) (cond ((eq mode 'pdf-view-mode) (if (> hpos 0) (setq htxt (format " H: %d%%" ...))) (if (or (> vpos 0) (> hpos 0)) (setq vtxt (format " V: %d%%" ...))) (select-window (org-noter--get-doc-window)) (setq pagelabel (pdf-view-current-pagelabel)) (select-window (org-noter--get-notes-window)) (format "%s%s%s" pagelabel vtxt htxt)) ((eq mode 'doc-view-mode) (org-noter-pdf--pretty-print-location location))))))
  (if (org-noter--valid-session session) (progn (progn (let ((mode (progn (or ... ...) (aref session 7))) (vpos (org-noter--get-location-top location)) (hpos (org-noter--get-location-left location)) (vtxt "") (htxt "") pagelabel) (cond ((eq mode 'pdf-view-mode) (if (> hpos 0) (setq htxt ...)) (if (or ... ...) (setq vtxt ...)) (select-window (org-noter--get-doc-window)) (setq pagelabel (pdf-view-current-pagelabel)) (select-window (org-noter--get-notes-window)) (format "%s%s%s" pagelabel vtxt htxt)) ((eq mode 'doc-view-mode) (org-noter-pdf--pretty-print-location location)))))))
  (let ((session org-noter--session)) (if (org-noter--valid-session session) (progn (progn (let ((mode (progn ... ...)) (vpos (org-noter--get-location-top location)) (hpos (org-noter--get-location-left location)) (vtxt "") (htxt "") pagelabel) (cond ((eq mode ...) (if ... ...) (if ... ...) (select-window ...) (setq pagelabel ...) (select-window ...) (format "%s%s%s" pagelabel vtxt htxt)) ((eq mode ...) (org-noter-pdf--pretty-print-location location))))))))
  org-noter-pdf--pretty-print-location-for-title(#s(org-noter-pdftools--location :path "pdf:/home/ramon/Downloads/test-annot-5.pdf" :page 1 :height 0.0 :annot-id "annot-1-19" :search-string nil :original-property "pdf:/home/ramon/Downloads/test-annot-5.pdf::1++0.0..."))
  run-hook-with-args-until-success(org-noter-pdf--pretty-print-location-for-title #s(org-noter-pdftools--location :path "pdf:/home/ramon/Downloads/test-annot-5.pdf" :page 1 :height 0.0 :annot-id "annot-1-19" :search-string nil :original-property "pdf:/home/ramon/Downloads/test-annot-5.pdf::1++0.0..."))
  (progn (run-hook-with-args-until-success 'org-noter--pretty-print-location-for-title-hook location))
  (progn (progn (run-hook-with-args-until-success 'org-noter--pretty-print-location-for-title-hook location)))
  (if (org-noter--valid-session session) (progn (progn (run-hook-with-args-until-success 'org-noter--pretty-print-location-for-title-hook location))))
  (let ((session org-noter--session)) (if (org-noter--valid-session session) (progn (progn (run-hook-with-args-until-success 'org-noter--pretty-print-location-for-title-hook location)))))
  org-noter--pretty-print-location-for-title(#s(org-noter-pdftools--location :path "pdf:/home/ramon/Downloads/test-annot-5.pdf" :page 1 :height 0.0 :annot-id "annot-1-19" :search-string nil :original-property "pdf:/home/ramon/Downloads/test-annot-5.pdf::1++0.0..."))
  (replace-regexp-in-string (regexp-quote "$p$") (org-noter--pretty-print-location-for-title location) org-noter-default-heading-title)
  (or short-selected-text (replace-regexp-in-string (regexp-quote "$p$") (org-noter--pretty-print-location-for-title location) org-noter-default-heading-title))
  (let ((view-info (org-noter--get-view-info current-view location)) (minibuffer-local-completion-map org-noter--completing-read-keymap) collection title note-body existing-note (default-title (or short-selected-text (replace-regexp-in-string (regexp-quote "$p$") (org-noter--pretty-print-location-for-title location) org-noter-default-heading-title))) (empty-lines-number (if org-noter-separate-notes-from-heading 2 1))) (if precise-info nil (let ((tail (progn (or (progn ...) (signal ... ...)) (aref view-info 1)))) (while tail (let ((note-cons (car tail))) (let ((display ...)) (setq collection (cons ... collection))) (setq tail (cdr tail)))))) (progn (setq collection (nreverse collection)) (setq title (if org-noter-insert-note-no-questions default-title (completing-read "Note: " collection nil nil nil nil default-title))) (setq note-body (if (and selected-text-p (not (equal title short-selected-text))) selected-text)) (setq existing-note (if precise-info nil (cdr (assoc title collection))))) (if existing-note (let* ((note (car existing-note)) (insert-before-element (cdr existing-note)) (has-content (eq (org-element-map (org-element-contents note) org-element-all-elements #'... nil t) t))) (if has-content (progn (setq empty-lines-number 2))) (if insert-before-element (goto-char (org-element-property :begin insert-before-element)) (goto-char (org-element-property :end note))) (if (org-at-heading-p) (progn (org-N-empty-lines-before-current empty-lines-number) (forward-line -1)) (if (bolp) nil (insert "\n")) (org-N-empty-lines-before-current (1- empty-lines-number))) (if (and org-noter-insert-selected-text-inside-note note-body) (progn (if short-selected-text (insert "``" note-body "''") (insert "#+BEGIN_QUOTE\n" note-body "\n#+END_QUOTE"))))) (let ((reference-element-cons (progn (or (progn ...) (signal ... ...)) (aref view-info 4))) level) (if reference-element-cons (progn (cond ((eq ... ...) (goto-char ...)) ((eq ... ...) (goto-char ...))) (if (org-at-heading-p) (progn (backward-char))) (setq level (org-element-property :level (cdr reference-element-cons)))) (goto-char (or (org-element-map contents 'section #'... nil t org-element-all-elements) (point-max)))) (setq level (or level (1+ (or (org-element-property :level ast) 0)))) (if (org-noter--no-heading-p) nil (outline-show-entry)) (org-noter--insert-heading level title empty-lines-number location) (let* ((s (and t highlight-location)) (serialized-highlight (and s (org-noter--get-serialized-highlight highlight-location)))) (if serialized-highlight (org-set-property "HIGHLIGHT" serialized-highlight) nil)) (if note-body (progn (save-excursion (if short-selected-text (insert "``" note-body "''") (insert "#+BEGIN_QUOTE\n" note-body "\n#+END_QUOTE"))))) (if (progn (or (progn (and ... t)) (signal 'wrong-type-argument (list ... session))) (aref session 17)) (progn (org-overview))) (progn (or (progn (and (memq ... cl-struct-org-noter--session-tags) t)) (signal 'wrong-type-argument (list 'org-noter--session session))) (let* ((v session)) (aset v 12 (1+ (progn ... ...))))))) (org-show-set-visibility t) (org-cycle-hide-drawers 'all) (org-cycle-show-empty-lines t) (if org-noter-highlight-selected-text (progn (select-frame-set-input-focus (progn (or (progn (and ... t)) (signal 'wrong-type-argument (list ... session))) (aref session 2))) (select-window (get-buffer-window (progn (or (progn ...) (signal ... ...)) (aref session 3)))) (run-hook-with-args-until-success 'org-noter--add-highlight-hook major-mode highlight-location))))
  (let ((inhibit-quit nil)) (select-frame-set-input-focus (window-frame window)) (select-window window) (let ((view-info (org-noter--get-view-info current-view location)) (minibuffer-local-completion-map org-noter--completing-read-keymap) collection title note-body existing-note (default-title (or short-selected-text (replace-regexp-in-string (regexp-quote "$p$") (org-noter--pretty-print-location-for-title location) org-noter-default-heading-title))) (empty-lines-number (if org-noter-separate-notes-from-heading 2 1))) (if precise-info nil (let ((tail (progn (or ... ...) (aref view-info 1)))) (while tail (let ((note-cons ...)) (let (...) (setq collection ...)) (setq tail (cdr tail)))))) (progn (setq collection (nreverse collection)) (setq title (if org-noter-insert-note-no-questions default-title (completing-read "Note: " collection nil nil nil nil default-title))) (setq note-body (if (and selected-text-p (not (equal title short-selected-text))) selected-text)) (setq existing-note (if precise-info nil (cdr (assoc title collection))))) (if existing-note (let* ((note (car existing-note)) (insert-before-element (cdr existing-note)) (has-content (eq (org-element-map ... org-element-all-elements ... nil t) t))) (if has-content (progn (setq empty-lines-number 2))) (if insert-before-element (goto-char (org-element-property :begin insert-before-element)) (goto-char (org-element-property :end note))) (if (org-at-heading-p) (progn (org-N-empty-lines-before-current empty-lines-number) (forward-line -1)) (if (bolp) nil (insert "\n")) (org-N-empty-lines-before-current (1- empty-lines-number))) (if (and org-noter-insert-selected-text-inside-note note-body) (progn (if short-selected-text (insert "``" note-body "''") (insert "#+BEGIN_QUOTE\n" note-body "\n#+END_QUOTE"))))) (let ((reference-element-cons (progn (or ... ...) (aref view-info 4))) level) (if reference-element-cons (progn (cond (... ...) (... ...)) (if (org-at-heading-p) (progn ...)) (setq level (org-element-property :level ...))) (goto-char (or (org-element-map contents ... ... nil t org-element-all-elements) (point-max)))) (setq level (or level (1+ (or ... 0)))) (if (org-noter--no-heading-p) nil (outline-show-entry)) (org-noter--insert-heading level title empty-lines-number location) (let* ((s (and t highlight-location)) (serialized-highlight (and s ...))) (if serialized-highlight (org-set-property "HIGHLIGHT" serialized-highlight) nil)) (if note-body (progn (save-excursion (if short-selected-text ... ...)))) (if (progn (or (progn ...) (signal ... ...)) (aref session 17)) (progn (org-overview))) (progn (or (progn (and ... t)) (signal 'wrong-type-argument (list ... session))) (let* ((v session)) (aset v 12 (1+ ...)))))) (org-show-set-visibility t) (org-cycle-hide-drawers 'all) (org-cycle-show-empty-lines t) (if org-noter-highlight-selected-text (progn (select-frame-set-input-focus (progn (or (progn ...) (signal ... ...)) (aref session 2))) (select-window (get-buffer-window (progn (or ... ...) (aref session 3)))) (run-hook-with-args-until-success 'org-noter--add-highlight-hook major-mode highlight-location)))))
  (condition-case nil (let ((inhibit-quit nil)) (select-frame-set-input-focus (window-frame window)) (select-window window) (let ((view-info (org-noter--get-view-info current-view location)) (minibuffer-local-completion-map org-noter--completing-read-keymap) collection title note-body existing-note (default-title (or short-selected-text (replace-regexp-in-string (regexp-quote "$p$") (org-noter--pretty-print-location-for-title location) org-noter-default-heading-title))) (empty-lines-number (if org-noter-separate-notes-from-heading 2 1))) (if precise-info nil (let ((tail (progn ... ...))) (while tail (let (...) (let ... ...) (setq tail ...))))) (progn (setq collection (nreverse collection)) (setq title (if org-noter-insert-note-no-questions default-title (completing-read "Note: " collection nil nil nil nil default-title))) (setq note-body (if (and selected-text-p (not ...)) selected-text)) (setq existing-note (if precise-info nil (cdr (assoc title collection))))) (if existing-note (let* ((note (car existing-note)) (insert-before-element (cdr existing-note)) (has-content (eq ... t))) (if has-content (progn (setq empty-lines-number 2))) (if insert-before-element (goto-char (org-element-property :begin insert-before-element)) (goto-char (org-element-property :end note))) (if (org-at-heading-p) (progn (org-N-empty-lines-before-current empty-lines-number) (forward-line -1)) (if (bolp) nil (insert "\n")) (org-N-empty-lines-before-current (1- empty-lines-number))) (if (and org-noter-insert-selected-text-inside-note note-body) (progn (if short-selected-text ... ...)))) (let ((reference-element-cons (progn ... ...)) level) (if reference-element-cons (progn (cond ... ...) (if ... ...) (setq level ...)) (goto-char (or ... ...))) (setq level (or level (1+ ...))) (if (org-noter--no-heading-p) nil (outline-show-entry)) (org-noter--insert-heading level title empty-lines-number location) (let* ((s ...) (serialized-highlight ...)) (if serialized-highlight (org-set-property "HIGHLIGHT" serialized-highlight) nil)) (if note-body (progn (save-excursion ...))) (if (progn (or ... ...) (aref session 17)) (progn (org-overview))) (progn (or (progn ...) (signal ... ...)) (let* (...) (aset v 12 ...))))) (org-show-set-visibility t) (org-cycle-hide-drawers 'all) (org-cycle-show-empty-lines t) (if org-noter-highlight-selected-text (progn (select-frame-set-input-focus (progn (or ... ...) (aref session 2))) (select-window (get-buffer-window (progn ... ...))) (run-hook-with-args-until-success 'org-noter--add-highlight-hook major-mode highlight-location))))) (quit (setq quit-flag t) (eval '(ignore nil) t)))
  (let* ((inhibit-quit t) (short-selected-text (if (and selected-text-p (<= (length selected-text) org-noter-max-short-selected-text-length)) selected-text)) (org-noter-highlight-selected-text (if toggle-highlight (not org-noter-highlight-selected-text) org-noter-highlight-selected-text)) (highlight-location (if org-noter-highlight-selected-text (org-noter--get-highlight-location)))) (condition-case nil (let ((inhibit-quit nil)) (select-frame-set-input-focus (window-frame window)) (select-window window) (let ((view-info (org-noter--get-view-info current-view location)) (minibuffer-local-completion-map org-noter--completing-read-keymap) collection title note-body existing-note (default-title (or short-selected-text (replace-regexp-in-string ... ... org-noter-default-heading-title))) (empty-lines-number (if org-noter-separate-notes-from-heading 2 1))) (if precise-info nil (let ((tail ...)) (while tail (let ... ... ...)))) (progn (setq collection (nreverse collection)) (setq title (if org-noter-insert-note-no-questions default-title (completing-read "Note: " collection nil nil nil nil default-title))) (setq note-body (if (and selected-text-p ...) selected-text)) (setq existing-note (if precise-info nil (cdr ...)))) (if existing-note (let* ((note ...) (insert-before-element ...) (has-content ...)) (if has-content (progn ...)) (if insert-before-element (goto-char ...) (goto-char ...)) (if (org-at-heading-p) (progn ... ...) (if ... nil ...) (org-N-empty-lines-before-current ...)) (if (and org-noter-insert-selected-text-inside-note note-body) (progn ...))) (let ((reference-element-cons ...) level) (if reference-element-cons (progn ... ... ...) (goto-char ...)) (setq level (or level ...)) (if (org-noter--no-heading-p) nil (outline-show-entry)) (org-noter--insert-heading level title empty-lines-number location) (let* (... ...) (if serialized-highlight ... nil)) (if note-body (progn ...)) (if (progn ... ...) (progn ...)) (progn (or ... ...) (let* ... ...)))) (org-show-set-visibility t) (org-cycle-hide-drawers 'all) (org-cycle-show-empty-lines t) (if org-noter-highlight-selected-text (progn (select-frame-set-input-focus (progn ... ...)) (select-window (get-buffer-window ...)) (run-hook-with-args-until-success 'org-noter--add-highlight-hook major-mode highlight-location))))) (quit (setq quit-flag t) (eval '(ignore nil) t))) (if quit-flag (progn (select-frame-set-input-focus (progn (or (progn (and ... t)) (signal 'wrong-type-argument (list ... session))) (aref session 2))) (select-window (get-buffer-window (progn (or (progn ...) (signal ... ...)) (aref session 3)))))))
  (let* ((ast (org-noter--parse-root)) (contents (org-element-contents ast)) (window (org-noter--get-notes-window 'force)) (selected-text (run-hook-with-args-until-success 'org-noter-get-selected-text-hook (progn (or (progn (and ... t)) (signal 'wrong-type-argument (list ... session))) (aref session 7)))) (selected-text-p (> (length selected-text) 0)) force-new (location (org-noter--doc-approx-location (or precise-info 'interactive) (cons #'(lambda nil force-new) #'(lambda (gv--val) (setq force-new gv--val))))) (current-view (org-noter--get-current-view))) (let* ((inhibit-quit t) (short-selected-text (if (and selected-text-p (<= (length selected-text) org-noter-max-short-selected-text-length)) selected-text)) (org-noter-highlight-selected-text (if toggle-highlight (not org-noter-highlight-selected-text) org-noter-highlight-selected-text)) (highlight-location (if org-noter-highlight-selected-text (org-noter--get-highlight-location)))) (condition-case nil (let ((inhibit-quit nil)) (select-frame-set-input-focus (window-frame window)) (select-window window) (let ((view-info (org-noter--get-view-info current-view location)) (minibuffer-local-completion-map org-noter--completing-read-keymap) collection title note-body existing-note (default-title (or short-selected-text ...)) (empty-lines-number (if org-noter-separate-notes-from-heading 2 1))) (if precise-info nil (let (...) (while tail ...))) (progn (setq collection (nreverse collection)) (setq title (if org-noter-insert-note-no-questions default-title ...)) (setq note-body (if ... selected-text)) (setq existing-note (if precise-info nil ...))) (if existing-note (let* (... ... ...) (if has-content ...) (if insert-before-element ... ...) (if ... ... ... ...) (if ... ...)) (let (... level) (if reference-element-cons ... ...) (setq level ...) (if ... nil ...) (org-noter--insert-heading level title empty-lines-number location) (let* ... ...) (if note-body ...) (if ... ...) (progn ... ...))) (org-show-set-visibility t) (org-cycle-hide-drawers 'all) (org-cycle-show-empty-lines t) (if org-noter-highlight-selected-text (progn (select-frame-set-input-focus ...) (select-window ...) (run-hook-with-args-until-success ... major-mode highlight-location))))) (quit (setq quit-flag t) (eval '(ignore nil) t))) (if quit-flag (progn (select-frame-set-input-focus (progn (or (progn ...) (signal ... ...)) (aref session 2))) (select-window (get-buffer-window (progn (or ... ...) (aref session 3))))))))
  (progn (let* ((ast (org-noter--parse-root)) (contents (org-element-contents ast)) (window (org-noter--get-notes-window 'force)) (selected-text (run-hook-with-args-until-success 'org-noter-get-selected-text-hook (progn (or (progn ...) (signal ... ...)) (aref session 7)))) (selected-text-p (> (length selected-text) 0)) force-new (location (org-noter--doc-approx-location (or precise-info 'interactive) (cons #'(lambda nil force-new) #'(lambda ... ...)))) (current-view (org-noter--get-current-view))) (let* ((inhibit-quit t) (short-selected-text (if (and selected-text-p (<= ... org-noter-max-short-selected-text-length)) selected-text)) (org-noter-highlight-selected-text (if toggle-highlight (not org-noter-highlight-selected-text) org-noter-highlight-selected-text)) (highlight-location (if org-noter-highlight-selected-text (org-noter--get-highlight-location)))) (condition-case nil (let ((inhibit-quit nil)) (select-frame-set-input-focus (window-frame window)) (select-window window) (let ((view-info ...) (minibuffer-local-completion-map org-noter--completing-read-keymap) collection title note-body existing-note (default-title ...) (empty-lines-number ...)) (if precise-info nil (let ... ...)) (progn (setq collection ...) (setq title ...) (setq note-body ...) (setq existing-note ...)) (if existing-note (let* ... ... ... ... ...) (let ... ... ... ... ... ... ... ... ...)) (org-show-set-visibility t) (org-cycle-hide-drawers 'all) (org-cycle-show-empty-lines t) (if org-noter-highlight-selected-text (progn ... ... ...)))) (quit (setq quit-flag t) (eval '(ignore nil) t))) (if quit-flag (progn (select-frame-set-input-focus (progn (or ... ...) (aref session 2))) (select-window (get-buffer-window (progn ... ...))))))))
  (progn (progn (let* ((ast (org-noter--parse-root)) (contents (org-element-contents ast)) (window (org-noter--get-notes-window 'force)) (selected-text (run-hook-with-args-until-success 'org-noter-get-selected-text-hook (progn (or ... ...) (aref session 7)))) (selected-text-p (> (length selected-text) 0)) force-new (location (org-noter--doc-approx-location (or precise-info 'interactive) (cons #'... #'...))) (current-view (org-noter--get-current-view))) (let* ((inhibit-quit t) (short-selected-text (if (and selected-text-p ...) selected-text)) (org-noter-highlight-selected-text (if toggle-highlight (not org-noter-highlight-selected-text) org-noter-highlight-selected-text)) (highlight-location (if org-noter-highlight-selected-text (org-noter--get-highlight-location)))) (condition-case nil (let ((inhibit-quit nil)) (select-frame-set-input-focus (window-frame window)) (select-window window) (let (... ... collection title note-body existing-note ... ...) (if precise-info nil ...) (progn ... ... ... ...) (if existing-note ... ...) (org-show-set-visibility t) (org-cycle-hide-drawers ...) (org-cycle-show-empty-lines t) (if org-noter-highlight-selected-text ...))) (quit (setq quit-flag t) (eval '... t))) (if quit-flag (progn (select-frame-set-input-focus (progn ... ...)) (select-window (get-buffer-window ...))))))))
  (if (org-noter--valid-session session) (progn (progn (let* ((ast (org-noter--parse-root)) (contents (org-element-contents ast)) (window (org-noter--get-notes-window 'force)) (selected-text (run-hook-with-args-until-success 'org-noter-get-selected-text-hook (progn ... ...))) (selected-text-p (> (length selected-text) 0)) force-new (location (org-noter--doc-approx-location (or precise-info ...) (cons ... ...))) (current-view (org-noter--get-current-view))) (let* ((inhibit-quit t) (short-selected-text (if ... selected-text)) (org-noter-highlight-selected-text (if toggle-highlight ... org-noter-highlight-selected-text)) (highlight-location (if org-noter-highlight-selected-text ...))) (condition-case nil (let (...) (select-frame-set-input-focus ...) (select-window window) (let ... ... ... ... ... ... ... ...)) (quit (setq quit-flag t) (eval ... t))) (if quit-flag (progn (select-frame-set-input-focus ...) (select-window ...))))))))
  (let ((session org-noter--session)) (if (org-noter--valid-session session) (progn (progn (let* ((ast (org-noter--parse-root)) (contents (org-element-contents ast)) (window (org-noter--get-notes-window ...)) (selected-text (run-hook-with-args-until-success ... ...)) (selected-text-p (> ... 0)) force-new (location (org-noter--doc-approx-location ... ...)) (current-view (org-noter--get-current-view))) (let* ((inhibit-quit t) (short-selected-text ...) (org-noter-highlight-selected-text ...) (highlight-location ...)) (condition-case nil (let ... ... ... ...) (quit ... ...)) (if quit-flag (progn ... ...))))))))
  org-noter-insert-note(nil)
  funcall-interactively(org-noter-insert-note nil)
  command-execute(org-noter-insert-note)

@petermao
Copy link

What I can see from the stack trace is that the location that is getting passed around is a structure as defined here:

(cl-defstruct org-noter-pdftools--location
path page height annot-id search-string original-property)

org-noter needs to have this converted into a cons cell probably using
(defun org-noter-pdftools--location-link-to-cons (location)
"Convert a org-pdftools link to old LOCATION cons."
(cons (org-noter-pdftools--location-page location) (or (org-noter-pdftools--location-height location) 0.0)))

If you are adept with the debugger, try stopping the code in org-noter--get-location-top (C-u C-M-x when the point is in that defun) and recording the value of location for 1 and 2 line text selections -- it's good to know what is happening when the code doesn't crash.
Take heart -- we will get to the bottom of this!

@rdiaz02
Copy link
Contributor

rdiaz02 commented Apr 1, 2023

That function (org-noter--get-location-top) seems to be called also on note creation (and the value of location seems to be, at that point, things like (1 . 0)).

Anyway, if use i on a multi-line selection, edebug stops and when I evaluate location (I use e: https://www.gnu.org/software/emacs/manual/html_node/elisp/Edebug-Eval.html ) I get:

#s(org-noter-pdftools--location :path "pdf:/home/ramon/Downloads/ta5.pdf" :page 1 :height 0.0 :annot-id "annot-1-19" :search-string nil :original-property "pdf:/home/ramon/Downloads/ta5.pdf::1++0.00;;annot-...")

and, on continucing (c) I get the error.

When on a single-line selection, that function is not called (unless there are other notes in the note already). I mean, the note is inserted without ever going through the function.

@petermao
Copy link

petermao commented Apr 1, 2023

I believe the crux of the problem lies with

(defun org-noter-pdftools--doc-approx-location (mode precise-info force-new-ref)
"Get approximate location in MODE buffer based on PRECISE-INFO and FORCE-NEW-REF."
which returns location as a structure rather than as a list. This is something that @fuxialexander or some enterprising user of this package will need to take up.

This gets called in org-noter here: https://github.com/org-noter/org-noter/blob/870109330a337751e44042d1b407d90d42ca9130/org-noter-core.el#L2104, and then the crash happens here: https://github.com/org-noter/org-noter/blob/870109330a337751e44042d1b407d90d42ca9130/org-noter-core.el#L2125-L2128, specifically, on line 2127.

Next experiment: customize org-noter-max-short-selected-text-length to 250 (from the default value of 80). Does it now work for 2 lines but fail for 4 or 5 lines? In the new org noter, short and long selections are differentiated by this variable. In the old org-noter, they were differentiated at 3 lines, based on the number of \n's in the selection (see https://github.com/org-noter/org-noter#changes-to-note-insertion-since-version-141-melpa-version-201910201212). If you have the old org-noter on your system, try taking a precise note with 5 lines selected from the PDF. Does the code crash?

rdiaz02 added a commit to rdiaz02/org-pdftools that referenced this issue Apr 1, 2023
@rdiaz02
Copy link
Contributor

rdiaz02 commented Apr 1, 2023

I believe the crux of the problem lies with

(defun org-noter-pdftools--doc-approx-location (mode precise-info force-new-ref)
"Get approximate location in MODE buffer based on PRECISE-INFO and FORCE-NEW-REF."

which returns location as a structure rather than as a list. This is something that @fuxialexander or some enterprising user of this package will need to take up.

This gets called in org-noter here: https://github.com/org-noter/org-noter/blob/870109330a337751e44042d1b407d90d42ca9130/org-noter-core.el#L2104, and then the crash happens here: https://github.com/org-noter/org-noter/blob/870109330a337751e44042d1b407d90d42ca9130/org-noter-core.el#L2125-L2128, specifically, on line 2127.

Next experiment: customize org-noter-max-short-selected-text-length to 250 (from the default value of 80). Does it now work for 2 lines but fail for 4 or 5 lines? In the new org noter, short and long selections are differentiated by this variable. In the old org-noter, they were differentiated at 3 lines, based on the number of \n's in the selection (see https://github.com/org-noter/org-noter#changes-to-note-insertion-since-version-141-melpa-version-201910201212).

I am not sure if I am supposed to use the new (current repo) or the old (1.4.1, as available from melpa) org-noter, and whether or not I should use the changed code in the last iterations.

So to be explicit my setup involves the new org-noter (commit 87010) and two versions of org-pdftools:

I then (setq org-noter-max-short-selected-text-length 250)

Using M-i works for 1, 2, 3, 4 lines, but fails for 5 with

listp: Wrong type argument: listp, #s(org-noter-pdftools--location "pdf:/home/ramon/Downloads/ta6.pdf" 2 0.0 "annot-2-16" nil "pdf:/home/ramon/Downloads/ta6.pdf::2++0.00;;annot-2-16")

Using i works for 1, 2, 3, 4 lines, but fails for 5 with

listp: Wrong type argument: listp, #s(org-noter-pdftools--location "pdf:/home/ramon/Downloads/ta6.pdf" 1 0.0 "annot-1-34" nil "pdf:/home/ramon/Downloads/ta6.pdf::1++0.00;;annot-1-34")

(so same error as for M-i)

Just for the hell of it, I increased (setq org-noter-max-short-selected-text-length 800) and I can M-i with regions of at least 12 lines.

  • Version 2: org-pdftools and org-noter-pdftools without modifications (as available from repo, commit 967f48)

Starting org-noter gives this error (which seems the same as org-noter/org-noter#8 and which, itself, brings as here).

Error running timer: (wrong-number-of-arguments (lambda (mode location) "Goto LOCATION in the corresponding MODE." (if (and (eq mode 'pdf-view-mode) (progn (and (memq (type-of location) cl-struct-org-noter-pdftools--location-tags) t))) (progn (if (progn (or (progn (and (memq (type-of location) cl-struct-org-noter-pdftools--location-tags) t)) (signal 'wrong-type-argument (list 'org-noter-pdftools--location location))) (aref location 2)) (progn (pdf-view-goto-page (progn (or (progn (and (memq (type-of location) cl-struct-org-noter-pdftools--location-tags) t)) (signal 'wrong-type-argument (list 'org-noter-pdftools--location location))) (aref location 2))))) (if (progn (or (progn (and (memq (type-of location) cl-struct-org-noter-pdftools--location-tags) t)) (signal 'wrong-type-argument (list 'org-noter-pdftools--location location))) (aref location 3)) (progn (image-set-window-vscroll (round (/ (* (progn (or (progn (and (memq (type-of location) cl-struct-org-noter-pdftools--location-tags) t)) (signal 'wrong-type-argument (list 'org-noter-pdftools--location location))) (aref location 3)) (cdr (pdf-view-image-size))) (frame-char-height)))))) (if (progn (or (progn (and (memq (type-of location) cl-struct-org-noter-pdftools--location-tags) t)) (signal 'wrong-type-argument (list 'org-noter-pdftools--location location))) (aref location 4)) (progn (pdf-annot-show-annotation (pdf-info-getannot (progn (or (progn (and (memq (type-of location) cl-struct-org-noter-pdftools--location-tags) t)) (signal 'wrong-type-argument (list 'org-noter-pdftools--location location))) (aref location 4))) t))) (if (progn (or (progn (and (memq (type-of location) cl-struct-org-noter-pdftools--location-tags) t)) (signal 'wrong-type-argument (list 'org-noter-pdftools--location location))) (aref location 5)) (progn (isearch-mode t) (isearch-yank-string (progn (or (progn (and (memq (type-of location) cl-struct-org-noter-pdftools--location-tags) t)) (signal 'wrong-type-argument (list 'org-noter-pdftools--location location))) (aref location 5))))) t))) 3)

I then (setq org-noter-max-short-selected-text-length 250)

Using M-i for a single line fails with error

run-hook-with-args-until-success: Wrong number of arguments: (lambda (mode) "Get precise info from MODE." (if (eq mode 'pdf-view-mode) (progn (let ((org-pdftools-free-pointer-icon org-noter-pdftools-free-pointer-icon) (org-pdftools-free-pointer-color org-noter-pdftools-free-pointer-color) (org-pdftools-free-pointer-opacity org-noter-pdftools-free-pointer-opacity) (org-pdftools-markup-pointer-color org-noter-pdftools-markup-pointer-color) (org-pdftools-markup-pointer-opacity org-noter-pdftools-markup-pointer-opacity) (org-pdftools-markup-pointer-function org-noter-pdftools-markup-pointer-function)) (org-noter-pdftools--parse-link (org-pdftools-get-link)))))), 2

For 2 and 3 lines M-i continues failing.

Using i works for a single line. For 5 lines fails with

listp: Wrong type argument: listp, #s(org-noter-pdftools--location "pdf:/home/ramon/Downloads/ta6.pdf" 1 0.58 "annot-1-35" nil "pdf:/home/ramon/Downloads/ta6.pdf::1++0.58;;annot-1-35")

It works for 2, 3, and 4 lines, though.

If you have the old org-noter on your system, try taking a precise note with 5 lines selected from the PDF. Does the code crash?

Using org-noter-1.4.1 (as available from melpa stable), without any modifications. I mark five lines, and M-x org-noter-insert-precise-note works fine. For that matter, so does marking 20 lines. Or 4 lines. I have just tested this with a single-column and a two-column pdf.

In case it matters, though, and as reported by me somewhere else (#54), the behavior of org-noter-insert-precise-note and org-noter-insert-note is identical.

I should add that I have used the old org-noter for over two years, marking short, long, very short, very long, regions, without trouble.

@rdiaz02
Copy link
Contributor

rdiaz02 commented Apr 1, 2023

Peter (@petermao), I really thank you for your patience. And maybe we are getting closer to something. But then, maybe not: some of the things I write above I am not sure are entirely self-consistent (i.e., they suggest I might have made a mistake), formerly fixed bugs seem to have re-emerged, etc. So despite our patience and best intentions, there are several problems with this "one-step-at-a-time-remote-debugging" we are using here:

  • I might be making mistakes, but I cannot double or triple check every single step ---I just caught one I made, where I was not using the right branch in the repo. It is easier to catch mistakes (and double check what really needs to be double-checked) from inconsistent behavior if I know what I am doing when debugging; but going blindly (my elisp ignorance) it is hard for me to tell what is/isn't a likely mistake; I am out of my depth here. And I am then giving you feedback from steps that might not be exactly what I report.
  • it is not always clear to me what exactly you want me to try.

So I think solving this requires someone who knows what they are doing with elisp (not my case at the moment) and who is willing to look into the issue running org-noter and org-pdftools/org-noter-pdftools.

I think for the time being, and until someone like that (maybe me when I learn enough elisp, which won't be soon) steps into the issue, the solution is to use org-noter 1.4.1.

If there is something else I can try, let me know, and I'll try to give it a try (pun kinda intended) but I don't think we'll be able to fix it. Again, I really thank you for your patience.

@petermao
Copy link

petermao commented Apr 2, 2023

@rdiaz02 confirms that the breakage occurs when org-noter treats the text selection as "long" rather than "short," which means that the org-noter-max-short-selected-text-length differentiates by the character count, rather than the number of lines. If it works with your workflow, then set ...-max-short-... to most-positive-fixnum, and if you don't want your selection to be the heading title, then you must specify the title manually, or M-n at the "Note: " prompt and then edit the selection down to the title you want.
Yet another possibility is to comment out

(org-noter--doc-approx-location-hook . org-noter-pdftools--doc-approx-location)
so that location is the output of the function in org-noter/modules/org-noter-pdf.el, but that may break other aspects of this package.
When I have some time, I may look into why this worked fine for long selections in org-noter 1.4.1.

@rdiaz02
Copy link
Contributor

rdiaz02 commented Apr 2, 2023

Perfect, thanks.

I've (setq org-noter-max-short-selected-text-length most-positive-fixnum). This works well and is very flexible:

  • Using i or M-i , and not editing the "Note:" prompt:
    • Inserts as Note heading the selected text up to the first newline in the selected text
    • Inserts below the heading all of the selected text.
  • Using i or M-i and editing the "Note:" prompt (.e.g, M-n at the "Note:" prompt):
    • Inserts as heading whatever you edit for the "Note" prompt
    • Inserts below the heading all of the selected text.
    • If you edit the "Note" prompt so it includes all of the selected text, that becomes the note heading, and there is no additional text inserted. This is the same as using Tab or C-M-i.
  • Using Tab or C-M-i asks no questions and inserts all of the selected text as the note heading (and there is no additional text inserted as note).

The above behavior is using the current development org-noter code (from https://github.com/org-noter/org-noter ---commit 8701093: org-noter/org-noter@8701093) and org-pdftools/org-noter-pdftools with cdr replaced by cadr (as indicated in #93 (comment) and #93 (comment)) and the change in #93 (comment), both available from branch https://github.com/rdiaz02/org-pdftools/tree/cadr_change, commit 51d372d ---rdiaz02@51d372b.


As reference this is the behavior using other versions of these packages.

org-noter 1.4.1 with org-noter-pdftools 20220320.300, org-pdftools 20220320.301 (i.e., commit 967f48f in this repo)

  • Using i or M-i , and not editing the "Note:" prompt:
    • Inserts as Note heading all of the selected text.
    • No additional text is inserted.
    • This is the same as using Tab in selected text.
  • Using i or M-i and editing the "Note:" prompt (.e.g, M-n at the "Note:" prompt):
    • Inserts as heading whatever you edit for the "Note" prompt.
    • No additional text is inserted (i.e., the content of the selected text is not inserted as part of the note).
  • Using Tab asks no questions and inserts all of the selected text as the note heading (and there is no additional text inserted as note).

org-noter 1.4.1 with org-pdftools/org-noter-pdftools with cdr replaced by cadr, as indicated in #93 (comment) and #93 (comment)), available from branch https://github.com/rdiaz02/org-pdftools/tree/cadr_change

Same as org-noter 1.4.1 with org-noter-pdftools 20220320.300, org-pdftools 20220320.301.


Pull request created

@petermao
Copy link

@fuxialexander I'm glad to see you're still involved in this package!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants