From: Mark Walters <markwalters1009@gmail.com>
To: David Edmondson <dme@dme.org>, notmuch@notmuchmail.org
Subject: Re: [PATCH v2] emacs: Report a lack of matches when calling
 `notmuch-show'.
In-Reply-To: <1455112878-23497-2-git-send-email-dme@dme.org>
References: <1455112878-23497-1-git-send-email-dme@dme.org>
 <1455112878-23497-2-git-send-email-dme@dme.org>
User-Agent: Notmuch/0.18.1+485~gca076ce (http://notmuchmail.org) Emacs/24.4.1
 (x86_64-pc-linux-gnu)
Date: Wed, 10 Feb 2016 18:50:14 +0000
Message-ID: <87oabo5rix.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: list



On Wed, 10 Feb 2016, David Edmondson <dme@dme.org> wrote:
> If the basic query passed to `notmuch-show' generates no results, ring
> the bell and inform the user that no messages matched the query rather
> than displaying an empty buffer and showing an obscure error.
>
> Similarly when refreshing a `notmuch-show' buffer and no messages match.

This basically looks fine to me and all tests pass. The code movement
and cleanup all looks fine.  Two minor things, one tiny nit below; and I
wonder whether just having the buffer say "No search results" (or
something similar) and leave the user to kill it would be nicer than
dinging (and more in line with the way search and tree behave).

[In some sense I think this way is right and search and tree are wrong,
but that is probably difficult to get round as search and tree run
asynchronously.]

Best wishes

Mark


> ---
>  emacs/notmuch-show.el | 144 +++++++++++++++++++++++++++++---------------------
>  emacs/notmuch.el      |   6 ++-
>  2 files changed, 89 insertions(+), 61 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 3345878..9a74848 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -153,27 +153,21 @@ indentation."
>  
>  (defvar notmuch-show-thread-id nil)
>  (make-variable-buffer-local 'notmuch-show-thread-id)
> -(put 'notmuch-show-thread-id 'permanent-local t)
>  
>  (defvar notmuch-show-parent-buffer nil)
>  (make-variable-buffer-local 'notmuch-show-parent-buffer)
> -(put 'notmuch-show-parent-buffer 'permanent-local t)
>  
>  (defvar notmuch-show-query-context nil)
>  (make-variable-buffer-local 'notmuch-show-query-context)
> -(put 'notmuch-show-query-context 'permanent-local t)
>  
>  (defvar notmuch-show-process-crypto nil)
>  (make-variable-buffer-local 'notmuch-show-process-crypto)
> -(put 'notmuch-show-process-crypto 'permanent-local t)
>  
>  (defvar notmuch-show-elide-non-matching-messages nil)
>  (make-variable-buffer-local 'notmuch-show-elide-non-matching-messages)
> -(put 'notmuch-show-elide-non-matching-messages 'permanent-local t)
>  
>  (defvar notmuch-show-indent-content t)
>  (make-variable-buffer-local 'notmuch-show-indent-content)
> -(put 'notmuch-show-indent-content 'permanent-local t)
>  
>  (defvar notmuch-show-attachment-debug nil
>    "If t log stdout and stderr from attachment handlers
> @@ -1197,71 +1191,101 @@ non-nil.
>  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."
> +function is used.
> +
> +Returns the buffer containing the messages, or NIL if no messages
> +matched."
>    (interactive "sNotmuch show: \nP")
>    (let ((buffer-name (generate-new-buffer-name
>  		      (or buffer-name
>  			  (concat "*notmuch-" thread-id "*")))))
>      (switch-to-buffer (get-buffer-create buffer-name))
> -    ;; Set the default value for `notmuch-show-process-crypto' in this
> -    ;; buffer.
> -    (setq notmuch-show-process-crypto notmuch-crypto-process-mime)
> -    ;; Set the default value for
> -    ;; `notmuch-show-elide-non-matching-messages' in this buffer. If
> -    ;; elide-toggle is set, invert the default.
> -    (setq notmuch-show-elide-non-matching-messages notmuch-show-only-matching-messages)
> -    (if elide-toggle
> -	(setq notmuch-show-elide-non-matching-messages (not notmuch-show-elide-non-matching-messages)))
> +    ;; No need to track undo information for this buffer.
> +    (setq buffer-undo-list t)
> +
> +    (notmuch-show-mode)
>  
> +    ;; Set various buffer local variables to their appropriate initial
> +    ;; state. Do this after enabling `notmuch-show-mode' so that they
> +    ;; aren't wiped out.
>      (setq notmuch-show-thread-id thread-id
>  	  notmuch-show-parent-buffer parent-buffer
> -	  notmuch-show-query-context query-context)
> -    (notmuch-show-build-buffer)
> -    (notmuch-show-goto-first-wanted-message)
> -    (current-buffer)))
> +	  notmuch-show-query-context query-context
>  
> -(defun notmuch-show-build-buffer ()
> -  (let ((inhibit-read-only t))
> +	  notmuch-show-process-crypto notmuch-crypto-process-mime
> +	  ;; If `elide-toggle', invert the default value.
> +	  notmuch-show-elide-non-matching-messages
> +	  (if elide-toggle
> +	      (not notmuch-show-only-matching-messages)
> +	    notmuch-show-only-matching-messages))
>  
> -    (notmuch-show-mode)
>      (add-hook 'post-command-hook #'notmuch-show-command-hook nil t)
> -
> -    ;; Don't track undo information for this buffer
> -    (set 'buffer-undo-list t)
> +    (jit-lock-register #'notmuch-show-buttonise-links)
>  
>      (notmuch-tag-clear-cache)
> -    (erase-buffer)
> -    (goto-char (point-min))
> -    (save-excursion
> -      (let* ((basic-args (list notmuch-show-thread-id))
> -	     (args (if notmuch-show-query-context
> -		       (append (list "\'") basic-args
> -			       (list "and (" notmuch-show-query-context ")\'"))
> -		     (append (list "\'") basic-args (list "\'"))))
> -	     (cli-args (cons "--exclude=false"
> -			     (when notmuch-show-elide-non-matching-messages
> -			       (list "--entire-thread=false")))))
> -
> -	(notmuch-show-insert-forest (notmuch-query-get-threads (append cli-args args)))
> -	;; If the query context reduced the results to nothing, run
> -	;; the basic query.
> -	(when (and (eq (buffer-size) 0)
> -		   notmuch-show-query-context)
> -	  (notmuch-show-insert-forest
> -	   (notmuch-query-get-threads (append cli-args basic-args)))))
> -
> -      (jit-lock-register #'notmuch-show-buttonise-links)
> -
> -      (notmuch-show-mapc (lambda () (notmuch-show-set-prop :orig-tags (notmuch-show-get-tags))))
> +
> +    (let ((inhibit-read-only t))
> +      (if (notmuch-show--build-buffer)
> +	  ;; Messages were inserted into the buffer.
> +	  (current-buffer)
> +
> +	;; No messages were inserted - presumably none matched the
> +	;; query.
> +	(kill-buffer (current-buffer))
> +	(ding)
> +	(message "No messages matched the query!")
> +	nil))))
> +
> +(defun notmuch-show--build-buffer (&optional state)
> +  "Display messages matching the current buffer context.
> +
> +Apply the previously saved STATE if supplied, otherwise show the
> +first relevant message.
> +
> +If no messages match the query return NIL."
> +  (let* ((basic-args (list notmuch-show-thread-id))
> +	 (args (if notmuch-show-query-context
> +		   (append (list "\'") basic-args
> +			   (list "and (" notmuch-show-query-context ")\'"))
> +		 (append (list "\'") basic-args (list "\'"))))
> +	 (cli-args (cons "--exclude=false"
> +			 (when notmuch-show-elide-non-matching-messages
> +			   (list "--entire-thread=false"))))
> +
> +	 (forest (or (notmuch-query-get-threads (append cli-args args))
> +		     ;; If a query context reduced the number of
> +		     ;; results to zero, try again without it.
> +		     (and notmuch-show-query-context
> +			  (notmuch-query-get-threads (append cli-args basic-args)))))
> +
> +	 ;; Must be reset every time we are going to start inserting
> +	 ;; messages into the buffer.
> +	 (notmuch-show-previous-subject ""))
> +
> +    (when forest
> +      (notmuch-show-insert-forest forest)
> +
> +      ;; Cache the original tags for each message so that we can display
> +      ;; changes.

^^ I think "Store the original tags for each message" would be better,
particularly as this is nothing to do with the tag cache as used by say
notmuch-tag-clear-cache.


> +      (notmuch-show-mapc
> +       (lambda () (notmuch-show-set-prop :orig-tags (notmuch-show-get-tags))))
>  
>        ;; Set the header line to the subject of the first message.
>        (setq header-line-format
>  	    (replace-regexp-in-string "%" "%%"
> -			    (notmuch-sanitize
> -			     (notmuch-show-strip-re
> -			      (notmuch-show-get-subject)))))
> +				      (notmuch-sanitize
> +				       (notmuch-show-strip-re
> +					(notmuch-show-get-subject)))))
>  
> -      (run-hooks 'notmuch-show-hook))))
> +      (run-hooks 'notmuch-show-hook)
> +
> +      (if state
> +	  (notmuch-show-apply-state state)
> +	;; With no state to apply, just go to the first message.
> +	(notmuch-show-goto-first-wanted-message)))
> +
> +    ;; Report back to the caller whether any messages matched.
> +    forest))
>  
>  (defun notmuch-show-capture-state ()
>    "Capture the state of the current buffer.
> @@ -1320,17 +1344,17 @@ reset based on the original query."
>    (let ((inhibit-read-only t)
>  	(state (unless reset-state
>  		 (notmuch-show-capture-state))))
> -    ;; erase-buffer does not seem to remove overlays, which can lead
> +    ;; `erase-buffer' does not seem to remove overlays, which can lead
>      ;; to weird effects such as remaining images, so remove them
>      ;; manually.
>      (remove-overlays)
>      (erase-buffer)
> -    (notmuch-show-build-buffer)
> -    (if state
> -	(notmuch-show-apply-state state)
> -      ;; We're resetting state, so navigate to the first open message
> -      ;; and mark it read, just like opening a new show buffer.
> -      (notmuch-show-goto-first-wanted-message))))
> +
> +    (unless (notmuch-show--build-buffer state)
> +      ;; No messages were inserted.
> +      (kill-buffer (current-buffer))
> +      (ding)
> +      (message "Refreshing the buffer resulted in no messages!"))))
>  
>  (defvar notmuch-show-stash-map
>    (let ((map (make-sparse-keymap)))
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 463b926..3100b97 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -458,7 +458,11 @@ no messages in the region then return nil."
>    (notmuch-search-properties-in-region :subject beg end))
>  
>  (defun notmuch-search-show-thread (&optional elide-toggle)
> -  "Display the currently selected thread."
> +  "Display the currently selected thread.
> +
> +With a prefix argument, invert the default value of
> +`notmuch-show-only-matching-messages' when displaying the
> +thread."
>    (interactive "P")
>    (let ((thread-id (notmuch-search-find-thread-id))
>  	(subject (notmuch-search-find-subject)))
> -- 
> 2.1.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch