From: Mark Walters Date: Sun, 15 Jul 2012 08:34:50 +0000 (+0100) Subject: Re: [PATCH v3 0/8] emacs: JSON-based search cleanups X-Git-Url: http://git.tremily.us/gitweb.cgi?a=commitdiff_plain;h=6c25d14bb7518b65d674723bf9756b117d23cc56;p=notmuch-archives.git Re: [PATCH v3 0/8] emacs: JSON-based search cleanups --- diff --git a/22/27b92af146d90432790257313635ccb5aed950 b/22/27b92af146d90432790257313635ccb5aed950 new file mode 100644 index 000000000..092647f82 --- /dev/null +++ b/22/27b92af146d90432790257313635ccb5aed950 @@ -0,0 +1,270 @@ +Return-Path: +X-Original-To: notmuch@notmuchmail.org +Delivered-To: notmuch@notmuchmail.org +Received: from localhost (localhost [127.0.0.1]) + by olra.theworths.org (Postfix) with ESMTP id F0888431FBD + for ; Sun, 15 Jul 2012 01:34:58 -0700 (PDT) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: -1.098 +X-Spam-Level: +X-Spam-Status: No, score=-1.098 tagged_above=-999 required=5 + tests=[DKIM_ADSP_CUSTOM_MED=0.001, FREEMAIL_FROM=0.001, + NML_ADSP_CUSTOM_MED=1.2, RCVD_IN_DNSWL_MED=-2.3] autolearn=disabled +Received: from olra.theworths.org ([127.0.0.1]) + by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) + with ESMTP id IU2HnO3dXohL for ; + Sun, 15 Jul 2012 01:34:58 -0700 (PDT) +Received: from mail2.qmul.ac.uk (mail2.qmul.ac.uk [138.37.6.6]) + (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) + (No client certificate requested) + by olra.theworths.org (Postfix) with ESMTPS id C4F61431FBC + for ; Sun, 15 Jul 2012 01:34:57 -0700 (PDT) +Received: from smtp.qmul.ac.uk ([138.37.6.40]) + by mail2.qmul.ac.uk with esmtp (Exim 4.71) + (envelope-from ) + id 1SqKI0-0005gI-Hv; Sun, 15 Jul 2012 09:34:52 +0100 +Received: from 94-192-233-223.zone6.bethere.co.uk ([94.192.233.223] + helo=localhost) + by smtp.qmul.ac.uk with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.69) + (envelope-from ) + id 1SqKI0-0005B9-15; Sun, 15 Jul 2012 09:34:52 +0100 +From: Mark Walters +To: Austin Clements , notmuch@notmuchmail.org +Subject: Re: [PATCH v3 0/8] emacs: JSON-based search cleanups +In-Reply-To: <1342306940-7499-1-git-send-email-amdragon@mit.edu> +References: <1342140319-19859-1-git-send-email-amdragon@mit.edu> + <1342306940-7499-1-git-send-email-amdragon@mit.edu> +User-Agent: Notmuch/0.13.2+90~g84fa1ef (http://notmuchmail.org) Emacs/23.4.1 + (x86_64-pc-linux-gnu) +Date: Sun, 15 Jul 2012 09:34:50 +0100 +Message-ID: <87fw8ttob9.fsf@qmul.ac.uk> +MIME-Version: 1.0 +Content-Type: text/plain; charset=us-ascii +X-Sender-Host-Address: 94.192.233.223 +X-QM-SPAM-Info: Sender has good ham record. :) +X-QM-Body-MD5: 1ec6910cedca53a5c076c0e4b06066f6 (of first 20000 bytes) +X-SpamAssassin-Score: -1.8 +X-SpamAssassin-SpamBar: - +X-SpamAssassin-Report: The QM spam filters have analysed this message to + determine if it is + spam. We require at least 5.0 points to mark a message as spam. + This message scored -1.8 points. + Summary of the scoring: + * -2.3 RCVD_IN_DNSWL_MED RBL: Sender listed at http://www.dnswl.org/, + * medium trust + * [138.37.6.40 listed in list.dnswl.org] + * 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail + provider * (markwalters1009[at]gmail.com) + * -0.0 T_RP_MATCHES_RCVD Envelope sender domain matches handover relay + * domain + * 0.5 AWL AWL: From: address is in the auto white-list +X-QM-Scan-Virus: ClamAV says the message is clean +X-BeenThere: notmuch@notmuchmail.org +X-Mailman-Version: 2.1.13 +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: Sun, 15 Jul 2012 08:34:59 -0000 + +On Sun, 15 Jul 2012, Austin Clements wrote: +> This version swaps out the notmuch-search-do-results macro for a +> higher-order function, notmuch-search-foreach-result. This requires +> less squiting to understand and clearly distinguishes the arguments +> passed in to the function from the arguments passed to the callback. +> This version also updates the docstring for +> notmuch-search-result-format to mention that multi-line result formats +> work and how to enter them, and it adds a NEWS patch. + +Hi + +In essence this version looks good and, contrary to what I though +before, I do prefer this function version to the macro. In addition it +seems to work well in testing. + +However, there are some problems with multiline search results (see +below) so I think we should either fix these or just downplay this new +functionality by, for example, removing the comments on newlines from +the defcustom and saying in NEWS that the feature is experimental/not +complete or similar. (NEWS could say how to enter newlines in the +defcustom) + +With this minor comment on the documentation my criticisms should not +hold up this excellent series. + +Multiline oddities and todo + +There is still some strange scrolling when updating a result. One +example that seems reproducible is if the search format has trailing +newline and the cursor is at the start of that line it may jump to the +far right of the previous line (possibly scrolling the screen +horizontally) upon a tag change (eg +unread). + +If the format has a newline at the start of the author field it +sometimes gets omitted from the output. (In addition to the known +problem at the end of the author field that Austin mentions in the +documentation for the defcustom). + +Examples of "incompleteness": these are rather more personal but it +seems odd to me to highlight one line rather than one result in the +buffer. Similarly I would expect to scroll up or down by +one result rather than one line. + +I have a draft patch which fixes these two and mostly fixes the +scrolling but it is bigger than I would like (and I haven't yet ironed +out/checked all corner cases). I will split it up and send to the list +so people can try it (but it is not intended as a submission +currently). + +As it seems to be tricky to get fully right it might be preferable +just to change the documentation as suggested above and leave +multi-line to the adventurous. + +Best wishes + +Mark + +> +> Diff from v2: +> +> diff --git a/NEWS b/NEWS +> index a1a6e93..7b33f0d 100644 +> --- a/NEWS +> +++ b/NEWS +> @@ -17,6 +17,20 @@ Maildir tag synchronization +> Emacs Interface +> --------------- +> +> +Search results now get re-colored when tags are updated +> + +> +The formatting of tags in search results can now be customized +> + +> + Previously, attempting to change the format of tags in +> + `notmuch-search-result-format` would usually break tagging from +> + search-mode. We no longer make assumptions about the format. +> + +> +Multi-line search result formats are now supported +> + +> + It is now possible to embed newlines in +> + `notmuch-search-result-format` to make individual search results +> + span multiple lines. +> + +> Search now uses the JSON format internally +> +> This should address problems with unusual characters in authors and +> diff --git a/emacs/notmuch.el b/emacs/notmuch.el +> index 7302fa7..ec760dc 100644 +> --- a/emacs/notmuch.el +> +++ b/emacs/notmuch.el +> @@ -69,7 +69,13 @@ +> date, count, authors, subject, tags +> For example: +> (setq notmuch-search-result-format \(\(\"authors\" . \"%-40s\"\) +> - \(\"subject\" . \"%s\"\)\)\)" +> + \(\"subject\" . \"%s\"\)\)\) +> +Line breaks are permitted in format strings. Note that a line +> +break at the end of an \"authors\" field will get elided if the +> +authors list is long; place it instead at the beginning of the +> +following field. To enter a line break when setting this +> +variable with setq, use \\n. To enter a line break in customize, +> +press \\[quoted-insert] C-j." +> :type '(alist :key-type (string) :value-type (string)) +> :group 'notmuch-search) +> +> @@ -433,32 +439,39 @@ returns nil" +> (next-single-property-change (or pos (point)) 'notmuch-search-result +> nil (point-max)))) +> +> -(defmacro notmuch-search-do-results (beg end pos-sym &rest body) +> - "Invoke BODY for each result between BEG and END. +> - +> -POS-SYM will be bound to the point at the beginning of the +> -current result." +> - (declare (indent 3)) +> - (let ((end-sym (make-symbol "end")) +> - (first-sym (make-symbol "first"))) +> - `(let ((,pos-sym (notmuch-search-result-beginning ,beg)) +> - ;; End must be a marker in case body changes the text +> - (,end-sym (copy-marker ,end)) +> - ;; Make sure we examine one result, even if (= beg end) +> - (,first-sym t)) +> - ;; We have to be careful if the region extends beyond the +> - ;; results. In this case, pos could be null or there could be +> - ;; no result at pos. +> - (while (and ,pos-sym (or (< ,pos-sym ,end-sym) ,first-sym)) +> - (when (notmuch-search-get-result ,pos-sym) +> - ,@body) +> - (setq ,pos-sym (notmuch-search-result-end ,pos-sym) +> - ,first-sym nil))))) +> +(defun notmuch-search-foreach-result (beg end function) +> + "Invoke FUNCTION for each result between BEG and END. +> + +> +FUNCTION should take one argument. It will be applied to the +> +character position of the beginning of each result that overlaps +> +the region between points BEG and END. As a special case, if (= +> +BEG END), FUNCTION will be applied to the result containing point +> +BEG." +> + +> + (lexical-let ((pos (notmuch-search-result-beginning beg)) +> + ;; End must be a marker in case function changes the +> + ;; text. +> + (end (copy-marker end)) +> + ;; Make sure we examine at least one result, even if +> + ;; (= beg end). +> + (first t)) +> + ;; We have to be careful if the region extends beyond the results. +> + ;; In this case, pos could be null or there could be no result at +> + ;; pos. +> + (while (and pos (or (< pos end) first)) +> + (when (notmuch-search-get-result pos) +> + (funcall function pos)) +> + (setq pos (notmuch-search-result-end pos) +> + first nil)))) +> +;; Unindent the function argument of notmuch-search-foreach-result so +> +;; the indentation of callers doesn't get out of hand. +> +(put 'notmuch-search-foreach-result 'lisp-indent-function 2) +> +> (defun notmuch-search-properties-in-region (property beg end) +> (let (output) +> - (notmuch-search-do-results beg end pos +> - (push (plist-get (notmuch-search-get-result pos) property) output)) +> + (notmuch-search-foreach-result beg end +> + (lambda (pos) +> + (push (plist-get (notmuch-search-get-result pos) property) output))) +> output)) +> +> (defun notmuch-search-find-thread-id () +> @@ -542,18 +555,20 @@ and will also appear in a buffer named \"*Notmuch errors*\"." +> +> (defun notmuch-search-get-tags-region (beg end) +> (let (output) +> - (notmuch-search-do-results beg end pos +> - (setq output (append output (notmuch-search-get-tags pos)))) +> + (notmuch-search-foreach-result beg end +> + (lambda (pos) +> + (setq output (append output (notmuch-search-get-tags pos))))) +> output)) +> +> (defun notmuch-search-tag-region (beg end &optional tag-changes) +> "Change tags for threads in the given region." +> (let ((search-string (notmuch-search-find-thread-id-region-search beg end))) +> (setq tag-changes (funcall 'notmuch-tag search-string tag-changes)) +> - (notmuch-search-do-results beg end pos +> - (notmuch-search-set-tags +> - (notmuch-update-tags (notmuch-search-get-tags pos) tag-changes) +> - pos)))) +> + (notmuch-search-foreach-result beg end +> + (lambda (pos) +> + (notmuch-search-set-tags +> + (notmuch-update-tags (notmuch-search-get-tags pos) tag-changes) +> + pos))))) +> +> (defun notmuch-search-tag (&optional tag-changes) +> "Change tags for the currently selected thread or region.