From: Mark Walters <markwalters1009@gmail.com>
To: Austin Clements <amdragon@MIT.EDU>
Subject: Re: [PATCH v2 0/9] JSON-based search-mode
In-Reply-To: <20120706002941.GB18195@mit.edu>
References: <1341354059-29396-1-git-send-email-amdragon@mit.edu>
	<1341521547-15502-1-git-send-email-amdragon@mit.edu>
	<87y5mx3mtd.fsf@qmul.ac.uk> <20120706002941.GB18195@mit.edu>
User-Agent: Notmuch/0.13.2+61~gf708609 (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Sat, 07 Jul 2012 17:27:43 +0100
Message-ID: <87ipdztu2o.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: tomi.ollila@iki.fi, notmuch@notmuchmail.org
Precedence: list

On Fri, 06 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> Quoth Mark Walters on Jul 05 at 10:44 pm:
>> 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.
>
> How about
>
> Returns 'retry if there is insufficient input to parse a complete JSON
> value (though it may still move point over separators or whitespace).
> If the parser is currently inside a compound value and the next token
> ends the list or object, this moves point just past the terminator and
> returns 'end.  Otherwise, this moves point to just past the end of the
> value and returns the value.
>
> ?

This sounds good.

>> >  
>> >    (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)?
>
> Ah, yes.
>
>   "Signal a json-error if there is more data in JP's buffer.
>
> Moves point to the beginning of any trailing data or to the end
> of the buffer if there is only trailing whitespace."
>
> ?

This is good too.

>> > +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?
>
> It still won't set done if there's trailing data because
> notmuch-json-eof will signal an error, which will unwind to the
> condition-case (which will then consume said trailing data and done
> will get set on the next time through the loop).


Ah! I had overlooked that. It all looks good now.

Best wishes

Mark