Re: master 65108998b1e: docview: imenu: check return value of 'mutool'

Re: master 65108998b1e: docview: imenu: check return value of 'mutool'

[Arash Esbati]

Eli Zaretskii <eliz@gnu.org> writes:

> branch: master
> commit 65108998b1ee0b42b57d478ba3b51f9040f04cd4
> Author: Morgan Smith <Morgan.J.Smith@outlook.com>
> Commit: Eli Zaretskii <eliz@gnu.org>
>
>     docview: imenu: check return value of 'mutool'
>     
>     While 'mutool' supports many filetypes, 'mutool show' only
>     supports PDF files.  This would lead to cryptic imenu errors
>     when opening other
>     file types (like EPUB) since we would parse the error output.
>     During my testing this caused 'imenu--index-alist' to have a
>     value of '(nil).
>     
>     * lisp/doc-view.el (doc-view--pdf-outline): Error when 'mutool'
>     returns an error.  Use 'call-process' to get the return value and
>     remove the call to 'shell-quote-argument' as 'call-process'
>     doesn't want any escapes.
>     (doc-view-mode): Handle possible error from 'doc-view-imenu-setup'.
>     (doc-view-imenu-enabled): Remove superfluous (and ... t).
>     (doc-view-imenu-setup): Remove check for mutool already ensured by
>     'doc-view-imenu-enabled' being non-nil.
>     (Bug#64516)
> ---
>  lisp/doc-view.el | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/lisp/doc-view.el b/lisp/doc-view.el
> index b14655fb274..847601872f5 100644
> --- a/lisp/doc-view.el
> +++ b/lisp/doc-view.el
> @@ -147,6 +147,8 @@
>  (require 'filenotify)
>  (eval-when-compile (require 'subr-x))
>  
> +(autoload 'imenu-unavailable-error "imenu")
> +
>  ;;;; Customization Options
>  
>  (defgroup doc-view nil
> @@ -214,7 +216,7 @@ are available (see Info node `(emacs)Document View')."
>    :type 'boolean
>    :version "30.1")
>  
> -(defcustom doc-view-imenu-enabled (and (executable-find "mutool") t)
> +(defcustom doc-view-imenu-enabled (executable-find "mutool")
>    "Whether to generate an imenu outline when \"mutool\" is available."
>    :type 'boolean
>    :version "29.1")

I'm not familiar with the code, so my apologies if this comment is off,
but the change above seems wrong.  With this change,
`doc-view-imenu-enabled' becomes a string or nil which doesn't match the
boolean type.  The (and ... t) part wasn't superfluous.

Best, Arash

Re: master 65108998b1e: docview: imenu: check return value of 'mutool'

[Eli Zaretskii]

> From: Arash Esbati <arash@gnu.org>
> Cc: "Morgan J. Smith" <Morgan.J.Smith@outlook.com>
> Date: Thu, 27 Jul 2023 20:49:27 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > branch: master
> > commit 65108998b1ee0b42b57d478ba3b51f9040f04cd4
> > Author: Morgan Smith <Morgan.J.Smith@outlook.com>
> > Commit: Eli Zaretskii <eliz@gnu.org>
> >
> >     docview: imenu: check return value of 'mutool'
> >     
> >     While 'mutool' supports many filetypes, 'mutool show' only
> >     supports PDF files.  This would lead to cryptic imenu errors
> >     when opening other
> >     file types (like EPUB) since we would parse the error output.
> >     During my testing this caused 'imenu--index-alist' to have a
> >     value of '(nil).
> >     
> >     * lisp/doc-view.el (doc-view--pdf-outline): Error when 'mutool'
> >     returns an error.  Use 'call-process' to get the return value and
> >     remove the call to 'shell-quote-argument' as 'call-process'
> >     doesn't want any escapes.
> >     (doc-view-mode): Handle possible error from 'doc-view-imenu-setup'.
> >     (doc-view-imenu-enabled): Remove superfluous (and ... t).
> >     (doc-view-imenu-setup): Remove check for mutool already ensured by
> >     'doc-view-imenu-enabled' being non-nil.
> >     (Bug#64516)
> > ---
> >  lisp/doc-view.el | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/lisp/doc-view.el b/lisp/doc-view.el
> > index b14655fb274..847601872f5 100644
> > --- a/lisp/doc-view.el
> > +++ b/lisp/doc-view.el
> > @@ -147,6 +147,8 @@
> >  (require 'filenotify)
> >  (eval-when-compile (require 'subr-x))
> >  
> > +(autoload 'imenu-unavailable-error "imenu")
> > +
> >  ;;;; Customization Options
> >  
> >  (defgroup doc-view nil
> > @@ -214,7 +216,7 @@ are available (see Info node `(emacs)Document View')."
> >    :type 'boolean
> >    :version "30.1")
> >  
> > -(defcustom doc-view-imenu-enabled (and (executable-find "mutool") t)
> > +(defcustom doc-view-imenu-enabled (executable-find "mutool")
> >    "Whether to generate an imenu outline when \"mutool\" is available."
> >    :type 'boolean
> >    :version "29.1")
> 
> I'm not familiar with the code, so my apologies if this comment is off,
> but the change above seems wrong.  With this change,
> `doc-view-imenu-enabled' becomes a string or nil which doesn't match the
> boolean type.  The (and ... t) part wasn't superfluous.

This was okayed by Tassilo, so I'm adding him to this discussion.

Re: master 65108998b1e: docview: imenu: check return value of 'mutool'

[Tassilo Horn]

Eli Zaretskii <eliz@gnu.org> writes:

>> > +(defcustom doc-view-imenu-enabled (executable-find "mutool")
>> >    "Whether to generate an imenu outline when \"mutool\" is available."
>> >    :type 'boolean
>> >    :version "29.1")
>> 
>> I'm not familiar with the code, so my apologies if this comment is
>> off, but the change above seems wrong.  With this change,
>> `doc-view-imenu-enabled' becomes a string or nil which doesn't match
>> the boolean type.  The (and ... t) part wasn't superfluous.
>
> This was okayed by Tassilo, so I'm adding him to this discussion.

Oh, I've not just okayed but actually smuggled in this logical
simplification into Morgan's patch which is obviously not correct in
this specific place.  I've just fixed it on master.

Thanks for the heads-up and apologies, especially to Morgan.

Bye,
Tassilo