From: Mark Walters <markwalters1009@gmail.com>
To: Austin Clements <amdragon@MIT.EDU>, notmuch@notmuchmail.org
Subject: Re: [PATCH v2 0/9] JSON-based search-mode
In-Reply-To: <1341521547-15502-1-git-send-email-amdragon@mit.edu>
References: <1341354059-29396-1-git-send-email-amdragon@mit.edu>
	<1341521547-15502-1-git-send-email-amdragon@mit.edu>
User-Agent: Notmuch/0.13.2+76~g7e62702 (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Thu, 05 Jul 2012 22:44:30 +0100
Message-ID: <87y5mx3mtd.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: tomi.ollila@iki.fi
Precedence: list

On Thu, 05 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> This should account for all of Mark's and Tomi's comments.  This
> version
> * renames the "format" variables to "format-string" and "spec" to be
>   less confusing,
> * reverts to the original behavior of ignoring the user's format
>   specification for tags (since we make assumptions about this format
>   elsewhere),
> * swaps the error helper and search-target patches to fix the
>   temporary issue with error message placement,
> * adds documentation on point movement in the JSON parser,
> * breaks out the JSON EOF testing function,
> * beefs up a few commit messages,
> * and adds a NEWS patch.
>
> For ease of reviewing, the diff diff is below.
>
> I've written most of a follow-on patch series that cleans up the
> handling of metadata and tag changes by attaching the JSON result
> object to the result.  This means we don't need a proliferation of
> text properties to store the result metadata, and we can make updates
> to a result (e.g., tag changes) by updating this result object and
> then re-rendering the result line from scratch, rather than trying to
> update the result line in place.  This makes it possible to obey user
> formatting for the tag list and has other perks like recoloring
> results when their tags change.  I'll send it along once this patch
> series is accepted.
>
> diff --git a/NEWS b/NEWS
> index d29ec5b..a1a6e93 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -14,6 +14,23 @@ Maildir tag synchronization
>    messages (typically causing new messages to not receive the "unread"
>    tag).
>  
> +Emacs Interface
> +---------------
> +
> +Search now uses the JSON format internally
> +
> +  This should address problems with unusual characters in authors and
> +  subject lines that could confuse the old text-based search parser.
> +
> +The date shown in search results is no longer padded before applying
> +user-specified formatting
> +
> +  Previously, the date in the search results was padded to fixed width
> +  before being formatted with `notmuch-search-result-format`.  It is
> +  no longer padded.  The default format has been updated, but if
> +  you've customized this variable, you may have to change your date
> +  format from `"%s "` to `"%12s "`.
> +
>  Notmuch 0.13.2 (2012-06-02)
>  ===========================
>  
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index f7cda33..9e04d97 100644
> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -399,8 +399,9 @@ resynchronize after an error by moving point."
>  
>  Returns 'retry if there is insufficient input to parse the
>  beginning of the compound.  If this is able to parse the
> -beginning of a compound, it returns t and later calls to
> -`notmuch-json-read' will return the compound's elements.
> +beginning of a compound, it moves point past the token that opens
> +the compound and returns t.  Later calls to `notmuch-json-read'
> +will return the compound's elements.
>  
>  Entering JSON objects is current unimplemented."
>  
> @@ -423,7 +424,8 @@ Entering JSON objects is current unimplemented."
>  Returns 'retry if there is insufficient input to parse a complete
>  JSON value.  If the parser is currently inside a compound value
>  and the next token ends the list or object, returns 'end.
> -Otherwise, returns the value."
> +Otherwise, moves point to just past the end of the value and
> +returns the value."

I think that point can move when 'retry is returned (past terminators
and commas for example). It might also be worth saying that it returns
the next value passing command and terminators and whitespace after
point.

>  
>    (with-current-buffer (notmuch-json-buffer jp)
>      (or
> @@ -474,11 +476,22 @@ Otherwise, returns the value."
>  	       (notmuch-json-partial-pos jp) nil
>  	       (notmuch-json-partial-state jp) nil)
>  	 ;; Parse the value
> -	 (let* ((json-object-type 'plist)
> -		(json-array-type 'list)
> -		(json-false nil))
> +	 (let ((json-object-type 'plist)
> +	       (json-array-type 'list)
> +	       (json-false nil))
>  	   (json-read)))))))
>  
> +(defun notmuch-json-eof (jp)
> +  "Signal a json-error if there is more input in JP's buffer.

Would `data' be better than `input' (to distinguish allowed whitespace
from disallowed data)?

> +Moves point to the beginning of any trailing garbage or to the
> +end of the buffer if there is no trailing garbage."
> +
> +  (with-current-buffer (notmuch-json-buffer jp)
> +    (skip-chars-forward " \t\r\n")
> +    (unless (eobp)
> +      (signal 'json-error (list "Trailing garbage following JSON data")))))
> +
>  (provide 'notmuch-lib)
>  
>  ;; Local Variables:
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 2a09a98..fabb7c0 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -702,28 +702,29 @@ non-authors is found, assume that all of the authors match."
>  	  (overlay-put overlay 'isearch-open-invisible #'delete-overlay)))
>        (insert padding))))
>  
> -(defun notmuch-search-insert-field (field format result)
> +(defun notmuch-search-insert-field (field format-string result)
>    (cond
>     ((string-equal field "date")
> -    (insert (propertize (format format (plist-get result :date_relative))
> +    (insert (propertize (format format-string (plist-get result :date_relative))
>  			'face 'notmuch-search-date)))
>     ((string-equal field "count")
> -    (insert (propertize (format format (format "[%s/%s]"
> -					       (plist-get result :matched)
> -					       (plist-get result :total)))
> +    (insert (propertize (format format-string
> +				(format "[%s/%s]" (plist-get result :matched)
> +					(plist-get result :total)))
>  			'face 'notmuch-search-count)))
>     ((string-equal field "subject")
> -    (insert (propertize (format format (plist-get result :subject))
> +    (insert (propertize (format format-string (plist-get result :subject))
>  			'face 'notmuch-search-subject)))
>  
>     ((string-equal field "authors")
> -    (notmuch-search-insert-authors format (plist-get result :authors)))
> +    (notmuch-search-insert-authors format-string (plist-get result :authors)))
>  
>     ((string-equal field "tags")
> -    (insert
> -     (format format (propertize
> -		     (mapconcat 'identity (plist-get result :tags) " ")
> -		     'font-lock-face 'notmuch-tag-face))))))
> +    ;; Ignore format-string here because notmuch-search-set-tags
> +    ;; depends on the format of this
> +    (insert (concat "(" (propertize
> +			 (mapconcat 'identity (plist-get result :tags) " ")
> +			 'font-lock-face 'notmuch-tag-face) ")")))))
>  
>  (defun notmuch-search-show-result (result)
>    ;; Ignore excluded matches
> @@ -731,8 +732,8 @@ non-authors is found, assume that all of the authors match."
>      (let ((beg (point-max)))
>        (save-excursion
>  	(goto-char beg)
> -	(dolist (format notmuch-search-result-format)
> -	  (notmuch-search-insert-field (car format) (cdr format) result))
> +	(dolist (spec notmuch-search-result-format)
> +	  (notmuch-search-insert-field (car spec) (cdr spec) result))
>  	(insert "\n")
>  	(notmuch-search-color-line beg (point) (plist-get result :tags))
>  	(put-text-property beg (point) 'notmuch-search-thread-id
> @@ -790,11 +791,8 @@ non-authors is found, assume that all of the authors match."
>  		     (otherwise (notmuch-search-show-result result)))))
>  		((end)
>  		 ;; Any trailing data is unexpected
> -		 (with-current-buffer parse-buf
> -		   (skip-chars-forward " \t\r\n")
> -		   (if (eobp)
> -		       (setq done t)
> -		     (signal 'json-error nil)))))
> +		 (notmuch-json-eof notmuch-search-json-parser)
> +		 (setq done t)))

I think this used to only set `done' if there was not trailing data but
now does so anyway?

Best wishes

Mark

>  	    (json-error
>  	     ;; Do our best to resynchronize and ensure forward
>  	     ;; progress