Date: Fri, 13 Jul 2012 14:33:28 -0400
From: Austin Clements <amdragon@MIT.EDU>
To: Mark Walters <markwalters1009@gmail.com>
Subject: Re: [PATCH 3/7] emacs: Update tags by rewriting the search result
	line in place
Message-ID: <20120713183328.GA31182@mit.edu>
References: <1342140319-19859-1-git-send-email-amdragon@mit.edu>
	<1342140319-19859-4-git-send-email-amdragon@mit.edu>
	<87a9z38ryo.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87a9z38ryo.fsf@qmul.ac.uk>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Quoth Mark Walters on Jul 13 at  6:57 pm:
> On Fri, 13 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> > Now that we keep the full thread result object, we can refresh a
> > result after any changes by simply deleting and reconstructing the
> > result line from scratch.
> >
> > A convenient side-effect of this wholesale replacement is that search
> > now re-applies notmuch-search-line-faces when tags change.
> > ---
> >  emacs/notmuch.el |   55 ++++++++++++++++++++++++++++++------------------------
> >  1 file changed, 31 insertions(+), 24 deletions(-)
> >
> > diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> > index 82c148d..2f83425 100644
> > --- a/emacs/notmuch.el
> > +++ b/emacs/notmuch.el
> > @@ -509,28 +509,12 @@ and will also appear in a buffer named \"*Notmuch errors*\"."
> >  	    (error (buffer-substring beg end))
> >  	    ))))))
> >  
> > -(defun notmuch-search-set-tags (tags)
> > -  (save-excursion
> > -    (end-of-line)
> > -    (re-search-backward "(")
> > -    (forward-char)
> > -    (let ((beg (point))
> > -	  (inhibit-read-only t))
> > -      (re-search-forward ")")
> > -      (backward-char)
> > -      (let ((end (point)))
> > -	(delete-region beg end)
> > -	(insert (propertize (mapconcat  'identity tags " ")
> > -			    'face 'notmuch-tag-face))))))
> > -
> > -(defun notmuch-search-get-tags ()
> > -  (save-excursion
> > -    (end-of-line)
> > -    (re-search-backward "(")
> > -    (let ((beg (+ (point) 1)))
> > -      (re-search-forward ")")
> > -      (let ((end (- (point) 1)))
> > -	(split-string (buffer-substring-no-properties beg end))))))
> > +(defun notmuch-search-set-tags (tags &optional pos)
> > +  (let ((new-result (plist-put (notmuch-search-get-result pos) :tags tags)))
> > +    (notmuch-search-update-result new-result pos)))
> > +
> > +(defun notmuch-search-get-tags (&optional pos)
> > +  (plist-get (notmuch-search-get-result pos) :tags))
> >  
> >  (defun notmuch-search-get-tags-region (beg end)
> >    (save-excursion
> > @@ -583,6 +567,29 @@ This function advances the next thread when finished."
> >    (notmuch-search-tag '("-inbox"))
> >    (notmuch-search-next-thread))
> >  
> > +(defun notmuch-search-update-result (result &optional pos)
> > +  "Update the result object of the current thread and redraw it."
> 
> I think this comment reads a little awkwardly. Maybe "Replace the result
> object of the current thread by RESULT and redraw it"?

Better.  I should also say that it's the thread at POS (or point).

> > +  (let ((start (notmuch-search-result-beginning pos))
> > +	(end (notmuch-search-result-end pos))
> > +	(init-point (point))
> > +	(inhibit-read-only t))
> > +    ;; Delete the current thread
> > +    (delete-region start end)
> > +    ;; Insert the updated thread
> > +    (notmuch-search-show-result result start)
> > +    ;; There may have been markers pointing into the text we just
> > +    ;; replaced.  For the most part, there's nothing we can do about
> > +    ;; this, but we can fix markers that were at point (which includes
> > +    ;; point itself and any save-excursions for which point hasn't
> > +    ;; moved) by re-inserting the text that should come before point
> > +    ;; before markers.
> > +    (when (and (>= init-point start) (<= init-point end))
> > +      (let* ((new-end (notmuch-search-result-end start))
> > +	     (new-point (if (= init-point end)
> > +			    new-end
> > +			  (min init-point (- new-end 1)))))
> > +	(insert-before-markers (delete-and-extract-region start new-point))))))
> 
> I think this doesn't always do quite the right thing with multiline
> results: the one that I can reproduce reliably is if you are on the
> second line of the first result, and the first line of this result is
> the first visible line in the window, and update it (eg by marking
> unread) then the window scrolls so that the second line of the first
> result is the first line visible in the window (ie the window scrolls up
> one line)

Nice catch!  I really wish Emacs had a "replace text" function that
did sane things with markers and the window position and such; without
that anything I do is going to be finicky.  I'll see if saving and
restoring the window scroll position fixes this.

> > +
> >  (defun notmuch-search-process-sentinel (proc msg)
> >    "Add a message to let user know when \"notmuch search\" exits"
> >    (let ((buffer (process-buffer proc))
> > @@ -745,10 +752,10 @@ non-authors is found, assume that all of the authors match."
> >  			 (mapconcat 'identity (plist-get result :tags) " ")
> >  			 'font-lock-face 'notmuch-tag-face) ")")))))
> >  
> > -(defun notmuch-search-show-result (result)
> > +(defun notmuch-search-show-result (result &optional pos)
> 
> 
> Perhaps a documentation line? along the lines of Insert RESULT in buffer
> at POS or the end of buffer.

Good idea.

> Best wishes
> 
> Mark
> 
> >    ;; Ignore excluded matches
> >    (unless (= (plist-get result :matched) 0)
> > -    (let ((beg (point-max)))
> > +    (let ((beg (or pos (point-max))))
> >        (save-excursion
> >  	(goto-char beg)
> >  	(dolist (spec notmuch-search-result-format)