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