[Patch/Question] Likely bugs in mailcap docs and mailcap-view-file

[Patch/Question] Likely bugs in mailcap docs and mailcap-view-file

[Tassilo Horn]

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

Hi all,

I wanted to use `mailcap-view-file' in some package (mu4e) in order to
view some attachment using some appropriate viewer.  I've tested with
some pdf attachment which triggered an error because I use pdf-tools
which adds an entry to `mailcap-mime-data' where the viewer is the
symbol `pdf-view-mode' (a major-mode) whereas `mailcap-view-file'
currently only handles the "viewer is a string naming some executable"
case.

The default value of `mailcap-mime-data' has tons of viewers being
major-mode symbols, so that can't be wrong.  It's docstring also says:

    Where VIEWERINFO specifies how the content-type is viewed.  Can be a
    string, in which case it is run through a shell, with appropriate
    parameters, or a symbol, in which case the symbol is ‘funcall’ed if
    and only if it exists as a function, with the buffer as an argument.

Well, that's strange.  All symbol-valued VIEWERINFOs are functions of
zero args, mostly major-modes.  They can't get the buffer (which
buffer?!) as an argument.

So I propose the attached patch which teaches `mailcap-view-file' the
symbol/function-valued viewer case and adjust the docs to match what's
already there.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: mailcap.diff --]
[-- Type: text/x-patch, Size: 4354 bytes --]

diff --git a/lisp/net/mailcap.el b/lisp/net/mailcap.el
index bf3c8edd1e..151c6e0509 100644
--- a/lisp/net/mailcap.el
+++ b/lisp/net/mailcap.el
@@ -319,8 +319,9 @@ mailcap-mime-data
 
 Where VIEWERINFO specifies how the content-type is viewed.  Can be
 a string, in which case it is run through a shell, with appropriate
-parameters, or a symbol, in which case the symbol is `funcall'ed if
-and only if it exists as a function, with the buffer as an argument.
+parameters, or a symbol, in which case the symbol must name a function
+of zero arguments which is called in a buffer holding the MIME part's
+content.
 
 TESTINFO is a test for the viewer's applicability, or nil.  If nil, it
 means the viewer is always valid.  If it is a Lisp function, it is
@@ -1174,34 +1175,45 @@ mailcap-view-file
   (mailcap-parse-mailcaps)
   (let ((command (mailcap-mime-info
                   (mailcap-extension-to-mime (file-name-extension file)))))
-    (unless command
-      (error "No viewer for %s" (file-name-extension file)))
-    ;; Remove quotes around the file name - we'll use shell-quote-argument.
-    (while (string-match "['\"]%s['\"]" command)
-      (setq command (replace-match "%s" t t command)))
-    (setq command (replace-regexp-in-string
-		   "%s"
-		   (shell-quote-argument (convert-standard-filename file))
-		   command
-		   nil t))
-    ;; Handlers such as "gio open" and kde-open5 start viewer in background
-    ;; and exit immediately.  Avoid `start-process' since it assumes
-    ;; :connection-type `pty' and kills children processes with SIGHUP
-    ;; when temporary terminal session is finished (Bug#44824).
-    ;; An alternative is `process-connection-type' let-bound to nil for
-    ;; `start-process-shell-command' call (with no chance to report failure).
-    (make-process
-     :name "mailcap-view-file"
-     :connection-type 'pipe
-     :buffer nil ; "*Messages*" may be suitable for debugging
-     :sentinel (lambda (proc event)
-                 (when (and (memq (process-status proc) '(exit signal))
-                            (/= (process-exit-status proc) 0))
-                   (message
-                    "Command %s: %s."
-                    (mapconcat #'identity (process-command proc) " ")
-                    (substring event 0 -1))))
-     :command (list shell-file-name shell-command-switch command))))
+    (if (functionp command)
+        ;; command is a viewer function (a mode) expecting the file
+        ;; contents to be in the current buffer.
+        (let ((buf (generate-new-buffer (file-name-nondirectory file))))
+          (set-buffer buf)
+          (insert-file-contents file)
+          (setq buffer-file-name file)
+          (funcall command)
+          (set-buffer-modified-p nil)
+          (pop-to-buffer buf))
+      ;; command is a program to run with file as an argument.
+      (unless command
+        (error "No viewer for %s" (file-name-extension file)))
+      ;; Remove quotes around the file name - we'll use shell-quote-argument.
+      (while (string-match "['\"]%s['\"]" command)
+        (setq command (replace-match "%s" t t command)))
+      (setq command (replace-regexp-in-string
+		     "%s"
+		     (shell-quote-argument (convert-standard-filename file))
+		     command
+		     nil t))
+      ;; Handlers such as "gio open" and kde-open5 start viewer in background
+      ;; and exit immediately.  Avoid `start-process' since it assumes
+      ;; :connection-type `pty' and kills children processes with SIGHUP
+      ;; when temporary terminal session is finished (Bug#44824).
+      ;; An alternative is `process-connection-type' let-bound to nil for
+      ;; `start-process-shell-command' call (with no chance to report failure).
+      (make-process
+       :name "mailcap-view-file"
+       :connection-type 'pipe
+       :buffer nil ; "*Messages*" may be suitable for debugging
+       :sentinel (lambda (proc event)
+                   (when (and (memq (process-status proc) '(exit signal))
+                              (/= (process-exit-status proc) 0))
+                     (message
+                      "Command %s: %s."
+                      (mapconcat #'identity (process-command proc) " ")
+                      (substring event 0 -1))))
+       :command (list shell-file-name shell-command-switch command)))))
 
 (provide 'mailcap)
 

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


Does anyone see a problem with that?  I'm not very familiar with that
code.

Bye,
Tassilo

Re: [Patch/Question] Likely bugs in mailcap docs and mailcap-view-file

[Stefan Monnier]

> So I propose the attached patch which teaches `mailcap-view-file' the
> symbol/function-valued viewer case and adjust the docs to match what's
> already there.
[...]
> Does anyone see a problem with that?  I'm not very familiar with that
> code.

I'm not familiar either, but it looks OK.  Any chance you could be
convinced to throw in some regression test?


        Stefan

Re: [Patch/Question] Likely bugs in mailcap docs and mailcap-view-file

[Tassilo Horn]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

Hi Stefan,

>> So I propose the attached patch which teaches `mailcap-view-file' the
>> symbol/function-valued viewer case and adjust the docs to match
>> what's already there.
> [...]
>> Does anyone see a problem with that?  I'm not very familiar with that
>> code.
>
> I'm not familiar either, but it looks OK.

Alright, thanks for checking.  Pushed!

> Any chance you could be convinced to throw in some regression test?

Of course.  I think I can write some tests that the viewer selection
procedure works as intended.  Is that what you have in mind?

Wrt. the changed function: I'm not exactly sure how I would test that.
I can probably test that the right viewer function is selected and run
on the right buffer but would skip the display stuff, i.e., that the
generated buffer is visible in some window.

Bye,
Tassilo

Re: [Patch/Question] Likely bugs in mailcap docs and mailcap-view-file

[Stefan Monnier]

> Wrt. the changed function: I'm not exactly sure how I would test that.

Neither am I.

> I can probably test that the right viewer function is selected and run
> on the right buffer

That's what I was thinking about (which includes "called with the
expected args").


        Stefan