[PATCH 0/4] emacs: Part command improvements

[PATCH 0/4] emacs: Part command improvements

[Austin Clements]

This is a follow-up of sorts to id:"8761ycc19t.fsf@qmul.ac.uk", where
Mark suggested that the part handling commands could all use the
correponding mm-* functions.  I ran with the idea and wound up with
this series, which, in addition to standardizing on the mm-* functions
for everything and simplifying the implementation overall, decouples
the part commands from part buttons, which removes an entire layer
from the implementation and adds the ability to invoke part commands
with point anywhere in a part (something I often find myself wanting).

[PATCH 1/4] emacs: Retain text properties when toggling buttons

[Austin Clements]

Previously, we lost any text properties applied to part buttons or
wash buttons when they were toggled because `insert' directly copies
the text properties of the string being inserted.  Fix this by
capturing the properties applied to the button beforehand and
re-applying them after inserting the new text.
---
 emacs/notmuch-show.el |    2 ++
 emacs/notmuch-wash.el |    2 ++
 2 files changed, 4 insertions(+)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index b0a8d8a..a080134 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -559,10 +559,12 @@ message at DEPTH in the current thread."
 	     (new-start (button-start button))
 	     (button-label (button-get button :base-label))
 	     (old-point (point))
+	     (properties (text-properties-at (point)))
 	     (inhibit-read-only t))
 	(overlay-put overlay 'invisible (not show))
 	(goto-char new-start)
 	(insert "[ " button-label (if show " ]" " (hidden) ]"))
+	(set-text-properties new-start (point) properties)
 	(let ((old-end (button-end button)))
 	  (move-overlay button new-start (point))
 	  (delete-region (point) old-end))
diff --git a/emacs/notmuch-wash.el b/emacs/notmuch-wash.el
index 8a68819..8fe91e1 100644
--- a/emacs/notmuch-wash.el
+++ b/emacs/notmuch-wash.el
@@ -104,9 +104,11 @@ lower).")
 	 (overlay (button-get cite-button 'overlay))
 	 (button-label (notmuch-wash-button-label overlay))
 	 (old-point (point))
+	 (properties (text-properties-at (point)))
 	 (inhibit-read-only t))
     (goto-char new-start)
     (insert button-label)
+    (set-text-properties new-start (point) properties)
     (let ((old-end (button-end cite-button)))
       (move-overlay cite-button new-start (point))
       (delete-region (point) old-end))
-- 
1.7.10.4

[PATCH 2/4] emacs: Record part p-list in a text property

[Austin Clements]

This is similar to what we already do with the message p-list, though
we apply the part's text property to the whole part's text, in
contrast with the message p-list, which is (rather obscurely) only
applied to the first character.
---
 emacs/notmuch-lib.el  |   16 ++++++++++++++++
 emacs/notmuch-show.el |   13 ++++++++++++-
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 790136e..09ce25e 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -360,6 +360,22 @@ OBJECT."
    below
    string))
 
+(defun notmuch-put-text-property-if-nil (start end property value
+					       &optional object)
+  "Like `put-text-property', but only set the property where it is nil."
+  (while (< start end)
+    (let ((start-nil (text-property-any start end property nil object)))
+      (if (null start-nil)
+	  ;; There are no more nil regions; exit the loop
+	  (setq start end)
+	;; Find the end of the nil region
+	(let ((end-nil
+	       (or (text-property-not-all start-nil end property nil object)
+		   end)))
+	  ;; Set the property
+	  (put-text-property start-nil end-nil property value object)
+	  (setq start end-nil))))))
+
 (defun notmuch-logged-error (msg &optional extra)
   "Log MSG and EXTRA to *Notmuch errors* and signal MSG.
 
diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index a080134..acd0b55 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -900,7 +900,10 @@ If HIDE is non-nil then initially hide this part."
     ;; Ensure that the part ends with a carriage return.
     (unless (bolp)
       (insert "\n"))
-    (notmuch-show-create-part-overlays msg beg (point) hide)))
+    (notmuch-show-create-part-overlays msg beg (point) hide)
+    ;; Record part information.  Since we already inserted subparts,
+    ;; don't override exiting :notmuch-part properties.
+    (notmuch-put-text-property-if-nil beg (point) :notmuch-part part)))
 
 (defun notmuch-show-insert-body (msg body depth)
   "Insert the body BODY at depth DEPTH in the current thread."
@@ -1404,6 +1407,14 @@ Some useful entries are:
     (notmuch-show-move-to-message-top)
     (get-text-property (point) :notmuch-message-properties)))
 
+(defun notmuch-show-get-part-properties ()
+  "Return the properties of the part containing point.
+
+This is the part property list retrieved from the CLI.  Signals
+an error if there is no part containing point."
+  (or (get-text-property (point) :notmuch-part)
+      (error "No message part here")))
+
 (defun notmuch-show-set-prop (prop val &optional props)
   (let ((inhibit-read-only t)
 	(props (or props
-- 
1.7.10.4

Re: [PATCH 2/4] emacs: Record part p-list in a text property

[Mark Walters]

A couple of small nits:

Austin Clements <amdragon@MIT.EDU> writes:

> This is similar to what we already do with the message p-list, though
> we apply the part's text property to the whole part's text, in
> contrast with the message p-list, which is (rather obscurely) only
> applied to the first character.
> ---
>  emacs/notmuch-lib.el  |   16 ++++++++++++++++
>  emacs/notmuch-show.el |   13 ++++++++++++-
>  2 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index 790136e..09ce25e 100644
> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -360,6 +360,22 @@ OBJECT."
>     below
>     string))
>  
> +(defun notmuch-put-text-property-if-nil (start end property value
> +					       &optional object)
> +  "Like `put-text-property', but only set the property where it is nil."
> +  (while (< start end)
> +    (let ((start-nil (text-property-any start end property nil object)))
> +      (if (null start-nil)
> +	  ;; There are no more nil regions; exit the loop
> +	  (setq start end)
> +	;; Find the end of the nil region
> +	(let ((end-nil
> +	       (or (text-property-not-all start-nil end property nil object)
> +		   end)))
> +	  ;; Set the property
> +	  (put-text-property start-nil end-nil property value object)
> +	  (setq start end-nil))))))
> +
>  (defun notmuch-logged-error (msg &optional extra)
>    "Log MSG and EXTRA to *Notmuch errors* and signal MSG.
>  
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index a080134..acd0b55 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -900,7 +900,10 @@ If HIDE is non-nil then initially hide this part."
>      ;; Ensure that the part ends with a carriage return.
>      (unless (bolp)
>        (insert "\n"))
> -    (notmuch-show-create-part-overlays msg beg (point) hide)))
> +    (notmuch-show-create-part-overlays msg beg (point) hide)
> +    ;; Record part information.  Since we already inserted subparts,
> +    ;; don't override exiting :notmuch-part properties.
                         ^^^
existing

> +    (notmuch-put-text-property-if-nil beg (point) :notmuch-part part)))
>  
>  (defun notmuch-show-insert-body (msg body depth)
>    "Insert the body BODY at depth DEPTH in the current thread."
> @@ -1404,6 +1407,14 @@ Some useful entries are:
>      (notmuch-show-move-to-message-top)
>      (get-text-property (point) :notmuch-message-properties)))
>  
> +(defun notmuch-show-get-part-properties ()
> +  "Return the properties of the part containing point.
> +
> +This is the part property list retrieved from the CLI.  Signals
> +an error if there is no part containing point."

I think something here should say innermost part or something similar.

Best wishes

Mark


> +  (or (get-text-property (point) :notmuch-part)
> +      (error "No message part here")))
> +
>  (defun notmuch-show-set-prop (prop val &optional props)
>    (let ((inhibit-read-only t)
>  	(props (or props
> -- 
> 1.7.10.4

[PATCH 3/4] emacs: Simplify MIME part command implementation

[Austin Clements]

This unifies the part button actions and the underlying part action
functions into single interactive command that simply applies to the
part containing point using the just-added part p-list text property
instead of button properties.  Since all part actions can be performed
by applying the appropriate mm function to an mm-handle, this patch
abstracts out the creation of mm handles, making the implementations
of the part commands trivial.  This also eliminates our special
handling for part save in favor of using the appropriate mm function.

This necessarily modifies the way we handle the default part button
action, but in a way that does not change the meaning of the
notmuch-show-part-button-default-action defcustom.

Since these commands are no longer specific to buttons, this patch
eliminates the extra metadata stored with each button.  This also
eliminates one rather special-purpose macro for a collection of
general purpose part handling utilities.
---
 emacs/notmuch-show.el |  133 +++++++++++++++++++++----------------------------
 test/emacs            |    4 +-
 2 files changed, 61 insertions(+), 76 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index acd0b55..b33d92d 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -474,10 +474,10 @@ message at DEPTH in the current thread."
 (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)
-    (define-key map "|" 'notmuch-show-part-button-pipe)
+    (define-key map "s" 'notmuch-show-save-part)
+    (define-key map "v" 'notmuch-show-view-part)
+    (define-key map "o" 'notmuch-show-interactively-view-part)
+    (define-key map "|" 'notmuch-show-pipe-part)
     map)
   "Submap for button commands")
 (fset 'notmuch-show-part-button-map notmuch-show-part-button-map)
@@ -494,61 +494,11 @@ message at DEPTH in the current thread."
 	  (insert-button
 	   (concat "[ " base-label " ]")
 	   :base-label base-label
-	   :type 'notmuch-show-part-button-type
-	   :notmuch-part nth
-	   :notmuch-filename name
-	   :notmuch-content-type content-type))
+	   :type 'notmuch-show-part-button-type))
     (insert "\n")
     ;; return button
     button))
 
-;; Functions handling particular MIME parts.
-
-(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-get-bodypart-internal ,message-id ,nth notmuch-show-process-crypto))
-	 ,@body))))
-
-(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))))
-
-(defun notmuch-show-view-part (message-id nth &optional filename content-type )
-  (notmuch-with-temp-part-buffer message-id nth
-    (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
-    (let ((handle (mm-make-handle (current-buffer) (list content-type))))
-      (mm-interactively-view-part handle))))
-
-(defun notmuch-show-pipe-part (message-id nth &optional filename content-type)
-  (notmuch-with-temp-part-buffer message-id nth
-    (let ((handle (mm-make-handle (current-buffer) (list content-type))))
-      (mm-pipe-part handle))))
-
 ;; This is taken from notmuch-wash: maybe it should be unified?
 (defun notmuch-show-toggle-part-invisibility (&optional button)
   (interactive)
@@ -570,6 +520,8 @@ message at DEPTH in the current thread."
 	  (delete-region (point) old-end))
 	(goto-char (min old-point (1- (button-end button))))))))
 
+;; MIME part renderers
+
 (defun notmuch-show-multipart/*-to-list (part)
   (mapcar (lambda (inner-part) (plist-get inner-part :content-type))
 	  (plist-get part :content)))
@@ -2016,40 +1968,71 @@ the user (see `notmuch-show-stash-mlarchive-link-alist')."
   (notmuch-show-stash-mlarchive-link mla)
   (browse-url (current-kill 0 t)))
 
-;; Commands typically bound to buttons.
+;; Interactive part functions and their helpers
+
+(defun notmuch-show-generate-part-buffer (message-id nth)
+  "Return a temporary buffer containing the specified part's content."
+  (let ((buf (generate-new-buffer " *notmuch-part*"))
+	(process-crypto notmuch-show-process-crypto))
+    (with-current-buffer buf
+      (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-get-bodypart-internal message-id nth notmuch-show-process-crypto)))
+    buf))
+
+(defun notmuch-show-current-part-handle ()
+  "Return an mm-handle for the part containing point.
+
+This creates a temporary buffer for the part's content; the
+caller is responsible for killing this buffer as appropriate."
+  (let* ((part (notmuch-show-get-part-properties))
+	 (message-id (notmuch-show-get-message-id))
+	 (nth (plist-get part :id))
+	 (buf (notmuch-show-generate-part-buffer message-id nth))
+	 (content-type (plist-get part :content-type))
+	 (filename (plist-get part :filename))
+	 (disposition (if filename `(attachment (filename . ,filename)))))
+    (mm-make-handle buf (list content-type) nil nil disposition)))
+
+(defun notmuch-show-apply-to-current-part-handle (fn)
+  "Apply FN to an mm-handle for the part containing point.
+
+This ensures that the temporary buffer created for the mm-handle
+is destroyed when FN returns."
+  (let ((handle (notmuch-show-current-part-handle)))
+    (unwind-protect
+	(funcall fn handle)
+      (kill-buffer (mm-handle-buffer handle)))))
 
 (defun notmuch-show-part-button-default (&optional button)
   (interactive)
   (let ((button (or button (button-at (point)))))
     (if (button-get button 'overlay)
 	(notmuch-show-toggle-part-invisibility button)
-      (notmuch-show-part-button-internal button notmuch-show-part-button-default-action))))
+      (call-interactively notmuch-show-part-button-default-action))))
 
-(defun notmuch-show-part-button-save (&optional button)
+(defun notmuch-show-save-part ()
+  "Save the MIME part containing point to a file."
   (interactive)
-  (notmuch-show-part-button-internal button #'notmuch-show-save-part))
+  (notmuch-show-apply-to-current-part-handle #'mm-save-part))
 
-(defun notmuch-show-part-button-view (&optional button)
+(defun notmuch-show-view-part ()
+  "View the MIME part containing point in an external viewer."
   (interactive)
-  (notmuch-show-part-button-internal button #'notmuch-show-view-part))
+  ;; Set mm-inlined-types to nil to force an external viewer
+  (let ((mm-inlined-types nil))
+    (notmuch-show-apply-to-current-part-handle #'mm-display-part)))
 
-(defun notmuch-show-part-button-interactively-view (&optional button)
+(defun notmuch-show-interactively-view-part ()
+  "View the MIME part containing point, prompting for a viewer."
   (interactive)
-  (notmuch-show-part-button-internal button #'notmuch-show-interactively-view-part))
+  (notmuch-show-apply-to-current-part-handle #'mm-interactively-view-part))
 
-(defun notmuch-show-part-button-pipe (&optional button)
+(defun notmuch-show-pipe-part ()
+  "Pipe the MIME part containing point to an external command."
   (interactive)
-  (notmuch-show-part-button-internal button #'notmuch-show-pipe-part))
+  (notmuch-show-apply-to-current-part-handle #'mm-pipe-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-filename)
-		       (button-get button :notmuch-content-type)))))))
-
-;;
 
 (provide 'notmuch-show)
diff --git a/test/emacs b/test/emacs
index f033bdf..3b26d32 100755
--- a/test/emacs
+++ b/test/emacs
@@ -525,7 +525,9 @@ test_expect_equal_file attachment1.gz "$EXPECTED/attachment"
 test_begin_subtest "Save attachment from within emacs using notmuch-show-save-part"
 # save as archive to test that Emacs does not re-compress .gz
 test_emacs '(let ((standard-input "\"attachment2.gz\""))
-	      (notmuch-show-save-part "id:cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a@mail.gmail.com" 5))'
+	      (notmuch-show "id:cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a@mail.gmail.com")
+	      (search-forward "0001-Deal-with")
+	      (notmuch-show-save-part))'
 test_expect_equal_file attachment2.gz "$EXPECTED/attachment"
 
 test_begin_subtest "Save 8bit attachment from within emacs using notmuch-show-save-attachments"
-- 
1.7.10.4

[PATCH 4/4] emacs: Bind MIME part commands to "." submap

[Austin Clements]

Since the part commands are no longer tied to a button, but can be
applied with point anywhere within a part, bind the part commands
keymap to "." everywhere in the show buffer.  This lets you save or
view parts without having to navigate to the part button, and is
particularly useful for parts that have no button.  These key bindings
are still available without the prefix when point is over a part
button.
---
 emacs/notmuch-show.el |   27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index b33d92d..380b144 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -471,17 +471,6 @@ message at DEPTH in the current thread."
   'face 'message-mml
   :supertype 'notmuch-button-type)
 
-(defvar notmuch-show-part-button-map
-  (let ((map (make-sparse-keymap)))
-    (set-keymap-parent map button-map)
-    (define-key map "s" 'notmuch-show-save-part)
-    (define-key map "v" 'notmuch-show-view-part)
-    (define-key map "o" 'notmuch-show-interactively-view-part)
-    (define-key map "|" 'notmuch-show-pipe-part)
-    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)
 	(base-label (concat (when name (concat name ": "))
@@ -1195,6 +1184,21 @@ reset based on the original query."
   "Submap for stash commands")
 (fset 'notmuch-show-stash-map notmuch-show-stash-map)
 
+(defvar notmuch-show-part-map
+  (let ((map (make-sparse-keymap)))
+    (define-key map "s" 'notmuch-show-save-part)
+    (define-key map "v" 'notmuch-show-view-part)
+    (define-key map "o" 'notmuch-show-interactively-view-part)
+    (define-key map "|" 'notmuch-show-pipe-part)
+    map)
+  "Submap for part commands")
+(fset 'notmuch-show-part-map notmuch-show-part-map)
+
+(defvar notmuch-show-part-button-map
+  (make-composed-keymap notmuch-show-part-map button-map)
+  "Keymap for part button commands")
+(fset 'notmuch-show-part-button-map notmuch-show-part-button-map)
+
 (defvar notmuch-show-mode-map
       (let ((map (make-sparse-keymap)))
 	(define-key map "?" 'notmuch-help)
@@ -1237,6 +1241,7 @@ reset based on the original query."
 	(define-key map "$" 'notmuch-show-toggle-process-crypto)
 	(define-key map "<" 'notmuch-show-toggle-thread-indentation)
 	(define-key map "t" 'toggle-truncate-lines)
+	(define-key map "." 'notmuch-show-part-map)
 	map)
       "Keymap for \"notmuch show\" buffers.")
 (fset 'notmuch-show-mode-map notmuch-show-mode-map)
-- 
1.7.10.4

Re: [PATCH 0/4] emacs: Part command improvements

[Mark Walters]

Austin Clements <amdragon@MIT.EDU> writes:

> This is a follow-up of sorts to id:"8761ycc19t.fsf@qmul.ac.uk", where
> Mark suggested that the part handling commands could all use the
> correponding mm-* functions.  I ran with the idea and wound up with
> this series, which, in addition to standardizing on the mm-* functions
> for everything and simplifying the implementation overall, decouples
> the part commands from part buttons, which removes an entire layer
> from the implementation and adds the ability to invoke part commands
> with point anywhere in a part (something I often find myself wanting).

Overall I really like this series. In addition to the clean up etc it
makes it easy to export the text/plain part (which doesn't have a part
button). I have recollection of this being difficult if it is base64
encoded.

I have a few small comments

As mentioned on irc (just included here in case other people are
testing) make-composed-keymap is emacs 24 only.

This does change the default directory for saving: not serious but it's
probably worth deciding do we want to use mailcap-download-directory or
home or where emacs was started or?

I don't know if we want to keep a special keymap for the button or just
always use the . prefix; the advantage is that you don't have 's' on a
button acting differently from 's' in the text (which has annoyed me
several times) otoh it is the extra keystroke which may annoy people
too. Let the bikeshedding begin! (obviously return for the default
action would remain.

Would it be worth having . return in the part body  as the default
action ?

Finally, with message indenting it's the start/end of the part are a
little unclear. I think it's the [ of the part button at the start of
the part to the character before the [ of the next part button. In
particular on the line of a new part but before the button is still the
old part. Since parts are whole lines it would be nice if the region
were line based but I don't know if that is easy.

Best wishes

Mark

Re: [PATCH 0/4] emacs: Part command improvements

[Austin Clements]

Quoth Mark Walters on May 27 at 11:30 pm:
> Austin Clements <amdragon@MIT.EDU> writes:
> 
> > This is a follow-up of sorts to id:"8761ycc19t.fsf@qmul.ac.uk", where
> > Mark suggested that the part handling commands could all use the
> > correponding mm-* functions.  I ran with the idea and wound up with
> > this series, which, in addition to standardizing on the mm-* functions
> > for everything and simplifying the implementation overall, decouples
> > the part commands from part buttons, which removes an entire layer
> > from the implementation and adds the ability to invoke part commands
> > with point anywhere in a part (something I often find myself wanting).
> 
> Overall I really like this series. In addition to the clean up etc it
> makes it easy to export the text/plain part (which doesn't have a part
> button). I have recollection of this being difficult if it is base64
> encoded.

Right.  That's one of the reasons I wanted a global part keymap (and
this series happened to be a convenient place to introduce that).
Also helpful is that the part bindings now appear in the show help,
which is good because I can never remember which key used the default
viewer and which prompted for a viewer.

> I have a few small comments
> 
> As mentioned on irc (just included here in case other people are
> testing) make-composed-keymap is emacs 24 only.

I've removed the button map entirely, so this is no longer a problem.

> This does change the default directory for saving: not serious but it's
> probably worth deciding do we want to use mailcap-download-directory or
> home or where emacs was started or?

I don't really care what the default directory is, as long as we're
consistent, which we currently aren't.  mm-default-directory seems
like a fine thing to be consistent with, since we use mm for
everything else.

> I don't know if we want to keep a special keymap for the button or just
> always use the . prefix; the advantage is that you don't have 's' on a
> button acting differently from 's' in the text (which has annoyed me
> several times) otoh it is the extra keystroke which may annoy people
> too. Let the bikeshedding begin! (obviously return for the default
> action would remain.

I'm all for removing the special button keymap.  The less hidden
functionality the better.

> Would it be worth having . return in the part body  as the default
> action ?

I played with this for a while and eventually decided it wasn't worth
the effort.  Maybe in a followup.

> Finally, with message indenting it's the start/end of the part are a
> little unclear. I think it's the [ of the part button at the start of
> the part to the character before the [ of the next part button. In
> particular on the line of a new part but before the button is still the
> old part. Since parts are whole lines it would be nice if the region
> were line based but I don't know if that is easy.

Fixed in v2.

> Best wishes
> 
> Mark