[PATCH 0/3] emacs: allow opting out of notmuch's address completion

[PATCH 0/3] emacs: allow opting out of notmuch's address completion

[Jonas Bernoulli]

Hello

Notmuch's address completion didn't work well for me.  I read the
respective code and found some issues, some of which are difficult
to address.

I wrote my own implementation, which might eventually be suitable as
a replacement for the current implementation, but it is not ready yet.

In a first step I would like to make it possible to use notmuch and
some other address completion mechanism without having to advice a
notmuch-address.el function to prevent it making destructive changes.
The second commit in this series does that.  Here is the hack I am
currently using instead:

,----
| ;;; Counteract notmuch-address.el
| 
| (defun notmuch-address-setup--noop (_fn)
|   "Prevent modification of `message-completion-alist'.")
| (advice-add 'notmuch-address-setup :around
|             'notmuch-address-setup--noop)
`----

You can find my implementation (named notmuch-addr.el because it is
like notmuch-address.el, but "smaller") here:

,----
| https://git.sr.ht/~tarsius/notmuch-addr
`----

The main reason I am not trying to improve that until it can serve as
a replacement for notmuch-address.el is that it depends on Emacs 27.1,
which was just released. (Aside from that it also omits features that
*I* don't need.)

The main reason I am listing defects of notmuch-address.el below is
that by the time we can fix all of them (when we drop support for
Emacs releases before 27.1) I will have forgotten about them, so it
seem like a good idea to document them.

* Emacs' address completion API used to be rather wacky and that was
  not fixed until version 27.1. "Callers" didn't merely have to
  provide a list of completion candidate, instead they actually had to
  perform completion themselves.  Starting with 27.1 the completion-
  at-point API is respected but there are kludges to support the old
  style as well, see https://git.savannah.gnu.org/cgit/emacs.git/com
  mit/?id=47a767c24e9cc4323432e29103b0a2cc46f8f3e4.

  Using capf API has the advantage that many things don't have to be
  re-implemented.  For example `company-mode' just works.  Notmuch
  currently has to implement support explicitly in notmuch-company.el.

* Some essentially random completion candidate is used as the "initial
  input".  The last commit in this series (which see) fixes that.

* The special cases when there is not matching candidate or just a
  single match are handled specifically, which IMO is an optimization
  that makes things worse.  That should also be an easy fix, but since
  it might also be a controversial change, I did not implement it.

* Completion candidates are pre-filered based on the text that was
  already at-point before at-point completion was invoked, which makes
  it possible to choose candidates that that initial text does not
  match.  IMO that's another optimization that badly back fires.

  I was surprised to learn that notmuch shared this feature/defect
  with the current default address implementation in Emacs 27.1.

  The initial commit of my implementation also shares this defect;
  but only so that I can use the second commit to demonstrate how
  that can be fixed.

     Cheers,
     Jonas

Jonas Bernoulli (3):
  emacs: notmuch-address-setup: cosmetics
  emacs: allow opting out of notmuch's address completion
  emacs: notmuch-address-expand-name: use the actual initial-input

 emacs/notmuch-address.el | 57 ++++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 28 deletions(-)

-- 
2.29.1

[PATCH 1/3] emacs: notmuch-address-setup: cosmetics

[Jonas Bernoulli]

---
 emacs/notmuch-address.el | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/emacs/notmuch-address.el b/emacs/notmuch-address.el
index 8a6d299c..3518beef 100644
--- a/emacs/notmuch-address.el
+++ b/emacs/notmuch-address.el
@@ -21,6 +21,8 @@
 
 ;;; Code:
 
+(eval-when-compile (require 'cl-lib))
+
 (require 'message)
 (require 'notmuch-parser)
 (require 'notmuch-lib)
@@ -154,15 +156,12 @@ (defcustom notmuch-address-use-company t
   :group 'notmuch-address)
 
 (defun notmuch-address-setup ()
-  (let* ((setup-company (and notmuch-address-use-company
-			     (require 'company nil t)))
-	 (pair (cons notmuch-address-completion-headers-regexp
-		     #'notmuch-address-expand-name)))
-    (when setup-company
-      (notmuch-company-setup))
-    (unless (member pair message-completion-alist)
-      (setq message-completion-alist
-	    (push pair message-completion-alist)))))
+  (when (and notmuch-address-use-company
+	     (require 'company nil t))
+    (notmuch-company-setup))
+  (cl-pushnew (cons notmuch-address-completion-headers-regexp
+		    #'notmuch-address-expand-name)
+	      message-completion-alist :test #'equal))
 
 (defun notmuch-address-toggle-internal-completion ()
   "Toggle use of internal completion for current buffer.
-- 
2.29.1

[PATCH 2/3] emacs: allow opting out of notmuch's address completion

[Jonas Bernoulli]

IMO Notmuch should not override the default completion mechanism by
default, at least not globally. But since users are already used to
this behavior it is probably too late to change it. Do the next best
thing and at least allow users to opt out.
---
 emacs/notmuch-address.el | 48 +++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 20 deletions(-)

diff --git a/emacs/notmuch-address.el b/emacs/notmuch-address.el
index 3518beef..6e29b99a 100644
--- a/emacs/notmuch-address.el
+++ b/emacs/notmuch-address.el
@@ -52,21 +52,28 @@ (defun notmuch-address--harvest-ready ()
 (defcustom notmuch-address-command 'internal
   "Determines how address completion candidates are generated.
 
-If it is a string then that string should be an external program
-which must take a single argument (searched string) and output a
-list of completion candidates, one per line.
-
-Alternatively, it can be the symbol `internal', in which case
-internal completion is used; the variable
-`notmuch-address-internal-completion' can be used to customize
-this case.
-
-Finally, if this variable is nil then address completion is
-disabled."
+If this is a string, then that string should be an external
+program, which must take a single argument (searched string)
+and output a list of completion candidates, one per line.
+
+If this is the symbol `internal', then an implementation is used
+that relies on the \"notmuch address\" command, but does not use
+any third-party (i.e. \"external\") programs.
+
+If this is the symbol `as-is', then Notmuch does not modify the
+value of `message-completion-alist'. This option has to be set to
+this value before `notmuch' is loaded, otherwise the modification
+to `message-completion-alist' may already have taken place. This
+setting obviously does not prevent `message-completion-alist'
+from being modified at all; the user or some third-party package
+may still modify it.
+
+Finally, if this is nil, then address completion is disabled."
   :type '(radio
-	  (const :tag "Use internal address completion" internal)
-	  (const :tag "Disable address completion" nil)
-	  (string :tag "Use external completion command"))
+	  (const  :tag "Use internal address completion" internal)
+	  (string :tag "Use external completion command")
+	  (const  :tag "Disable address completion" nil)
+	  (const  :tag "Use default or third-party mechanism" as-is))
   :group 'notmuch-send
   :group 'notmuch-address
   :group 'notmuch-external)
@@ -156,12 +163,13 @@ (defcustom notmuch-address-use-company t
   :group 'notmuch-address)
 
 (defun notmuch-address-setup ()
-  (when (and notmuch-address-use-company
-	     (require 'company nil t))
-    (notmuch-company-setup))
-  (cl-pushnew (cons notmuch-address-completion-headers-regexp
-		    #'notmuch-address-expand-name)
-	      message-completion-alist :test #'equal))
+  (unless (eq notmuch-address-command 'as-is)
+    (when (and notmuch-address-use-company
+	       (require 'company nil t))
+      (notmuch-company-setup))
+    (cl-pushnew (cons notmuch-address-completion-headers-regexp
+		      #'notmuch-address-expand-name)
+		message-completion-alist :test #'equal)))
 
 (defun notmuch-address-toggle-internal-completion ()
   "Toggle use of internal completion for current buffer.
-- 
2.29.1

[PATCH 3/3] emacs: notmuch-address-expand-name: use the actual initial-input

[Jonas Bernoulli]

Users may type some text into the buffer on an address line, before
actually invoking address completion.  We now use that text as the
initial input when we begin address completion.

Previously we did knowingly replace the actual initial input with some
completion candidate that happens to match. Which candidate is used is
essentially random, at least when the actual initial input is short.
As a result users very often had to begin completion by deleting the
less than helpful "initial input".
---
 emacs/notmuch-address.el | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/emacs/notmuch-address.el b/emacs/notmuch-address.el
index 6e29b99a..591ad7ae 100644
--- a/emacs/notmuch-address.el
+++ b/emacs/notmuch-address.el
@@ -238,14 +238,8 @@ (defun notmuch-address-expand-name ()
 		    (t
 		     (funcall notmuch-address-selection-function
 			      (format "Address (%s matches): " num-options)
-			      ;; We put the first match as the initial
-			      ;; input; we put all the matches as
-			      ;; possible completions, moving the
-			      ;; first match to the end of the list
-			      ;; makes cursor up/down in the list work
-			      ;; better.
-			      (append (cdr options) (list (car options)))
-			      (car options))))))
+			      options
+			      orig)))))
       (if chosen
 	  (progn
 	    (push chosen notmuch-address-history)
-- 
2.29.1