Re: [O] org-pdfview-open doesn't work anymore

[Julien Cubizolles]

Nicolas Goaziou  writes:


> I think the simplest solution may be to follow the advice in ORG-NEWS
> and use
>
>   (lambda (file link) (org-pdfview-open file))

It's working, thanks.

Julien.

Re: [O] org-pdfview-open doesn't work anymore

[Nicolas Goaziou]

Hello,

Michael Brand  writes:

> Remixed patch for review attached.

It looks good. Please apply it on master.

Thank you.

Regards,

-- 
Nicolas Goaziou

Re: [O] org-pdfview-open doesn't work anymore

[Michael Brand]

Hi Nicolas

On Sat, Feb 6, 2016 at 5:41 PM, Nicolas Goaziou  wrote:

> IMO, notifying user that there's something rotten in the state of
> `org-file-apps' is enough. There's no need to go into the gory details
> of the problem.

I agree and finally found what I was missing: Add `debug' to the
handler of `condition-case' to finally not disable support of further
investigation with `toggle-debug-on-error'. It obsoletes my trials to
provide enough context about the Lisp error in the handler itself.
Remixed patch for review attached.

Michael
From 96aa89840c15c71c534faa0ce265530d5ff88c0a Mon Sep 17 00:00:00 2001
From: Michael Brand 
Date: Sun, 7 Feb 2016 11:07:56 +0100
Subject: [PATCH] `org-file-apps' add migration hint for function signature

* lisp/org.el (org-open-file): Add a user error for when the function
signature does not match.
---
 lisp/org.el | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index cce4f3a..e77fd4a 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -11325,15 +11325,22 @@ If the file does not exist, an error is thrown."
  ((functionp cmd)
   (save-match-data
 	(set-match-data link-match-data)
-	(funcall cmd file link)))
+	(condition-case nil
+	(funcall cmd file link)
+	  ;; FIXME: Remove this check when most default installations
+	  ;; of Emacs have at least Org 9.0.
+	  ((debug wrong-number-of-arguments wrong-type-argument
+	invalid-function)
+	   (user-error "Please see Org News for version 9.0 about \
+`org-file-apps'--Lisp error: %S" cmd)
  ((consp cmd)
   ;; FIXME: Remove this check when most default installations of
   ;; Emacs have at least Org 9.0.
   ;; Heads-up instead of silently fall back to
   ;; `org-link-frame-setup' for an old usage of `org-file-apps'
   ;; with sexp instead of a function for `cmd'.
-  (user-error
-   "Please see Org News for version 9.0 about `org-file-apps'"))
+  (user-error "Please see Org News for version 9.0 about \
+`org-file-apps'--Error: Deprecated usage of %S" cmd))
  (t (funcall (cdr (assq 'file org-link-frame-setup)) file)))
 (and (derived-mode-p 'org-mode)
 	 (eq old-mode 'org-mode)
-- 
2.4.9 (Apple Git-60)

Re: [O] org-pdfview-open doesn't work anymore

[Nicolas Goaziou]

Hello,

Michael Brand  writes:

> I just notice that in our case at least in case of wrong-type-argument the
> `cmd' is missing, so I suggest
>
> ((wrong-number-of-arguments wrong-type-argument invalid-function)
>  (user-error "Please see Org News for version 9.0 about \
> `org-file-apps'--Lisp error: The function %S leads to %S" cmd err))
>
> for the attached intermediate patch version. For the above example of
> (zerop nil) it would not only report "wrong-type-argument" and
> "numberp" together with "nil" but also "zerop" which in our case is
> the registered application, the source of the problem where the user
> needs to look.

IMO, notifying user that there's something rotten in the state of
`org-file-apps' is enough. There's no need to go into the gory details
of the problem.

Anyway, if you feel strongly about it, your patch looks good and you can
push it.

Thank you.


Regards,

-- 
Nicolas Goaziou

Re: [O] org-pdfview-open doesn't work anymore

[Michael Brand]

Hi Nicolas

On Fri, Feb 5, 2016 at 11:43 PM, Nicolas Goaziou  wrote:

> Michael Brand  writes:

>> + ;; FIXME: Remove this check when most default installations of
>> + ;; Emacs have at least Org 9.0.
>> + ((wrong-number-of-arguments invalid-function)
>> +  (user-error
>> +   (concat
>> +"Please see Org News for version 9.0 about `org-file-apps', "
>> +"error: "
>> +(prin1-to-string err))
>
> (user-error
>  "Please see Org News for version 9.0 about `org-file-apps', error: %S"
>  (nth 1 err))

The above does not provide (nth 0 err) which is the important error
type and (nth 2 err) which I find also helpful. See following zerop
examples.

What am I missing that it should not be

((wrong-number-of-arguments wrong-type-argument invalid-function)
 (user-error "Please see Org News for version 9.0 about \
`org-file-apps'--Lisp error: %S" err))

to get

Please see Org News for version 9.0 about \
`org-file-apps'--Lisp error: (wrong-number-of-arguments zerop 2)

Please see Org News for version 9.0 about \
`org-file-apps'--Lisp error: (wrong-type-argument numberp nil)

from

(condition-case err (zerop nil nil) ...)

(condition-case err (zerop nil) ...)

to mimic

Debugger entered--Lisp error: (wrong-number-of-arguments zerop 2)

Debugger entered--Lisp error: (wrong-type-argument numberp nil)

from

(zerop nil nil)

(zerop nil)

as far as possible?

I just notice that in our case at least in case of wrong-type-argument the
`cmd' is missing, so I suggest

((wrong-number-of-arguments wrong-type-argument invalid-function)
 (user-error "Please see Org News for version 9.0 about \
`org-file-apps'--Lisp error: The function %S leads to %S" cmd err))

for the attached intermediate patch version. For the above example of
(zerop nil) it would not only report "wrong-type-argument" and
"numberp" together with "nil" but also "zerop" which in our case is
the registered application, the source of the problem where the user
needs to look.

Michael
From 873e99c9ee03594e45dd3e82d880c4b8a90d2192 Mon Sep 17 00:00:00 2001
From: Michael Brand 
Date: Sat, 6 Feb 2016 09:03:17 +0100
Subject: [PATCH] `org-file-apps' add migration hint for function signature

* lisp/org.el (org-open-file): Add an error for when the function
signature does not match.
---
 lisp/org.el | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index cce4f3a..cacae0f 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -11325,15 +11325,21 @@ If the file does not exist, an error is thrown."
  ((functionp cmd)
   (save-match-data
 	(set-match-data link-match-data)
-	(funcall cmd file link)))
+	(condition-case err
+	(funcall cmd file link)
+	  ;; FIXME: Remove this check when most default installations
+	  ;; of Emacs have at least Org 9.0.
+	  ((wrong-number-of-arguments wrong-type-argument invalid-function)
+	   (user-error "Please see Org News for version 9.0 about \
+`org-file-apps'--Lisp error: The function %S leads to %S" cmd err)
  ((consp cmd)
   ;; FIXME: Remove this check when most default installations of
   ;; Emacs have at least Org 9.0.
   ;; Heads-up instead of silently fall back to
   ;; `org-link-frame-setup' for an old usage of `org-file-apps'
   ;; with sexp instead of a function for `cmd'.
-  (user-error
-   "Please see Org News for version 9.0 about `org-file-apps'"))
+  (user-error "Please see Org News for version 9.0 about \
+`org-file-apps'--error: Deprecated usage of %S" cmd))
  (t (funcall (cdr (assq 'file org-link-frame-setup)) file)))
 (and (derived-mode-p 'org-mode)
 	 (eq old-mode 'org-mode)
-- 
2.4.9 (Apple Git-60)

Re: [O] org-pdfview-open doesn't work anymore

[Nicolas Goaziou]

Hello,

Michael Brand  writes:

> +  ;; FIXME: Remove this check when most default installations of
> +  ;; Emacs have at least Org 9.0.
> +  (let ((arglist (help-function-arglist cmd)))
> + (when (or (memq ' arglist)
> +   (memq ' arglist)
> +   (/= 2 (length arglist)))
> +   (user-error
> +(format
> + "%s%s%S"
> + "Please see Org News for version 9.0 about `org-file-apps', "
> + "this function signature is wrong: "
> + cmd

I have the feeling there is some over-engineering involved there. 

In any case, instead of relying on `help-function-arglist', I suggest to
use something lightweight:

(condition-case err
(funcall ...)
  (wrong-number-of-arguments
   (user-error "Please ..."))
  (invalid-function
   (user-error "Please ...")))


Regards,

-- 
Nicolas Goaziou

Re: [O] org-pdfview-open doesn't work anymore

[Michael Brand]

Hi Julien

On Fri, Feb 5, 2016 at 6:46 AM, Julien Cubizolles  wrote:
> I've been using org-pdfview (from
> https://github.com/markus1189/org-pdfview) to have org-mode open pdf
> files generated during export.
>
> --8<---cut here---start->8---
> (pdf-tools-install)
> (eval-after-load 'org '(progn (require 'org-pdfview)
>   (add-to-list 'org-file-apps '("\\.pdf\\'" . 
> org-pdfview-open))
>   ))
> --8<---cut here---end--->8---
>
> Since a recent upgrade, this fails with:
>
> --8<---cut here---start->8---
> (wrong-number-of-arguments #[(link) "\304\305 \"\2031\306\307 \"\310\306\311 
> \"!\310\306\312 \"!\313\307\"\210\314
> !\210\315\316 \317 @_\320 \245!!+\207\304\321 \"\203N\306\307 \"\310\306\311 
> \"!\313\307\"\210\314
> !*\207\313 \307\"\207" [link path page height string-match 
> "\\(.*\\)::\\([0-9]*\\)\\+\\+\\([[0-9]\\.*[0-9]*\\)" match-string 1 
> string-to-number 2 3 org-open-file pdf-view-goto-page 
> image-set-window-vscroll round pdf-view-image-size frame-char-height 
> "\\(.*\\)::\\([0-9]+\\)$"] 4 
> ("/home/wilk/.emacs.d/elpa/org-pdfview-20160125.1254/org-pdfview.elc" . 662)] 
> 2)
>   
> org-pdfview-open("/home/wilk/enseignement/2015-2016/topos/topo-tipe-beamer.pdf"
>  "/home/wilk/enseignement/2015-2016/topos/topo-tipe-beamer.pdf")
> --8<---cut here---end--->8---
>
> Is it a bug in Org-mode or should I report the issue to the org-pdfview
> author ?

Due to lexical binding in org.el there was a change in
`org-file-apps', see Org News for version 9.0 and e. g. this thread:
http://thread.gmane.org/gmane.emacs.orgmode/104272
I think the most convenient would be if `org-open-file' tries to find
out that `cmd' in this case is a function with only one argument and
call it with just `file'.

@Nicolas: Is this reasonable for you to implement?

Michael

Re: [O] org-pdfview-open doesn't work anymore

[Nicolas Goaziou]

Hello,

Michael Brand  writes:

> + ;; FIXME: Remove this check when most default installations of
> + ;; Emacs have at least Org 9.0.
> + ((wrong-number-of-arguments invalid-function)
> +  (user-error
> +   (concat
> +"Please see Org News for version 9.0 about `org-file-apps', "
> +"error: "
> +(prin1-to-string err))

(user-error
 "Please see Org News for version 9.0 about `org-file-apps', error: %S"
 (nth 1 err))

You can use continuation character (i.e., "\" followed by a newline) if
string is too large.

>   ((consp cmd)
>;; FIXME: Remove this check when most default installations of
>;; Emacs have at least Org 9.0.
> @@ -11333,7 +11342,9 @@ If the file does not exist, an error is thrown."
>;; `org-link-frame-setup' for an old usage of `org-file-apps'
>;; with sexp instead of a function for `cmd'.
>(user-error
> -   "Please see Org News for version 9.0 about `org-file-apps'"))
> +   (concat "Please see Org News for version 9.0 about `org-file-apps', "
> +"error: deprecated usage of "
> +(prin1-to-string cmd

Ditto.


Regards,

-- 
Nicolas Goaziou

Re: [O] org-pdfview-open doesn't work anymore

[Michael Brand]

Hi Nicolas

On Fri, Feb 5, 2016 at 6:22 PM, Nicolas Goaziou  wrote:

> Michael Brand  writes:

>> +  ;; FIXME: Remove this check when most default installations of
>> +  ;; Emacs have at least Org 9.0.
>> +  (let ((arglist (help-function-arglist cmd)))
>> + (when (or (memq ' arglist)
>> +   (memq ' arglist)
>> +   (/= 2 (length arglist)))
>> +   (user-error
>> +(format
>> + "%s%s%S"
>> + "Please see Org News for version 9.0 about `org-file-apps', "
>> + "this function signature is wrong: "
>> + cmd
>
> I have the feeling there is some over-engineering involved there.

Also it should have allowed at least optional arguments

(when (or (memq
   ;; Too complicated to parse regarding that such functions
   ;; are probably not useful here.
   '
   arglist)
  (/= 2 (length (delete ' arglist
  (user-error

for e. g. a simple

(add-to-list 'org-file-apps
 (cons (concat org-player-file-extensions-regexp "$")
   'org-player-play-file))

which refers to the existing

(defun org-player-play-file (filename  pos)

> In any case, instead of relying on `help-function-arglist', I suggest to
> use something lightweight:
>
> (condition-case err
> (funcall ...)
>   (wrong-number-of-arguments
>(user-error "Please ..."))
>   (invalid-function
>(user-error "Please ...")))

Of course. I didn't know about this possibility, remixed patch attached.

Michael
From f1c382cabe5b34f52db22df70d5c25e02de2a18a Mon Sep 17 00:00:00 2001
From: Michael Brand 
Date: Fri, 5 Feb 2016 19:42:55 +0100
Subject: [PATCH] `org-file-apps' add migration hint for function signature

* lisp/org.el (org-open-file): Add an error when the function
signature does not match.
---
 lisp/org.el | 19 +++
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index d4fb8a6..f6c5f89 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -11323,9 +11323,18 @@ If the file does not exist, an error is thrown."
 		  (when (derived-mode-p 'org-mode) (org-reveal)))
 	(search (org-link-search search
  ((functionp cmd)
-  (save-match-data
-	(set-match-data link-match-data)
-	(funcall cmd file link)))
+  (condition-case err
+	  (save-match-data
+	(set-match-data link-match-data)
+	(funcall cmd file link))
+	;; FIXME: Remove this check when most default installations of
+	;; Emacs have at least Org 9.0.
+	((wrong-number-of-arguments invalid-function)
+	 (user-error
+	  (concat
+	   "Please see Org News for version 9.0 about `org-file-apps', "
+	   "error: "
+	   (prin1-to-string err))
  ((consp cmd)
   ;; FIXME: Remove this check when most default installations of
   ;; Emacs have at least Org 9.0.
@@ -11333,7 +11342,9 @@ If the file does not exist, an error is thrown."
   ;; `org-link-frame-setup' for an old usage of `org-file-apps'
   ;; with sexp instead of a function for `cmd'.
   (user-error
-   "Please see Org News for version 9.0 about `org-file-apps'"))
+   (concat "Please see Org News for version 9.0 about `org-file-apps', "
+	   "error: deprecated usage of "
+	   (prin1-to-string cmd
  (t (funcall (cdr (assq 'file org-link-frame-setup)) file)))
 (and (derived-mode-p 'org-mode)
 	 (eq old-mode 'org-mode)
-- 
2.1.3.dirty

Re: [O] org-pdfview-open doesn't work anymore

[Nicolas Goaziou]

Hello,

Michael Brand  writes:

> Hi Julien
>
> On Fri, Feb 5, 2016 at 6:46 AM, Julien Cubizolles  
> wrote:
>> I've been using org-pdfview (from
>> https://github.com/markus1189/org-pdfview) to have org-mode open pdf
>> files generated during export.
>>
>> --8<---cut here---start->8---
>> (pdf-tools-install)
>> (eval-after-load 'org '(progn (require 'org-pdfview)
>>   (add-to-list 'org-file-apps '("\\.pdf\\'" . 
>> org-pdfview-open))
>>   ))
>> --8<---cut here---end--->8---
>>
>> Since a recent upgrade, this fails with:
>>
>> --8<---cut here---start->8---
>> (wrong-number-of-arguments #[(link) "\304\305 \"\2031\306\307 \"\310\306\311 
>> \"!\310\306\312 \"!\313\307\"\210\314
>> !\210\315\316 \317 @_\320 \245!!+\207\304\321 \"\203N\306\307 \"\310\306\311 
>> \"!\313\307\"\210\314
>> !*\207\313 \307\"\207" [link path page height string-match 
>> "\\(.*\\)::\\([0-9]*\\)\\+\\+\\([[0-9]\\.*[0-9]*\\)" match-string 1 
>> string-to-number 2 3 org-open-file pdf-view-goto-page 
>> image-set-window-vscroll round pdf-view-image-size frame-char-height 
>> "\\(.*\\)::\\([0-9]+\\)$"] 4 
>> ("/home/wilk/.emacs.d/elpa/org-pdfview-20160125.1254/org-pdfview.elc" . 
>> 662)] 2)
>>   
>> org-pdfview-open("/home/wilk/enseignement/2015-2016/topos/topo-tipe-beamer.pdf"
>>  "/home/wilk/enseignement/2015-2016/topos/topo-tipe-beamer.pdf")
>> --8<---cut here---end--->8---
>>
>> Is it a bug in Org-mode or should I report the issue to the org-pdfview
>> author ?
>
> Due to lexical binding in org.el there was a change in
> `org-file-apps', see Org News for version 9.0 and e. g. this thread:
> http://thread.gmane.org/gmane.emacs.orgmode/104272
> I think the most convenient would be if `org-open-file' tries to find
> out that `cmd' in this case is a function with only one argument and
> call it with just `file'.
>
> @Nicolas: Is this reasonable for you to implement?

I think the simplest solution may be to follow the advice in ORG-NEWS
and use

  (lambda (file link) (org-pdfview-open file))


Regards,

-- 
Nicolas Goaziou

Re: [O] org-pdfview-open doesn't work anymore

[Michael Brand]

Hi Nicolas

On Fri, Feb 5, 2016 at 9:33 AM, Michael Brand
 wrote:

> Due to lexical binding in org.el there was a change in
> `org-file-apps', see Org News for version 9.0 and e. g. this thread:
> http://thread.gmane.org/gmane.emacs.orgmode/104272
> I think the most convenient would be if `org-open-file' tries to find
> out that `cmd' in this case is a function with only one argument and
> call it with just `file'.

Only after a closer look I saw that the single parameter of
`org-pdfview-open' is not `file' but `link'. It is probably better for
`org-open-file' to not guess in case of `cmd' with a single parameter
whether it is meant to be `file' or `link'. That leads me to suggest
the attached patch to be reviewed that checks the function signature.

Michael
From 9788cb03d2714cde555fbe2abb55ddd383a885c1 Mon Sep 17 00:00:00 2001
From: Michael Brand 
Date: Fri, 5 Feb 2016 14:44:26 +0100
Subject: [PATCH] `org-file-apps' add migration hint for function signature

* lisp/org.el (org-open-file): Add an error when the function
signature does not match.
---
 lisp/org.el | 17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/lisp/org.el b/lisp/org.el
index 5a6c74e..9ebabf8 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -11323,6 +11323,18 @@ If the file does not exist, an error is thrown."
 		  (when (derived-mode-p 'org-mode) (org-reveal)))
 	(search (org-link-search search
  ((functionp cmd)
+  ;; FIXME: Remove this check when most default installations of
+  ;; Emacs have at least Org 9.0.
+  (let ((arglist (help-function-arglist cmd)))
+	(when (or (memq ' arglist)
+		  (memq ' arglist)
+		  (/= 2 (length arglist)))
+	  (user-error
+	   (format
+	"%s%s%S"
+	"Please see Org News for version 9.0 about `org-file-apps', "
+	"this function signature is wrong: "
+	cmd
   (save-match-data
 	(set-match-data link-match-data)
 	(funcall cmd file link)))
@@ -11333,7 +11345,10 @@ If the file does not exist, an error is thrown."
   ;; `org-link-frame-setup' for an old usage of `org-file-apps'
   ;; with sexp instead of a function for `cmd'.
   (user-error
-   "Please see Org News for version 9.0 about `org-file-apps'"))
+   (format "%s%s%S"
+	   "Please see Org News for version 9.0 about `org-file-apps', "
+	   "this usage is wrong: "
+	   cmd)))
  (t (funcall (cdr (assq 'file org-link-frame-setup)) file)))
 (and (derived-mode-p 'org-mode)
 	 (eq old-mode 'org-mode)
-- 
2.1.3.dirty