[notmuch] [PATCH] Rework saving of attachments.

2010-02-24 Thread Keith Amidon
Resending to the list as well as just Carl...

{-- Tue, 23 Feb 2010 11:12:36 -0800: Carl  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 

Re: [notmuch] [PATCH] Rework saving of attachments.

2010-02-24 Thread Keith Amidon
Resending to the list as well as just Carl...

{-- Tue, 23 Feb 2010 11:12:36 -0800: Carl cwo...@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.

  CarlNow, 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

[notmuch] [PATCH] Rework saving of attachments.

2010-02-23 Thread Carl Worth
On Mon, 14 Dec 2009 10:13:58 -0800, camalot at picnicpark.org wrote:
> From: Keith Amidon 

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
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: 



Re: [notmuch] [PATCH] Rework saving of attachments.

2010-02-23 Thread Carl Worth
On Mon, 14 Dec 2009 10:13:58 -0800, cama...@picnicpark.org wrote:
 From: Keith Amidon ke...@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


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


[notmuch] [PATCH] Rework saving of attachments.

2009-12-14 Thread cama...@picnicpark.org
From: Keith Amidon 

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  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)
+  

[notmuch] [PATCH] Rework saving of attachments.

2009-12-14 Thread camalot
From: Keith Amidon ke...@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