unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / code / Atom feed
* use of flet in notmuch-emacs
@ 2013-05-19 11:31 David Bremner
  2013-05-19 15:14 ` [PATCH] emacs: Avoid deprecated function flet Austin Clements
  0 siblings, 1 reply; 12+ messages in thread
From: David Bremner @ 2013-05-19 11:31 UTC (permalink / raw)
  To: Notmuch Mail List


Today I noticed

,----
| emacs --quick --directory emacs -batch -f batch-byte-compile emacs/notmuch-show.el
| 
| In notmuch-show-view-part:
| emacs/notmuch-show.el:541:22:Warning: `flet' is an obsolete macro (as of
|     24.3); use either `cl-flet' or `cl-letf'.
`----

Chatter on #emacs suggests that cl-flet is not in fact a replacement for
our use case since it does not do dynamic binding. I'm not really 100%
sure what the best plan is, but the old definition of flet is available
at

        https://github.com/sigma/el-x/blob/master/lisp/dflet.el

d

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

* [PATCH] emacs: Avoid deprecated function flet
  2013-05-19 11:31 use of flet in notmuch-emacs David Bremner
@ 2013-05-19 15:14 ` Austin Clements
  2013-05-20 15:45   ` David Bremner
  0 siblings, 1 reply; 12+ messages in thread
From: Austin Clements @ 2013-05-19 15:14 UTC (permalink / raw)
  To: notmuch

flet was deprecated in Emacs 24.3 and replaced with cl-flet.  However,
cl-flet lexically binds the function symbol, while we depend on flet
dynamically binding the function symbol.  Hence, this patch replaces
the deprecated flet use with letf, which lets us dynamically bind the
function symbol, while remaining compatible with both Emacs 23 and 24.

Since we don't have an automated test for this, this was tested
manually in Emacs 24.3 and 23.4.
---
 emacs/notmuch-show.el |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 423dd58..7101236 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -539,8 +539,9 @@ message at DEPTH in the current thread."
 		    (nth nth)
 		    (filename filename)
 		    (content-type content-type))
-	(flet ((mm-save-part (&rest args) (notmuch-show-save-part
-					   message-id nth filename content-type)))
+	(letf (((symbol-function 'mm-save-part)
+		(lambda (&rest args) (notmuch-show-save-part
+				      message-id nth filename content-type))))
 	  (mm-display-part handle))))))
 
 (defun notmuch-show-interactively-view-part (message-id nth &optional filename content-type)
-- 
1.7.10.4

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

* Re: [PATCH] emacs: Avoid deprecated function flet
  2013-05-19 15:14 ` [PATCH] emacs: Avoid deprecated function flet Austin Clements
@ 2013-05-20 15:45   ` David Bremner
  2013-05-20 17:04     ` Austin Clements
  0 siblings, 1 reply; 12+ messages in thread
From: David Bremner @ 2013-05-20 15:45 UTC (permalink / raw)
  To: Austin Clements, notmuch

Austin Clements <amdragon@MIT.EDU> writes:

> flet was deprecated in Emacs 24.3 and replaced with cl-flet.  However,
> cl-flet lexically binds the function symbol, while we depend on flet
> dynamically binding the function symbol.  Hence, this patch replaces
> the deprecated flet use with letf, which lets us dynamically bind the
> function symbol, while remaining compatible with both Emacs 23 and 24.

The bad news: letf is also marked as obsolete, although there is no
yelling from the byte-compiler yet.

In my simple tests, it _seemed_ to work to replace letf with cl-letf,
although

- that would require some kind of compatability alias
- the docstring for letf mutters something about "deprecated usage of
  `symbol-function' in place forms.

On the third hand, 

   http://www.gnu.org/software/emacs/manual/html_node/elisp/Setting-Generalized-Variables.html#Setting-Generalized-Variables 

suggests using symbol-function with setf is legitimate.

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

* Re: [PATCH] emacs: Avoid deprecated function flet
  2013-05-20 15:45   ` David Bremner
@ 2013-05-20 17:04     ` Austin Clements
  2013-05-20 20:08       ` [PATCH] emacs: Don't override mm-show-part in notmuch-show-view-part Austin Clements
  0 siblings, 1 reply; 12+ messages in thread
From: Austin Clements @ 2013-05-20 17:04 UTC (permalink / raw)
  To: David Bremner; +Cc: notmuch

Quoth David Bremner on May 20 at 12:45 pm:
> Austin Clements <amdragon@MIT.EDU> writes:
> 
> > flet was deprecated in Emacs 24.3 and replaced with cl-flet.  However,
> > cl-flet lexically binds the function symbol, while we depend on flet
> > dynamically binding the function symbol.  Hence, this patch replaces
> > the deprecated flet use with letf, which lets us dynamically bind the
> > function symbol, while remaining compatible with both Emacs 23 and 24.
> 
> The bad news: letf is also marked as obsolete, although there is no
> yelling from the byte-compiler yet.

From what I understand, all non-cl-prefixed functions are now
considered obsolete, but the non-prefixed aliases are going to have to
stick around for a long time and we won't be able to use the prefixed
ones until we drop support for pre-24.3 Emacs.

flet is a more complicated story, since it was deprecated not just in
name, but in semantics, which I think is why the compiler singles it
out.

> In my simple tests, it _seemed_ to work to replace letf with cl-letf,
> although
> 
> - that would require some kind of compatability alias
> - the docstring for letf mutters something about "deprecated usage of
>   `symbol-function' in place forms.
> 
> On the third hand, 
> 
>    http://www.gnu.org/software/emacs/manual/html_node/elisp/Setting-Generalized-Variables.html#Setting-Generalized-Variables 
> 
> suggests using symbol-function with setf is legitimate.

My concern would be that letf is a cl function, and cl's documentation
does *not* list symbol-function as a supported generalized variable:

  http://www.gnu.org/software/emacs/manual/html_mono/cl.html#Setf-Extensions

We should probably just use fset in an unwind-protect.

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

* [PATCH] emacs: Don't override mm-show-part in notmuch-show-view-part
  2013-05-20 17:04     ` Austin Clements
@ 2013-05-20 20:08       ` Austin Clements
  2013-05-21  6:09         ` Mark Walters
  0 siblings, 1 reply; 12+ messages in thread
From: Austin Clements @ 2013-05-20 20:08 UTC (permalink / raw)
  To: notmuch

Previously, notmuch-show-view-part overrode the function binding of
mm-show-part to redirect it to notmuch-show-save-part to get notmuch's
default file name handling in case mm-display-part decided to fall
back to saving the part.  In addition to being messy, this depended on
the now-deprecated dynamic binding behavior of flet.

This patch removes the mm-show-part override in favor of passing the
file name in to mm-show-part the way it expects, so we get its default
file name handling.  It's not clear why we didn't do this before;
mm-show-part has supported default file names since at least Emacs
23.1.
---

This takes a different approach from the previous patch by eliminating
the function override altogether, so we don't need flet or anything
like it.  I tested it by hand in Emacs 24.3 and 23.4 and checked that
mm-save-part's code has not changed since at least 23.1.

 emacs/notmuch-show.el |   18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 423dd58..19bcb29 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -529,19 +529,11 @@ message at DEPTH in the current thread."
 (defun notmuch-show-view-part (message-id nth &optional filename content-type )
   (notmuch-with-temp-part-buffer message-id nth
     ;; set mm-inlined-types to nil to force an external viewer
-    (let ((handle (mm-make-handle (current-buffer) (list content-type)))
-	  (mm-inlined-types nil))
-      ;; We override mm-save-part as notmuch-show-save-part is better
-      ;; since it offers the filename. We need to lexically bind
-      ;; everything we need for notmuch-show-save-part to prevent
-      ;; potential dynamic shadowing.
-      (lexical-let ((message-id message-id)
-		    (nth nth)
-		    (filename filename)
-		    (content-type content-type))
-	(flet ((mm-save-part (&rest args) (notmuch-show-save-part
-					   message-id nth filename content-type)))
-	  (mm-display-part handle))))))
+    (let* ((disposition (if filename `(attachment (filename . ,filename))))
+	   (handle (mm-make-handle (current-buffer) (list content-type)
+				   nil nil disposition))
+	   (mm-inlined-types nil))
+      (mm-display-part handle))))
 
 (defun notmuch-show-interactively-view-part (message-id nth &optional filename content-type)
   (notmuch-with-temp-part-buffer message-id nth
-- 
1.7.10.4

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

* Re: [PATCH] emacs: Don't override mm-show-part in notmuch-show-view-part
  2013-05-20 20:08       ` [PATCH] emacs: Don't override mm-show-part in notmuch-show-view-part Austin Clements
@ 2013-05-21  6:09         ` Mark Walters
  2013-05-21 19:13           ` Mark Walters
  2013-05-26  6:34           ` [PATCH v2] " Austin Clements
  0 siblings, 2 replies; 12+ messages in thread
From: Mark Walters @ 2013-05-21  6:09 UTC (permalink / raw)
  To: Austin Clements, notmuch


Hi

> Previously, notmuch-show-view-part overrode the function binding of
> mm-show-part to redirect it to notmuch-show-save-part to get notmuch's
> default file name handling in case mm-display-part decided to fall
> back to saving the part.  In addition to being messy, this depended on
> the now-deprecated dynamic binding behavior of flet.
>
> This patch removes the mm-show-part override in favor of passing the
> file name in to mm-show-part the way it expects, so we get its default
> file name handling.  It's not clear why we didn't do this before;
> mm-show-part has supported default file names since at least Emacs
> 23.1.

The new code is much simpler (and nicer). However, one small annoyance
is it makes notmuch-show-save-part and notmuch-show-view-part behave
differently on parts which can only be saved (eg
application/octet-stream): view-part (ie mm-save-part) offers the
current directory (where emacs was started) whereas the notmuch
save-part explicitly offers mailcap-download-directory or ~/. I have no
preference which is used but think they should be the same. Perhaps
notmuch-show-save-part could just call mm-save-part? I have tried that
and the tests pass. (If we can do that I think the whole part button handling
stuff could be unified/simplified significantly)

Best wishes

Mark


> ---
>
> This takes a different approach from the previous patch by eliminating
> the function override altogether, so we don't need flet or anything
> like it.  I tested it by hand in Emacs 24.3 and 23.4 and checked that
> mm-save-part's code has not changed since at least 23.1.
>
>  emacs/notmuch-show.el |   18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 423dd58..19bcb29 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -529,19 +529,11 @@ message at DEPTH in the current thread."
>  (defun notmuch-show-view-part (message-id nth &optional filename content-type )
>    (notmuch-with-temp-part-buffer message-id nth
>      ;; set mm-inlined-types to nil to force an external viewer
> -    (let ((handle (mm-make-handle (current-buffer) (list content-type)))
> -	  (mm-inlined-types nil))
> -      ;; We override mm-save-part as notmuch-show-save-part is better
> -      ;; since it offers the filename. We need to lexically bind
> -      ;; everything we need for notmuch-show-save-part to prevent
> -      ;; potential dynamic shadowing.
> -      (lexical-let ((message-id message-id)
> -		    (nth nth)
> -		    (filename filename)
> -		    (content-type content-type))
> -	(flet ((mm-save-part (&rest args) (notmuch-show-save-part
> -					   message-id nth filename content-type)))
> -	  (mm-display-part handle))))))
> +    (let* ((disposition (if filename `(attachment (filename . ,filename))))
> +	   (handle (mm-make-handle (current-buffer) (list content-type)
> +				   nil nil disposition))
> +	   (mm-inlined-types nil))
> +      (mm-display-part handle))))
>  
>  (defun notmuch-show-interactively-view-part (message-id nth &optional filename content-type)
>    (notmuch-with-temp-part-buffer message-id nth
> -- 
> 1.7.10.4

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

* Re: [PATCH] emacs: Don't override mm-show-part in notmuch-show-view-part
  2013-05-21  6:09         ` Mark Walters
@ 2013-05-21 19:13           ` Mark Walters
  2013-05-26  6:36             ` Austin Clements
  2013-05-26  6:34           ` [PATCH v2] " Austin Clements
  1 sibling, 1 reply; 12+ messages in thread
From: Mark Walters @ 2013-05-21 19:13 UTC (permalink / raw)
  To: Austin Clements, notmuch


On Tue, 21 May 2013, Mark Walters <markwalters1009@gmail.com> wrote:
> Hi
>
>> Previously, notmuch-show-view-part overrode the function binding of
>> mm-show-part to redirect it to notmuch-show-save-part to get notmuch's
>> default file name handling in case mm-display-part decided to fall
>> back to saving the part.  In addition to being messy, this depended on
>> the now-deprecated dynamic binding behavior of flet.
>>
>> This patch removes the mm-show-part override in favor of passing the
>> file name in to mm-show-part the way it expects, so we get its default
>> file name handling.  It's not clear why we didn't do this before;
>> mm-show-part has supported default file names since at least Emacs
>> 23.1.
>
> The new code is much simpler (and nicer). However, one small annoyance
> is it makes notmuch-show-save-part and notmuch-show-view-part behave
> differently on parts which can only be saved (eg
> application/octet-stream): view-part (ie mm-save-part) offers the
> current directory (where emacs was started) whereas the notmuch
> save-part explicitly offers mailcap-download-directory or ~/. I have no
> preference which is used but think they should be the same. Perhaps
> notmuch-show-save-part could just call mm-save-part? I have tried that
> and the tests pass. (If we can do that I think the whole part button handling
> stuff could be unified/simplified significantly)

Here is the code I was using to try using mm-save-part rather than our
own version. I don't know why we have our own version: this does pass
the tests and seems to work (though as mentioned above the semantics of
which default path is used are different)

Best wishes

Mark

---
 emacs/notmuch-show.el |   13 ++++---------
 1 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 45039bd..a63b857 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -516,15 +516,10 @@ message at DEPTH in the current thread."
 
 (defun notmuch-show-save-part (message-id nth &optional filename content-type)
   (notmuch-with-temp-part-buffer message-id nth
-    (let ((file (read-file-name
-		 "Filename to save as: "
-		 (or mailcap-download-directory "~/")
-		 nil nil
-		 filename)))
-      ;; Don't re-compress .gz & al.  Arguably we should make
-      ;; `file-name-handler-alist' nil, but that would chop
-      ;; ange-ftp, which is reasonable to use here.
-      (mm-write-region (point-min) (point-max) file nil nil nil 'no-conversion t))))
+    (let* ((disposition (if filename `(attachment (filename . ,filename))))
+	   (handle (mm-make-handle (current-buffer) (list content-type)
+				   nil nil disposition)))
+      (mm-save-part handle))))
 
 (defun notmuch-show-view-part (message-id nth &optional filename content-type )
   (notmuch-with-temp-part-buffer message-id nth
-- 
1.7.9.1

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

* [PATCH v2] emacs: Don't override mm-show-part in notmuch-show-view-part
  2013-05-21  6:09         ` Mark Walters
  2013-05-21 19:13           ` Mark Walters
@ 2013-05-26  6:34           ` Austin Clements
  2013-05-26  7:04             ` Mark Walters
                               ` (2 more replies)
  1 sibling, 3 replies; 12+ messages in thread
From: Austin Clements @ 2013-05-26  6:34 UTC (permalink / raw)
  To: notmuch

Previously, notmuch-show-view-part overrode the function binding of
mm-show-part to redirect it to notmuch-show-save-part to get notmuch's
default file name handling in case mm-display-part decided to fall
back to saving the part.  In addition to being messy, this depended on
the now-deprecated dynamic binding behavior of flet.

This patch removes the mm-show-part override in favor of passing the
file name in to mm-show-part the way it expects, so we get its default
file name handling.  It's not clear why we didn't do this before;
mm-show-part has supported default file names since at least Emacs
23.1.
---
This version overrides the default save directory to make it
consistent with `notmuch-show-save-part'.

 emacs/notmuch-show.el |   23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 423dd58..d773396 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -528,20 +528,15 @@ message at DEPTH in the current thread."
 
 (defun notmuch-show-view-part (message-id nth &optional filename content-type )
   (notmuch-with-temp-part-buffer message-id nth
-    ;; set mm-inlined-types to nil to force an external viewer
-    (let ((handle (mm-make-handle (current-buffer) (list content-type)))
-	  (mm-inlined-types nil))
-      ;; We override mm-save-part as notmuch-show-save-part is better
-      ;; since it offers the filename. We need to lexically bind
-      ;; everything we need for notmuch-show-save-part to prevent
-      ;; potential dynamic shadowing.
-      (lexical-let ((message-id message-id)
-		    (nth nth)
-		    (filename filename)
-		    (content-type content-type))
-	(flet ((mm-save-part (&rest args) (notmuch-show-save-part
-					   message-id nth filename content-type)))
-	  (mm-display-part handle))))))
+    (let* ((disposition (if filename `(attachment (filename . ,filename))))
+	   (handle (mm-make-handle (current-buffer) (list content-type)
+				   nil nil disposition))
+	   ;; Set the default save directory to be consistent with
+	   ;; `notmuch-show-save-part'.
+	   (mm-default-directory (or mailcap-download-directory "~/"))
+	   ;; set mm-inlined-types to nil to force an external viewer
+	   (mm-inlined-types nil))
+      (mm-display-part handle))))
 
 (defun notmuch-show-interactively-view-part (message-id nth &optional filename content-type)
   (notmuch-with-temp-part-buffer message-id nth
-- 
1.7.10.4

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

* Re: [PATCH] emacs: Don't override mm-show-part in notmuch-show-view-part
  2013-05-21 19:13           ` Mark Walters
@ 2013-05-26  6:36             ` Austin Clements
  0 siblings, 0 replies; 12+ messages in thread
From: Austin Clements @ 2013-05-26  6:36 UTC (permalink / raw)
  To: Mark Walters; +Cc: notmuch

Quoth Mark Walters on May 21 at  8:13 pm:
> 
> On Tue, 21 May 2013, Mark Walters <markwalters1009@gmail.com> wrote:
> > Hi
> >
> >> Previously, notmuch-show-view-part overrode the function binding of
> >> mm-show-part to redirect it to notmuch-show-save-part to get notmuch's
> >> default file name handling in case mm-display-part decided to fall
> >> back to saving the part.  In addition to being messy, this depended on
> >> the now-deprecated dynamic binding behavior of flet.
> >>
> >> This patch removes the mm-show-part override in favor of passing the
> >> file name in to mm-show-part the way it expects, so we get its default
> >> file name handling.  It's not clear why we didn't do this before;
> >> mm-show-part has supported default file names since at least Emacs
> >> 23.1.
> >
> > The new code is much simpler (and nicer). However, one small annoyance
> > is it makes notmuch-show-save-part and notmuch-show-view-part behave
> > differently on parts which can only be saved (eg
> > application/octet-stream): view-part (ie mm-save-part) offers the
> > current directory (where emacs was started) whereas the notmuch
> > save-part explicitly offers mailcap-download-directory or ~/. I have no
> > preference which is used but think they should be the same. Perhaps
> > notmuch-show-save-part could just call mm-save-part? I have tried that
> > and the tests pass. (If we can do that I think the whole part button handling
> > stuff could be unified/simplified significantly)
> 
> Here is the code I was using to try using mm-save-part rather than our
> own version. I don't know why we have our own version: this does pass
> the tests and seems to work (though as mentioned above the semantics of
> which default path is used are different)

I think this is a good idea.  I'm putting together a patch that cleans
up and simplifies all of the part button handling code by taking
better advantage of mm.  I should have it ready in the next few days.

> Best wishes
> 
> Mark
> 
> ---
>  emacs/notmuch-show.el |   13 ++++---------
>  1 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 45039bd..a63b857 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -516,15 +516,10 @@ message at DEPTH in the current thread."
>  
>  (defun notmuch-show-save-part (message-id nth &optional filename content-type)
>    (notmuch-with-temp-part-buffer message-id nth
> -    (let ((file (read-file-name
> -		 "Filename to save as: "
> -		 (or mailcap-download-directory "~/")
> -		 nil nil
> -		 filename)))
> -      ;; Don't re-compress .gz & al.  Arguably we should make
> -      ;; `file-name-handler-alist' nil, but that would chop
> -      ;; ange-ftp, which is reasonable to use here.
> -      (mm-write-region (point-min) (point-max) file nil nil nil 'no-conversion t))))
> +    (let* ((disposition (if filename `(attachment (filename . ,filename))))
> +	   (handle (mm-make-handle (current-buffer) (list content-type)
> +				   nil nil disposition)))
> +      (mm-save-part handle))))
>  
>  (defun notmuch-show-view-part (message-id nth &optional filename content-type )
>    (notmuch-with-temp-part-buffer message-id nth

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

* Re: [PATCH v2] emacs: Don't override mm-show-part in notmuch-show-view-part
  2013-05-26  6:34           ` [PATCH v2] " Austin Clements
@ 2013-05-26  7:04             ` Mark Walters
  2013-05-26  7:31             ` Tomi Ollila
  2013-05-26 23:35             ` David Bremner
  2 siblings, 0 replies; 12+ messages in thread
From: Mark Walters @ 2013-05-26  7:04 UTC (permalink / raw)
  To: Austin Clements, notmuch


LGTM +1

This does not rely on the followup Austin mentioned in id:20130526063627.GQ5999@mit.edu is
so I think this should go in as is as it fixes the
flet thing without changing the behaviour.

Best wishes

Mark

On Sun, 26 May 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> Previously, notmuch-show-view-part overrode the function binding of
> mm-show-part to redirect it to notmuch-show-save-part to get notmuch's
> default file name handling in case mm-display-part decided to fall
> back to saving the part.  In addition to being messy, this depended on
> the now-deprecated dynamic binding behavior of flet.
>
> This patch removes the mm-show-part override in favor of passing the
> file name in to mm-show-part the way it expects, so we get its default
> file name handling.  It's not clear why we didn't do this before;
> mm-show-part has supported default file names since at least Emacs
> 23.1.
> ---
> This version overrides the default save directory to make it
> consistent with `notmuch-show-save-part'.
>
>  emacs/notmuch-show.el |   23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 423dd58..d773396 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -528,20 +528,15 @@ message at DEPTH in the current thread."
>  
>  (defun notmuch-show-view-part (message-id nth &optional filename content-type )
>    (notmuch-with-temp-part-buffer message-id nth
> -    ;; set mm-inlined-types to nil to force an external viewer
> -    (let ((handle (mm-make-handle (current-buffer) (list content-type)))
> -	  (mm-inlined-types nil))
> -      ;; We override mm-save-part as notmuch-show-save-part is better
> -      ;; since it offers the filename. We need to lexically bind
> -      ;; everything we need for notmuch-show-save-part to prevent
> -      ;; potential dynamic shadowing.
> -      (lexical-let ((message-id message-id)
> -		    (nth nth)
> -		    (filename filename)
> -		    (content-type content-type))
> -	(flet ((mm-save-part (&rest args) (notmuch-show-save-part
> -					   message-id nth filename content-type)))
> -	  (mm-display-part handle))))))
> +    (let* ((disposition (if filename `(attachment (filename . ,filename))))
> +	   (handle (mm-make-handle (current-buffer) (list content-type)
> +				   nil nil disposition))
> +	   ;; Set the default save directory to be consistent with
> +	   ;; `notmuch-show-save-part'.
> +	   (mm-default-directory (or mailcap-download-directory "~/"))
> +	   ;; set mm-inlined-types to nil to force an external viewer
> +	   (mm-inlined-types nil))
> +      (mm-display-part handle))))
>  
>  (defun notmuch-show-interactively-view-part (message-id nth &optional filename content-type)
>    (notmuch-with-temp-part-buffer message-id nth
> -- 
> 1.7.10.4

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

* Re: [PATCH v2] emacs: Don't override mm-show-part in notmuch-show-view-part
  2013-05-26  6:34           ` [PATCH v2] " Austin Clements
  2013-05-26  7:04             ` Mark Walters
@ 2013-05-26  7:31             ` Tomi Ollila
  2013-05-26 23:35             ` David Bremner
  2 siblings, 0 replies; 12+ messages in thread
From: Tomi Ollila @ 2013-05-26  7:31 UTC (permalink / raw)
  To: Austin Clements, notmuch

On Sun, May 26 2013, Austin Clements <amdragon@MIT.EDU> wrote:

> Previously, notmuch-show-view-part overrode the function binding of
> mm-show-part to redirect it to notmuch-show-save-part to get notmuch's
> default file name handling in case mm-display-part decided to fall
> back to saving the part.  In addition to being messy, this depended on
> the now-deprecated dynamic binding behavior of flet.
>
> This patch removes the mm-show-part override in favor of passing the
> file name in to mm-show-part the way it expects, so we get its default
> file name handling.  It's not clear why we didn't do this before;
> mm-show-part has supported default file names since at least Emacs
> 23.1.
> ---
> This version overrides the default save directory to make it
> consistent with `notmuch-show-save-part'.

LGTM. +1.

Tomi


>
>  emacs/notmuch-show.el |   23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 423dd58..d773396 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -528,20 +528,15 @@ message at DEPTH in the current thread."
>  
>  (defun notmuch-show-view-part (message-id nth &optional filename content-type )
>    (notmuch-with-temp-part-buffer message-id nth
> -    ;; set mm-inlined-types to nil to force an external viewer
> -    (let ((handle (mm-make-handle (current-buffer) (list content-type)))
> -	  (mm-inlined-types nil))
> -      ;; We override mm-save-part as notmuch-show-save-part is better
> -      ;; since it offers the filename. We need to lexically bind
> -      ;; everything we need for notmuch-show-save-part to prevent
> -      ;; potential dynamic shadowing.
> -      (lexical-let ((message-id message-id)
> -		    (nth nth)
> -		    (filename filename)
> -		    (content-type content-type))
> -	(flet ((mm-save-part (&rest args) (notmuch-show-save-part
> -					   message-id nth filename content-type)))
> -	  (mm-display-part handle))))))
> +    (let* ((disposition (if filename `(attachment (filename . ,filename))))
> +	   (handle (mm-make-handle (current-buffer) (list content-type)
> +				   nil nil disposition))
> +	   ;; Set the default save directory to be consistent with
> +	   ;; `notmuch-show-save-part'.
> +	   (mm-default-directory (or mailcap-download-directory "~/"))
> +	   ;; set mm-inlined-types to nil to force an external viewer
> +	   (mm-inlined-types nil))
> +      (mm-display-part handle))))
>  
>  (defun notmuch-show-interactively-view-part (message-id nth &optional filename content-type)
>    (notmuch-with-temp-part-buffer message-id nth
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

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

* Re: [PATCH v2] emacs: Don't override mm-show-part in notmuch-show-view-part
  2013-05-26  6:34           ` [PATCH v2] " Austin Clements
  2013-05-26  7:04             ` Mark Walters
  2013-05-26  7:31             ` Tomi Ollila
@ 2013-05-26 23:35             ` David Bremner
  2 siblings, 0 replies; 12+ messages in thread
From: David Bremner @ 2013-05-26 23:35 UTC (permalink / raw)
  To: Austin Clements, notmuch

Austin Clements <amdragon@MIT.EDU> writes:

>
> This patch removes the mm-show-part override in favor of passing the
> file name in to mm-show-part the way it expects, so we get its default

Pushed,

d

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

end of thread, other threads:[~2013-05-26 23:35 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-05-19 11:31 use of flet in notmuch-emacs David Bremner
2013-05-19 15:14 ` [PATCH] emacs: Avoid deprecated function flet Austin Clements
2013-05-20 15:45   ` David Bremner
2013-05-20 17:04     ` Austin Clements
2013-05-20 20:08       ` [PATCH] emacs: Don't override mm-show-part in notmuch-show-view-part Austin Clements
2013-05-21  6:09         ` Mark Walters
2013-05-21 19:13           ` Mark Walters
2013-05-26  6:36             ` Austin Clements
2013-05-26  6:34           ` [PATCH v2] " Austin Clements
2013-05-26  7:04             ` Mark Walters
2013-05-26  7:31             ` Tomi Ollila
2013-05-26 23:35             ` David Bremner

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

	https://yhetil.org/notmuch.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).