Re: [PATCH v3 3/4] emacs: add refresh buffer optional no-display arg

[Ioan-Adrian Ratiu <adi@adirat.com>]

On Thu, 06 Oct 2016, Mark Walters <markwalters1009@gmail.com> wrote:
> On Thu, 06 Oct 2016, Ioan-Adrian Ratiu <adi@adirat.com> wrote:
>> Add an optional no-display arg to the generic buffer refresh function,
>> notmuch-refresh-this-buffer, which works the same way as notmuch-hello
>> mode's notmuch-hello-update no-display arg.
>>
>> The idea is for the generic notmuch-refresh-this-buffer to pass down
>> this arg to the "mode-specific" refresh functions to be able to update
>> buffers without bringing them to the foreground (if they are already
>> foregrounded, i.e. displayed in a window, this has no effect).
>>
>> When updating a search buffer, notmuch currently always brings results
>> in a window to the foreground. Perhaps counter-intuitively, we do not
>> want this behaviour necessarily, because we want to auto-refresh any
>> kind of search buffers, even those backgrounded (not displayed in any
>> window/frame) from previous searches. This is why we add a no-display
>> arg to notmuch-search.
>>
>> We do this to show which mails have appeard or dissapeared since the
>> last search refresh and have this information updated in real time
>> even when switching buffers. The ultimate goal of this is to have all
>> notmuch buffers auto-refresh when the email client syncs (this function
>> is added in the next commit).
>>
>> Signed-off-by: Ioan-Adrian Ratiu <adi@adirat.com>
>> ---
>>  emacs/notmuch-lib.el | 10 +++++++---
>>  emacs/notmuch.el     | 17 ++++++++++++-----
>>  2 files changed, 19 insertions(+), 8 deletions(-)
>>
>> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
>> index b2cdace..af6a8f4 100644
>> --- a/emacs/notmuch-lib.el
>> +++ b/emacs/notmuch-lib.el
>> @@ -413,14 +413,18 @@ of its command symbol."
>>    "Function to call to refresh the current buffer.")
>>  (make-variable-buffer-local 'notmuch-buffer-refresh-function)
>>  
>> -(defun notmuch-refresh-this-buffer ()
>> -  "Refresh the current buffer."
>> +(defun notmuch-refresh-this-buffer (&optional no-display)
>> +  "Refresh the current buffer.
>> +
>> +If no-display is non-nil do not try to bring the buffer to the
>> +foreground. If the buffer is already foregrounded i.e. displayed
>> +in a window on screen, no-display has no effect."
>>    (interactive)
>>    (when notmuch-buffer-refresh-function
>>      (if (commandp notmuch-buffer-refresh-function)
>>  	;; Pass prefix argument, etc.
>>  	(call-interactively notmuch-buffer-refresh-function)
>> -      (funcall notmuch-buffer-refresh-function))))
>> +      (funcall notmuch-buffer-refresh-function no-display))))
>
> Hi
>
> I think this is very fragile -- it relies on the fact that the refresh
> functions in show and tree mode are interactive, so get called but the
> call-interactive line (which doesn't have the no-display argument)
> whereas the refresh functions in hello and search mode are not
> interactive so get called by the funcall line and so do get the
> no-display argument.
>
> [In fact the notmuch-tree seems to bind "=" to notmuch-tree-refresh-view
> whereas it could use the generic framework, which would mean it would
> plausibly lose the interactive.]
>
> However, I am not sure what the correct solution is.

I agree it's messy, but I don't know if adding the no-display arg to
notmuch-show and notmuch-tree makes any sense because they don't force
the buffer to become visible (like how notmuch-search/hello do by using
switch-to-buffer). notmuch-show and tree assume the current buffer and
don't change it's visibility. This is how to current code works.

If it does make sense to add to them the no-display arg then we either
have to make all refresh functions interactive or non-interactive. By
doing this we can call all functions using a single code path and pass
the no-display arg to all in one call.

For notmuch-hello-update the situation looks pretty simple, it calls
notmuch-hello which is interactive. It already has the no-display arg,
we make it interactive and we're done. With notmuch-search-refresh-view
the situation is similar. Then call-interactively all and pass no-display.

Making them all non-interactive seems much harder and I think will
likely break stuff. So I prefer 1. using the code as is or 2. making
all interactive.

Any other ideas, everyone? I'm open to any kind of sugestions on this.

Ionel

>
> Best wishes
>
> Mark
>
>
>
>>  
>>  (defun notmuch-poll-and-refresh-this-buffer ()
>>    "Invoke `notmuch-poll' to import mail, then refresh the current buffer."
>> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
>> index 586c84e..f3912d4 100644
>> --- a/emacs/notmuch.el
>> +++ b/emacs/notmuch.el
>> @@ -925,7 +925,7 @@ PROMPT is the string to prompt with."
>>  
>>  ;;;###autoload
>>  (put 'notmuch-search 'notmuch-doc "Search for messages.")
>> -(defun notmuch-search (&optional query oldest-first target-thread target-line)
>> +(defun notmuch-search (&optional query oldest-first target-thread target-line no-display)
>>    "Display threads matching QUERY in a notmuch-search buffer.
>>  
>>  If QUERY is nil, it is read interactively from the minibuffer.
>> @@ -936,6 +936,9 @@ Other optional parameters are used as follows:
>>                   current if it appears in the search results.
>>    TARGET-LINE: The line number to move to if the target thread does not
>>                 appear in the search results.
>> +  NO-DISPLAY: Do not try to foreground the search results buffer. If it is
>> +              already foregrounded i.e. displayed in a window, this has no
>> +              effect, meaning the buffer will remain visible.
>>  
>>  When called interactively, this will prompt for a query and use
>>  the configured default sort order."
>> @@ -949,7 +952,9 @@ the configured default sort order."
>>  
>>    (let* ((query (or query (notmuch-read-query "Notmuch search: ")))
>>  	 (buffer (get-buffer-create (notmuch-search-buffer-title query))))
>> -    (switch-to-buffer buffer)
>> +    (if no-display
>> +	(set-buffer buffer)
>> +      (switch-to-buffer buffer))
>>      (notmuch-search-mode)
>>      ;; Don't track undo information for this buffer
>>      (set 'buffer-undo-list t)
>> @@ -982,14 +987,16 @@ the configured default sort order."
>>  	  (set-process-query-on-exit-flag proc nil))))
>>      (run-hooks 'notmuch-search-hook)))
>>  
>> -(defun notmuch-search-refresh-view ()
>> +(defun notmuch-search-refresh-view (&optional no-display)
>>    "Refresh the current view.
>>  
>>  Erases the current buffer and runs a new search with the same
>>  query string as the current search. If the current thread is in
>>  the new search results, then point will be placed on the same
>>  thread. Otherwise, point will be moved to attempt to be in the
>> -same relative position within the new buffer."
>> +same relative position within the new buffer. If no-display is
>> +non-nil, the search results buffer will not be foregrounded, if
>> +it already is displayed in a window, then no-display has no effect."
>>    (let ((target-line (line-number-at-pos))
>>  	(oldest-first notmuch-search-oldest-first)
>>  	(target-thread (notmuch-search-find-thread-id 'bare))
>> @@ -997,7 +1004,7 @@ same relative position within the new buffer."
>>  	(inhibit-read-only t))
>>      (remove-overlays)
>>      (erase-buffer)
>> -    (notmuch-search query oldest-first target-thread target-line)
>> +    (notmuch-search query oldest-first target-thread target-line no-display)
>>      (goto-char (point-min))))
>>  
>>  (defun notmuch-search-toggle-order ()
>> -- 
>> 2.10.0