From: Mark Walters <markwalters1009@gmail.com>
To: "W. Trevor King" <wking@tremily.us>, notmuch@notmuchmail.org
Subject: Re: [PATCH 2/2] emacs: Prefer Content-Description over filename for
	part buttons
In-Reply-To:
 <27be295875a7df782a83c9a2c09d06f9d321fe9e.1391423201.git.wking@tremily.us>
References: <877g9chbay.fsf@qmul.ac.uk>
 <cover.1391423201.git.wking@tremily.us>
	<27be295875a7df782a83c9a2c09d06f9d321fe9e.1391423201.git.wking@tremily.us>
User-Agent: Notmuch/0.15.2+484~gfb59956 (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Mon, 03 Feb 2014 20:15:35 +0000
Message-ID: <87vbwwosuw.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list


On Mon, 03 Feb 2014, "W. Trevor King" <wking@tremily.us> wrote:
> On the rss2email list, Victor Orlikowski pointed out [1] that a number
> of MUAs don't use the Subject header of attached message/rfc822 parts
> to label multipart/digest subparts [2].  Instead, notmuch and several
> other MUAs use the filename parameter [3] as a content hint.  Using
> the filename parameter seems more sane than diving into the
> message/rfc822 part header, but that's still not what the filename
> parameter was designed for.  It makes more sense to me to use the
> message/rfc822 part's Content-Description header (which I just taught
> notmuch-show to export), falling back on the filename parameter if
> Content-Description isn't set.
>
> [1]: http://article.gmane.org/gmane.mail.rss2email/211
> [2]: Digests: http://tools.ietf.org/html/rfc2046#section-5.1.5
> [3]: Filename: http://tools.ietf.org/search/rfc2183#section-2.3

I tried this and it all works. However, I looked through my collection
an I have a lot of emails which have Content-Description headers that
are empty. Thus, I think we should only display/use the
content-description if it exists and is non-empty. Something like

	 (description (plist-get part :content-description))
	 (name (if (and description (not (equal description "")))
		   description
		 (plist-get part :filename)))

My only other comment on the series is that you should update
devel/schemata in the first patch to reflect this addition.

Otherwise it looks good to me

Best wishes

Mark


> ---
>  NEWS                                               |  7 ++
>  emacs/notmuch-show.el                              |  4 +-
>  test/T450-emacs-show.sh                            | 12 ++++
>  test/corpus/cur/24:2,                              |  1 +
>  .../notmuch-show-buttons-content-description       | 84 ++++++++++++++++++++++
>  .../notmuch-show-buttons-filename                  | 74 +++++++++++++++++++
>  6 files changed, 181 insertions(+), 1 deletion(-)
>  create mode 100644 test/emacs-show.expected-output/notmuch-show-buttons-content-description
>  create mode 100644 test/emacs-show.expected-output/notmuch-show-buttons-filename
>
> diff --git a/NEWS b/NEWS
> index 0f7b1c8..2154872 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -10,6 +10,13 @@ Command-Line Interface
>  
>  `notmuch show` now includes envelope Content-Description headers.
>  
> +Emacs Interface
> +---------------
> +
> +`notmuch-show` mode prefers Content-Description to filename when
> +naming part buttons.  This is useful for finding interesting parts of
> +multipart/digest messages, assuming the digest-creator set that field.
> +
>  Notmuch 0.17 (2013-12-30)
>  =========================
>  
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 1ac80ca..dbff3a8 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -874,13 +874,15 @@ useful for quoting in replies)."
>  			content-type))
>  	 (nth (plist-get part :id))
>  	 (beg (point))
> +	 (name (or (plist-get part :content-description)
> +		   (plist-get part :filename)))
>  	 ;; Hide the part initially if HIDE is t.
>  	 (show-part (not (equal hide t)))
>  	 ;; We omit the part button for the first (or only) part if
>  	 ;; this is text/plain, or HIDE is 'no-buttons.
>  	 (button (unless (or (equal hide 'no-buttons)
>  			     (and (string= mime-type "text/plain") (<= nth 1)))
> -		   (notmuch-show-insert-part-header nth mime-type content-type (plist-get part :filename))))
> +		   (notmuch-show-insert-part-header nth mime-type content-type name)))
>  	 (content-beg (point)))
>  
>      ;; Store the computed mime-type for later use (e.g. by attachment handlers).
> diff --git a/test/T450-emacs-show.sh b/test/T450-emacs-show.sh
> index 2a3a535..5650f04 100755
> --- a/test/T450-emacs-show.sh
> +++ b/test/T450-emacs-show.sh
> @@ -106,6 +106,18 @@ test_emacs '(notmuch-search "from:lars@seas.harvard.edu and subject:\"Maildir st
>  	(test-visible-output)'
>  test_expect_equal_file OUTPUT $EXPECTED/notmuch-show-indent-thread-content-off
>  
> +test_begin_subtest "buttons prefer Content-Description"
> +test_emacs '(let ((notmuch-crypto-process-mime nil))
> +	(notmuch-show "id:20091118010116.GC25380@dottiness.seas.harvard.edu")
> +	(test-visible-output))'
> +test_expect_equal_file OUTPUT $EXPECTED/notmuch-show-buttons-content-description
> +
> +test_begin_subtest "buttons fallback to filename"
> +test_emacs '(let ((notmuch-crypto-process-mime nil))
> +	(notmuch-show "id:20091118005829.GB25380@dottiness.seas.harvard.edu")
> +	(test-visible-output))'
> +test_expect_equal_file OUTPUT $EXPECTED/notmuch-show-buttons-filename
> +
>  test_begin_subtest "id buttonization"
>  add_message '[body]="
>  id:abc
> diff --git a/test/corpus/cur/24:2, b/test/corpus/cur/24:2,
> index c800020..f9418d2 100644
> --- a/test/corpus/cur/24:2,
> +++ b/test/corpus/cur/24:2,
> @@ -104,6 +104,7 @@ Harvard University School of Engineering and Applied Sciences
>  
>  --KdquIMZPjGJQvRdI
>  Content-Type: text/plain; charset=us-ascii
> +Content-Description: v2 of the selectable usage() stream patch
>  Content-Disposition: attachment; filename="notmuch-help.patch"
>  Content-Transfer-Encoding: quoted-printable
>  
> diff --git a/test/emacs-show.expected-output/notmuch-show-buttons-content-description b/test/emacs-show.expected-output/notmuch-show-buttons-content-description
> new file mode 100644
> index 0000000..622c94e
> --- /dev/null
> +++ b/test/emacs-show.expected-output/notmuch-show-buttons-content-description
> @@ -0,0 +1,84 @@
> +Lars Kellogg-Stedman <lars@seas.harvard.edu> (2009-11-18) (attachment inbox signed unread)
> +Subject: [notmuch] "notmuch help" outputs to stderr?
> + Lars Kellogg-Stedman <lars@seas.harvard.edu> (2009-11-18) (attachment inbox signed)
> + Subject: Re: [notmuch] "notmuch help" outputs to stderr?
> + To: notmuch <notmuch@notmuchmail.org>
> + Date: Tue, 17 Nov 2009 20:01:16 -0500
> +
> + [ multipart/mixed ]
> + [ multipart/signed ]
> + [ multipart/mixed ]
> + [ text/plain ]
> + > I've attached a patch that lets usage() take a FILE * argument so that
> + > you can output to stderr in response to usage errors, and stdout in
> + > response to an explicit request.
> +
> + Whoops, missed a couple of stderr's in that last patch.  New one
> + attached.
> +
> + [ 4-line signature. Click/Enter to show. ]
> + [ v2 of the selectable usage() stream patch: text/plain ]
> + diff --git a/notmuch.c b/notmuch.c
> + index c47e640..446c810 100644
> + --- a/notmuch.c
> + +++ b/notmuch.c
> + @@ -157,23 +157,23 @@ command_t commands[] = {
> +  };
> +
> +  static void
> + -usage (void)
> + +usage (FILE *out)
> +  {
> +      command_t *command;
> +      unsigned int i;
> +
> + -    fprintf (stderr, "Usage: notmuch <command> [args...]\n");
> + -    fprintf (stderr, "\n");
> + -    fprintf (stderr, "Where <command> and [args...] are as follows:\n");
> + -    fprintf (stderr, "\n");
> + +    fprintf (out, "Usage: notmuch <command> [args...]\n");
> + +    fprintf (out, "\n");
> + +    fprintf (out, "Where <command> and [args...] are as follows:\n");
> + +    fprintf (out, "\n");
> +
> +      for (i = 0; i < ARRAY_SIZE (commands); i++) {
> +	 command = &commands[i];
> +
> + -	fprintf (stderr, "\t%s\t%s\n\n", command->name, command->summary);
> + +	fprintf (out, "\t%s\t%s\n\n", command->name, command->summary);
> +      }
> +
> + -    fprintf (stderr, "Use \"notmuch help <command>\" for more details on
> + each command.\n\n");
> + +    fprintf (out, "Use \"notmuch help <command>\" for more details on each
> + command.\n\n");
> +  }
> +
> +  static int
> + @@ -183,8 +183,8 @@ notmuch_help_command (unused (void *ctx), int argc, char
> + *argv[])
> +      unsigned int i;
> +
> +      if (argc == 0) {
> + -	fprintf (stderr, "The notmuch mail system.\n\n");
> + -	usage ();
> + +	fprintf (stdout, "The notmuch mail system.\n\n");
> + +	usage (stdout);
> +	 return 0;
> +      }
> +
> + @@ -192,8 +192,8 @@ notmuch_help_command (unused (void *ctx), int argc, char
> + *argv[])
> +	 command = &commands[i];
> +
> +	 if (strcmp (argv[0], command->name) == 0) {
> + -	    fprintf (stderr, "Help for \"notmuch %s\":\n\n", argv[0]);
> + -	    fprintf (stderr, "\t%s\t%s\n\n%s\n\n", command->name,
> + +	    fprintf (stdout, "Help for \"notmuch %s\":\n\n", argv[0]);
> + +	    fprintf (stdout, "\t%s\t%s\n\n%s\n\n", command->name,
> +		      command->summary, command->documentation);
> +	     return 0;
> +	 }
> + [ application/pgp-signature ]
> + [ text/plain ]
> + [ 4-line signature. Click/Enter to show. ]
> diff --git a/test/emacs-show.expected-output/notmuch-show-buttons-filename b/test/emacs-show.expected-output/notmuch-show-buttons-filename
> new file mode 100644
> index 0000000..8fb4d89
> --- /dev/null
> +++ b/test/emacs-show.expected-output/notmuch-show-buttons-filename
> @@ -0,0 +1,74 @@
> +Lars Kellogg-Stedman <lars@seas.harvard.edu> (2009-11-18) (attachment inbox signed)
> +Subject: [notmuch] "notmuch help" outputs to stderr?
> +To: notmuch <notmuch@notmuchmail.org>
> +Date: Tue, 17 Nov 2009 19:58:29 -0500
> +
> +[ multipart/mixed ]
> +[ multipart/signed ]
> +[ multipart/mixed ]
> +[ text/plain ]
> +I'm just noticing that 'notmuch help ...' outputs to stderr, which
> +isn't terribly intuitive.  For example, the obvious invocation:
> +
> +  notmuch help | less
> +
> +...isn't terribly helpful.
> +
> +I've attached a patch that lets usage() take a FILE * argument so that
> +you can output to stderr in response to usage errors, and stdout in
> +response to an explicit request.
> +
> +[ 4-line signature. Click/Enter to show. ]
> +[ notmuch-help.patch: text/plain ]
> +diff --git a/notmuch.c b/notmuch.c
> +index c47e640..a35cb99 100644
> +--- a/notmuch.c
> ++++ b/notmuch.c
> +@@ -157,23 +157,23 @@ command_t commands[] = {
> + };
> +
> + static void
> +-usage (void)
> ++usage (FILE *out)
> + {
> +     command_t *command;
> +     unsigned int i;
> +
> +-    fprintf (stderr, "Usage: notmuch <command> [args...]\n");
> +-    fprintf (stderr, "\n");
> +-    fprintf (stderr, "Where <command> and [args...] are as follows:\n");
> +-    fprintf (stderr, "\n");
> ++    fprintf (out, "Usage: notmuch <command> [args...]\n");
> ++    fprintf (out, "\n");
> ++    fprintf (out, "Where <command> and [args...] are as follows:\n");
> ++    fprintf (out, "\n");
> +
> +     for (i = 0; i < ARRAY_SIZE (commands); i++) {
> +	command = &commands[i];
> +
> +-	fprintf (stderr, "\t%s\t%s\n\n", command->name, command->summary);
> ++	fprintf (out, "\t%s\t%s\n\n", command->name, command->summary);
> +     }
> +
> +-    fprintf (stderr, "Use \"notmuch help <command>\" for more details on each
> +command.\n\n");
> ++    fprintf (out, "Use \"notmuch help <command>\" for more details on each
> +command.\n\n");
> + }
> +
> + static int
> +@@ -183,8 +183,8 @@ notmuch_help_command (unused (void *ctx), int argc, char
> +*argv[])
> +     unsigned int i;
> +
> +     if (argc == 0) {
> +-	fprintf (stderr, "The notmuch mail system.\n\n");
> +-	usage ();
> ++	fprintf (stdout, "The notmuch mail system.\n\n");
> ++	usage (stdout);
> +	return 0;
> +     }
> +[ application/pgp-signature ]
> +[ text/plain ]
> +[ 4-line signature. Click/Enter to show. ]
> + Lars Kellogg-Stedman <lars@seas.harvard.edu> (2009-11-18) (attachment inbox signed)
> -- 
> 1.8.5.2.8.g0f6c0d1