Date: Sun, 13 Oct 2013 11:28:06 -0400
From: Austin Clements <amdragon@MIT.EDU>
To: Mark Walters <markwalters1009@gmail.com>
Subject: Re: [PATCH v2] emacs: show: use interactive instead of
	current-prefix-arg
Message-ID: <20131013152806.GF10539@mit.edu>
References: <1381649952-23122-1-git-send-email-markwalters1009@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1381649952-23122-1-git-send-email-markwalters1009@gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

LGTM.

Quoth Mark Walters on Oct 13 at  8:39 am:
> Currently notmuch-show looks at the prefix-arg directly via
> current-prefix-arg. This changes it to use the interactive
> specification.
> 
> One test (for elide-toggle functionality) set the prefix arg
> directly. Update this test to set the new argument directly.
> ---
> 
> This is version 2 of the patches at
> id:1381348886-5673-1-git-send-email-markwalters1009@gmail.com The
> changes are in fixes in response to Austin's review
> id:20131013033157.GE10539@mit.edu and his comments on irc.
> 
> Specifically, this updates the doc string for elide-toggle in
> notmuch-show and folds the test fix in (so the tests should always
> pass)
> 
> I can also confirm that C-u RET on an id:-button link does work as
> expected (and as currently): opening the appropriate message with
> inverted elide behaviour.
> 
> Best wishes 
> 
> Mark
> 
> 
> 
>  emacs/notmuch-show.el |   12 +++++++-----
>  emacs/notmuch.el      |    5 +++--
>  test/emacs-show       |    3 +--
>  3 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 5d7e24b..3189dda 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -1080,15 +1080,17 @@ buttons for a corresponding notmuch search."
>  	(make-text-button (first link) (second link)
>  			  :type 'notmuch-button-type
>  			  'action `(lambda (arg)
> -				     (notmuch-show ,(third link)))
> +				     (notmuch-show ,(third link) current-prefix-arg))
>  			  'follow-link t
>  			  'help-echo "Mouse-1, RET: search for this message"
>  			  'face goto-address-mail-face)))))
>  
>  ;;;###autoload
> -(defun notmuch-show (thread-id &optional parent-buffer query-context buffer-name)
> +(defun notmuch-show (thread-id &optional elide-toggle parent-buffer query-context buffer-name)
>    "Run \"notmuch show\" with the given thread ID and display results.
>  
> +ELIDE-TOGGLE, if non-nil, inverts the default elide behavior.
> +
>  The optional PARENT-BUFFER is the notmuch-search buffer from
>  which this notmuch-show command was executed, (so that the
>  next thread from that buffer can be show when done with this
> @@ -1102,7 +1104,7 @@ The optional BUFFER-NAME provides the name of the buffer in
>  which the message thread is shown. If it is nil (which occurs
>  when the command is called interactively) the argument to the
>  function is used."
> -  (interactive "sNotmuch show: ")
> +  (interactive "sNotmuch show: \nP")
>    (let ((buffer-name (generate-new-buffer-name
>  		      (or buffer-name
>  			  (concat "*notmuch-" thread-id "*")))))
> @@ -1112,9 +1114,9 @@ function is used."
>      (setq notmuch-show-process-crypto notmuch-crypto-process-mime)
>      ;; Set the default value for
>      ;; `notmuch-show-elide-non-matching-messages' in this buffer. If
> -    ;; there is a prefix argument, invert the default.
> +    ;; elide-toggle is set, invert the default.
>      (setq notmuch-show-elide-non-matching-messages notmuch-show-only-matching-messages)
> -    (if current-prefix-arg
> +    (if elide-toggle
>  	(setq notmuch-show-elide-non-matching-messages (not notmuch-show-elide-non-matching-messages)))
>  
>      (setq notmuch-show-thread-id thread-id
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 0ff248b..7124d0a 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -486,13 +486,14 @@ If BARE is set then do not prefix with \"thread:\""
>    "Return a list of authors for the current region"
>    (notmuch-search-properties-in-region :subject beg end))
>  
> -(defun notmuch-search-show-thread ()
> +(defun notmuch-search-show-thread (&optional elide-toggle)
>    "Display the currently selected thread."
> -  (interactive)
> +  (interactive "P")
>    (let ((thread-id (notmuch-search-find-thread-id))
>  	(subject (notmuch-search-find-subject)))
>      (if (> (length thread-id) 0)
>  	(notmuch-show thread-id
> +		      elide-toggle
>  		      (current-buffer)
>  		      notmuch-search-query-string
>  		      ;; Name the buffer based on the subject.
> diff --git a/test/emacs-show b/test/emacs-show
> index ae70053..fb23db4 100755
> --- a/test/emacs-show
> +++ b/test/emacs-show
> @@ -91,8 +91,7 @@ test_begin_subtest "notmuch-show: elide non-matching messages (w/ prefix arg to
>  test_emacs '(let ((notmuch-show-only-matching-messages nil))
>  	(notmuch-search "from:lars@seas.harvard.edu and subject:\"Maildir storage\"")
>  	(notmuch-test-wait)
> -	(let ((current-prefix-arg t))
> -	  (notmuch-search-show-thread))
> +	(notmuch-search-show-thread t)
>  	(notmuch-test-wait)
>  	(test-visible-output))'
>  test_expect_equal_file OUTPUT $EXPECTED/notmuch-show-elide-non-matching-messages-on