[PATCH 1/1] Make buttons for attachments allow viewing as well as saving

[Mark Walters]

> 
> Oops, actually there was a bug in that macro.  It should have been
> 
> (defmacro notmuch-with-temp-part-buffer (message-id nth &rest body)
>   (declare (indent 2))
>   (let ((process-crypto (make-symbol "process-crypto")))
> `(let ((,process-crypto notmuch-show-process-crypto))
>(with-temp-buffer
>  (setq notmuch-show-process-crypto ,process-crypto)
>  ;; Always acquires the part via `notmuch part', even if it is
>  ;; available in the JSON output.
>  (insert (notmuch-show-get-bodypart-internal ,message-id ,nth))
>  , at body
> 
> The only difference is on the "insert" line.  Sorry about that.

Fixed.

[Snip excellent explanation of defmacro]

Thanks for the excellent explanation!

> > Finally, I have discovered one bug/misfeature. If you try to "view" an
> > attachment then it will offer to save it but will not offer a
> > filename. If you try and save it (or use the default action) it will
> > offer a filename as now. As far as I can see this is not fixable if I
> > use mm-display-part: however, I could include a slight tweaked version,
> > notmuch-show-mm-display-part say, which would fix this corner
> > case. (Essentially, it would call notmuch-show-save-part if it failed to
> > find a handler rather than mailcap-save-binary-file.) However, this is
> > about 50 lines of lisp so I am not sure it is worth it.
> 
> Hmm.  This is probably worth fixing, but probably in a separate patch.
> Duplicating mm-display-part is probably not the way to go.  It think
> it will work to pass t as the no-default argument to mm-display-part
> and check the return value, which should be 'inline if it was able to
> handle it internally or 'external if it found an external helper.  I'm
> pretty sure it will never fall in to mailcap-save-binary-file in that
> case.  If that doesn't work, you could flet mailcap-save-binary-file
> around the call to mm-display-part.

I had tried passing t to mm-display-part and that didn't work as I
expected. I will experiment some more and try your flet suggestion but I
think that will be a separate patch.

I will send a potential final version of this patch as a reply to this
email. Many thanks for all the guidance and help!

Best wishes

Mark

[PATCH 1/1] Make buttons for attachments allow viewing as well as saving

[Mark Walters]

> In general, yes, I think so.  A few comments on your draft below.

Ok I include a newer version which I am fairly happy with but I do have
some queries.

> > +(defvar notmuch-show-part-button-map
> > +  (let ((map (make-sparse-keymap)))
> > +   (set-keymap-parent map button-map)
> > +   (define-key map "s" 'notmuch-show-part-button-save)
> > +   (define-key map "v" 'notmuch-show-part-button-view)
> > +   (define-key map "o" 'notmuch-show-part-button-interactively-view)
> > +map)
> > +  "Submap for button commands")
> > +(fset 'notmuch-show-part-button-map notmuch-show-part-button-map)
> 
> I don't think this fset is necessary.  Actually, I've never seen this
> outside of the notmuch code.  It looks like it does appear in code
> shipped with Emacs, but only in a handful of places.  All of those
> places look like very old code, so maybe this was necessary once upon
> a time?

I have no idea on this: at the moment I have left it in as fset for
keymaps seems to occur throughout notmuch (I have the fset because I
copied it from somewhere).

> (defmacro notmuch-with-temp-part-buffer (message-id nth &rest body)
>   (declare (indent 2))
>   (let ((process-crypto (make-symbol "process-crypto")))
> `(let ((,process-crypto notmuch-show-process-crypto))
>(with-temp-buffer
>  (setq notmuch-show-process-crypto ,process-crypto)
>  ;; Always acquires the part via `notmuch part', even if it is
>  ;; available in the JSON output.
>  (insert (notmuch-show-get-bodypart-internal message-id nth))
>  , at body

I have followed the macro approach: since notmuch-show-save-part also
uses it (which doesn't appear in the diff as it was unchanged). I have
made all three functions use notmuch-with-temp-part-buffer. However, I
used the macro exactly as you wrote it (and it seems to work) but I
moderately understand why but could not justify it to someone! 

> (defun notmuch-show-interactively-view-part (message-id nth content-type)
>   (notmuch-with-temp-part-buffer message-id nth
> (let ((handle (mm-make-handle (current-buffer) (list content-type
>   (mm-interactively-view-part handle)

Emacs wants to indent the (let line level with message-id in the line
above which looks odd (and makes the lines too long). Do I overrule
emacs, or put message-id and nth onto a separate line or is there
something better?

Also note that, because of the unification with notmuch-show-save-part
all three functions have to have the four arguments message-id, nth,
filename and content-type (even though currently each individual
function only uses three of them). However see below for another comment
on this.

> > +(defcustom notmuch-show-part-button-default-action 
> > 'notmuch-show-part-button-save
> > +  "Default part header button action (on ENTER or mouse click)."
> > +  :group 'notmuch
> > +  :type '(choice (const :tag "Save part"
> > +   notmuch-show-part-button-save)
> > +(const :tag "View part"
> > +   notmuch-show-part-button-view)
> > +(const :tag "View interactively"
> > +   notmuch-show-part-button-interactively-view)))
> 
> You probably want this to be the handler function, rather than the
> button function, since the interface to the button function is rather
> awkward.  That is, if someone wanted to plug in their own action, they
> would want to define it in terms of the high-level handler interface
> that you use above, rather than the low-level
> button-with-magic-properties interface that Emacs forces you to use
> below.

I have done this.

> This duplication is much worse, but also less necessary.
> 
> (defun notmuch-show-part-button-interactively-view (&optional button)
>   (interactive)
>   (notmuch-show-part-button-internal button 
> #'notmuch-show-interactively-view-part))
> 
> (defun notmuch-show-part-button-internal (button handler)
>   (let ((button (or button (button-at (point)
> (if button
>   (let ((nth (button-get button :notmuch-part)))
> (if nth
> (funcall handler (notmuch-show-get-message-id) nth
>  (button-get button :notmuch-content-type))
>   (message "Not a valid part (is it a fake part?)."))

Yes this is much nicer and I have done this too (modulo the extra
argument mentioned above).

Finally, I have discovered one bug/misfeature. If you try to "view" an
attachment then it will offer to save it but will not offer a
filename. If you try and save it (or use the default action) it will
offer a filename as now. As far as I can see this is not fixable if I
use mm-display-part: however, I could include a slight tweaked version,
notmuch-show-mm-display-part say, which would fix this corner
case. (Essentially, it would call notmuch-show-save-part if it failed to
find a handler rather than mailcap-save-binary-file.) However, this is
about 50 lines of lisp so I am not sure it is worth it.

Best

[PATCH 1/1] Make buttons for attachments allow viewing as well as saving

[Mark Walters]

> > I am happy to make that change. My original patch in the summer was more
> > like that:
> > id:"CALUdzSWAto+4mCUOOMk+8vFs+Pog-xUma6u-Aqx2M6-sbyQROg at mail.gmail.com"
> 
> Is this the right id?  I couldn't find it in the list archive.

Sorry I messed up: it should be id:"87mxehqhbl.fsf at r102.config" However
I have included my current draft along these lines. I think it is
working but I am not submitting yet: just asking if this is the right
idea.

Best wishes 

Mark

>From 9e52414b9871369c1cbb5c3e72d833b56bb236d4 Mon Sep 17 00:00:00 2001
From: Mark Walters 
Date: Sat, 14 Jan 2012 18:04:22 +
Subject: [PATCH] Make buttons for attachments allow viewing as well as saving

Define a keymap for attachment buttons to allow multiple actions.
Define 3 possible actions:
save attachment: exactly as currently,
view attachment: uses mailcap entry,
view attachment with user chosen program

Keymap on a button is: s for save, v for view and o for view with
other program. Default (i.e. enter or mouse button) is save but this
is configurable in notmuch customize.

One implementation detail: the view attachment function forces all
attachments to be "displayed" using mailcap even if emacs could
display them itself. Thus, for example, text/html appears in a browser
and text/plain asks whether to save (on a standard debian setup)
---
 emacs/notmuch-show.el |   87 
 1 files changed, 79 insertions(+), 8 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 03c1f6b..2413caa 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -281,10 +281,21 @@ message at DEPTH in the current thread."
(run-hooks 'notmuch-show-markup-headers-hook)

 (define-button-type 'notmuch-show-part-button-type
-  'action 'notmuch-show-part-button-action
+  'action 'notmuch-show-part-button-default
+  'keymap 'notmuch-show-part-button-map
   'follow-link t
   'face 'message-mml)

+(defvar notmuch-show-part-button-map
+  (let ((map (make-sparse-keymap)))
+   (set-keymap-parent map button-map)
+   (define-key map "s" 'notmuch-show-part-button-save)
+   (define-key map "v" 'notmuch-show-part-button-view)
+   (define-key map "o" 'notmuch-show-part-button-interactively-view)
+map)
+  "Submap for button commands")
+(fset 'notmuch-show-part-button-map notmuch-show-part-button-map)
+
 (defun notmuch-show-insert-part-header (nth content-type declared-type 
&optional name comment)
   (let ((button))
 (setq button
@@ -299,7 +310,8 @@ message at DEPTH in the current thread."
   " ]")
   :type 'notmuch-show-part-button-type
   :notmuch-part nth
-  :notmuch-filename name))
+  :notmuch-filename name
+  :notmuch-content-type content-type))
 (insert "\n")
 ;; return button
 button))
@@ -323,6 +335,28 @@ message at DEPTH in the current thread."
;; ange-ftp, which is reasonable to use here.
(mm-write-region (point-min) (point-max) file nil nil nil 
'no-conversion t)

+(defun notmuch-show-view-part (message-id nth content-type)
+  (let ((process-crypto notmuch-show-process-crypto))
+(with-temp-buffer
+  (setq notmuch-show-process-crypto process-crypto)
+  ;; Always acquires the part via `notmuch part', even if it is
+  ;; available in the JSON output.
+  (insert (notmuch-show-get-bodypart-internal 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))
+   (mm-display-part handle t)
+
+(defun notmuch-show-interactively-view-part (message-id nth content-type)
+  (let ((process-crypto notmuch-show-process-crypto))
+(with-temp-buffer
+  (setq notmuch-show-process-crypto process-crypto)
+  ;; Always acquires the part via `notmuch part', even if it is
+  ;; available in the JSON output.
+  (insert (notmuch-show-get-bodypart-internal message-id nth))
+  (let ((handle (mm-make-handle (current-buffer) (list content-type
+   (mm-interactively-view-part handle)
+
 (defun notmuch-show-mm-display-part-inline (msg part nth content-type)
   "Use the mm-decode/mm-view functions to display a part in the
 current buffer, if possible."
@@ -1502,12 +1536,49 @@ buffer."

 ;; Commands typically bound to buttons.

-(defun notmuch-show-part-button-action (button)
-  (let ((nth (button-get button :notmuch-part)))
-(if nth
-   (notmuch-show-save-part (notmuch-show-get-message-id) nth
-   (button-get button :notmuch-filename))
-  (message "Not a valid part (is it a fake part?)."
+(defcustom notmuch-show-part-button-default-action 
'notmuch-show-part-button-save
+  "Default part header button action (on ENTER or mouse click)."
+  :group 'notmuch
+  :type '(choice (const :tag "Save part"
+   notmuch-show-part-button-s

[PATCH 1/1] Make buttons for attachments allow viewing as well as saving

[Austin Clements]

Quoth Mark Walters on Jan 17 at 10:27 pm:
> > (defmacro notmuch-with-temp-part-buffer (message-id nth &rest body)
> >   (declare (indent 2))
> >   (let ((process-crypto (make-symbol "process-crypto")))
> > `(let ((,process-crypto notmuch-show-process-crypto))
> >(with-temp-buffer
> >  (setq notmuch-show-process-crypto ,process-crypto)
> >  ;; Always acquires the part via `notmuch part', even if it is
> >  ;; available in the JSON output.
> >  (insert (notmuch-show-get-bodypart-internal message-id nth))
> >  , at body
> 
> I have followed the macro approach: since notmuch-show-save-part also
> uses it (which doesn't appear in the diff as it was unchanged). I have
> made all three functions use notmuch-with-temp-part-buffer. However, I
> used the macro exactly as you wrote it (and it seems to work) but I
> moderately understand why but could not justify it to someone! 

Oops, actually there was a bug in that macro.  It should have been

(defmacro notmuch-with-temp-part-buffer (message-id nth &rest body)
  (declare (indent 2))
  (let ((process-crypto (make-symbol "process-crypto")))
`(let ((,process-crypto notmuch-show-process-crypto))
   (with-temp-buffer
 (setq notmuch-show-process-crypto ,process-crypto)
 ;; Always acquires the part via `notmuch part', even if it is
 ;; available in the JSON output.
 (insert (notmuch-show-get-bodypart-internal ,message-id ,nth))
 , at body

The only difference is on the "insert" line.  Sorry about that.

I don't know how familiar you are with syntactic abstraction, so
here's a top-down explanation.  A macro is like the compile-time
equivalent of a function: rather than the arguments and return being
values that result from evaluation, they are pieces of code, the body
of the macro executes at compile time instead of at run time, and the
compiler replaces the invocation of the macro with the code that the
macro returns.  This is not unlike a C/C++ macro, but the
implementation of the macro is regular Lisp code that runs at compile
time and the code is represented as S-expressions rather than strings
(one beauty of Lisp is that the representation of code and data is the
same).

The above macro returns the code starting after the "`" (pronounced
quasiquote), so that's what invocations of the macro get replaced
with.  Quasiquote (like quote) inhibits the evaluation of what follows
it and results in the code as data, rather than the result of
evaluating the code.  Unlike quote, quasiquote lets you jump back into
the evaluator using "," and ",@", so it's great for piecing together
code from a template, which is what macros spend most of their time
doing.

The "declare" is simply a specification to emacs-lisp-mode about how
to indent uses of this macro.  The "make-symbol" is necessary to avoid
conflicting with variable names that may appear in the code that uses
this macro (since the invoking code and the code returned by the macro
will be interleaved, you have to worry about variable conflicts).

> > (defun notmuch-show-interactively-view-part (message-id nth content-type)
> >   (notmuch-with-temp-part-buffer message-id nth
> > (let ((handle (mm-make-handle (current-buffer) (list content-type
> >   (mm-interactively-view-part handle)
> 
> Emacs wants to indent the (let line level with message-id in the line
> above which looks odd (and makes the lines too long). Do I overrule
> emacs, or put message-id and nth onto a separate line or is there
> something better?

If you evaluate the defmacro, it'll pick up on that declare line at
the beginning of it and indent this correctly.

> Also note that, because of the unification with notmuch-show-save-part
> all three functions have to have the four arguments message-id, nth,
> filename and content-type (even though currently each individual
> function only uses three of them). However see below for another comment
> on this.

That makes sense.

> > > +(defcustom notmuch-show-part-button-default-action 
> > > 'notmuch-show-part-button-save
> > > +  "Default part header button action (on ENTER or mouse click)."
> > > +  :group 'notmuch
> > > +  :type '(choice (const :tag "Save part"
> > > + notmuch-show-part-button-save)
> > > +  (const :tag "View part"
> > > + notmuch-show-part-button-view)
> > > +  (const :tag "View interactively"
> > > + notmuch-show-part-button-interactively-view)))
> > 
> > You probably want this to be the handler function, rather than the
> > button function, since the interface to the button function is rather
> > awkward.  That is, if someone wanted to plug in their own action, they
> > would want to define it in terms of the high-level handler interface
> > that you use above, rather than the low-level
> > button-with-magic-properties interface that Emacs forces you to use
> > below.
> 
> I have done this.
> 
> > This duplicati

[PATCH 1/1] Make buttons for attachments allow viewing as well as saving

[Austin Clements]

Quoth Mark Walters on Jan 17 at  8:39 pm:
> > > I am happy to make that change. My original patch in the summer was more
> > > like that:
> > > id:"CALUdzSWAto+4mCUOOMk+8vFs+Pog-xUma6u-Aqx2M6-sbyQROg at mail.gmail.com"
> > 
> > Is this the right id?  I couldn't find it in the list archive.
> 
> Sorry I messed up: it should be id:"87mxehqhbl.fsf at r102.config" However
> I have included my current draft along these lines. I think it is
> working but I am not submitting yet: just asking if this is the right
> idea.

In general, yes, I think so.  A few comments on your draft below.

> Best wishes 
> 
> Mark
> 
> From 9e52414b9871369c1cbb5c3e72d833b56bb236d4 Mon Sep 17 00:00:00 2001
> From: Mark Walters 
> Date: Sat, 14 Jan 2012 18:04:22 +
> Subject: [PATCH] Make buttons for attachments allow viewing as well as saving
> 
> Define a keymap for attachment buttons to allow multiple actions.
> Define 3 possible actions:
> save attachment: exactly as currently,
> view attachment: uses mailcap entry,
> view attachment with user chosen program
> 
> Keymap on a button is: s for save, v for view and o for view with
> other program. Default (i.e. enter or mouse button) is save but this
> is configurable in notmuch customize.
> 
> One implementation detail: the view attachment function forces all
> attachments to be "displayed" using mailcap even if emacs could
> display them itself. Thus, for example, text/html appears in a browser
> and text/plain asks whether to save (on a standard debian setup)
> ---
>  emacs/notmuch-show.el |   87 
>  1 files changed, 79 insertions(+), 8 deletions(-)
> 
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 03c1f6b..2413caa 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -281,10 +281,21 @@ message at DEPTH in the current thread."
>   (run-hooks 'notmuch-show-markup-headers-hook)
>  
>  (define-button-type 'notmuch-show-part-button-type
> -  'action 'notmuch-show-part-button-action
> +  'action 'notmuch-show-part-button-default
> +  'keymap 'notmuch-show-part-button-map
>'follow-link t
>'face 'message-mml)
>  
> +(defvar notmuch-show-part-button-map
> +  (let ((map (make-sparse-keymap)))
> +   (set-keymap-parent map button-map)
> +   (define-key map "s" 'notmuch-show-part-button-save)
> +   (define-key map "v" 'notmuch-show-part-button-view)
> +   (define-key map "o" 'notmuch-show-part-button-interactively-view)
> +map)
> +  "Submap for button commands")
> +(fset 'notmuch-show-part-button-map notmuch-show-part-button-map)

I don't think this fset is necessary.  Actually, I've never seen this
outside of the notmuch code.  It looks like it does appear in code
shipped with Emacs, but only in a handful of places.  All of those
places look like very old code, so maybe this was necessary once upon
a time?

> +
>  (defun notmuch-show-insert-part-header (nth content-type declared-type 
> &optional name comment)
>(let ((button))
>  (setq button
> @@ -299,7 +310,8 @@ message at DEPTH in the current thread."
>  " ]")
>  :type 'notmuch-show-part-button-type
>  :notmuch-part nth
> -:notmuch-filename name))
> +:notmuch-filename name
> +:notmuch-content-type content-type))
>  (insert "\n")
>  ;; return button
>  button))
> @@ -323,6 +335,28 @@ message at DEPTH in the current thread."
>   ;; ange-ftp, which is reasonable to use here.
>   (mm-write-region (point-min) (point-max) file nil nil nil 
> 'no-conversion t)
>  
> +(defun notmuch-show-view-part (message-id nth content-type)
> +  (let ((process-crypto notmuch-show-process-crypto))
> +(with-temp-buffer
> +  (setq notmuch-show-process-crypto process-crypto)
> +  ;; Always acquires the part via `notmuch part', even if it is
> +  ;; available in the JSON output.
> +  (insert (notmuch-show-get-bodypart-internal 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))
> + (mm-display-part handle t)
> +
> +(defun notmuch-show-interactively-view-part (message-id nth content-type)
> +  (let ((process-crypto notmuch-show-process-crypto))
> +(with-temp-buffer
> +  (setq notmuch-show-process-crypto process-crypto)
> +  ;; Always acquires the part via `notmuch part', even if it is
> +  ;; available in the JSON output.
> +  (insert (notmuch-show-get-bodypart-internal message-id nth))
> +  (let ((handle (mm-make-handle (current-buffer) (list content-type
> + (mm-interactively-view-part handle)
> +

Yeah.  Though you're right that the duplication is a little annoying.
With the snippet I sent earlier, these could change to, e.g.,

(defun notmuch-show-interactively-view-part (message-id nth content-type)
  (notmuch-with-part-temp-buffer

[PATCH 1/1] Make buttons for attachments allow viewing as well as saving

[Aaron Ecay]

On Tue, 17 Jan 2012 15:26:03 -0500, Austin Clements  wrote:
> Quoth Mark Walters on Jan 17 at  9:06 am:
> > 
> > > > I wonder if the "problem" comes from me doing things in a non-lispy
> > > > fashion (I am completely new to lisp). Thus
> > > > notmuch-show-part-button-default-action is a variable that gets passed
> > > > around rather than a function.
> > > 
> > > Sorry, I should have looked at the bigger context in this patch.  I
> > > think Jameson was implying that notmuch-show-part-button-default
> > > should change to
> > > 
> > > (defun notmuch-show-part-button-default (&optional button)
> > >   (interactive)
> > >   (funcall notmuch-show-part-button-default-action button))
> > > 
> > > I would go one step further and say that each action should probably
> > > be a separate function.  That is, break notmuch-show-part-action into
> > > separate functions and simply invoke the appropriate function, rather
> > > than performing a fixed data dispatch.  This would be more flexible
> > > and Lispy.  It may be that your approach works out better, but I'd at
> > > least give this a shot.
> > 
> > I am happy to make that change. My original patch in the summer was more
> > like that:
> > id:"CALUdzSWAto+4mCUOOMk+8vFs+Pog-xUma6u-Aqx2M6-sbyQROg at mail.gmail.com"
> 
> Is this the right id?  I couldn't find it in the list archive.
> 
> > Is that more what you had in mind? (Only in broad terms: Obviously I
> > would need to add in the customization and default function etc). I
> > decided that I didn't like the code duplication (but I am completely new
> > to lisp) which is why I changed it for this submission.
> 
> Yes, I wondered about this, too.  It seems like at worst the
> notmuch-show-process-crypto stuff would be duplicated.  This might be
> little enough that it's not worth worrying about, or it might be worth
> introducing something like
> 
> (defun notmuch-with-temp-part-buffer (message-id nth action)
>   (let ((process-crypto notmuch-show-process-crypto))
> (with-temp-buffer
>   (setq notmuch-show-process-crypto process-crypto)
>   ;; Always acquires the part via `notmuch part', even if it is
>   ;; available in the JSON output.
>   (insert (notmuch-show-get-bodypart-internal message-id nth))
>   (funcall action
> 
> You could also do this as a macro, but that definitely seems like
> overkill.

It seems to me that a macro is in fact the best solution.  If you do it
with a function, you need two defuns per action: one to do the actual
work:
(defun notmuch-show-part-button-whatever-worker ()
  ;; do stuff...
)
and one that says:
(defun notmuch-show-part-button-whatever ()
  (notmuch-with-temp-part-buffer
   id part-number #'notmuch-show-part-button-whatever-worker))

It would be the latter function that the key would be bound to.  If a
macro is used, the split between the worker and glue fns can be
abandoned, and only one function is needed:
(defun notmuch-show-part-button-whatever ()
  (notmuch-with-temp-part-buffer
  ;; do stuff...
  ))

A further advantage is if interactive arguments (e.g. C-u prefix) are
needed for the function, there is no need to thread them through as
arguments of notmuch-with-temp-part-buffer.

-- 
Aaron Ecay

Re: [PATCH 1/1] Make buttons for attachments allow viewing as well as saving

[Mark Walters]

> 
> Oops, actually there was a bug in that macro.  It should have been
> 
> (defmacro notmuch-with-temp-part-buffer (message-id nth &rest body)
>   (declare (indent 2))
>   (let ((process-crypto (make-symbol "process-crypto")))
> `(let ((,process-crypto notmuch-show-process-crypto))
>(with-temp-buffer
>  (setq notmuch-show-process-crypto ,process-crypto)
>  ;; Always acquires the part via `notmuch part', even if it is
>  ;; available in the JSON output.
>  (insert (notmuch-show-get-bodypart-internal ,message-id ,nth))
>  ,@body
> 
> The only difference is on the "insert" line.  Sorry about that.

Fixed.

[Snip excellent explanation of defmacro]

Thanks for the excellent explanation!

> > Finally, I have discovered one bug/misfeature. If you try to "view" an
> > attachment then it will offer to save it but will not offer a
> > filename. If you try and save it (or use the default action) it will
> > offer a filename as now. As far as I can see this is not fixable if I
> > use mm-display-part: however, I could include a slight tweaked version,
> > notmuch-show-mm-display-part say, which would fix this corner
> > case. (Essentially, it would call notmuch-show-save-part if it failed to
> > find a handler rather than mailcap-save-binary-file.) However, this is
> > about 50 lines of lisp so I am not sure it is worth it.
> 
> Hmm.  This is probably worth fixing, but probably in a separate patch.
> Duplicating mm-display-part is probably not the way to go.  It think
> it will work to pass t as the no-default argument to mm-display-part
> and check the return value, which should be 'inline if it was able to
> handle it internally or 'external if it found an external helper.  I'm
> pretty sure it will never fall in to mailcap-save-binary-file in that
> case.  If that doesn't work, you could flet mailcap-save-binary-file
> around the call to mm-display-part.

I had tried passing t to mm-display-part and that didn't work as I
expected. I will experiment some more and try your flet suggestion but I
think that will be a separate patch.

I will send a potential final version of this patch as a reply to this
email. Many thanks for all the guidance and help!

Best wishes

Mark

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 1/1] Make buttons for attachments allow viewing as well as saving

[Austin Clements]

Quoth Mark Walters on Jan 17 at  9:06 am:
> 
> > > I wonder if the "problem" comes from me doing things in a non-lispy
> > > fashion (I am completely new to lisp). Thus
> > > notmuch-show-part-button-default-action is a variable that gets passed
> > > around rather than a function.
> > 
> > Sorry, I should have looked at the bigger context in this patch.  I
> > think Jameson was implying that notmuch-show-part-button-default
> > should change to
> > 
> > (defun notmuch-show-part-button-default (&optional button)
> >   (interactive)
> >   (funcall notmuch-show-part-button-default-action button))
> > 
> > I would go one step further and say that each action should probably
> > be a separate function.  That is, break notmuch-show-part-action into
> > separate functions and simply invoke the appropriate function, rather
> > than performing a fixed data dispatch.  This would be more flexible
> > and Lispy.  It may be that your approach works out better, but I'd at
> > least give this a shot.
> 
> I am happy to make that change. My original patch in the summer was more
> like that:
> id:"CALUdzSWAto+4mCUOOMk+8vFs+Pog-xUma6u-Aqx2M6-sbyQROg at mail.gmail.com"

Is this the right id?  I couldn't find it in the list archive.

> Is that more what you had in mind? (Only in broad terms: Obviously I
> would need to add in the customization and default function etc). I
> decided that I didn't like the code duplication (but I am completely new
> to lisp) which is why I changed it for this submission.

Yes, I wondered about this, too.  It seems like at worst the
notmuch-show-process-crypto stuff would be duplicated.  This might be
little enough that it's not worth worrying about, or it might be worth
introducing something like

(defun notmuch-with-temp-part-buffer (message-id nth action)
  (let ((process-crypto notmuch-show-process-crypto))
(with-temp-buffer
  (setq notmuch-show-process-crypto process-crypto)
  ;; Always acquires the part via `notmuch part', even if it is
  ;; available in the JSON output.
  (insert (notmuch-show-get-bodypart-internal message-id nth))
  (funcall action

You could also do this as a macro, but that definitely seems like
overkill.

Re: [PATCH 1/1] Make buttons for attachments allow viewing as well as saving

[Austin Clements]

Quoth Mark Walters on Jan 17 at 10:27 pm:
> > (defmacro notmuch-with-temp-part-buffer (message-id nth &rest body)
> >   (declare (indent 2))
> >   (let ((process-crypto (make-symbol "process-crypto")))
> > `(let ((,process-crypto notmuch-show-process-crypto))
> >(with-temp-buffer
> >  (setq notmuch-show-process-crypto ,process-crypto)
> >  ;; Always acquires the part via `notmuch part', even if it is
> >  ;; available in the JSON output.
> >  (insert (notmuch-show-get-bodypart-internal message-id nth))
> >  ,@body
> 
> I have followed the macro approach: since notmuch-show-save-part also
> uses it (which doesn't appear in the diff as it was unchanged). I have
> made all three functions use notmuch-with-temp-part-buffer. However, I
> used the macro exactly as you wrote it (and it seems to work) but I
> moderately understand why but could not justify it to someone! 

Oops, actually there was a bug in that macro.  It should have been

(defmacro notmuch-with-temp-part-buffer (message-id nth &rest body)
  (declare (indent 2))
  (let ((process-crypto (make-symbol "process-crypto")))
`(let ((,process-crypto notmuch-show-process-crypto))
   (with-temp-buffer
 (setq notmuch-show-process-crypto ,process-crypto)
 ;; Always acquires the part via `notmuch part', even if it is
 ;; available in the JSON output.
 (insert (notmuch-show-get-bodypart-internal ,message-id ,nth))
 ,@body

The only difference is on the "insert" line.  Sorry about that.

I don't know how familiar you are with syntactic abstraction, so
here's a top-down explanation.  A macro is like the compile-time
equivalent of a function: rather than the arguments and return being
values that result from evaluation, they are pieces of code, the body
of the macro executes at compile time instead of at run time, and the
compiler replaces the invocation of the macro with the code that the
macro returns.  This is not unlike a C/C++ macro, but the
implementation of the macro is regular Lisp code that runs at compile
time and the code is represented as S-expressions rather than strings
(one beauty of Lisp is that the representation of code and data is the
same).

The above macro returns the code starting after the "`" (pronounced
quasiquote), so that's what invocations of the macro get replaced
with.  Quasiquote (like quote) inhibits the evaluation of what follows
it and results in the code as data, rather than the result of
evaluating the code.  Unlike quote, quasiquote lets you jump back into
the evaluator using "," and ",@", so it's great for piecing together
code from a template, which is what macros spend most of their time
doing.

The "declare" is simply a specification to emacs-lisp-mode about how
to indent uses of this macro.  The "make-symbol" is necessary to avoid
conflicting with variable names that may appear in the code that uses
this macro (since the invoking code and the code returned by the macro
will be interleaved, you have to worry about variable conflicts).

> > (defun notmuch-show-interactively-view-part (message-id nth content-type)
> >   (notmuch-with-temp-part-buffer message-id nth
> > (let ((handle (mm-make-handle (current-buffer) (list content-type
> >   (mm-interactively-view-part handle)
> 
> Emacs wants to indent the (let line level with message-id in the line
> above which looks odd (and makes the lines too long). Do I overrule
> emacs, or put message-id and nth onto a separate line or is there
> something better?

If you evaluate the defmacro, it'll pick up on that declare line at
the beginning of it and indent this correctly.

> Also note that, because of the unification with notmuch-show-save-part
> all three functions have to have the four arguments message-id, nth,
> filename and content-type (even though currently each individual
> function only uses three of them). However see below for another comment
> on this.

That makes sense.

> > > +(defcustom notmuch-show-part-button-default-action 
> > > 'notmuch-show-part-button-save
> > > +  "Default part header button action (on ENTER or mouse click)."
> > > +  :group 'notmuch
> > > +  :type '(choice (const :tag "Save part"
> > > + notmuch-show-part-button-save)
> > > +  (const :tag "View part"
> > > + notmuch-show-part-button-view)
> > > +  (const :tag "View interactively"
> > > + notmuch-show-part-button-interactively-view)))
> > 
> > You probably want this to be the handler function, rather than the
> > button function, since the interface to the button function is rather
> > awkward.  That is, if someone wanted to plug in their own action, they
> > would want to define it in terms of the high-level handler interface
> > that you use above, rather than the low-level
> > button-with-magic-properties interface that Emacs forces you to use
> > below.
> 
> I have done this.
> 
> > This duplication is 

Re: [PATCH 1/1] Make buttons for attachments allow viewing as well as saving

[Mark Walters]

> In general, yes, I think so.  A few comments on your draft below.

Ok I include a newer version which I am fairly happy with but I do have
some queries.

> > +(defvar notmuch-show-part-button-map
> > +  (let ((map (make-sparse-keymap)))
> > +   (set-keymap-parent map button-map)
> > +   (define-key map "s" 'notmuch-show-part-button-save)
> > +   (define-key map "v" 'notmuch-show-part-button-view)
> > +   (define-key map "o" 'notmuch-show-part-button-interactively-view)
> > +map)
> > +  "Submap for button commands")
> > +(fset 'notmuch-show-part-button-map notmuch-show-part-button-map)
> 
> I don't think this fset is necessary.  Actually, I've never seen this
> outside of the notmuch code.  It looks like it does appear in code
> shipped with Emacs, but only in a handful of places.  All of those
> places look like very old code, so maybe this was necessary once upon
> a time?

I have no idea on this: at the moment I have left it in as fset for
keymaps seems to occur throughout notmuch (I have the fset because I
copied it from somewhere).

> (defmacro notmuch-with-temp-part-buffer (message-id nth &rest body)
>   (declare (indent 2))
>   (let ((process-crypto (make-symbol "process-crypto")))
> `(let ((,process-crypto notmuch-show-process-crypto))
>(with-temp-buffer
>  (setq notmuch-show-process-crypto ,process-crypto)
>  ;; Always acquires the part via `notmuch part', even if it is
>  ;; available in the JSON output.
>  (insert (notmuch-show-get-bodypart-internal message-id nth))
>  ,@body

I have followed the macro approach: since notmuch-show-save-part also
uses it (which doesn't appear in the diff as it was unchanged). I have
made all three functions use notmuch-with-temp-part-buffer. However, I
used the macro exactly as you wrote it (and it seems to work) but I
moderately understand why but could not justify it to someone! 

> (defun notmuch-show-interactively-view-part (message-id nth content-type)
>   (notmuch-with-temp-part-buffer message-id nth
> (let ((handle (mm-make-handle (current-buffer) (list content-type
>   (mm-interactively-view-part handle)

Emacs wants to indent the (let line level with message-id in the line
above which looks odd (and makes the lines too long). Do I overrule
emacs, or put message-id and nth onto a separate line or is there
something better?

Also note that, because of the unification with notmuch-show-save-part
all three functions have to have the four arguments message-id, nth,
filename and content-type (even though currently each individual
function only uses three of them). However see below for another comment
on this.

> > +(defcustom notmuch-show-part-button-default-action 
> > 'notmuch-show-part-button-save
> > +  "Default part header button action (on ENTER or mouse click)."
> > +  :group 'notmuch
> > +  :type '(choice (const :tag "Save part"
> > +   notmuch-show-part-button-save)
> > +(const :tag "View part"
> > +   notmuch-show-part-button-view)
> > +(const :tag "View interactively"
> > +   notmuch-show-part-button-interactively-view)))
> 
> You probably want this to be the handler function, rather than the
> button function, since the interface to the button function is rather
> awkward.  That is, if someone wanted to plug in their own action, they
> would want to define it in terms of the high-level handler interface
> that you use above, rather than the low-level
> button-with-magic-properties interface that Emacs forces you to use
> below.

I have done this.

> This duplication is much worse, but also less necessary.
> 
> (defun notmuch-show-part-button-interactively-view (&optional button)
>   (interactive)
>   (notmuch-show-part-button-internal button 
> #'notmuch-show-interactively-view-part))
> 
> (defun notmuch-show-part-button-internal (button handler)
>   (let ((button (or button (button-at (point)
> (if button
>   (let ((nth (button-get button :notmuch-part)))
> (if nth
> (funcall handler (notmuch-show-get-message-id) nth
>  (button-get button :notmuch-content-type))
>   (message "Not a valid part (is it a fake part?)."))

Yes this is much nicer and I have done this too (modulo the extra
argument mentioned above).

Finally, I have discovered one bug/misfeature. If you try to "view" an
attachment then it will offer to save it but will not offer a
filename. If you try and save it (or use the default action) it will
offer a filename as now. As far as I can see this is not fixable if I
use mm-display-part: however, I could include a slight tweaked version,
notmuch-show-mm-display-part say, which would fix this corner
case. (Essentially, it would call notmuch-show-save-part if it failed to
find a handler rather than mailcap-save-binary-file.) However, this is
about 50 lines of lisp so I am not sure it is worth it.

Best wi

Re: [PATCH 1/1] Make buttons for attachments allow viewing as well as saving

[Austin Clements]

Quoth Mark Walters on Jan 17 at  8:39 pm:
> > > I am happy to make that change. My original patch in the summer was more
> > > like that:
> > > id:"caludzswato+4mcuoomk+8vfs+pog-xuma6u-aqx2m6-sbyq...@mail.gmail.com"
> > 
> > Is this the right id?  I couldn't find it in the list archive.
> 
> Sorry I messed up: it should be id:"87mxehqhbl.fsf@r102.config" However
> I have included my current draft along these lines. I think it is
> working but I am not submitting yet: just asking if this is the right
> idea.

In general, yes, I think so.  A few comments on your draft below.

> Best wishes 
> 
> Mark
> 
> From 9e52414b9871369c1cbb5c3e72d833b56bb236d4 Mon Sep 17 00:00:00 2001
> From: Mark Walters 
> Date: Sat, 14 Jan 2012 18:04:22 +
> Subject: [PATCH] Make buttons for attachments allow viewing as well as saving
> 
> Define a keymap for attachment buttons to allow multiple actions.
> Define 3 possible actions:
> save attachment: exactly as currently,
> view attachment: uses mailcap entry,
> view attachment with user chosen program
> 
> Keymap on a button is: s for save, v for view and o for view with
> other program. Default (i.e. enter or mouse button) is save but this
> is configurable in notmuch customize.
> 
> One implementation detail: the view attachment function forces all
> attachments to be "displayed" using mailcap even if emacs could
> display them itself. Thus, for example, text/html appears in a browser
> and text/plain asks whether to save (on a standard debian setup)
> ---
>  emacs/notmuch-show.el |   87 
>  1 files changed, 79 insertions(+), 8 deletions(-)
> 
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 03c1f6b..2413caa 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -281,10 +281,21 @@ message at DEPTH in the current thread."
>   (run-hooks 'notmuch-show-markup-headers-hook)
>  
>  (define-button-type 'notmuch-show-part-button-type
> -  'action 'notmuch-show-part-button-action
> +  'action 'notmuch-show-part-button-default
> +  'keymap 'notmuch-show-part-button-map
>'follow-link t
>'face 'message-mml)
>  
> +(defvar notmuch-show-part-button-map
> +  (let ((map (make-sparse-keymap)))
> +   (set-keymap-parent map button-map)
> +   (define-key map "s" 'notmuch-show-part-button-save)
> +   (define-key map "v" 'notmuch-show-part-button-view)
> +   (define-key map "o" 'notmuch-show-part-button-interactively-view)
> +map)
> +  "Submap for button commands")
> +(fset 'notmuch-show-part-button-map notmuch-show-part-button-map)

I don't think this fset is necessary.  Actually, I've never seen this
outside of the notmuch code.  It looks like it does appear in code
shipped with Emacs, but only in a handful of places.  All of those
places look like very old code, so maybe this was necessary once upon
a time?

> +
>  (defun notmuch-show-insert-part-header (nth content-type declared-type 
> &optional name comment)
>(let ((button))
>  (setq button
> @@ -299,7 +310,8 @@ message at DEPTH in the current thread."
>  " ]")
>  :type 'notmuch-show-part-button-type
>  :notmuch-part nth
> -:notmuch-filename name))
> +:notmuch-filename name
> +:notmuch-content-type content-type))
>  (insert "\n")
>  ;; return button
>  button))
> @@ -323,6 +335,28 @@ message at DEPTH in the current thread."
>   ;; ange-ftp, which is reasonable to use here.
>   (mm-write-region (point-min) (point-max) file nil nil nil 
> 'no-conversion t)
>  
> +(defun notmuch-show-view-part (message-id nth content-type)
> +  (let ((process-crypto notmuch-show-process-crypto))
> +(with-temp-buffer
> +  (setq notmuch-show-process-crypto process-crypto)
> +  ;; Always acquires the part via `notmuch part', even if it is
> +  ;; available in the JSON output.
> +  (insert (notmuch-show-get-bodypart-internal 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))
> + (mm-display-part handle t)
> +
> +(defun notmuch-show-interactively-view-part (message-id nth content-type)
> +  (let ((process-crypto notmuch-show-process-crypto))
> +(with-temp-buffer
> +  (setq notmuch-show-process-crypto process-crypto)
> +  ;; Always acquires the part via `notmuch part', even if it is
> +  ;; available in the JSON output.
> +  (insert (notmuch-show-get-bodypart-internal message-id nth))
> +  (let ((handle (mm-make-handle (current-buffer) (list content-type
> + (mm-interactively-view-part handle)
> +

Yeah.  Though you're right that the duplication is a little annoying.
With the snippet I sent earlier, these could change to, e.g.,

(defun notmuch-show-interactively-view-part (message-id nth content-type)
  (notmuch-with-part-temp-buffer
   mes

Re: [PATCH 1/1] Make buttons for attachments allow viewing as well as saving

[Aaron Ecay]

On Tue, 17 Jan 2012 15:26:03 -0500, Austin Clements  wrote:
> Quoth Mark Walters on Jan 17 at  9:06 am:
> > 
> > > > I wonder if the "problem" comes from me doing things in a non-lispy
> > > > fashion (I am completely new to lisp). Thus
> > > > notmuch-show-part-button-default-action is a variable that gets passed
> > > > around rather than a function.
> > > 
> > > Sorry, I should have looked at the bigger context in this patch.  I
> > > think Jameson was implying that notmuch-show-part-button-default
> > > should change to
> > > 
> > > (defun notmuch-show-part-button-default (&optional button)
> > >   (interactive)
> > >   (funcall notmuch-show-part-button-default-action button))
> > > 
> > > I would go one step further and say that each action should probably
> > > be a separate function.  That is, break notmuch-show-part-action into
> > > separate functions and simply invoke the appropriate function, rather
> > > than performing a fixed data dispatch.  This would be more flexible
> > > and Lispy.  It may be that your approach works out better, but I'd at
> > > least give this a shot.
> > 
> > I am happy to make that change. My original patch in the summer was more
> > like that:
> > id:"caludzswato+4mcuoomk+8vfs+pog-xuma6u-aqx2m6-sbyq...@mail.gmail.com"
> 
> Is this the right id?  I couldn't find it in the list archive.
> 
> > Is that more what you had in mind? (Only in broad terms: Obviously I
> > would need to add in the customization and default function etc). I
> > decided that I didn't like the code duplication (but I am completely new
> > to lisp) which is why I changed it for this submission.
> 
> Yes, I wondered about this, too.  It seems like at worst the
> notmuch-show-process-crypto stuff would be duplicated.  This might be
> little enough that it's not worth worrying about, or it might be worth
> introducing something like
> 
> (defun notmuch-with-temp-part-buffer (message-id nth action)
>   (let ((process-crypto notmuch-show-process-crypto))
> (with-temp-buffer
>   (setq notmuch-show-process-crypto process-crypto)
>   ;; Always acquires the part via `notmuch part', even if it is
>   ;; available in the JSON output.
>   (insert (notmuch-show-get-bodypart-internal message-id nth))
>   (funcall action
> 
> You could also do this as a macro, but that definitely seems like
> overkill.

It seems to me that a macro is in fact the best solution.  If you do it
with a function, you need two defuns per action: one to do the actual
work:
(defun notmuch-show-part-button-whatever-worker ()
  ;; do stuff...
)
and one that says:
(defun notmuch-show-part-button-whatever ()
  (notmuch-with-temp-part-buffer
   id part-number #'notmuch-show-part-button-whatever-worker))

It would be the latter function that the key would be bound to.  If a
macro is used, the split between the worker and glue fns can be
abandoned, and only one function is needed:
(defun notmuch-show-part-button-whatever ()
  (notmuch-with-temp-part-buffer
  ;; do stuff...
  ))

A further advantage is if interactive arguments (e.g. C-u prefix) are
needed for the function, there is no need to thread them through as
arguments of notmuch-with-temp-part-buffer.

-- 
Aaron Ecay
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 1/1] Make buttons for attachments allow viewing as well as saving

[Mark Walters]

> > I am happy to make that change. My original patch in the summer was more
> > like that:
> > id:"caludzswato+4mcuoomk+8vfs+pog-xuma6u-aqx2m6-sbyq...@mail.gmail.com"
> 
> Is this the right id?  I couldn't find it in the list archive.

Sorry I messed up: it should be id:"87mxehqhbl.fsf@r102.config" However
I have included my current draft along these lines. I think it is
working but I am not submitting yet: just asking if this is the right
idea.

Best wishes 

Mark

>From 9e52414b9871369c1cbb5c3e72d833b56bb236d4 Mon Sep 17 00:00:00 2001
From: Mark Walters 
Date: Sat, 14 Jan 2012 18:04:22 +
Subject: [PATCH] Make buttons for attachments allow viewing as well as saving

Define a keymap for attachment buttons to allow multiple actions.
Define 3 possible actions:
save attachment: exactly as currently,
view attachment: uses mailcap entry,
view attachment with user chosen program

Keymap on a button is: s for save, v for view and o for view with
other program. Default (i.e. enter or mouse button) is save but this
is configurable in notmuch customize.

One implementation detail: the view attachment function forces all
attachments to be "displayed" using mailcap even if emacs could
display them itself. Thus, for example, text/html appears in a browser
and text/plain asks whether to save (on a standard debian setup)
---
 emacs/notmuch-show.el |   87 
 1 files changed, 79 insertions(+), 8 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 03c1f6b..2413caa 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -281,10 +281,21 @@ message at DEPTH in the current thread."
(run-hooks 'notmuch-show-markup-headers-hook)
 
 (define-button-type 'notmuch-show-part-button-type
-  'action 'notmuch-show-part-button-action
+  'action 'notmuch-show-part-button-default
+  'keymap 'notmuch-show-part-button-map
   'follow-link t
   'face 'message-mml)
 
+(defvar notmuch-show-part-button-map
+  (let ((map (make-sparse-keymap)))
+   (set-keymap-parent map button-map)
+   (define-key map "s" 'notmuch-show-part-button-save)
+   (define-key map "v" 'notmuch-show-part-button-view)
+   (define-key map "o" 'notmuch-show-part-button-interactively-view)
+map)
+  "Submap for button commands")
+(fset 'notmuch-show-part-button-map notmuch-show-part-button-map)
+
 (defun notmuch-show-insert-part-header (nth content-type declared-type 
&optional name comment)
   (let ((button))
 (setq button
@@ -299,7 +310,8 @@ message at DEPTH in the current thread."
   " ]")
   :type 'notmuch-show-part-button-type
   :notmuch-part nth
-  :notmuch-filename name))
+  :notmuch-filename name
+  :notmuch-content-type content-type))
 (insert "\n")
 ;; return button
 button))
@@ -323,6 +335,28 @@ message at DEPTH in the current thread."
;; ange-ftp, which is reasonable to use here.
(mm-write-region (point-min) (point-max) file nil nil nil 
'no-conversion t)
 
+(defun notmuch-show-view-part (message-id nth content-type)
+  (let ((process-crypto notmuch-show-process-crypto))
+(with-temp-buffer
+  (setq notmuch-show-process-crypto process-crypto)
+  ;; Always acquires the part via `notmuch part', even if it is
+  ;; available in the JSON output.
+  (insert (notmuch-show-get-bodypart-internal 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))
+   (mm-display-part handle t)
+
+(defun notmuch-show-interactively-view-part (message-id nth content-type)
+  (let ((process-crypto notmuch-show-process-crypto))
+(with-temp-buffer
+  (setq notmuch-show-process-crypto process-crypto)
+  ;; Always acquires the part via `notmuch part', even if it is
+  ;; available in the JSON output.
+  (insert (notmuch-show-get-bodypart-internal message-id nth))
+  (let ((handle (mm-make-handle (current-buffer) (list content-type
+   (mm-interactively-view-part handle)
+
 (defun notmuch-show-mm-display-part-inline (msg part nth content-type)
   "Use the mm-decode/mm-view functions to display a part in the
 current buffer, if possible."
@@ -1502,12 +1536,49 @@ buffer."
 
 ;; Commands typically bound to buttons.
 
-(defun notmuch-show-part-button-action (button)
-  (let ((nth (button-get button :notmuch-part)))
-(if nth
-   (notmuch-show-save-part (notmuch-show-get-message-id) nth
-   (button-get button :notmuch-filename))
-  (message "Not a valid part (is it a fake part?)."
+(defcustom notmuch-show-part-button-default-action 
'notmuch-show-part-button-save
+  "Default part header button action (on ENTER or mouse click)."
+  :group 'notmuch
+  :type '(choice (const :tag "Save part"
+   notmuch-show-part-button-sa

Re: [PATCH 1/1] Make buttons for attachments allow viewing as well as saving

[Austin Clements]

Quoth Mark Walters on Jan 17 at  9:06 am:
> 
> > > I wonder if the "problem" comes from me doing things in a non-lispy
> > > fashion (I am completely new to lisp). Thus
> > > notmuch-show-part-button-default-action is a variable that gets passed
> > > around rather than a function.
> > 
> > Sorry, I should have looked at the bigger context in this patch.  I
> > think Jameson was implying that notmuch-show-part-button-default
> > should change to
> > 
> > (defun notmuch-show-part-button-default (&optional button)
> >   (interactive)
> >   (funcall notmuch-show-part-button-default-action button))
> > 
> > I would go one step further and say that each action should probably
> > be a separate function.  That is, break notmuch-show-part-action into
> > separate functions and simply invoke the appropriate function, rather
> > than performing a fixed data dispatch.  This would be more flexible
> > and Lispy.  It may be that your approach works out better, but I'd at
> > least give this a shot.
> 
> I am happy to make that change. My original patch in the summer was more
> like that:
> id:"caludzswato+4mcuoomk+8vfs+pog-xuma6u-aqx2m6-sbyq...@mail.gmail.com"

Is this the right id?  I couldn't find it in the list archive.

> Is that more what you had in mind? (Only in broad terms: Obviously I
> would need to add in the customization and default function etc). I
> decided that I didn't like the code duplication (but I am completely new
> to lisp) which is why I changed it for this submission.

Yes, I wondered about this, too.  It seems like at worst the
notmuch-show-process-crypto stuff would be duplicated.  This might be
little enough that it's not worth worrying about, or it might be worth
introducing something like

(defun notmuch-with-temp-part-buffer (message-id nth action)
  (let ((process-crypto notmuch-show-process-crypto))
(with-temp-buffer
  (setq notmuch-show-process-crypto process-crypto)
  ;; Always acquires the part via `notmuch part', even if it is
  ;; available in the JSON output.
  (insert (notmuch-show-get-bodypart-internal message-id nth))
  (funcall action

You could also do this as a macro, but that definitely seems like
overkill.
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 1/1] Make buttons for attachments allow viewing as well as saving

[Mark Walters]

> > I wonder if the "problem" comes from me doing things in a non-lispy
> > fashion (I am completely new to lisp). Thus
> > notmuch-show-part-button-default-action is a variable that gets passed
> > around rather than a function.
> 
> Sorry, I should have looked at the bigger context in this patch.  I
> think Jameson was implying that notmuch-show-part-button-default
> should change to
> 
> (defun notmuch-show-part-button-default (&optional button)
>   (interactive)
>   (funcall notmuch-show-part-button-default-action button))
> 
> I would go one step further and say that each action should probably
> be a separate function.  That is, break notmuch-show-part-action into
> separate functions and simply invoke the appropriate function, rather
> than performing a fixed data dispatch.  This would be more flexible
> and Lispy.  It may be that your approach works out better, but I'd at
> least give this a shot.

I am happy to make that change. My original patch in the summer was more
like that:
id:"CALUdzSWAto+4mCUOOMk+8vFs+Pog-xUma6u-Aqx2M6-sbyQROg at mail.gmail.com"

Is that more what you had in mind? (Only in broad terms: Obviously I
would need to add in the customization and default function etc). I
decided that I didn't like the code duplication (but I am completely new
to lisp) which is why I changed it for this submission.

Best wishes

Mark

Re: [PATCH 1/1] Make buttons for attachments allow viewing as well as saving

[Mark Walters]

> > I wonder if the "problem" comes from me doing things in a non-lispy
> > fashion (I am completely new to lisp). Thus
> > notmuch-show-part-button-default-action is a variable that gets passed
> > around rather than a function.
> 
> Sorry, I should have looked at the bigger context in this patch.  I
> think Jameson was implying that notmuch-show-part-button-default
> should change to
> 
> (defun notmuch-show-part-button-default (&optional button)
>   (interactive)
>   (funcall notmuch-show-part-button-default-action button))
> 
> I would go one step further and say that each action should probably
> be a separate function.  That is, break notmuch-show-part-action into
> separate functions and simply invoke the appropriate function, rather
> than performing a fixed data dispatch.  This would be more flexible
> and Lispy.  It may be that your approach works out better, but I'd at
> least give this a shot.

I am happy to make that change. My original patch in the summer was more
like that:
id:"caludzswato+4mcuoomk+8vfs+pog-xuma6u-aqx2m6-sbyq...@mail.gmail.com"

Is that more what you had in mind? (Only in broad terms: Obviously I
would need to add in the customization and default function etc). I
decided that I didn't like the code duplication (but I am completely new
to lisp) which is why I changed it for this submission.

Best wishes

Mark
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 1/1] Make buttons for attachments allow viewing as well as saving

[Mark Walters]

(My apologies, owing to various errors on my part the patch has ended up
in a separate thread:
id:"1326749910-30437-1-git-send-email-markwalters1009 at gmail.com")

On Mon, 16 Jan 2012 11:31:16 -0800, Jameson Graef Rollins  wrote:
> > Keymap on a button is: s for save, v for view and o for view with
> > other program. Default (i.e. enter or mouse button) is save but is
> > easily configurable e.g. set to view with
> > (setq notmuch-show-part-button-default-action 
> > 'notmuch-show-part-view-action)
> 
> Actually, this should really be a defcustom.  Maybe something like this:
> 
> (defcustom notmuch-show-part-button-default-action 
> 'notmuch-show-part-button-save
>   "Default part header button action (on ENTER or mouse click)."
>   :group 'notmuch
>   :type '(choice (function :tag "Save part"
>   :value notmuch-show-part-button-save)
> (function :tag "View part"
>   :value notmuch-show-part-button-view)
> (function :tag "View interactively"
>   :value notmuch-show-part-button-interactively-view))
> 
> Unfortunately this isn't quite working right, since it's not setting the
> default properly, but if someone can help me figure out what I'm doing
> wrong, I think this is at least the right idea.

This did not work for me, nor did Austin's suggestion but the following
does
+(defcustom notmuch-show-part-button-default-action 
'notmuch-show-part-save-action
+  "Default part header button action (on ENTER or mouse click)."
+  :group 'notmuch
+  :type '(choice (const :tag "Save part"
+   notmuch-show-part-save-action)
+(const :tag "View part"
+   notmuch-show-part-view-action)
+(const :tag "View interactively"
+   notmuch-show-part-interactively-view-action)))

I wonder if the "problem" comes from me doing things in a non-lispy
fashion (I am completely new to lisp). Thus
notmuch-show-part-button-default-action is a variable that gets passed
around rather than a function.

> There's a white space at the end of this line, which produces the
> following git warning:
> 
>   Applying: Make buttons for attachments allow viewing as well as saving
>   /home/jrollins/src/notmuch/git/.git/rebase-apply/patch:96: trailing 
> whitespace.
>   (defvar notmuch-show-part-button-default-action 
> 'notmuch-show-part-save-action)
>   warning: 1 line adds whitespace errors.
> 
> So if you go with (an improved version of) my defcustom suggestion above
> you can kill two birds with one stone:

Right I have fixed this as above and think the whitespace is ok now (it
passes git diff --check). I intended to send the patch as a reply to
this email but it has ended up here:
id:"1326749910-30437-1-git-send-email-markwalters1009 at gmail.com"

Best wishes

Mark

[PATCH 1/1] Make buttons for attachments allow viewing as well as saving

[Austin Clements]

Quoth Mark Walters on Jan 16 at  9:44 pm:
> On Mon, 16 Jan 2012 11:31:16 -0800, Jameson Graef Rollins  finestructure.net> wrote:
> > > Keymap on a button is: s for save, v for view and o for view with
> > > other program. Default (i.e. enter or mouse button) is save but is
> > > easily configurable e.g. set to view with
> > > (setq notmuch-show-part-button-default-action 
> > > 'notmuch-show-part-view-action)
> > 
> > Actually, this should really be a defcustom.  Maybe something like this:
> > 
> > (defcustom notmuch-show-part-button-default-action 
> > 'notmuch-show-part-button-save
> >   "Default part header button action (on ENTER or mouse click)."
> >   :group 'notmuch
> >   :type '(choice (function :tag "Save part"
> >   :value notmuch-show-part-button-save)
> > (function :tag "View part"
> >   :value notmuch-show-part-button-view)
> > (function :tag "View interactively"
> >   :value 
> > notmuch-show-part-button-interactively-view))
> > 
> > Unfortunately this isn't quite working right, since it's not setting the
> > default properly, but if someone can help me figure out what I'm doing
> > wrong, I think this is at least the right idea.
> 
> This did not work for me, nor did Austin's suggestion but the following
> does
> +(defcustom notmuch-show-part-button-default-action 
> 'notmuch-show-part-save-action
> +  "Default part header button action (on ENTER or mouse click)."
> +  :group 'notmuch
> +  :type '(choice (const :tag "Save part"
> +   notmuch-show-part-save-action)
> +(const :tag "View part"
> +   notmuch-show-part-view-action)
> +(const :tag "View interactively"
> +   notmuch-show-part-interactively-view-action)))
> 
> I wonder if the "problem" comes from me doing things in a non-lispy
> fashion (I am completely new to lisp). Thus
> notmuch-show-part-button-default-action is a variable that gets passed
> around rather than a function.

Sorry, I should have looked at the bigger context in this patch.  I
think Jameson was implying that notmuch-show-part-button-default
should change to

(defun notmuch-show-part-button-default (&optional button)
  (interactive)
  (funcall notmuch-show-part-button-default-action button))

I would go one step further and say that each action should probably
be a separate function.  That is, break notmuch-show-part-action into
separate functions and simply invoke the appropriate function, rather
than performing a fixed data dispatch.  This would be more flexible
and Lispy.  It may be that your approach works out better, but I'd at
least give this a shot.

Re: [PATCH 1/1] Make buttons for attachments allow viewing as well as saving

[Austin Clements]

Quoth Mark Walters on Jan 16 at  9:44 pm:
> On Mon, 16 Jan 2012 11:31:16 -0800, Jameson Graef Rollins 
>  wrote:
> > > Keymap on a button is: s for save, v for view and o for view with
> > > other program. Default (i.e. enter or mouse button) is save but is
> > > easily configurable e.g. set to view with
> > > (setq notmuch-show-part-button-default-action 
> > > 'notmuch-show-part-view-action)
> > 
> > Actually, this should really be a defcustom.  Maybe something like this:
> > 
> > (defcustom notmuch-show-part-button-default-action 
> > 'notmuch-show-part-button-save
> >   "Default part header button action (on ENTER or mouse click)."
> >   :group 'notmuch
> >   :type '(choice (function :tag "Save part"
> >   :value notmuch-show-part-button-save)
> > (function :tag "View part"
> >   :value notmuch-show-part-button-view)
> > (function :tag "View interactively"
> >   :value 
> > notmuch-show-part-button-interactively-view))
> > 
> > Unfortunately this isn't quite working right, since it's not setting the
> > default properly, but if someone can help me figure out what I'm doing
> > wrong, I think this is at least the right idea.
> 
> This did not work for me, nor did Austin's suggestion but the following
> does
> +(defcustom notmuch-show-part-button-default-action 
> 'notmuch-show-part-save-action
> +  "Default part header button action (on ENTER or mouse click)."
> +  :group 'notmuch
> +  :type '(choice (const :tag "Save part"
> +   notmuch-show-part-save-action)
> +(const :tag "View part"
> +   notmuch-show-part-view-action)
> +(const :tag "View interactively"
> +   notmuch-show-part-interactively-view-action)))
> 
> I wonder if the "problem" comes from me doing things in a non-lispy
> fashion (I am completely new to lisp). Thus
> notmuch-show-part-button-default-action is a variable that gets passed
> around rather than a function.

Sorry, I should have looked at the bigger context in this patch.  I
think Jameson was implying that notmuch-show-part-button-default
should change to

(defun notmuch-show-part-button-default (&optional button)
  (interactive)
  (funcall notmuch-show-part-button-default-action button))

I would go one step further and say that each action should probably
be a separate function.  That is, break notmuch-show-part-action into
separate functions and simply invoke the appropriate function, rather
than performing a fixed data dispatch.  This would be more flexible
and Lispy.  It may be that your approach works out better, but I'd at
least give this a shot.
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 1/1] Make buttons for attachments allow viewing as well as saving

[Austin Clements]

Quoth Jameson Graef Rollins on Jan 16 at 11:31 am:
> On Sun, 15 Jan 2012 12:16:36 +, Mark Walters  gmail.com> wrote:
> > Define a keymap for attachment buttons to allow multiple actions.
> > Define 3 possible actions:
> > save attachment: exactly as currently,
> > view attachment: uses mailcap entry,
> > view attachment with user chosen program
> 
> Great improvement, Mark!  Thanks for this.  I've been wanting this kind
> of functionality for a while, actually, and this is a really great
> implementation.  It works like a charm, and the code looks good to me,
> modulo a couple small comments below.
> 
> > Keymap on a button is: s for save, v for view and o for view with
> > other program. Default (i.e. enter or mouse button) is save but is
> > easily configurable e.g. set to view with
> > (setq notmuch-show-part-button-default-action 
> > 'notmuch-show-part-view-action)
> 
> Actually, this should really be a defcustom.  Maybe something like this:
> 
> (defcustom notmuch-show-part-button-default-action 
> 'notmuch-show-part-button-save
>   "Default part header button action (on ENTER or mouse click)."
>   :group 'notmuch
>   :type '(choice (function :tag "Save part"
>   :value notmuch-show-part-button-save)
> (function :tag "View part"
>   :value notmuch-show-part-button-view)
> (function :tag "View interactively"
>   :value notmuch-show-part-button-interactively-view))
> 
> Unfortunately this isn't quite working right, since it's not setting the
> default properly, but if someone can help me figure out what I'm doing
> wrong, I think this is at least the right idea.

Jamie's defcustom doesn't work for me either (apparently it works even
less for me than it does for Jamie), but the following works for me

(defcustom notmuch-show-part-button-default-action 
'notmuch-show-part-button-save
  "Default part header button action (on ENTER or mouse click)."
  :group 'notmuch
  :type '(choice (const :tag "Save part"
notmuch-show-part-button-save)
 (const :tag "View part"
notmuch-show-part-button-view)
 (const :tag "View interactively"
notmuch-show-part-button-interactively-view)))

Re: [PATCH 1/1] Make buttons for attachments allow viewing as well as saving

[Mark Walters]

(My apologies, owing to various errors on my part the patch has ended up
in a separate thread:
id:"1326749910-30437-1-git-send-email-markwalters1...@gmail.com")

On Mon, 16 Jan 2012 11:31:16 -0800, Jameson Graef Rollins 
 wrote:
> > Keymap on a button is: s for save, v for view and o for view with
> > other program. Default (i.e. enter or mouse button) is save but is
> > easily configurable e.g. set to view with
> > (setq notmuch-show-part-button-default-action 
> > 'notmuch-show-part-view-action)
> 
> Actually, this should really be a defcustom.  Maybe something like this:
> 
> (defcustom notmuch-show-part-button-default-action 
> 'notmuch-show-part-button-save
>   "Default part header button action (on ENTER or mouse click)."
>   :group 'notmuch
>   :type '(choice (function :tag "Save part"
>   :value notmuch-show-part-button-save)
> (function :tag "View part"
>   :value notmuch-show-part-button-view)
> (function :tag "View interactively"
>   :value notmuch-show-part-button-interactively-view))
> 
> Unfortunately this isn't quite working right, since it's not setting the
> default properly, but if someone can help me figure out what I'm doing
> wrong, I think this is at least the right idea.

This did not work for me, nor did Austin's suggestion but the following
does
+(defcustom notmuch-show-part-button-default-action 
'notmuch-show-part-save-action
+  "Default part header button action (on ENTER or mouse click)."
+  :group 'notmuch
+  :type '(choice (const :tag "Save part"
+   notmuch-show-part-save-action)
+(const :tag "View part"
+   notmuch-show-part-view-action)
+(const :tag "View interactively"
+   notmuch-show-part-interactively-view-action)))

I wonder if the "problem" comes from me doing things in a non-lispy
fashion (I am completely new to lisp). Thus
notmuch-show-part-button-default-action is a variable that gets passed
around rather than a function.

> There's a white space at the end of this line, which produces the
> following git warning:
> 
>   Applying: Make buttons for attachments allow viewing as well as saving
>   /home/jrollins/src/notmuch/git/.git/rebase-apply/patch:96: trailing 
> whitespace.
>   (defvar notmuch-show-part-button-default-action 
> 'notmuch-show-part-save-action)
>   warning: 1 line adds whitespace errors.
> 
> So if you go with (an improved version of) my defcustom suggestion above
> you can kill two birds with one stone:

Right I have fixed this as above and think the whitespace is ok now (it
passes git diff --check). I intended to send the patch as a reply to
this email but it has ended up here:
id:"1326749910-30437-1-git-send-email-markwalters1...@gmail.com"

Best wishes

Mark

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 1/1] Make buttons for attachments allow viewing as well as saving

[Austin Clements]

Quoth Jameson Graef Rollins on Jan 16 at 11:31 am:
> On Sun, 15 Jan 2012 12:16:36 +, Mark Walters  
> wrote:
> > Define a keymap for attachment buttons to allow multiple actions.
> > Define 3 possible actions:
> > save attachment: exactly as currently,
> > view attachment: uses mailcap entry,
> > view attachment with user chosen program
> 
> Great improvement, Mark!  Thanks for this.  I've been wanting this kind
> of functionality for a while, actually, and this is a really great
> implementation.  It works like a charm, and the code looks good to me,
> modulo a couple small comments below.
> 
> > Keymap on a button is: s for save, v for view and o for view with
> > other program. Default (i.e. enter or mouse button) is save but is
> > easily configurable e.g. set to view with
> > (setq notmuch-show-part-button-default-action 
> > 'notmuch-show-part-view-action)
> 
> Actually, this should really be a defcustom.  Maybe something like this:
> 
> (defcustom notmuch-show-part-button-default-action 
> 'notmuch-show-part-button-save
>   "Default part header button action (on ENTER or mouse click)."
>   :group 'notmuch
>   :type '(choice (function :tag "Save part"
>   :value notmuch-show-part-button-save)
> (function :tag "View part"
>   :value notmuch-show-part-button-view)
> (function :tag "View interactively"
>   :value notmuch-show-part-button-interactively-view))
> 
> Unfortunately this isn't quite working right, since it's not setting the
> default properly, but if someone can help me figure out what I'm doing
> wrong, I think this is at least the right idea.

Jamie's defcustom doesn't work for me either (apparently it works even
less for me than it does for Jamie), but the following works for me

(defcustom notmuch-show-part-button-default-action 
'notmuch-show-part-button-save
  "Default part header button action (on ENTER or mouse click)."
  :group 'notmuch
  :type '(choice (const :tag "Save part"
notmuch-show-part-button-save)
 (const :tag "View part"
notmuch-show-part-button-view)
 (const :tag "View interactively"
notmuch-show-part-button-interactively-view)))
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 1/1] Make buttons for attachments allow viewing as well as saving

[Jameson Graef Rollins]

On Sun, 15 Jan 2012 12:16:36 +, Mark Walters  
wrote:
> Define a keymap for attachment buttons to allow multiple actions.
> Define 3 possible actions:
> save attachment: exactly as currently,
> view attachment: uses mailcap entry,
> view attachment with user chosen program

Great improvement, Mark!  Thanks for this.  I've been wanting this kind
of functionality for a while, actually, and this is a really great
implementation.  It works like a charm, and the code looks good to me,
modulo a couple small comments below.

> Keymap on a button is: s for save, v for view and o for view with
> other program. Default (i.e. enter or mouse button) is save but is
> easily configurable e.g. set to view with
> (setq notmuch-show-part-button-default-action 'notmuch-show-part-view-action)

Actually, this should really be a defcustom.  Maybe something like this:

(defcustom notmuch-show-part-button-default-action 
'notmuch-show-part-button-save
  "Default part header button action (on ENTER or mouse click)."
  :group 'notmuch
  :type '(choice (function :tag "Save part"
  :value notmuch-show-part-button-save)
(function :tag "View part"
  :value notmuch-show-part-button-view)
(function :tag "View interactively"
  :value notmuch-show-part-button-interactively-view))

Unfortunately this isn't quite working right, since it's not setting the
default properly, but if someone can help me figure out what I'm doing
wrong, I think this is at least the right idea.

> One implementation detail: the view attachment function forces all
> attachments to be "displayed" using mailcap even if emacs could
> display them itself. Thus, for example, text/html appears in a browser
> and text/plain asks whether to save (on a standard debian setup)

I think this is good.

> +(defvar notmuch-show-part-button-default-action 
> 'notmuch-show-part-save-action) 

There's a white space at the end of this line, which produces the
following git warning:

  Applying: Make buttons for attachments allow viewing as well as saving
  /home/jrollins/src/notmuch/git/.git/rebase-apply/patch:96: trailing 
whitespace.
  (defvar notmuch-show-part-button-default-action 
'notmuch-show-part-save-action)
  warning: 1 line adds whitespace errors.

So if you go with (an improved version of) my defcustom suggestion above
you can kill two birds with one stone:

-(defvar notmuch-show-part-button-default-action 
'notmuch-show-part-save-action) 
+(defcustom notmuch-show-part-button-default-action '(notmuch-show-part-but=
ton-save)
+  "Default part header button action (on ENTER or mouse click)."
+  :group 'notmuch
+  :type '(choice (function :tag "Save part"
+  :value notmuch-show-part-button-save)
+(function :tag "View part"
+  :value notmuch-show-part-button-view)
+(function :tag "View interactively"
+  :value notmuch-show-part-button-interactively-view))

jamie.


pgpcjXUv1q1NO.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 1/1] Make buttons for attachments allow viewing as well as saving

[Jameson Graef Rollins]

On Sun, 15 Jan 2012 12:16:36 +, Mark Walters  
wrote:
> Define a keymap for attachment buttons to allow multiple actions.
> Define 3 possible actions:
> save attachment: exactly as currently,
> view attachment: uses mailcap entry,
> view attachment with user chosen program

Great improvement, Mark!  Thanks for this.  I've been wanting this kind
of functionality for a while, actually, and this is a really great
implementation.  It works like a charm, and the code looks good to me,
modulo a couple small comments below.

> Keymap on a button is: s for save, v for view and o for view with
> other program. Default (i.e. enter or mouse button) is save but is
> easily configurable e.g. set to view with
> (setq notmuch-show-part-button-default-action 'notmuch-show-part-view-action)

Actually, this should really be a defcustom.  Maybe something like this:

(defcustom notmuch-show-part-button-default-action 
'notmuch-show-part-button-save
  "Default part header button action (on ENTER or mouse click)."
  :group 'notmuch
  :type '(choice (function :tag "Save part"
  :value notmuch-show-part-button-save)
(function :tag "View part"
  :value notmuch-show-part-button-view)
(function :tag "View interactively"
  :value notmuch-show-part-button-interactively-view))

Unfortunately this isn't quite working right, since it's not setting the
default properly, but if someone can help me figure out what I'm doing
wrong, I think this is at least the right idea.

> One implementation detail: the view attachment function forces all
> attachments to be "displayed" using mailcap even if emacs could
> display them itself. Thus, for example, text/html appears in a browser
> and text/plain asks whether to save (on a standard debian setup)

I think this is good.

> +(defvar notmuch-show-part-button-default-action 
> 'notmuch-show-part-save-action) 

There's a white space at the end of this line, which produces the
following git warning:

  Applying: Make buttons for attachments allow viewing as well as saving
  /home/jrollins/src/notmuch/git/.git/rebase-apply/patch:96: trailing 
whitespace.
  (defvar notmuch-show-part-button-default-action 
'notmuch-show-part-save-action)
  warning: 1 line adds whitespace errors.

So if you go with (an improved version of) my defcustom suggestion above
you can kill two birds with one stone:

-(defvar notmuch-show-part-button-default-action 
'notmuch-show-part-save-action) 
+(defcustom notmuch-show-part-button-default-action '(notmuch-show-part-but=
ton-save)
+  "Default part header button action (on ENTER or mouse click)."
+  :group 'notmuch
+  :type '(choice (function :tag "Save part"
+  :value notmuch-show-part-button-save)
+(function :tag "View part"
+  :value notmuch-show-part-button-view)
+(function :tag "View interactively"
+  :value notmuch-show-part-button-interactively-view))

jamie.
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: not available
URL: 

[PATCH 1/1] Make buttons for attachments allow viewing as well as saving

[Mark Walters]

Define a keymap for attachment buttons to allow multiple actions.
Define 3 possible actions:
save attachment: exactly as currently,
view attachment: uses mailcap entry,
view attachment with user chosen program

Keymap on a button is: s for save, v for view and o for view with
other program. Default (i.e. enter or mouse button) is save but is
easily configurable e.g. set to view with
(setq notmuch-show-part-button-default-action 'notmuch-show-part-view-action)

One implementation detail: the view attachment function forces all
attachments to be "displayed" using mailcap even if emacs could
display them itself. Thus, for example, text/html appears in a browser
and text/plain asks whether to save (on a standard debian setup)
---
 emacs/notmuch-show.el |   81 ++--
 1 files changed, 64 insertions(+), 17 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 03c1f6b..a1c0e63 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -281,10 +281,21 @@ message at DEPTH in the current thread."
(run-hooks 'notmuch-show-markup-headers-hook)

 (define-button-type 'notmuch-show-part-button-type
-  'action 'notmuch-show-part-button-action
+  'action 'notmuch-show-part-button-default
+  'keymap 'notmuch-show-part-button-map
   'follow-link t
   'face 'message-mml)

+(defvar notmuch-show-part-button-map
+  (let ((map (make-sparse-keymap)))
+   (set-keymap-parent map button-map)
+   (define-key map "s" 'notmuch-show-part-button-save)
+   (define-key map "v" 'notmuch-show-part-button-view)
+   (define-key map "o" 'notmuch-show-part-button-interactively-view)
+map)
+  "Submap for button commands")
+(fset 'notmuch-show-part-button-map notmuch-show-part-button-map)
+
 (defun notmuch-show-insert-part-header (nth content-type declared-type 
&optional name comment)
   (let ((button))
 (setq button
@@ -299,29 +310,43 @@ message at DEPTH in the current thread."
   " ]")
   :type 'notmuch-show-part-button-type
   :notmuch-part nth
-  :notmuch-filename name))
+  :notmuch-filename name
+  :notmuch-content-type content-type))
 (insert "\n")
 ;; return button
 button))

 ;; Functions handling particular MIME parts.

+;; this function is kept for the tests and any external users
 (defun notmuch-show-save-part (message-id nth &optional filename)
+  (notmuch-show-part-action 'notmuch-show-part-save-action message-id nth nil 
filename))
+
+(defun notmuch-show-part-action (action message-id nth content-type &optional 
filename)
   (let ((process-crypto notmuch-show-process-crypto))
 (with-temp-buffer
   (setq notmuch-show-process-crypto process-crypto)
   ;; Always acquires the part via `notmuch part', even if it is
   ;; available in the JSON output.
   (insert (notmuch-show-get-bodypart-internal 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)
+  (cond ((eq action 'notmuch-show-part-save-action)
+(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)))
+   ((eq action 'notmuch-show-part-view-action)
+;; 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))
+  (mm-display-part handle)))
+   ((eq action 'notmuch-show-part-interactively-view-action)
+(let ((handle (mm-make-handle (current-buffer) (list 
content-type
+  (mm-interactively-view-part handle)))

 (defun notmuch-show-mm-display-part-inline (msg part nth content-type)
   "Use the mm-decode/mm-view functions to display a part in the
@@ -1502,12 +1527,34 @@ buffer."

 ;; Commands typically bound to buttons.

-(defun notmuch-show-part-button-action (button)
-  (let ((nth (button-get button :notmuch-part)))
-(if nth
-   (notmuch-show-save-part (notmuch-show-get-message-id) nth
-   (button-get button :notmuch-filename))
-  (message "Not a valid part (is it a fake pa

[PATCH 1/1] Make buttons for attachments allow viewing as well as saving

[Mark Walters]

Define a keymap for attachment buttons to allow multiple actions.
Define 3 possible actions:
save attachment: exactly as currently,
view attachment: uses mailcap entry,
view attachment with user chosen program

Keymap on a button is: s for save, v for view and o for view with
other program. Default (i.e. enter or mouse button) is save but is
easily configurable e.g. set to view with
(setq notmuch-show-part-button-default-action 'notmuch-show-part-view-action)

One implementation detail: the view attachment function forces all
attachments to be "displayed" using mailcap even if emacs could
display them itself. Thus, for example, text/html appears in a browser
and text/plain asks whether to save (on a standard debian setup)
---
 emacs/notmuch-show.el |   81 ++--
 1 files changed, 64 insertions(+), 17 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 03c1f6b..a1c0e63 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -281,10 +281,21 @@ message at DEPTH in the current thread."
(run-hooks 'notmuch-show-markup-headers-hook)
 
 (define-button-type 'notmuch-show-part-button-type
-  'action 'notmuch-show-part-button-action
+  'action 'notmuch-show-part-button-default
+  'keymap 'notmuch-show-part-button-map
   'follow-link t
   'face 'message-mml)
 
+(defvar notmuch-show-part-button-map
+  (let ((map (make-sparse-keymap)))
+   (set-keymap-parent map button-map)
+   (define-key map "s" 'notmuch-show-part-button-save)
+   (define-key map "v" 'notmuch-show-part-button-view)
+   (define-key map "o" 'notmuch-show-part-button-interactively-view)
+map)
+  "Submap for button commands")
+(fset 'notmuch-show-part-button-map notmuch-show-part-button-map)
+
 (defun notmuch-show-insert-part-header (nth content-type declared-type 
&optional name comment)
   (let ((button))
 (setq button
@@ -299,29 +310,43 @@ message at DEPTH in the current thread."
   " ]")
   :type 'notmuch-show-part-button-type
   :notmuch-part nth
-  :notmuch-filename name))
+  :notmuch-filename name
+  :notmuch-content-type content-type))
 (insert "\n")
 ;; return button
 button))
 
 ;; Functions handling particular MIME parts.
 
+;; this function is kept for the tests and any external users
 (defun notmuch-show-save-part (message-id nth &optional filename)
+  (notmuch-show-part-action 'notmuch-show-part-save-action message-id nth nil 
filename))
+
+(defun notmuch-show-part-action (action message-id nth content-type &optional 
filename)
   (let ((process-crypto notmuch-show-process-crypto))
 (with-temp-buffer
   (setq notmuch-show-process-crypto process-crypto)
   ;; Always acquires the part via `notmuch part', even if it is
   ;; available in the JSON output.
   (insert (notmuch-show-get-bodypart-internal 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)
+  (cond ((eq action 'notmuch-show-part-save-action)
+(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)))
+   ((eq action 'notmuch-show-part-view-action)
+;; 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))
+  (mm-display-part handle)))
+   ((eq action 'notmuch-show-part-interactively-view-action)
+(let ((handle (mm-make-handle (current-buffer) (list 
content-type
+  (mm-interactively-view-part handle)))
 
 (defun notmuch-show-mm-display-part-inline (msg part nth content-type)
   "Use the mm-decode/mm-view functions to display a part in the
@@ -1502,12 +1527,34 @@ buffer."
 
 ;; Commands typically bound to buttons.
 
-(defun notmuch-show-part-button-action (button)
-  (let ((nth (button-get button :notmuch-part)))
-(if nth
-   (notmuch-show-save-part (notmuch-show-get-message-id) nth
-   (button-get button :notmuch-filename))
-  (message "Not a valid part (is it a