Re: [notmuch] [PATCH] Rework saving of attachments.
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
[notmuch] [PATCH] Rework saving of attachments.
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.
On Mon, 14 Dec 2009 10:13:58 -0800, cama...@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
pgpRXJXlSHTlA.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
[notmuch] [PATCH] Rework saving of attachments.
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:
[notmuch] [PATCH] Rework saving of attachments.
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 &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 nfoun
[notmuch] [PATCH] Rework saving of attachments.
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 &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)
