fuxialexander / org-pdftools

A custom org link type for pdf-tools
GNU General Public License v3.0
331 stars 35 forks source link

Error when inserting link or precise link #93

Closed uliw closed 12 months ago

uliw commented 1 year ago

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

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

bigodel commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

uliw commented 1 year ago

ok. thanks

bigodel commented 1 year ago

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 commented 1 year ago

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

petermao commented 1 year ago

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 commented 1 year ago

@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 https://github.com/org-noter/org-noter/issues/6 and https://github.com/org-noter/org-noter/issues/8) and installing 1.4.1 solved them.

rdiaz02 commented 1 year ago

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 https://github.com/fuxialexander/org-pdftools/issues/93#issuecomment-1483236732 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")
  2. 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")
  3. 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 commented 1 year ago

@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: https://github.com/fuxialexander/org-pdftools/blob/967f48fb5038bba32915ee9da8dc4e8b10ba3376/org-noter-pdftools.el#L134 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 commented 1 year ago

another one that needs to be changed to cadr location. https://github.com/fuxialexander/org-pdftools/blob/967f48fb5038bba32915ee9da8dc4e8b10ba3376/org-noter-pdftools.el#L301

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

rdiaz02 commented 1 year ago

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 commented 1 year ago

What I can see from the stack trace is that the location that is getting passed around is a structure as defined here: https://github.com/fuxialexander/org-pdftools/blob/967f48fb5038bba32915ee9da8dc4e8b10ba3376/org-noter-pdftools.el#L111-L112 org-noter needs to have this converted into a cons cell probably using https://github.com/fuxialexander/org-pdftools/blob/967f48fb5038bba32915ee9da8dc4e8b10ba3376/org-noter-pdftools.el#L141-L143 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 commented 1 year ago

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 commented 1 year ago

I believe the crux of the problem lies with https://github.com/fuxialexander/org-pdftools/blob/967f48fb5038bba32915ee9da8dc4e8b10ba3376/org-noter-pdftools.el#L235-L236 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 commented 1 year ago

I believe the crux of the problem lies with

https://github.com/fuxialexander/org-pdftools/blob/967f48fb5038bba32915ee9da8dc4e8b10ba3376/org-noter-pdftools.el#L235-L236 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.

Starting org-noter gives this error (which seems the same as https://github.com/org-noter/org-noter/issues/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 (https://github.com/fuxialexander/org-pdftools/issues/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 commented 1 year ago

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:

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 commented 1 year ago

@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 https://github.com/fuxialexander/org-pdftools/blob/967f48fb5038bba32915ee9da8dc4e8b10ba3376/org-noter-pdftools.el#L277 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 commented 1 year ago

Perfect, thanks.

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

The above behavior is using the current development org-noter code (from https://github.com/org-noter/org-noter ---commit 8701093: https://github.com/org-noter/org-noter/commit/870109330a337751e44042d1b407d90d42ca9130) and org-pdftools/org-noter-pdftools with cdr replaced by cadr (as indicated in https://github.com/fuxialexander/org-pdftools/issues/93#issuecomment-1486200412 and https://github.com/fuxialexander/org-pdftools/issues/93#issuecomment-1486044761) and the change in https://github.com/fuxialexander/org-pdftools/issues/93#issuecomment-1483974935, both available from branch https://github.com/rdiaz02/org-pdftools/tree/cadr_change, commit 51d372d ---https://github.com/rdiaz02/org-pdftools/commit/51d372b5b9f519add7996f0a6cada8fc9286a54f.


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)

org-noter 1.4.1 with org-pdftools/org-noter-pdftools with cdr replaced by cadr, as indicated in https://github.com/fuxialexander/org-pdftools/issues/93#issuecomment-1486200412 and https://github.com/fuxialexander/org-pdftools/issues/93#issuecomment-1486044761), 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 commented 12 months ago

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