bug#64516: [PATCH] docview: Only enable imenu when supported

bug#64516: [PATCH] docview: Only enable imenu when supported

[Morgan Smith]

[-- Attachment #1: Type: text/plain, Size: 522 bytes --]

Hello,

More info is in the commit message.

To re-create the error open up an EPUB file (or possibly any non-PDF
file in doc-view) and press 'M-g i' to run 'imenu'.  Doing that I get
the following error:

Debugger entered--Lisp error: (wrong-type-argument stringp nil)
  string-match("\\`PK\\'" nil)
  imenu-find-default("PK" (("*Rescan*" . -99) nil))
  imenu--completion-buffer((("*Rescan*" . -99) nil) nil)
  imenu-choose-buffer-index()
  byte-code("\300 C\207" [imenu-choose-buffer-index] 1)
  command-execute(imenu)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-docview-Only-enable-imenu-when-supported.patch --]
[-- Type: text/x-patch, Size: 1939 bytes --]

From cd73281f324a5b21c304789ca7b828295e8718d9 Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Fri, 7 Jul 2023 11:17:15 -0400
Subject: [PATCH] docview: Only enable imenu when supported

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.
(doc-view-imenu-setup): Don't check for 'mutool' as that is already
done by 'doc-view-imenu-enabled'.  Only enable imenu for PDF
documents.
---
 lisp/doc-view.el | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lisp/doc-view.el b/lisp/doc-view.el
index b14655fb274..d9c12fc49c0 100644
--- a/lisp/doc-view.el
+++ b/lisp/doc-view.el
@@ -1912,7 +1912,8 @@ doc-view--pdf-outline
       (let ((outline nil)
             (fn (shell-quote-argument (expand-file-name fn))))
         (with-temp-buffer
-          (insert (shell-command-to-string (format "mutool show %s outline" fn)))
+          (unless (= 0 (call-process "mutool" nil (current-buffer) nil "show" fn "outline"))
+            (error "Unable to create imenu index using `mutool'"))
           (goto-char (point-min))
           (while (re-search-forward doc-view--outline-rx nil t)
             (push `((level . ,(length (match-string 1)))
@@ -1961,7 +1962,7 @@ doc-view-imenu-index
 
 (defun doc-view-imenu-setup ()
   "Set up local state in the current buffer for imenu, if needed."
-  (when (and doc-view-imenu-enabled (executable-find "mutool"))
+  (when (and doc-view-imenu-enabled (eq 'pdf doc-view-doc-type))
     (setq-local imenu-create-index-function #'doc-view-imenu-index
                 imenu-submenus-on-top nil
                 imenu-sort-function nil
-- 
2.40.1

bug#64516: [PATCH] docview: Only enable imenu when supported

[Morgan Smith]

[-- Attachment #1: Type: text/plain, Size: 200 bytes --]

Sorry, here is a V2.  The previous patch would fail for files with funny
filenames that need escaping.  This I have removed the call to
'shell-quote-argument' as 'call-process' doesn't want escapes.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-docview-Only-enable-imenu-when-supported.patch --]
[-- Type: text/x-patch, Size: 3348 bytes --]

From 1f4ef75d0ed4fedb3be1ef67bb32fd89a3577db5 Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Fri, 7 Jul 2023 11:17:15 -0400
Subject: [PATCH] docview: Only enable imenu when supported

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.  Simplify a little by removing a 'let' statement.
(doc-view-imenu-setup): Don't check for 'mutool' as that is already
done by 'doc-view-imenu-enabled'.  Only enable imenu for PDF
documents. (bug#64516)
---
 lisp/doc-view.el | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/lisp/doc-view.el b/lisp/doc-view.el
index b14655fb274..693a5ae5509 100644
--- a/lisp/doc-view.el
+++ b/lisp/doc-view.el
@@ -1907,20 +1907,20 @@ doc-view--pdf-outline
 Each element in the returned list contains information about a section's
 title, nesting level and page number.  The list is flat: its tree
 structure is extracted by `doc-view--imenu-subtree'."
-  (let ((fn (or file-name (buffer-file-name))))
+  (let ((fn (expand-file-name (or file-name (buffer-file-name))))
+        outline)
     (when fn
-      (let ((outline nil)
-            (fn (shell-quote-argument (expand-file-name fn))))
-        (with-temp-buffer
-          (insert (shell-command-to-string (format "mutool show %s outline" fn)))
-          (goto-char (point-min))
-          (while (re-search-forward doc-view--outline-rx nil t)
-            (push `((level . ,(length (match-string 1)))
-                    (title . ,(replace-regexp-in-string "\\\\[rt]" " "
-                                                        (match-string 2)))
-                    (page . ,(string-to-number (match-string 3))))
-                  outline)))
-        (nreverse outline)))))
+      (with-temp-buffer
+        (unless (= 0 (call-process "mutool" nil (current-buffer) nil "show" fn "outline"))
+          (error "Unable to create imenu index using `mutool'"))
+        (goto-char (point-min))
+        (while (re-search-forward doc-view--outline-rx nil t)
+          (push `((level . ,(length (match-string 1)))
+                  (title . ,(replace-regexp-in-string "\\\\[rt]" " "
+                                                      (match-string 2)))
+                  (page . ,(string-to-number (match-string 3))))
+                outline)))
+      (nreverse outline))))
 
 (defun doc-view--imenu-subtree (outline act)
   "Construct a tree of imenu items for the given outline list and action.
@@ -1961,7 +1961,7 @@ doc-view-imenu-index
 
 (defun doc-view-imenu-setup ()
   "Set up local state in the current buffer for imenu, if needed."
-  (when (and doc-view-imenu-enabled (executable-find "mutool"))
+  (when (and doc-view-imenu-enabled (eq 'pdf doc-view-doc-type))
     (setq-local imenu-create-index-function #'doc-view-imenu-index
                 imenu-submenus-on-top nil
                 imenu-sort-function nil
-- 
2.40.1

bug#64516: [PATCH] docview: Only enable imenu when supported

[Morgan Smith]

[-- Attachment #1: Type: text/plain, Size: 230 bytes --]

Sorry, I wasn't thinking very hard about the simplifications I made in
the V2 patch.  Now it would error out if '(or file-name
(buffer-file-name))' was nil which I assume can happen.

Here is a V3 patch which should be good now.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-docview-Only-enable-imenu-when-supported.patch --]
[-- Type: text/x-patch, Size: 2192 bytes --]

From 011beaccb3b2e004240ed80a4948d791fc4efbb9 Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Fri, 7 Jul 2023 11:17:15 -0400
Subject: [PATCH] docview: Only enable imenu when supported

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-imenu-setup): Don't check for 'mutool' as that is already
done by 'doc-view-imenu-enabled'.  Only enable imenu for PDF
documents. (bug#64516)
---
 lisp/doc-view.el | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lisp/doc-view.el b/lisp/doc-view.el
index b14655fb274..101de04297f 100644
--- a/lisp/doc-view.el
+++ b/lisp/doc-view.el
@@ -1910,9 +1910,10 @@ doc-view--pdf-outline
   (let ((fn (or file-name (buffer-file-name))))
     (when fn
       (let ((outline nil)
-            (fn (shell-quote-argument (expand-file-name fn))))
+            (fn (expand-file-name fn)))
         (with-temp-buffer
-          (insert (shell-command-to-string (format "mutool show %s outline" fn)))
+          (unless (= 0 (call-process "mutool" nil (current-buffer) nil "show" fn "outline"))
+            (error "Unable to create imenu index using `mutool'"))
           (goto-char (point-min))
           (while (re-search-forward doc-view--outline-rx nil t)
             (push `((level . ,(length (match-string 1)))
@@ -1961,7 +1962,7 @@ doc-view-imenu-index
 
 (defun doc-view-imenu-setup ()
   "Set up local state in the current buffer for imenu, if needed."
-  (when (and doc-view-imenu-enabled (executable-find "mutool"))
+  (when (and doc-view-imenu-enabled (eq 'pdf doc-view-doc-type))
     (setq-local imenu-create-index-function #'doc-view-imenu-index
                 imenu-submenus-on-top nil
                 imenu-sort-function nil
-- 
2.40.1

bug#64516: [PATCH] docview: Only enable imenu when supported

[Eli Zaretskii]

> From: Morgan Smith <Morgan.J.Smith@outlook.com>
> Date: Fri, 07 Jul 2023 11:24:01 -0400
> 
> More info is in the commit message.
> 
> To re-create the error open up an EPUB file (or possibly any non-PDF
> file in doc-view) and press 'M-g i' to run 'imenu'.  Doing that I get
> the following error:
> 
> Debugger entered--Lisp error: (wrong-type-argument stringp nil)
>   string-match("\\`PK\\'" nil)
>   imenu-find-default("PK" (("*Rescan*" . -99) nil))
>   imenu--completion-buffer((("*Rescan*" . -99) nil) nil)
>   imenu-choose-buffer-index()
>   byte-code("\300 C\207" [imenu-choose-buffer-index] 1)
>   command-execute(imenu)

Thanks.

Tassilo, any comments to the problem and the proposed patch?

bug#64516: [PATCH] docview: Only enable imenu when supported

[Tassilo Horn]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Morgan & Eli,

>> To re-create the error open up an EPUB file (or possibly any non-PDF
>> file in doc-view) and press 'M-g i' to run 'imenu'.  Doing that I get
>> the following error:
>> 
>> Debugger entered--Lisp error: (wrong-type-argument stringp nil)
>>   string-match("\\`PK\\'" nil)
>>   imenu-find-default("PK" (("*Rescan*" . -99) nil))
>>   imenu--completion-buffer((("*Rescan*" . -99) nil) nil)
>>   imenu-choose-buffer-index()
>>   byte-code("\300 C\207" [imenu-choose-buffer-index] 1)
>>   command-execute(imenu)
>
> Thanks.
>
> Tassilo, any comments to the problem and the proposed patch?

I couldn't test it because all epub files I found result in

  imenu unavailable: "No items suitable for an index found in this buffer"

and not the described error.  Anyhow, I've looked at the patch (V3)
which basically looks good to me.

However, I'm not sure if setting doc-view-imenu-enabled to nil if mutool
is there but the doc is not a PDF is the right thing.  I think the only
difference is the error you get when invoking imenu, i.e., if we'd skip
that hunk of the patch and cater for the possibility that "mutool show"
might eventually support epub files, you'd get

  "Unable to create imenu index using `mutool'"

right now.  With that hunk, you get

  "This buffer cannot use ‘imenu-default-create-index-function’"

which is not really better.

So I'd suggest leave the doc-view-imenu-enabled initialization as-is and
in the "mutool show exits non-zero case", we signal a
imenu-unavailable-error (rather than a plain error) with the message
that's already there in the patch.

Bye,
Tassilo

bug#64516: [PATCH] docview: Only enable imenu when supported

[Morgan Smith]

[-- Attachment #1: Type: text/plain, Size: 688 bytes --]

Hello,

Tassilo Horn <tsdh@gnu.org> writes:

>
> I couldn't test it because all epub files I found result in
>
>   imenu unavailable: "No items suitable for an index found in this buffer"
>
> and not the described error.

I feel like my reviewers often can't reproduce my errors.  I'm running
Emacs from a very recent commit and I have mutool installed.  I really
feel like the error should be reproducible for anyone that meets those
two requirements.  Do I need to include more information in my bug
reports?  If so, what information?


Anyways I modified my patch with Tassilo's feedback.  However, I
realized that it partially reverts commit
b23e062d7463b76d25dfd9ba4a80c1848a448e42


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-docview-imenu-check-return-value-of-mutool.patch --]
[-- Type: text/x-patch, Size: 2425 bytes --]

From 5b24d23e3317491c31ae1392c29ac7cdf303f9d4 Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Tue, 11 Jul 2023 13:31:40 -0400
Subject: [PATCH] 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-imenu-setup): Do not preemptively run 'doc-view--pdf-outline'
as an error here would inhibit display of the document.
---
 lisp/doc-view.el | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/lisp/doc-view.el b/lisp/doc-view.el
index b14655fb274..d5ccfc19853 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
@@ -1910,9 +1912,10 @@ doc-view--pdf-outline
   (let ((fn (or file-name (buffer-file-name))))
     (when fn
       (let ((outline nil)
-            (fn (shell-quote-argument (expand-file-name fn))))
+            (fn (expand-file-name fn)))
         (with-temp-buffer
-          (insert (shell-command-to-string (format "mutool show %s outline" fn)))
+          (unless (= 0 (call-process "mutool" nil (current-buffer) nil "show" fn "outline"))
+            (imenu-unavailable-error "Unable to create imenu index using `mutool'"))
           (goto-char (point-min))
           (while (re-search-forward doc-view--outline-rx nil t)
             (push `((level . ,(length (match-string 1)))
@@ -1964,8 +1967,7 @@ doc-view-imenu-setup
   (when (and doc-view-imenu-enabled (executable-find "mutool"))
     (setq-local imenu-create-index-function #'doc-view-imenu-index
                 imenu-submenus-on-top nil
-                imenu-sort-function nil
-                doc-view--outline (doc-view--pdf-outline))
+                imenu-sort-function nil)
     (when doc-view--outline (imenu-add-to-menubar "Outline"))))
 
 ;;;; User interface commands and the mode
-- 
2.40.1


[-- Attachment #3: Type: text/plain, Size: 202 bytes --]



I can't really seem to figure out why we call 'doc-view--pdf-outline' in
doc-view-imenu-setup even after reading the two bug reports that are
linked in the commit message.  For this reason I CC'd jao

bug#64516: [PATCH] docview: Only enable imenu when supported

[Morgan Smith]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: 0001-docview-imenu-check-return-value-of-mutool.patch --]
[-- Type: text/x-patch, Size: 2331 bytes --]

From d2315c945d6fd87a2371eefb82ec0241287e66a1 Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Tue, 11 Jul 2023 14:08:24 -0400
Subject: [PATCH] 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): Run 'doc-view-imenu-setup' in the
'doc-view-mode-hook' so that errors don't prevent displaying the
document.
---
 lisp/doc-view.el | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lisp/doc-view.el b/lisp/doc-view.el
index b14655fb274..f64c3a9719b 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
@@ -1910,9 +1912,10 @@ doc-view--pdf-outline
   (let ((fn (or file-name (buffer-file-name))))
     (when fn
       (let ((outline nil)
-            (fn (shell-quote-argument (expand-file-name fn))))
+            (fn (expand-file-name fn)))
         (with-temp-buffer
-          (insert (shell-command-to-string (format "mutool show %s outline" fn)))
+          (unless (= 0 (call-process "mutool" nil (current-buffer) nil "show" fn "outline"))
+            (imenu-unavailable-error "Unable to create imenu index using `mutool'"))
           (goto-char (point-min))
           (while (re-search-forward doc-view--outline-rx nil t)
             (push `((level . ,(length (match-string 1)))
@@ -2236,7 +2239,7 @@ doc-view-mode
     (setq mode-name "DocView"
 	  buffer-read-only t
 	  major-mode 'doc-view-mode)
-    (doc-view-imenu-setup)
+    (add-hook 'doc-view-mode-hook #'doc-view-imenu-setup nil t)
     (doc-view-initiate-display)
     ;; Switch off view-mode explicitly, because doc-view-mode is the
     ;; canonical view mode for PDF/PS/DVI files.  This could be
-- 
2.40.1


[-- Attachment #2: Type: text/plain, Size: 778 bytes --]



Hello,

I apologize for my "oops I figured it out immediately after I sent the
email" syndrome.  I was trying to find the answer in the bug reports
when the answer was in the code and quite obvious.

Anyways here is a patch that unconditionally enables imenu but checks
the return value of mutool and will raise an imenu-unavailable-error
when it is not successful.  However, that error stops docview from
displaying the document so I decided to put the setup function in a hook
so the error occurs after the document is displayed.

I looked briefly for documentation on how errors work in hooks but could
not find any.  I don't want this error to prevent the other hooks from
being run (which would be user defined hooks.  we don't currently use
this hook).

Thanks,

Morgan

bug#64516: [PATCH] docview: Only enable imenu when supported

[Eli Zaretskii]

> From: Morgan Smith <Morgan.J.Smith@outlook.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  64516@debbugs.gnu.org, jao@gnu.org
> Date: Tue, 11 Jul 2023 13:49:25 -0400
> 
> Anyways I modified my patch with Tassilo's feedback.  However, I
> realized that it partially reverts commit
> b23e062d7463b76d25dfd9ba4a80c1848a448e42

And reintroduces the problem which that commit fixed?

bug#64516: [PATCH] docview: Only enable imenu when supported

[Eli Zaretskii]

> From: Morgan Smith <Morgan.J.Smith@outlook.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  64516@debbugs.gnu.org,  jao@gnu.org
> Date: Tue, 11 Jul 2023 14:22:49 -0400
> 
> I apologize for my "oops I figured it out immediately after I sent the
> email" syndrome.  I was trying to find the answer in the bug reports
> when the answer was in the code and quite obvious.
> 
> Anyways here is a patch that unconditionally enables imenu but checks
> the return value of mutool and will raise an imenu-unavailable-error
> when it is not successful.  However, that error stops docview from
> displaying the document so I decided to put the setup function in a hook
> so the error occurs after the document is displayed.
> 
> I looked briefly for documentation on how errors work in hooks but could
> not find any.  I don't want this error to prevent the other hooks from
> being run (which would be user defined hooks.  we don't currently use
> this hook).

Tassilo, is this okay, in your opinion?

bug#64516: [PATCH] docview: Only enable imenu when supported

[Tassilo Horn]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Morgan & Eli,

>> Anyways here is a patch that unconditionally enables imenu but checks
>> the return value of mutool and will raise an imenu-unavailable-error
>> when it is not successful.  However, that error stops docview from
>> displaying the document so I decided to put the setup function in a
>> hook so the error occurs after the document is displayed.
>
> Tassilo, is this okay, in your opinion?

Yes, almost.  I'd rather handle the error rather putting the setup
function in a hook and let the error hit top-level and flash the screen.

--8<---------------cut here---------------start------------->8---
1 file changed, 11 insertions(+), 5 deletions(-)
lisp/doc-view.el | 16 +++++++++++-----

modified   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 @@ doc-view-mupdf-use-svg
   :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")
@@ -1910,9 +1912,10 @@ doc-view--pdf-outline
   (let ((fn (or file-name (buffer-file-name))))
     (when fn
       (let ((outline nil)
-            (fn (shell-quote-argument (expand-file-name fn))))
+            (fn (expand-file-name fn)))
         (with-temp-buffer
-          (insert (shell-command-to-string (format "mutool show %s outline" fn)))
+          (unless (= 0 (call-process "mutool" nil (current-buffer) nil "show" fn "outline"))
+            (imenu-unavailable-error "Unable to create imenu index using `mutool'"))
           (goto-char (point-min))
           (while (re-search-forward doc-view--outline-rx nil t)
             (push `((level . ,(length (match-string 1)))
@@ -1961,7 +1964,7 @@ doc-view-imenu-index
 
 (defun doc-view-imenu-setup ()
   "Set up local state in the current buffer for imenu, if needed."
-  (when (and doc-view-imenu-enabled (executable-find "mutool"))
+  (when doc-view-imenu-enabled
     (setq-local imenu-create-index-function #'doc-view-imenu-index
                 imenu-submenus-on-top nil
                 imenu-sort-function nil
@@ -2236,7 +2239,10 @@ doc-view-mode
     (setq mode-name "DocView"
 	  buffer-read-only t
 	  major-mode 'doc-view-mode)
-    (doc-view-imenu-setup)
+    (condition-case imenu-error
+        (doc-view-imenu-setup)
+      (imenu-unavailable (message "imenu support unavailable: %s"
+                                  (cadr imenu-error))))
     (doc-view-initiate-display)
     ;; Switch off view-mode explicitly, because doc-view-mode is the
     ;; canonical view mode for PDF/PS/DVI files.  This could be
--8<---------------cut here---------------end--------------->8---

Bye,
Tassilo

bug#64516: [PATCH] docview: Only enable imenu when supported

[Morgan Smith]

[-- Attachment #1: Type: text/plain, Size: 261 bytes --]

Tassilo Horn <tsdh@gnu.org> writes:

>
> Yes, almost.  I'd rather handle the error rather putting the setup
> function in a hook and let the error hit top-level and flash the screen.
>

I've made the requested change.  Please see attached.

Thank you,

Morgan


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-docview-imenu-check-return-value-of-mutool.patch --]
[-- Type: text/x-patch, Size: 2287 bytes --]

From 7de1362395597d2d7210dc78dcdb104071d758ab Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Tue, 11 Jul 2023 14:08:24 -0400
Subject: [PATCH] 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): Ignore imenu-unavailable errors when calling
'doc-view-imenu-setup'.
---
 lisp/doc-view.el | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lisp/doc-view.el b/lisp/doc-view.el
index b14655fb274..7d604a7bc86 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
@@ -1910,9 +1912,10 @@ doc-view--pdf-outline
   (let ((fn (or file-name (buffer-file-name))))
     (when fn
       (let ((outline nil)
-            (fn (shell-quote-argument (expand-file-name fn))))
+            (fn (expand-file-name fn)))
         (with-temp-buffer
-          (insert (shell-command-to-string (format "mutool show %s outline" fn)))
+          (unless (= 0 (call-process "mutool" nil (current-buffer) nil "show" fn "outline"))
+            (imenu-unavailable-error "Unable to create imenu index using `mutool'"))
           (goto-char (point-min))
           (while (re-search-forward doc-view--outline-rx nil t)
             (push `((level . ,(length (match-string 1)))
@@ -2236,7 +2239,7 @@ doc-view-mode
     (setq mode-name "DocView"
 	  buffer-read-only t
 	  major-mode 'doc-view-mode)
-    (doc-view-imenu-setup)
+    (ignore-error imenu-unavailable (doc-view-imenu-setup))
     (doc-view-initiate-display)
     ;; Switch off view-mode explicitly, because doc-view-mode is the
     ;; canonical view mode for PDF/PS/DVI files.  This could be
-- 
2.41.0

bug#64516: [PATCH] docview: Only enable imenu when supported

[Tassilo Horn]

Hi Morgan,

Why do you ignore the error altogether? FWIW, just use the patch from my last message and we're good to commit.

Thanks,
Tassilo

16.07.2023 01:56:09 Morgan Smith <Morgan.J.Smith@outlook.com>:

> Tassilo Horn <tsdh@gnu.org> writes:
> 
>> 
>> Yes, almost.  I'd rather handle the error rather putting the setup
>> function in a hook and let the error hit top-level and flash the screen.
>> 
> 
> I've made the requested change.  Please see attached.
> 
> Thank you,
> 
> Morgan

bug#64516: [PATCH] docview: Only enable imenu when supported

[Morgan Smith]

Hello,

First of all I'd like to apologize for dragging this out and missing
some details.  If I paid a little more attention this would likely be
done by now :P

Tassilo Horn <tsdh@gnu.org> writes:

> Why do you ignore the error altogether?

So we have 3 scenarios: mutool not installed, mutool failed, and mutool
is successful.

Your patch would send a message on setup when mutool fails, but would
not say anything in the other two scenarios.

My patch would be silent on setup.

Both patches would give the user an error if they actually try to use
imenu.

In my opinion, we should either be completely silent (on setup) or send a message
when imenu is set up correctly.  If you wanted to send a message when it
fails I would accept that, but maybe we should consider also sending the
message when mutool is not installed.


Also, we have a macro 'with-demoted-errors' but we are missing a
'with-demoted-error' macro that lets you specify what error to demote.
It would be cool if you converted this part of your patch into a
definition of a 'with-demoted-error' macro.


--8<---------------cut here---------------start------------->8---
+    (condition-case imenu-error
+        (doc-view-imenu-setup)
+      (imenu-unavailable (message "imenu support unavailable: %s"
+                                  (cadr imenu-error))))
--8<---------------cut here---------------end--------------->8---

bug#64516: [PATCH] docview: Only enable imenu when supported

[Tassilo Horn]

Hi Morgan,

my reasoning was to have an explanation in the *Messages* buffer for people who ask themselves "where the heck is the Outline menu item"? If it's done by condition-case or with-demoted-errors doesn't really matter. Feel free to adapt it to your preference.

Thanks,
Tassilo

16.07.2023 17:03:05 Morgan Smith <Morgan.J.Smith@outlook.com>:

> Hello,
> 
> First of all I'd like to apologize for dragging this out and missing
> some details.  If I paid a little more attention this would likely be
> done by now :P
> 
> Tassilo Horn <tsdh@gnu.org> writes:
> 
>> Why do you ignore the error altogether?
> 
> So we have 3 scenarios: mutool not installed, mutool failed, and mutool
> is successful.
> 
> Your patch would send a message on setup when mutool fails, but would
> not say anything in the other two scenarios.
> 
> My patch would be silent on setup.
> 
> Both patches would give the user an error if they actually try to use
> imenu.
> 
> In my opinion, we should either be completely silent (on setup) or send a message
> when imenu is set up correctly.  If you wanted to send a message when it
> fails I would accept that, but maybe we should consider also sending the
> message when mutool is not installed.
> 
> 
> Also, we have a macro 'with-demoted-errors' but we are missing a
> 'with-demoted-error' macro that lets you specify what error to demote.
> It would be cool if you converted this part of your patch into a
> definition of a 'with-demoted-error' macro.
> 
> 
> --8<---------------cut here---------------start------------->8---
> +    (condition-case imenu-error
> +        (doc-view-imenu-setup)
> +      (imenu-unavailable (message "imenu support unavailable: %s"
> +                                  (cadr imenu-error))))
> --8<---------------cut here---------------end--------------->8---

bug#64516: [PATCH] docview: Only enable imenu when supported

[Morgan Smith]

Tassilo Horn <tsdh@gnu.org> writes:

> my reasoning was to have an explanation in the *Messages* buffer for people who
> ask themselves "where the heck is the Outline menu item"?

I didn't think of users using the toolbar.  I think you can go ahead and
use the patch you sent a couple emails back.  I'm happy with that one.

Thank you for all your help!

Morgan

bug#64516: [PATCH] docview: Only enable imenu when supported

[Tassilo Horn]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]

Morgan Smith <Morgan.J.Smith@outlook.com> writes:

Hi Morgan,

>> my reasoning was to have an explanation in the *Messages* buffer for
>> people who ask themselves "where the heck is the Outline menu item"?
>
> I didn't think of users using the toolbar.

I've heared there are three of them.  But we are talking about the menu
bar of which there are a good dozen users. :-)

> I think you can go ahead and use the patch you sent a couple emails
> back.  I'm happy with that one.

Alright, I've amended your patch with my slight modifications.  Eli,
feel free to "git am" it where you see fit.  It should apply both on
master and on emacs-29.

Thanks,
Tassilo

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-docview-imenu-check-return-value-of-mutool.patch --]
[-- Type: text/x-patch, Size: 3291 bytes --]

From 2a297bc4c1de575f45fa7fdb9e2b9ea2f23294a2 Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Tue, 11 Jul 2023 14:08:24 -0400
Subject: [PATCH] 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.
---
 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 @@ doc-view-mupdf-use-svg
   :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")
@@ -1910,9 +1912,10 @@ doc-view--pdf-outline
   (let ((fn (or file-name (buffer-file-name))))
     (when fn
       (let ((outline nil)
-            (fn (shell-quote-argument (expand-file-name fn))))
+            (fn (expand-file-name fn)))
         (with-temp-buffer
-          (insert (shell-command-to-string (format "mutool show %s outline" fn)))
+          (unless (= 0 (call-process "mutool" nil (current-buffer) nil "show" fn "outline"))
+            (imenu-unavailable-error "Unable to create imenu index using `mutool'"))
           (goto-char (point-min))
           (while (re-search-forward doc-view--outline-rx nil t)
             (push `((level . ,(length (match-string 1)))
@@ -1961,7 +1964,7 @@ doc-view-imenu-index
 
 (defun doc-view-imenu-setup ()
   "Set up local state in the current buffer for imenu, if needed."
-  (when (and doc-view-imenu-enabled (executable-find "mutool"))
+  (when doc-view-imenu-enabled
     (setq-local imenu-create-index-function #'doc-view-imenu-index
                 imenu-submenus-on-top nil
                 imenu-sort-function nil
@@ -2236,7 +2239,10 @@ doc-view-mode
     (setq mode-name "DocView"
 	  buffer-read-only t
 	  major-mode 'doc-view-mode)
-    (doc-view-imenu-setup)
+    (condition-case imenu-error
+        (doc-view-imenu-setup)
+      (imenu-unavailable (message "imenu support unavailable: %s"
+                                  (cadr imenu-error))))
     (doc-view-initiate-display)
     ;; Switch off view-mode explicitly, because doc-view-mode is the
     ;; canonical view mode for PDF/PS/DVI files.  This could be
-- 
2.41.0

bug#64516: [PATCH] docview: Only enable imenu when supported

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> Cc: Eli Zaretskii <eliz@gnu.org>, 64516@debbugs.gnu.org, jao@gnu.org
> Date: Tue, 18 Jul 2023 21:14:35 +0200
> 
> Alright, I've amended your patch with my slight modifications.  Eli,
> feel free to "git am" it where you see fit.  It should apply both on
> master and on emacs-29.

Thanks to both of you.  I've now installed this on the master branch,
and I'm closing the bug.

bug#64516: [PATCH] docview: Only enable imenu when supported

[Basil L. Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 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 @@
[...]
> +          (unless (= 0 (call-process "mutool" nil (current-buffer) nil "show" fn "outline"))
> +            (imenu-unavailable-error "Unable to create imenu index using `mutool'"))

Is call-process guaranteed to return a number?
Would eq/l be more suitable than =?

[ The same also appears in doc-view--revert-buffer. ]

Thanks,
-- 
Basil

bug#64516: [PATCH] docview: Only enable imenu when supported

[Tassilo Horn]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

>> 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 @@
> [...]
>> +          (unless (= 0 (call-process "mutool" nil (current-buffer) nil "show" fn "outline"))
>> +            (imenu-unavailable-error "Unable to create imenu index using `mutool'"))
>
> Is call-process guaranteed to return a number?
> Would eq/l be more suitable than =?

Good question, the docs say:

  If DESTINATION is 0, ‘call-process’ returns immediately with value nil.
  Otherwise it waits for PROGRAM to terminate
  and returns a numeric exit status or a signal description string.

I guess if mutool is killed externally while emacs runs it, it could be
such a "signal description string" in which case eql would be better
even though that situation seems unlikely.  I'll change that later.

Thanks for the heads-up,
Tassilo

bug#64516: [PATCH] docview: Only enable imenu when supported

[Basil L. Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Tassilo Horn [2023-07-28 12:19 +0200] wrote:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>
>> Is call-process guaranteed to return a number?
>> Would eq/l be more suitable than =?
>
> Good question, the docs say:
>
>   If DESTINATION is 0, ‘call-process’ returns immediately with value nil.
>   Otherwise it waits for PROGRAM to terminate
>   and returns a numeric exit status or a signal description string.
>
> I guess if mutool is killed externally while emacs runs it, it could be
> such a "signal description string" in which case eql would be better
> even though that situation seems unlikely.

Right, and it's often hard to decide whether/how the user should be made
aware of such exceptional circumstances.

> I'll change that later.

Thanks,
-- 
Basil