From: Mark Walters <markwalters1009@gmail.com>
To: David Bremner <david@tethera.net>, notmuch@notmuchmail.org
Subject: Re: [Patch v7 3/3] Emacs: Add address completion based on
 company-mode
In-Reply-To: <1445786638-13763-4-git-send-email-david@tethera.net>
References: <1445786638-13763-1-git-send-email-david@tethera.net>
 <1445786638-13763-4-git-send-email-david@tethera.net>
User-Agent: Notmuch/0.21~rc1+29~g057f24d (http://notmuchmail.org) Emacs/23.4.1
 (x86_64-pc-linux-gnu)
Date: Sun, 25 Oct 2015 17:26:25 +0000
Message-ID: <87bnbmdflq.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list


On Sun, 25 Oct 2015, David Bremner <david@tethera.net> wrote:
> From: Michal Sojka <sojkam1@fel.cvut.cz>
>
> With this patch, address completion candidates are shown automatically
> after short typing delay in a nice popup box. This requires company-mode
> to be installed and it works only on Emacs >= 24. The completion is
> based entirely on the asynchronous address harvesting from
> notmuch-address.el so the GUI is theoretically not blocked for long
> time.
>
> The completion works similarly as the TAB-initiated completion from
> notmuch-address.el, i.e. quick harvest based on user input is executed
> first and only after full harvesting is finished, in-memory cached data
> is used.
> ---

This series looks good to me. There are a couple of very small nits that
might be worth fixing (on the level of typos/whitespace see below and a
reply to patch 2). But +1 from me for this version.


Best wishes

Mark

>  emacs/Makefile.local     |  1 +
>  emacs/notmuch-address.el | 18 ++++++++--
>  emacs/notmuch-company.el | 86 ++++++++++++++++++++++++++++++++++++++++++++++++
>  emacs/notmuch-mua.el     |  2 +-
>  4 files changed, 104 insertions(+), 3 deletions(-)
>  create mode 100644 emacs/notmuch-company.el
>
> diff --git a/emacs/Makefile.local b/emacs/Makefile.local
> index 1109cfa..4c06c52 100644
> --- a/emacs/Makefile.local
> +++ b/emacs/Makefile.local
> @@ -20,6 +20,7 @@ emacs_sources := \
>  	$(dir)/notmuch-print.el \
>  	$(dir)/notmuch-version.el \
>  	$(dir)/notmuch-jump.el \
> +	$(dir)/notmuch-company.el
>  
>  $(dir)/notmuch-version.el: $(dir)/Makefile.local version.stamp
>  $(dir)/notmuch-version.el: $(srcdir)/$(dir)/notmuch-version.el.tmpl
> diff --git a/emacs/notmuch-address.el b/emacs/notmuch-address.el
> index aa6228d..5456d5c 100644
> --- a/emacs/notmuch-address.el
> +++ b/emacs/notmuch-address.el
> @@ -22,7 +22,9 @@
>  (require 'message)
>  (require 'notmuch-parser)
>  (require 'notmuch-lib)
> +(require 'notmuch-company)
>  ;;
> +(declare-function company-manual-begin "company")
>  
>  (defcustom notmuch-address-command 'internal
>    "The command which generates possible addresses. It must take a
> @@ -72,9 +74,21 @@ finished")
>  (defun notmuch-address-message-insinuate ()
>    (message "calling notmuch-address-message-insinuate is no longer needed"))
>  
> +(defcustom notmuch-address-use-company t
> +  "If available, use company mode for address completion"
> +  :type 'boolean
> +  :group 'notmuch-send)
> +
>  (defun notmuch-address-setup ()
> -  (let ((pair (cons notmuch-address-completion-headers-regexp
> -		    #'notmuch-address-expand-name)))
> +  (let* ((use-company (and notmuch-address-use-company
> +			   (eq notmuch-address-command 'internal)
> +			   (require 'company nil t)))
> +	 (pair (cons notmuch-address-completion-headers-regexp
> +		     (if use-company
> +			 #'company-manual-begin
> +		       #'notmuch-address-expand-name))))
> +      (when use-company
> +	(notmuch-company-setup))
>        (unless (memq pair message-completion-alist)
>  	(setq message-completion-alist
>  	      (push pair message-completion-alist)))))
> diff --git a/emacs/notmuch-company.el b/emacs/notmuch-company.el
> new file mode 100644
> index 0000000..49d1d81
> --- /dev/null
> +++ b/emacs/notmuch-company.el
> @@ -0,0 +1,86 @@
> +;; notmuch-company.el --- Mail address completion for notmuch via company-mode  -*- lexical-binding: t -*-
> +
> +;; Authors: Trevor Jim <tjim@mac.com>
> +;; 	    Michal Sojka <sojkam1@fel.cvut.cz>
> +;;
> +;; Keywords: mail, completion
> +
> +;; This program is free software; you can redistribute it and/or modify
> +;; it under the terms of the GNU General Public License as published by
> +;; the Free Software Foundation, either version 3 of the License, or
> +;; (at your option) any later version.
> +
> +;; This program is distributed in the hope that it will be useful,
> +;; but WITHOUT ANY WARRANTY; without even the implied warranty of
> +;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +;; GNU General Public License for more details.
> +
> +;; You should have received a copy of the GNU General Public License
> +;; along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +;;; Commentary:
> +
> +;; To enable this, install company mode (https://company-mode.github.io/)
> +;;
> +;; NB company-minimum-prefix-length defaults to 3 so you don't get
> +;; completion unless you type 3 characters
> +
> +;;; Code:
> +
> +(eval-when-compile (require 'cl))
> +
> +(defvar notmuch-company-last-prefix nil)
> +(make-variable-buffer-local 'notmuch-company-last-prefix)
> +(declare-function company-begin-backend "company")
> +(declare-function company-grab "company")
> +(declare-function company-mode "company")
> +(declare-function company-manual-begin "company")
> +(defvar company-backends)
> +
> +(declare-function notmuch-address-harvest "notmuch-address")
> +(declare-function notmuch-address-harvest-trigger "notmuch-address")
> +(declare-function notmuch-address-matching "notmuch-address")
> +(defvar notmuch-address-full-harvest-finished)
> +(defvar notmuch-address-completion-headers-regexp)
> +
> +;;;###autoload
> +(defun notmuch-company-setup ()
> +  (company-mode)
> +  (make-local-variable 'company-backends)
> +  (setq company-backends '(notmuch-company)))
> +
> +;;;###autoload
> +(defun notmuch-company (command &optional arg &rest _ignore)
> +  "`company-mode' completion back-end for `notmuch'."
> +  (interactive (list 'interactive))
> +  (require 'company)
> +  (let ((case-fold-search t)
> +	(completion-ignore-case t))
> +    (case command
> +      (interactive (company-begin-backend 'notmuch-company))
> +      (prefix (and (derived-mode-p 'message-mode)
> +		   (looking-back (concat notmuch-address-completion-headers-regexp ".*")
> +				 (line-beginning-position))
> +		   (setq notmuch-company-last-prefix (company-grab "[:,][ \t]*\\(.*\\)" 1 (point-at-bol)))))
> +      (candidates (cond
> +		   (notmuch-address-full-harvest-finished
> +		    ;; Update harvested addressed from time to time
> +		    (notmuch-address-harvest-trigger)
> +		    (notmuch-address-matching arg))
> +		   (t
> +		    (cons :async
> +			  (lambda (callback)
> +			    ;; First run quick asynchronous harvest based on what the user entered so far
> +			    (notmuch-address-harvest
> +			     (format "to:%s*" arg) nil
> +			     (lambda (_proc _event)
> +			       (funcall callback (notmuch-address-matching arg))
> +			       ;; Then (re)start potentially long-running full asynchronous harvesting
> +			       (notmuch-address-harvest-trigger))))))))

Could we reword this comment? restart definitely suggests to me starting
a stopped/paused thing not starting it for a second run. Perhaps "Start
the (potentially long-running) full asynchronous harvest if necessary"?
(The same comment occurs in Patch 2)

 
> +      (match (if (string-match notmuch-company-last-prefix arg)
> +		 (match-end 0)
> +	       0))
> +      (no-cache t))))
> +
> +
> +(provide 'notmuch-company)
> diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
> index fd98ea4..c12054c 100644
> --- a/emacs/notmuch-mua.el
> +++ b/emacs/notmuch-mua.el
> @@ -271,7 +271,7 @@ Note that these functions use `mail-citation-hook' if that is non-nil."
>  (define-derived-mode notmuch-message-mode message-mode "Message[Notmuch]"
>    "Notmuch message composition mode. Mostly like `message-mode'"
>    (when notmuch-address-command
> -    (notmuch-address-setup)))
> +      (notmuch-address-setup)))

An accidental whitespace change? There were also a couple on
added/deleted lines in the earlier patches.

>  
>  (define-key notmuch-message-mode-map (kbd "C-c C-c") #'notmuch-mua-send-and-exit)
>  (define-key notmuch-message-mode-map (kbd "C-c C-s") #'notmuch-mua-send)
> -- 
> 2.6.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch