[patch] Display what's really being executed

[patch] Display what's really being executed

[Hrvoje Niksic]

This is a somewhat related followup on my previous bug report.

In mm-display-external, we're calling an external command to display
an article MIME part.  We display "Displaying " followed by the
expanded inline method.  This has several problems:

1) The expansion is executed simple-mindedly, using (format method
   file), which will fail when method is something like "display %s -x
   %s".  (But mm-mailcap-command handles this correctly.)

2) We don't show any of the quoting done by mm-mailcap-command.  This
   is confusing in the cases where the quoting displayed is "obviously
   wrong", such as "Displaying xv foo bar.jpg", but Gnus gets it
   right.  It's also confusing in the cases where the displayed
   quoting is obviously right, such as in "Displaying 'foo bar.jpg'",
   but Gnus gets it wrong.

3) A corollary of #2 is that it's simply wrong to show something other
   than what is being executed.  Either show structured information to
   the user, such as "Applying filename FOO to method 'FROB %s'" or
   show the actual command that *is* going to be executed, such as
   "Displaying xv foo\ bar.jpg".  I opted for the latter.

Since this function always executes an external command, I believe
that it is safe to simply always print that command after
"Displaying..."

BTW isn't mm-* supposed to be independent from Gnus?  What's the call
to gnus-configure-windows doing in the guts of mm-display-external?

The patch follows.  Note that indentation makes it appear much larger
than it is.  The only change is the addition of `let' forms and of
using those variables as the arguments to both `start-process' and
`message'.

I will apply the patch unless someone objects.

2002-08-13  Hrvoje Niksic  <hniksic@xemacs.org>

	* (mm-display-external): Display the actual command that has been
	executed in the echo area.

--- lisp/mm-decode.el.orig	Tue Aug 13 20:16:59 2002
+++ lisp/mm-decode.el	Tue Aug 13 20:20:56 2002
@@ -700,36 +700,33 @@
 	  (message "Viewing with %s" method)
 	  (cond
 	   (needsterm
-	    (unwind-protect
-		(if window-system
-		    (start-process "*display*" nil
-				   mm-external-terminal-program
-				   "-e" shell-file-name
-				   shell-command-switch
-				   (mm-mailcap-command
-				    method file (mm-handle-type handle)))
-		  (require 'term)
-		  (require 'gnus-win)
-		  (set-buffer
-		   (setq buffer
-			 (make-term "display"
-				    shell-file-name
-				    nil
-				    shell-command-switch
-				    (mm-mailcap-command
-				     method file
-				     (mm-handle-type handle)))))
-		  (term-mode)
-		  (term-char-mode)
-		  (set-process-sentinel
-		   (get-buffer-process buffer)
-		   `(lambda (process state)
-		      (if (eq 'exit (process-status process))
-			  (gnus-configure-windows
-			   ',gnus-current-window-configuration))))
-		  (gnus-configure-windows 'display-term))
-	      (mm-handle-set-external-undisplayer handle (cons file buffer)))
-	    (message "Displaying %s..." (format method file))
+	    (let ((command (mm-mailcap-command
+			    method file (mm-handle-type handle))))
+	      (unwind-protect
+		  (if window-system
+		      (start-process "*display*" nil
+				     mm-external-terminal-program
+				     "-e" shell-file-name
+				     shell-command-switch command)
+		    (require 'term)
+		    (require 'gnus-win)
+		    (set-buffer
+		     (setq buffer
+			   (make-term "display"
+				      shell-file-name
+				      nil
+				      shell-command-switch command)))
+		    (term-mode)
+		    (term-char-mode)
+		    (set-process-sentinel
+		     (get-buffer-process buffer)
+		     `(lambda (process state)
+			(if (eq 'exit (process-status process))
+			    (gnus-configure-windows
+			     ',gnus-current-window-configuration))))
+		    (gnus-configure-windows 'display-term))
+		(mm-handle-set-external-undisplayer handle (cons file buffer)))
+	      (message "Displaying %s..." command))
 	    'external)
 	   (copiousoutput
 	    (with-current-buffer outbuf
@@ -756,17 +753,17 @@
 		   (ignore-errors (kill-buffer buffer))))))
 	    'inline)
 	   (t
-	    (unwind-protect
-		(start-process "*display*"
-			       (setq buffer
-				     (generate-new-buffer " *mm*"))
-			       shell-file-name
-			       shell-command-switch
-			       (mm-mailcap-command
-				method file (mm-handle-type handle)))
-	      (mm-handle-set-external-undisplayer
-	       handle (cons file buffer)))
-	    (message "Displaying %s..." (format method file))
+	    (let ((command (mm-mailcap-command
+			    method file (mm-handle-type handle))))
+	      (unwind-protect
+		  (start-process "*display*"
+				 (setq buffer
+				       (generate-new-buffer " *mm*"))
+				 shell-file-name
+				 shell-command-switch command)
+		(mm-handle-set-external-undisplayer
+		 handle (cons file buffer)))
+	      (message "Displaying %s..." command))
 	    'external)))))))
 
 (defun mm-mailcap-command (method file type-list)

Re: [patch] Display what's really being executed

[Lars Magne Ingebrigtsen]

Hrvoje Niksic <hniksic@xemacs.org> writes:

> BTW isn't mm-* supposed to be independent from Gnus?  What's the call
> to gnus-configure-windows doing in the guts of mm-display-external?

It's supposed to be independent, but these things have a tendency to
creep in...  The window config stuff should probably be passed from
Gnus in a hook function or the like, to make it unnecessary for mm to
know anything about Gnus internal.

> The patch follows.

Applied.

> I will apply the patch unless someone objects.

Oops.  It didn't look like it had been applied, so perhaps you
changed your mind?  Should I reverse the patch?

-- 
(domestic pets only, the antidote for overdose, milk.)
   larsi@gnus.org * Lars Magne Ingebrigtsen