On Sun, May 05 2013, Mark Walters <markwalters1009@gmail.com> wrote:

> 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.

notmuch-search-next-thread & notmuch-search-previous-thread behaves
differently when it comes to the end... previous always points
to thread but next goes to the end. This is OK for interactive point
of view but for noninteractive use one needs to do special handling...

> 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.

... what we need is lower level (noninteractive) function (taking PREVIOUS
as an arg) which signals when it cannot get next/previous function...
and then this notmuch-show-next-thread (&optional show-message previous)
just calls it (with proper handling block) in place of
notmuch-search-next-thread (maybe notmuch-search-previous-or-next-thread).

This would be a bit cleaner.

If this is not done, then I'd just go with Mark's suggestion of checking
the return value of notmuch-search-previous-thread, and if you (jani)
wish combine this functionality to notmuch-show-next-thread.


> Best wishes
>
> Mark

Tomi

>
>
>> +(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