Simple improvements to saving attachments

Simple improvements to saving attachments

[Keith Amidon]

I found there were some attachments I couldn't save using the existing
functionality.  Since I was already touching the code I implemented a
rough cut at the "save all attachments" function Carl requested as
well.

I don't think this is the best code in the world but it gets the job
done for now and I expect it will get replaced with something much
better when some more thought has been put into the split in
mime-handling functionality between the frontend and the backend.

       --- Keith

[PATCH 1/2] Expand scope of items considered when saving attachments

[Keith Amidon]

Previously only mime parts that indicated specified a "disposition" of
"attachment" were saved.  However there are time when it is important
to be able to save inline content as well.  After this commit any mime
part that specifies a filename will be considered when saving
attachments.
---
 notmuch.el |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/notmuch.el b/notmuch.el
index c504f46..8d51709 100644
--- a/notmuch.el
+++ b/notmuch.el
@@ -322,7 +322,9 @@ buffer."
      (lambda (p)
        (let ((disposition (mm-handle-disposition p)))
          (and (listp disposition)
-              (equal (car disposition) "attachment")
+              (or (equal (car disposition) "attachment")
+                  (and (equal (car disposition) "inline")
+                       (assq 'filename disposition)))
               (incf count))))
      mm-handle)
     count))
@@ -332,7 +334,9 @@ buffer."
    (lambda (p)
      (let ((disposition (mm-handle-disposition p)))
        (and (listp disposition)
-            (equal (car disposition) "attachment")
+            (or (equal (car disposition) "attachment")
+                (and (equal (car disposition) "inline")
+                     (assq 'filename disposition)))
             (or (not queryp)
                 (y-or-n-p
                  (concat "Save '" (cdr (assq 'filename disposition)) "' ")))
-- 
1.6.5.4

[PATCH 2/2] Save all attachments to a directory

[Keith Amidon]

Prompt for a directory and write all attachments of the current
message to that directory, prompting for a filename for each with a
default value of the filename specified in the attachment.

The behavior of this function differs in two ways from the existing
notmuch-show-save-attachments function:

1) It first prompts for the directory in which to save attachments
   instead of assuming the current default directory.

2) If there is more than one attachment in the message, it assumes all
   should be saved and only prompts for filenames instead of asking
   first whether each attachment should be saved.
---
 notmuch.el |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/notmuch.el b/notmuch.el
index 8d51709..08bd114 100644
--- a/notmuch.el
+++ b/notmuch.el
@@ -65,6 +65,7 @@
     (define-key map "r" 'notmuch-show-reply)
     (define-key map "|" 'notmuch-show-pipe-message)
     (define-key map "w" 'notmuch-show-save-attachments)
+    (define-key map "W" 'notmuch-show-save-all-attachments)
     (define-key map "V" 'notmuch-show-view-raw-message)
     (define-key map "v" 'notmuch-show-view-all-mime-parts)
     (define-key map "-" 'notmuch-show-remove-tag)
@@ -352,6 +353,17 @@ buffer."
       mm-handle (> (notmuch-count-attachments mm-handle) 1))))
   (message "Done"))
 
+(defun notmuch-show-save-all-attachments (directory)
+  "Save all attachments of a message to a directory."
+  (interactive "G")
+  (let ((dirname (file-name-as-directory directory)))
+    (make-directory dirname t)
+    (with-current-notmuch-show-message
+     (let ((mm-handle (mm-dissect-buffer))
+           (mm-default-directory dirname))
+       (notmuch-save-attachments mm-handle nil))))
+  (message "Done"))
+
 (defun notmuch-reply (query-string)
   (switch-to-buffer (generate-new-buffer "notmuch-draft"))
   (call-process notmuch-command nil t nil "reply" query-string)
-- 
1.6.5.4

Re: [PATCH 2/2] Save all attachments to a directory

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 2160 bytes --]

On Sat,  5 Dec 2009 14:54:00 -0800, Keith Amidon <keith@nicira.com> wrote:
> Prompt for a directory and write all attachments of the current
> message to that directory, prompting for a filename for each with a
> default value of the filename specified in the attachment.

I really like the idea of this function. I'd just like to see a few,
little improvements.

> The behavior of this function differs in two ways from the existing
> notmuch-show-save-attachments function:

First, I'm not sure whether we need two different variations of what's
effectively the same operation here ("save all attachments").

What I would like is one command to save a single attachment, and then
one command to save all attachments. So if we assume that the current
'w' keybinding is really for "write one attachment" (with a lame
implementation currently), and 'W' is for "write all attachments", then
I think I'd be OK with that.

As for the changes we need here, the prompting for the directory needs
a string telling the user what's being prompted for. Something like
"Save all attachments to: ", which should just be another argument to
the interactive call, right?

Second, the command needs to provide a little bit of feedback as to what
was saved. I ended up running this command a couple of times before I
realized it was never going to save the inline patch with no filename
that I was looking at[*].

So it at least needs to message something like "N files written to DIR"
or so.

Should be fairly trivial improvements to the current patch I think.

-Carl

[*] So there's something else I think we need here. I was seeing a patch
in a message, but wanted to get it into a file before piping it off to
something, (so '|' didn't work). The patch wasn't an attachment so 'w'
didn't work as described above. I tried using 'V' to view the raw
message, but then found that the MIME part I wanted was actually encoded
as quoted-printable, so just saving the raw message wasn't useful
either.

So do we need a command to write the current "cooked" message to a file?
I suppose I could have just used:

	| cat > /some/filename

but that feels a little bit like cat abuse.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2/2] Save all attachments to a directory

[Keith Amidon]

{-- Thu, 10 Dec 2009 16:57:01 -0800: Carl <cworth@cworth.org> wrote: --}

  Carl> First, I'm not sure whether we need two different variations of
  Carl> what's effectively the same operation here ("save all
  Carl> attachments").

Yes, I agree it is an ugly solution.  Thanks for not letting me get away
with it.  :-)

  Carl> What I would like is one command to save a single attachment,
  Carl> and then one command to save all attachments. So if we assume
  Carl> that the current 'w' keybinding is really for "write one
  Carl> attachment" (with a lame implementation currently), and 'W' is
  Carl> for "write all attachments", then I think I'd be OK with that.

What do you see as the "write one" behavior for a message with multiple
attachments?  I think there needs to be some way to select the
attachment to be written.  Maybe we use the prefix-argument for this so
that 'M-2 w' would write the second attachment, 'M-3 w' would write the
third attachment, etc.  Since the default is 1, a plain 'w' would write
the first attachment which is the correct default for the single message
case.  It's not the most discoverable approach, but it wouldn't be too
bad.  

  Carl> As for the changes we need here, the prompting for the directory
  Carl> needs a string telling the user what's being prompted
  Carl> for. Something like "Save all attachments to: ", which should
  Carl> just be another argument to the interactive call, right?

Yes, you're right the current approach should have had a proper prompt.
I've been thinking about this though and I wonder if we should skip
separately prompting for the directory and instead do the following:

1) Have customizable "default save directory" both types of attachment
   saving default to.  Use this as the path part of the prompt for the
   filename to which the attachment will be written.
2) After the user has adjusted the path as required, verify that the
   full directory path exists and if not create it.
3) Use the same directory path as the default for any subsequent
   attachments that are being saved.

This seems like it would lesson the number of keystrokes required for
at least some common cases.

  Carl> Second, the command needs to provide a little bit of feedback as
  Carl> to what was saved. I ended up running this command a couple of
  Carl> times before I realized it was never going to save the inline
  Carl> patch with no filename that I was looking at[*].

  Carl> So it at least needs to message something like "N files written
  Carl> to DIR" or so.

Sure, that's easy to add and makes a lot of sense.  We should add
similar error reporting for other common error cases like selecting a
non-existing single attachment to save if we implement the
keystroke-based approach suggested above.

  Carl> [*] So there's something else I think we need here. I was seeing
  Carl> a patch in a message, but wanted to get it into a file before
  Carl> piping it off to something, (so '|' didn't work). The patch
  Carl> wasn't an attachment so 'w' didn't work as described above. I
  Carl> tried using 'V' to view the raw message, but then found that the
  Carl> MIME part I wanted was actually encoded as quoted-printable, so
  Carl> just saving the raw message wasn't useful either.

I'm not sure it is the most usable solution, but I believe selecting the
text to save in the rendered message in the thread view and using "M-x
write-region" should handle this use case.

Re: [PATCH 2/2] Save all attachments to a directory

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 1890 bytes --]

On Fri, 11 Dec 2009 17:32:51 -0800, Keith Amidon <camalot@picnicpark.org> wrote:
> {-- Thu, 10 Dec 2009 16:57:01 -0800: Carl <cworth@cworth.org> wrote: --}
> What do you see as the "write one" behavior for a message with multiple
> attachments?

I was imagining a direct-manipulation approach by clicking on (or
pressing a key while point is on) a button representing the attachment.

And then it would probably be reasonable to make that same keypress save
the next attachment if point is positioned between attachment buttons.

> Yes, you're right the current approach should have had a proper prompt.
> I've been thinking about this though and I wonder if we should skip
> separately prompting for the directory and instead do the following:
> 
> 1) Have customizable "default save directory" both types of attachment
>    saving default to.  Use this as the path part of the prompt for the
>    filename to which the attachment will be written.
> 2) After the user has adjusted the path as required, verify that the
>    full directory path exists and if not create it.
> 3) Use the same directory path as the default for any subsequent
>    attachments that are being saved.

Yes, the above sounds good to me. If the directory doesn't exist it
should probably give a y-or-n prompt before creating it, (and drop back
to prompting for the full filename path again if the users says no to
creating the directory).

> This seems like it would lesson the number of keystrokes required for
> at least some common cases.

Spoken as a man after my own heart.

> I'm not sure it is the most usable solution, but I believe selecting the
> text to save in the rendered message in the thread view and using "M-x
> write-region" should handle this use case.

Funny---I don't think I've ever used that command before. But it should
be handy---thanks!

-Carl

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Rework of attachment saving

[camalot]

I think I've reworked the attachment savings to behave as we've been
discussing.  I don't know anything about the button handling so I
haven't attempted to implement the direct manipulation approach of of
saving using the buttons.  That would certainly be nice to have
however and I belive this change should make it easy for someone who
knows how buttons work to implement it.

     --- Keith

[PATCH] Rework saving of attachments.

[camalot]

From: Keith Amidon <keith@nicira.com>

With this commit notmuch-show-mode supports saving a single attachment
from a message (bound to 'w') and saving all attachments to the
message (bound to 'W').  The new variable notmuch-default-save-dir
allows the user to specify a directory within which attachments should
be saved by default.  The user can modify this default path, even
specifying a non-existent directory path, in which case he or she will
be prompted to create the path.  Reporting of the actions taken is
also improved.
---
 notmuch.el |   93 ++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 72 insertions(+), 21 deletions(-)

diff --git a/notmuch.el b/notmuch.el
index 97914f2..b72548d 100644
--- a/notmuch.el
+++ b/notmuch.el
@@ -64,7 +64,8 @@
     (define-key map "f" 'notmuch-show-forward-current)
     (define-key map "r" 'notmuch-show-reply)
     (define-key map "|" 'notmuch-show-pipe-message)
-    (define-key map "w" 'notmuch-show-save-attachments)
+    (define-key map "w" 'notmuch-show-save-attachment)
+    (define-key map "W" 'notmuch-show-save-all-attachments)
     (define-key map "V" 'notmuch-show-view-raw-message)
     (define-key map "v" 'notmuch-show-view-all-mime-parts)
     (define-key map "-" 'notmuch-show-remove-tag)
@@ -98,6 +99,9 @@ pattern can still test against the entire line).")
 (defvar notmuch-command "notmuch"
   "Command to run the notmuch binary.")
 
+(defvar notmuch-default-save-dir (file-name-as-directory (getenv "HOME" ))
+  "Default directory in which attachments are saved.")
+
 (defvar notmuch-show-message-begin-regexp    "\fmessage{")
 (defvar notmuch-show-message-end-regexp      "\fmessage}")
 (defvar notmuch-show-header-begin-regexp     "\fheader{")
@@ -329,28 +333,75 @@ buffer."
      mm-handle)
     count))
 
-(defun notmuch-save-attachments (mm-handle &optional queryp)
-  (notmuch-foreach-mime-part
-   (lambda (p)
-     (let ((disposition (mm-handle-disposition p)))
-       (and (listp disposition)
-            (or (equal (car disposition) "attachment")
-                (and (equal (car disposition) "inline")
-                     (assq 'filename disposition)))
-            (or (not queryp)
-                (y-or-n-p
-                 (concat "Save '" (cdr (assq 'filename disposition)) "' ")))
-            (mm-save-part p))))
-   mm-handle))
-
-(defun notmuch-show-save-attachments ()
-  "Save all attachments from the current message."
-  (interactive)
+(defun notmuch-attachment-q (mm-handle)
+  (let ((disposition (mm-handle-disposition p)))
+    (and (listp disposition)
+         (assq 'filename disposition))))
+
+(defun notmuch-get-save-path (filename default-dir)
+  (let ((savepath nil))
+    (while (not savepath)
+      (let* ((ddir (file-name-as-directory default-dir))
+             (fn (read-file-name "Save to: " ddir nil nil filename))
+             (efn (expand-file-name fn))
+             (dir (file-name-directory efn)))
+        (when (not (file-accessible-directory-p dir))
+          (when (y-or-n-p (concat "Create directory " dir " "))
+            (make-directory dir t)))
+        (if (file-accessible-directory-p dir)
+            (setq savepath fn)
+          (setq default-dir (file-name-directory fn)))))
+    savepath))
+
+(defun notmuch-save-attachment (mm-handle default-dir)
+  "Save the current attachment part to a file."
+  (cond ((not (notmuch-attachment-q mm-handle))
+         (message "Current part is not an attachment.")
+         nil)
+        (t
+         (let* ((fn (cdr (assq 'filename (mm-handle-disposition mm-handle))))
+                (savepath (notmuch-get-save-path fn default-dir)))
+           (mm-save-part-to-file mm-handle savepath)
+           savepath))))
+
+(defun notmuch-save-attachment-num (mm-handle part-num)
+  "Save the nth part number"
+  (let ((nfound 0)
+        (nsaved 0))
+    (notmuch-foreach-mime-part
+     (lambda (p)
+       (when (notmuch-attachment-q p)
+         (cond ((equal (+ 1 nfound) part-num)
+                (when (notmuch-save-attachment p notmuch-default-save-dir)
+                  (incf nsaved))))
+         (incf nfound))) mm-handle)
+    (equal nsaved 1)))
+
+(defun notmuch-show-save-attachment (num)
+  "Save a single attachment."
+  (interactive "p")
   (with-current-notmuch-show-message
    (let ((mm-handle (mm-dissect-buffer)))
-     (notmuch-save-attachments
-      mm-handle (> (notmuch-count-attachments mm-handle) 1))))
-  (message "Done"))
+     (if (notmuch-save-attachment-num mm-handle num)
+         (message "Attachment %d saved." num)
+       (message "Failed to save attachment.")))))
+
+(defun notmuch-show-save-all-attachments ()
+  "Save all attachments of a message to a directory."
+  (interactive)
+  (with-current-notmuch-show-message
+   (let ((nfound 0)
+         (nsaved 0)
+         (default-dir notmuch-default-save-dir)
+         (mm-handle (mm-dissect-buffer)))
+     (notmuch-foreach-mime-part
+      (lambda (p)
+        (when (notmuch-attachment-q p)
+          (incf nfound)
+          (let ((savepath (notmuch-save-attachment p default-dir)))
+            (when savepath
+              (setq default-dir (file-name-directory savepath)))))) mm-handle)
+     (message "Saved %d attachments" nfound))))
 
 (defun notmuch-reply (query-string)
   (switch-to-buffer (generate-new-buffer "notmuch-draft"))
-- 
1.6.5.6

Re: [PATCH] Rework saving of attachments.

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 2358 bytes --]

On Mon, 14 Dec 2009 10:13:58 -0800, camalot@picnicpark.org wrote:
> From: Keith Amidon <keith@nicira.com>

Hi Keith,

I apologize for the extraordinarly-late review, but here it is...

I tried this patch out, wanted to like it, and almost pushed it out, but
I decided against it in its current form. Here are some thoughts:

1. The commit message ("rework saving of attachments") is not
   adequate. It doesn't actually say what the commit does, (how can I
   test whether things have been reworked?). If the vagueness of the
   message is because the commit is changing several different aspects
   of behavior, then I would argue that the commit should be split up
   into separate pieces.

2. A binding to save a single attachment (with only a prefix argument to
   select which) just isn't usable. First, there's nothing in the UI to
   indicate the appropriate numbers to pass as the prefix argument,
   (other than manually counting the attachments). And second, the
   functionality is simply too hidden and non-obvious. This is most
   dangerous because in the common case of a single attachment, the 'w'
   binding will seem to be saving all attachments setting up confusion
   if the user tries to save multiple attachments with this same
   keybinding.

   Now, having a function to save a single attachment is just fine,
   (leaving someone else to hook up a binding to a particular button,
   say). So I'd accept a patch that added that, but didn't add a direct
   key-binding for it.

3. For saving multiple attachments, the feature I'd really like to see
   is the ability to specify a single directory and have all the
   attachments saved there.

Obviously, this third feature is just something different than what the
patch does, so not necessarily a reason to reject the patch. So what is
it that the patch actually does again?

I think the big advantage of the patch is getting rid of the initial
prompting "save this attachment (foo)?". It occurs to me that a simpler
way to get rid of that would be to simply not ask that question when the
user hits 'w' and there *is* only a single attachment. Then, with
multiple attachments, 'w' could prompt in turn as currently.

That would leave open the ability to use 'W' for a command to write all
attachments to a particular directory.

So that's one idea, at least. What do you think?

-Carl

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Rework saving of attachments.

[Keith Amidon]

Resending to the list as well as just Carl...

{-- Tue, 23 Feb 2010 11:12:36 -0800: Carl <cworth@cworth.org> wrote: --}
  Carl> I apologize for the extraordinarly-late review, but here it
  Carl> is...

No problem at all.  You're updates on status have been sufficient to
convince me you were making progress and would get to everything
eventually and I've not exactly had any significant time to be playing
with notmuch  code anyway.  :-)

  Carl> I tried this patch out, wanted to like it, and almost pushed it
  Carl> out, but I decided against it in its current form. Here are some
  Carl> thoughts:

  Carl> 1. The commit message ("rework saving of attachments") is not
  Carl> adequate....

Sure, I can make more granular commits.  Much of the work in this one
was inter-related in that my goal for the behavior couldn't be tested
until most of the work was done and I didn't take much care to commit
interim steps due to an over-eagerness to complete the changes. Easily
correctable.

  Carl> 2. A binding to save a single attachment (with only a prefix
  Carl> argument to select which) just isn't usable. 

Yes, I agree the current implementation for the save single attachment
is not the best.

  Carl> First, there's nothing in the UI to indicate the appropriate
  Carl> numbers to pass as the prefix argument, (other than manually
  Carl> counting the attachments). 

This is the real problem in my opinion.  My plan, which I've had no time
to implement, was to do something similar to what Gnus does; make a
button for each part and in the button text include the number of each
part.  This way the user would no longer have to manually count.

  Carl> And second, the functionality is simply too hidden and
  Carl> non-obvious. This is most dangerous because in the common case
  Carl> of a single attachment, the 'w' binding will seem to be saving
  Carl> all attachments setting up confusion if the user tries to save
  Carl> multiple attachments with this same keybinding.

  Carl>    Now, having a function to save a single attachment is just
  Carl> fine, (leaving someone else to hook up a binding to a particular
  Carl> button, say). So I'd accept a patch that added that, but didn't
  Carl> add a direct key-binding for it.

I personally think that there should be a key-binding that allows saving
individual attachments and doesn't require navigating to a button in the
message and then doing something.  There are key-bindings in Gnus to do
this that I use all the time and I think notmuch should support
something similar.  Maybe the right approach is to combine both
functions on a single key-binding for example if no prefix argument is
given all attachments are saved and a prefix selects the specific
attachment.  

  Carl> 3. For saving multiple attachments, the feature I'd really like
  Carl> to see is the ability to specify a single directory and have all
  Carl> the attachments saved there.

I think the current code does support this functionality, although it
may not be completely obvious.  It adds a default directory in which to
save attachments (notmuch-default-save-dir).  When you type 'W' to save
all attachments it prompts for the location to save the first attachment
with a path built from the combination of notmuch-default-save-dir and
the filename or description of the attachment.  You can edit this path
to your heart's content.  Any subsequent attachments to be saved will
default to the directory into which you saved the most recent
attachment.

In use, if you want all attachments saved to the default directory with
their default filenames all you have to do is hit 'W' followed by one
carriage return per attachment.  If you want all of them saved to the
same directory but different from the default save directory you hit 'W'
edit the first path, then hit enter for each subsequent attachment.  The
changes support creating the destination directory path if it is not
already there.

  Carl> Obviously, this third feature is just something different than
  Carl> what the patch does, so not necessarily a reason to reject the
  Carl> patch. So what is it that the patch actually does again?

  Carl> I think the big advantage of the patch is getting rid of the
  Carl> initial prompting "save this attachment (foo)?". It occurs to me
  Carl> that a simpler way to get rid of that would be to simply not ask
  Carl> that question when the user hits 'w' and there *is* only a
  Carl> single attachment. Then, with multiple attachments, 'w' could
  Carl> prompt in turn as currently.

In my mind the key advantage of the patch was the much improved behavior
of the 'W' keybinding, described above.  Maybe that just wasn't obvious?

  Carl> That would leave open the ability to use 'W' for a command to
  Carl> write all attachments to a particular directory.

For this are you imagining that the user would first be prompted for the
directory in which to save the attachments and then all attachments
would be saved with some set of default names and no need for further
keypresses from the users?  I thought about doing something similar but
was worried that there might be situations in which the resulting
filenames to which the attachments were saved might not be easy for the
user to correlate back to what they thought they were saving from within
notmuch.

If we combined the behavior of the current code into a single 'w'
key-binding that accomplished the behavior of both 'w' and 'W' in this
patch it would leave 'W' open for something like this if you think it
would be a significant convenience.

  Carl> So that's one idea, at least. What do you think?

I've probably said enough on that score already.  :-)

        --- Keith

Re: Rework of attachment saving

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 574 bytes --]

On Mon, 14 Dec 2009 10:13:57 -0800, camalot@picnicpark.org wrote:
> I think I've reworked the attachment savings to behave as we've been
> discussing.  I don't know anything about the button handling so I
> haven't attempted to implement the direct manipulation approach of of
> saving using the buttons.  That would certainly be nice to have
> however and I belive this change should make it easy for someone who
> knows how buttons work to implement it.

Excellent, Keith!

I'm just writing to say thanks for now, and I will review the patch in
more detail later.

-Carl


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 1/2] Expand scope of items considered when saving attachments

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 461 bytes --]

On Sat,  5 Dec 2009 14:53:59 -0800, Keith Amidon <keith@nicira.com> wrote:
> Previously only mime parts that indicated specified a "disposition" of
> "attachment" were saved.  However there are time when it is important
> to be able to save inline content as well.  After this commit any mime
> part that specifies a filename will be considered when saving
> attachments.

Yes, this seems obviously like what we want---thanks!

I've pushed this out now.

-Carl

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]