From: Keith Amidon <camalot@picnicpark.org>
To: Carl Worth <cworth@cworth.org>
Organization: picnicpark.org
References: <87my1m323m.fsf@yoom.home.cworth.org>
	<1260814438-4195-1-git-send-email-camalot@picnicpark.org>
	<1260814438-4195-2-git-send-email-camalot@picnicpark.org>
	<87d3zvwyu3.fsf@yoom.home.cworth.org>
Date: Wed, 24 Feb 2010 06:57:48 -0800
In-Reply-To: <87d3zvwyu3.fsf@yoom.home.cworth.org> (Carl Worth's message of
	"Tue, 23 Feb 2010 11:12:36 -0800")
Message-ID: <87mxyyptoz.fsf@friend.picnicpark.org>
User-Agent: Gnus/5.110009 (No Gnus v0.9) Emacs/23.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: notmuch@notmuchmail.org
Subject: Re: [notmuch] [PATCH] Rework saving of attachments.
Precedence: list

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