From: Mark Walters <markwalters1009@gmail.com>
To: Jani Nikula <jani@nikula.org>, notmuch@notmuchmail.org
Subject: Re: [PATCH REBASE] emacs: add show view bindings to move to
	previous/next thread
In-Reply-To: <1367514302-30196-1-git-send-email-jani@nikula.org>
References: <1367514302-30196-1-git-send-email-jani@nikula.org>
User-Agent: Notmuch/0.14+255~gff3cc55 (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Sun, 05 May 2013 00:16:52 +0100
Message-ID: <87bo8q72u3.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list


Hi

This seems like a useful addition to me. I have a couple of comments and
a little bikeshedding below.

On Thu, 02 May 2013, Jani Nikula <jani@nikula.org> wrote:
> We have most of the plumbing in place, add the bindings M-n and M-p.
> ---
>  emacs/notmuch-show.el |   24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index face2a0..7f6ea65 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -39,6 +39,7 @@
>  
>  (declare-function notmuch-call-notmuch-process "notmuch" (&rest args))
>  (declare-function notmuch-search-next-thread "notmuch" nil)
> +(declare-function notmuch-search-previous-thread "notmuch" nil)
>  (declare-function notmuch-search-show-thread "notmuch" nil)
>  
>  (defcustom notmuch-message-headers '("Subject" "To" "Cc" "Date")
> @@ -1267,6 +1268,8 @@ reset based on the original query."
>  	(define-key map "P" 'notmuch-show-previous-message)
>  	(define-key map "n" 'notmuch-show-next-open-message)
>  	(define-key map "p" 'notmuch-show-previous-open-message)
> +	(define-key map (kbd "M-n") 'notmuch-show-next-thread-show)
> +	(define-key map (kbd "M-p") 'notmuch-show-previous-thread-show)

These seem sensible bindings.

>  	(define-key map (kbd "DEL") 'notmuch-show-rewind)
>  	(define-key map " " 'notmuch-show-advance-and-archive)
>  	(define-key map (kbd "M-RET") 'notmuch-show-open-or-close-all)
> @@ -1839,6 +1842,27 @@ argument, hide all of the messages."
>        (if show-next
>  	  (notmuch-search-show-thread)))))
>  
> +(defun notmuch-show-previous-thread (&optional show-previous)
> +  "Move to the next item in the search results, if any."
                  ^^
Should be previous item.

> +  (interactive "P")
> +  (let ((parent-buffer notmuch-show-parent-buffer))
> +    (notmuch-kill-this-buffer)
> +    (when (buffer-live-p parent-buffer)
> +      (switch-to-buffer parent-buffer)
> +      (notmuch-search-previous-thread)
> +      (if show-previous
> +	  (notmuch-search-show-thread)))))
> +

If you change this to 
   (if  (and (notmuch-search-previous-thread) show-previous)
	  (notmuch-search-show-thread)))))
then if you apply it to the first thread  it jumps back to the search menu which is
consistent with the next-thread version.

I would have a slight preference for adding another optional argument 
notmuch-show-next-thread (&optional show-message previous)

where if PREVIOUS is set then it would go back otherwise forward. But I
with the duplicated version too.

Best wishes

Mark


> +(defun notmuch-show-next-thread-show ()
> +  "Show the next thread in the search results, if any."
> +  (interactive)
> +  (notmuch-show-next-thread t))
> +
> +(defun notmuch-show-previous-thread-show ()
> +  "Show the previous thread in the search results, if any."
> +  (interactive)
> +  (notmuch-show-previous-thread t))
> +
>  (defun notmuch-show-archive-thread (&optional unarchive)
>    "Archive each message in thread.
>  
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch