From: Michal Sojka Date: Fri, 13 May 2016 21:41:58 +0000 (+0200) Subject: Re: [PATCH v2] emacs: address completion, allow sender/recipient and filters X-Git-Url: http://git.tremily.us/?a=commitdiff_plain;h=d693954c55df05032d6fa6459426581574f9e867;p=notmuch-archives.git Re: [PATCH v2] emacs: address completion, allow sender/recipient and filters --- diff --git a/be/52c846d7448babdfd41be89c3c2f924a306afa b/be/52c846d7448babdfd41be89c3c2f924a306afa new file mode 100644 index 000000000..5a595a42a --- /dev/null +++ b/be/52c846d7448babdfd41be89c3c2f924a306afa @@ -0,0 +1,307 @@ +Return-Path: +X-Original-To: notmuch@notmuchmail.org +Delivered-To: notmuch@notmuchmail.org +Received: from localhost (localhost [127.0.0.1]) + by arlo.cworth.org (Postfix) with ESMTP id 1DB6F6DE02B2 + for ; Fri, 13 May 2016 14:42:12 -0700 (PDT) +X-Virus-Scanned: Debian amavisd-new at cworth.org +X-Spam-Flag: NO +X-Spam-Score: -1.635 +X-Spam-Level: +X-Spam-Status: No, score=-1.635 tagged_above=-999 required=5 tests=[AWL=0.675, + RCVD_IN_DNSWL_MED=-2.3, T_RP_MATCHES_RCVD=-0.01] autolearn=disabled +Received: from arlo.cworth.org ([127.0.0.1]) + by localhost (arlo.cworth.org [127.0.0.1]) (amavisd-new, port 10024) + with ESMTP id osPu82yF95Js for ; + Fri, 13 May 2016 14:42:03 -0700 (PDT) +Received: from max.feld.cvut.cz (max.feld.cvut.cz [147.32.192.36]) + by arlo.cworth.org (Postfix) with ESMTP id 2C9646DE0317 + for ; Fri, 13 May 2016 14:42:03 -0700 (PDT) +Received: from localhost (unknown [192.168.200.7]) + by max.feld.cvut.cz (Postfix) with ESMTP id 977F45CD662; + Fri, 13 May 2016 23:42:01 +0200 (CEST) +X-Virus-Scanned: IMAP STYX AMAVIS +Received: from max.feld.cvut.cz ([192.168.200.1]) + by localhost (styx.feld.cvut.cz [192.168.200.7]) (amavisd-new, port 10044) + with ESMTP id oFXwixJwOxl4; Fri, 13 May 2016 23:41:59 +0200 (CEST) +Received: from imap.feld.cvut.cz (imap.feld.cvut.cz [147.32.192.34]) + by max.feld.cvut.cz (Postfix) with ESMTP id 974735CD275; + Fri, 13 May 2016 23:41:59 +0200 (CEST) +Received: from wsh by steelpick.2x.cz with local (Exim 4.87) + (envelope-from ) + id 1b1KqM-00014s-Va; Fri, 13 May 2016 23:41:58 +0200 +From: Michal Sojka +To: Mark Walters , notmuch@notmuchmail.org +Subject: Re: [PATCH v2] emacs: address completion, + allow sender/recipient and filters +In-Reply-To: <1463153337-11381-1-git-send-email-markwalters1009@gmail.com> +References: <874mf2yeq7.fsf@steelpick.2x.cz> + <1463153337-11381-1-git-send-email-markwalters1009@gmail.com> +User-Agent: Notmuch/0.22 (http://notmuchmail.org) Emacs/24.5.1 + (x86_64-pc-linux-gnu) +Date: Fri, 13 May 2016 23:41:58 +0200 +Message-ID: <87h9e11vq1.fsf@steelpick.2x.cz> +MIME-Version: 1.0 +Content-Type: text/plain +X-BeenThere: notmuch@notmuchmail.org +X-Mailman-Version: 2.1.20 +Precedence: list +List-Id: "Use and development of the notmuch mail system." + +List-Unsubscribe: , + +List-Archive: +List-Post: +List-Help: +List-Subscribe: , + +X-List-Received-Date: Fri, 13 May 2016 21:42:12 -0000 + +Hi Mark, + +On Fri, May 13 2016, Mark Walters wrote: +> This commit lets the user customize the address completion. +> +> The first change controls whether to build the address completion list +> based on messages you have sent or you have received (the latter is +> much faster). +> +> The second change add a possible filter query to limit the messages +> used -- for example, setting this to date:1y.. would limit the +> address completions to addresses used in the last year. This speeds up +> the address harvest and may also make the search less cluttered as old +> addresses may well no longer be valid. +> --- +> +> I have fixed the bug pointed out by Michal in his review. It seems to +> work, but there are a lot of possible configurations, +> +> I also don't like my docstring for the notmuch-address-command +> defcustom, so any suggestions gratefully received. +> +> Also, I am not sure whether this is all to much complexity for this +> feature. + +A big part of the patch is moving variable definitions and I'd say the +rest is OK (complexity-wise). Other comments below. + +> emacs/notmuch-address.el | 104 +++++++++++++++++++++++++++++++---------------- +> emacs/notmuch-company.el | 2 +- +> 2 files changed, 71 insertions(+), 35 deletions(-) +> +> diff --git a/emacs/notmuch-address.el b/emacs/notmuch-address.el +> index aafbe5f..8b84a4c 100644 +> --- a/emacs/notmuch-address.el +> +++ b/emacs/notmuch-address.el +> @@ -28,15 +28,50 @@ +> ;; +> (declare-function company-manual-begin "company") +> +> -(defcustom notmuch-address-command 'internal +> - "The command which generates possible addresses. It must take a +> -single argument and output a list of possible matches, one per +> -line. The default value of `internal' uses built-in address +> -completion." +> +(defvar notmuch-address-last-harvest 0 +> + "Time of last address harvest") +> + +> +(defvar notmuch-address-completions (make-hash-table :test 'equal) +> + "Hash of email addresses for completion during email composition. +> + This variable is set by calling `notmuch-address-harvest'.") +> + +> +(defvar notmuch-address-full-harvest-finished nil +> + "t indicates that full completion address harvesting has been +> +finished") +> + +> +(defcustom notmuch-address-command '(sent nil) +> + "The command which generates possible addresses. +> + +> +It can be a (non-nil) list, in which case internal completion is +> +used; in this case the first list item 'sent/'received specifies +> +whether you match message sent by the user or received by the +> +user (note received by is much faster), and the second list item +> +should be nil or a filter-string, such as \"date:1y..\" to append +> +to the query. +> + +> +If this variable is nil then address completion is disabled. +> + +> +If it is a string then that string should be an external program +> +which must take a single argument and output a list of possible +> +matches, one per line." + +What about the following? (I think that the string value should be +described first because it corresponds to the name of the variable) + + "Determines how to generate address completion candidates. + + 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 a (non-nil) list, in which case internal + completion is used; in this case the list should have form + '(DIRECTION FILTER), where DIRECTION is either sent or received and + specifies whether the candidates are searched in messages sent by the + user or received by the user (note received by is much faster), and + FILTER is either nil or a filter-string, such as \"date:1y..\" to + append to the query. + + If this variable is nil then address completion is disabled." + +> :type '(radio +> - (const :tag "Use internal address completion" internal) +> + (list :tag "Use internal address completion" +> + (radio +> + :tag "Build list based on messages you have" + +What about "Base completion on messages you have"? + +> + :value sent +> + (const :tag "sent" sent) +> + (const :tag "received" received)) +> + (radio :tag "Filter messages used for completion" +> + (const :tag "Use all messages" nil) +> + (string :tag "Filter query"))) +> (const :tag "Disable address completion" nil) +> - (string :tag "Use external completion command" "notmuch-addresses")) +> + (string :tag "Use external completion command")) +> + ;; We override set so that we can clear the cache when this changes +> + :set (lambda (symbol value) +> + (set-default symbol value) +> + (setq notmuch-address-last-harvest 0) +> + (setq notmuch-address-completions (clrhash notmuch-address-completions)) +> + (setq notmuch-address-full-harvest-finished nil)) +> :group 'notmuch-send +> :group 'notmuch-external) +> +> @@ -51,17 +86,6 @@ to know how address selection is made by default." +> :group 'notmuch-send +> :group 'notmuch-external) +> +> -(defvar notmuch-address-last-harvest 0 +> - "Time of last address harvest") +> - +> -(defvar notmuch-address-completions (make-hash-table :test 'equal) +> - "Hash of email addresses for completion during email composition. +> - This variable is set by calling `notmuch-address-harvest'.") +> - +> -(defvar notmuch-address-full-harvest-finished nil +> - "t indicates that full completion address harvesting has been +> -finished") +> - +> (defun notmuch-address-selection-function (prompt collection initial-input) +> "Call (`completing-read' +> PROMPT COLLECTION nil nil INITIAL-INPUT 'notmuch-address-history)" +> @@ -83,7 +107,8 @@ finished") +> +> (defun notmuch-address-setup () +> (let* ((use-company (and notmuch-address-use-company +> - (eq notmuch-address-command 'internal) +> + notmuch-address-command +> + (listp notmuch-address-command) +> (require 'company nil t))) +> (pair (cons notmuch-address-completion-headers-regexp +> (if use-company +> @@ -111,11 +136,11 @@ The candidates are taken from `notmuch-address-completions'." +> elisp-based implementation or older implementation requiring +> external commands." +> (cond +> - ((eq notmuch-address-command 'internal) +> + ((and notmuch-address-command (listp notmuch-address-command)) +> (when (not notmuch-address-full-harvest-finished) +> ;; First, run quick synchronous harvest based on what the user +> ;; entered so far +> - (notmuch-address-harvest (format "to:%s*" original) t)) +> + (notmuch-address-harvest original t)) +> (prog1 (notmuch-address-matching original) +> ;; Then start the (potentially long-running) full asynchronous harvest if necessary +> (notmuch-address-harvest-trigger))) +> @@ -191,21 +216,32 @@ external commands." +> +> The car is a partial harvest, and the cdr is a full harvest") +> +> -(defun notmuch-address-harvest (&optional filter-query synchronous callback) +> +(defun notmuch-address-harvest (&optional filter-string synchronous callback) +> "Collect addresses completion candidates. It queries the +> -notmuch database for all messages sent by the user optionally +> -matching FILTER-QUERY (if not nil). It collects the destination +> -addresses from those messages and stores them in +> -`notmuch-address-completions'. Address harvesting may take some +> -time so the address collection runs asynchronously unless +> -SYNCHRONOUS is t. In case of asynchronous execution, CALLBACK is +> -called when harvesting finishes." +> - (let* ((from-me-query (mapconcat (lambda (x) (concat "from:" x)) (notmuch-user-emails) " or ")) +> - (query (if filter-query +> - (format "(%s) and (%s)" from-me-query filter-query) +> - from-me-query)) +> +notmuch database for messages sent/received by the user +> +optionally with to/from matching FILTER-STRING (if not nil). It +> +collects the destination addresses from those messages and stores +> +them in `notmuch-address-completions'. Address harvesting may +> +take some time so the address collection runs asynchronously +> +unless SYNCHRONOUS is t. In case of asynchronous execution, +> +CALLBACK is called when harvesting finishes." + +I'm a bit lost in the names of variables here so I suggest more +descriptive names below. Also the doc string could be IMHO better. + +(defun notmuch-address-harvest (&optional addr-prefix synchronous callback) + "Collect addresses completion candidates. + + It queries the notmuch database for messages sent/received (as + configured with `notmuch-address-command`) by the user, collects + destination/source addresses from those messages and stores them in + `notmuch-address-completions'. + + If ADDR-PREFIX is not nil, only messages with to/from addresses + matching ADDR-PREFIX*' are queried. + + Address harvesting may take some time so the address collection runs + asynchronously unless SYNCHRONOUS is t. In case of asynchronous + execution, CALLBACK is called when harvesting finishes." + +> + (let* ((sent (eq (car notmuch-address-command) 'sent)) +> + (user-query (cadr notmuch-address-command)) + +config-query + +> + (filter-query (when filter-string +> + (format "%s:%s*" (if sent "to" "from") filter-string))) + +prefix-query + +> + (from-or-to-me-query +> + (mapconcat (lambda (x) +> + (concat (if sent "from:" "to:") x)) +> + (notmuch-user-emails) " or ")) +> + (query (if (or filter-query user-query) +> + (concat (format "(%s)" from-or-to-me-query) +> + (when filter-query +> + (format " and (%s)" filter-query)) +> + (when user-query +> + (format " and (%s)" user-query))) +> + from-or-to-me-query)) +> (args `("address" "--format=sexp" "--format-version=2" +> - "--output=recipients" +> + ,(if sent "--output=recipients" "--output=sender") +> "--deduplicate=address" +> ,query))) +> (if synchronous +> diff --git a/emacs/notmuch-company.el b/emacs/notmuch-company.el +> index b881d6d..dcb59cd 100644 +> --- a/emacs/notmuch-company.el +> +++ b/emacs/notmuch-company.el +> @@ -72,7 +72,7 @@ +> (lambda (callback) +> ;; First run quick asynchronous harvest based on what the user entered so far +> (notmuch-address-harvest +> - (format "to:%s*" arg) nil +> + arg nil +> (lambda (_proc _event) +> (funcall callback (notmuch-address-matching arg)) +> ;; Then start the (potentially long-running) full asynchronous harvest if necessary +> -- +> 2.1.4 + +Cheers +-Michal