unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* [Patch/Question] Likely bugs in mailcap docs and mailcap-view-file
@ 2022-01-14 21:24 Tassilo Horn
  2022-01-14 22:32 ` Stefan Monnier
  0 siblings, 1 reply; 4+ messages in thread
From: Tassilo Horn @ 2022-01-14 21:24 UTC (permalink / raw)
  To: emacs-devel

[-- 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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [Patch/Question] Likely bugs in mailcap docs and mailcap-view-file
  2022-01-14 21:24 [Patch/Question] Likely bugs in mailcap docs and mailcap-view-file Tassilo Horn
@ 2022-01-14 22:32 ` Stefan Monnier
  2022-01-15 10:27   ` Tassilo Horn
  0 siblings, 1 reply; 4+ messages in thread
From: Stefan Monnier @ 2022-01-14 22:32 UTC (permalink / raw)
  To: Tassilo Horn; +Cc: emacs-devel

> 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




^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [Patch/Question] Likely bugs in mailcap docs and mailcap-view-file
  2022-01-14 22:32 ` Stefan Monnier
@ 2022-01-15 10:27   ` Tassilo Horn
  2022-01-15 14:52     ` Stefan Monnier
  0 siblings, 1 reply; 4+ messages in thread
From: Tassilo Horn @ 2022-01-15 10:27 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

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



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [Patch/Question] Likely bugs in mailcap docs and mailcap-view-file
  2022-01-15 10:27   ` Tassilo Horn
@ 2022-01-15 14:52     ` Stefan Monnier
  0 siblings, 0 replies; 4+ messages in thread
From: Stefan Monnier @ 2022-01-15 14:52 UTC (permalink / raw)
  To: Tassilo Horn; +Cc: emacs-devel

> 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




^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2022-01-15 14:52 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-14 21:24 [Patch/Question] Likely bugs in mailcap docs and mailcap-view-file Tassilo Horn
2022-01-14 22:32 ` Stefan Monnier
2022-01-15 10:27   ` Tassilo Horn
2022-01-15 14:52     ` Stefan Monnier

Code repositories for project(s) associated with this inbox:

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).