[PATCH 0/2] Update search results when tags change

[PATCH 0/2] Update search results when tags change

[Vladimir Panteleev]

One of the things that stood out to me right after I tried using
notmuch, is that whenever I performed a search (which included some
unread messages), opened an unread search result, and went back to the
search buffer, it would still show that thread as unread. To update
the display, I'd need to invoke notmuch-refresh-this-buffer, which is
not only pretty slow, but also resets point, i.e. all-in-all rather
annoying.

Fortunately, this was very easy to fix, as the machinery for updating
individual search results was already in place.

[PATCH 1/2] notmuch-tag.el: Fix minor grammar error

[Vladimir Panteleev]

---
 emacs/notmuch-tag.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el
index 09d182df..0500927d 100644
--- a/emacs/notmuch-tag.el
+++ b/emacs/notmuch-tag.el
@@ -457,7 +457,7 @@ QUERY should be a string containing the search-terms.
 TAG-CHANGES is a list of strings of the form \"+tag\" or
 \"-tag\" to add or remove tags, respectively.
 
-Note: Other code should always use this function alter tags of
+Note: Other code should always use this function to alter tags of
 messages instead of running (notmuch-call-notmuch-process \"tag\" ..)
 directly, so that hooks specified in notmuch-before-tag-hook and
 notmuch-after-tag-hook will be run."
-- 
2.13.3

[PATCH 2/2] emacs: Add notmuch-update-search-tags

[Vladimir Panteleev]

Implement an option which, when enabled, causes any tag changes done
from within notmuch-emacs to instantly update matching threads in open
search buffers.
---
 emacs/notmuch-tag.el | 16 ++++++++++++++++
 emacs/notmuch.el     | 15 +++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el
index 0500927d..41fdeaef 100644
--- a/emacs/notmuch-tag.el
+++ b/emacs/notmuch-tag.el
@@ -364,6 +364,15 @@ the messages that were tagged"
   :options '(notmuch-hl-line-mode)
   :group 'notmuch-hooks)
 
+(defcustom notmuch-update-search-tags t
+  "Update open `notmuch-search' buffers after tags of a message are modified.
+
+Instantly update any matching results in open `notmuch-search'
+buffers, so that all tag changes are immediately reflected."
+  :type 'boolean
+  :group 'notmuch-search
+  :group 'notmuch-tag)
+
 (defvar notmuch-select-tag-history nil
   "Variable to store minibuffer history for
 `notmuch-select-tag-with-completion' function.")
@@ -477,6 +486,13 @@ notmuch-after-tag-hook will be run."
       (let ((batch-op (concat (mapconcat #'notmuch-hex-encode tag-changes " ")
 			      " -- " query)))
 	(notmuch-call-notmuch-process :stdin-string batch-op "tag" "--batch")))
+    (when notmuch-update-search-tags
+      (let ((results (notmuch-call-notmuch-sexp
+		      "search" "--format=sexp" "--format-version=4" query)))
+	(dolist (buffer (buffer-list))
+	  (when (eq (buffer-local-value 'major-mode buffer) 'notmuch-search-mode)
+	    (with-current-buffer buffer
+	      (notmuch-search-update-results results))))))
     (run-hooks 'notmuch-after-tag-hook)))
 
 (defun notmuch-tag-change-list (tags &optional reverse)
diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 44402f8a..b5fe4e60 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -662,6 +662,21 @@ of the result."
 			  (min init-point (- new-end 1)))))
 	(goto-char new-point)))))
 
+(defun notmuch-search-update-results (results)
+  "Replace results with threads matching RESULTS in-place and redraw them."
+  (let ((pos (next-single-property-change 1 'notmuch-search-result))
+	(results-alist
+	 (mapcar (lambda (result) (cons (plist-get result :thread) result))
+		 results)))
+    (while pos
+      (let* ((prop (get-text-property pos 'notmuch-search-result))
+	     (thread (when prop (plist-get prop :thread)))
+	     (result (when thread (assoc-default thread results-alist))))
+	;; (message "found matching thread: %s" thread)
+	(when result
+	  (notmuch-search-update-result result pos)))
+      (setq pos (next-single-property-change pos 'notmuch-search-result)))))
+
 (defun notmuch-search-process-sentinel (proc msg)
   "Add a message to let user know when \"notmuch search\" exits"
   (let ((buffer (process-buffer proc))
-- 
2.13.3

Re: [PATCH 1/2] notmuch-tag.el: Fix minor grammar error

[David Bremner]

Vladimir Panteleev <git@thecybershadow.net> writes:

> ---
>  emacs/notmuch-tag.el | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

pushed to master

Re: [PATCH 2/2] emacs: Add notmuch-update-search-tags

[David Bremner]

Vladimir Panteleev <git@thecybershadow.net> writes:

> Implement an option which, when enabled, causes any tag changes done
> from within notmuch-emacs to instantly update matching threads in open
> search buffers.
> ---

One side effect of this seems to be a regression, namely that in the
current buffer the "visual history" of tag changes is broken. Previously
deletions were shown by red strike-through and additions by green
underline. The deletions are now removed by the global update, and
additions shown on the wrong tags.

Another aspect that is a bit surprising is that it doesn't change whether
threads actually match. For example if I have search for tag:inbox,
adding tag inbox to a thread won't cause the corresponding thread to
show up. This is maybe defensible, but it wasn't clear to me if it was
intentional.

d

Re: [PATCH 2/2] emacs: Add notmuch-update-search-tags

[Vladimir Panteleev]

On 2017-08-25 11:12, David Bremner wrote:
> One side effect of this seems to be a regression, namely that in the
> current buffer the "visual history" of tag changes is broken. Previously
> deletions were shown by red strike-through and additions by green
> underline. The deletions are now removed by the global update, and
> additions shown on the wrong tags.

Thanks for pointing that out. Fixed in v2.

> Another aspect that is a bit surprising is that it doesn't change whether
> threads actually match. For example if I have search for tag:inbox,
> adding tag inbox to a thread won't cause the corresponding thread to
> show up. This is maybe defensible, but it wasn't clear to me if it was
> intentional.

It is certainly intentional, though only reluctantly so.

As far as I can tell, there is no way to update existing search buffers 
efficiently AND correctly, where "efficiently" means not simply 
performing the entire search query and repopulating the search buffer, 
and "correctly" means updating the buffer for deletions and additions so 
that the buffer contents would remain effectively the same before and 
after a notmuch-refresh-this-buffer invocation.

notmuch-search is pretty slow; populating the "all mail" search result 
takes well over two minutes here (comparatively, for Thunderbird it 
takes no time at all). Refreshing all notmuch search buffers just 
because a tag changed somewhere (like point moving over an unread 
message in notmuch-show) is not a practical approach.

Even though it would be possible to calculate the intersection between 
affected tags and a search buffer's query to find out which messages 
have been added or removed as an effect of the tagging operation, that 
still doesn't tell us where in the buffer the newly-added results would 
need to be inserted. Just plopping them at the end is an option, but not 
really a satisfying one. Same with duplicating the result ordering logic 
from the backend in the UI.

Ideally, notmuch would offer a subscription mechanism, which would allow 
clients to subscribe to live changes in search queries, and receive 
updates as deltas from the original results. That's more of a daydream, 
though.

In any case, I've updated the documentation of the variable to clarify 
that it does not cause results to be added or removed, but only update 
the display of the existing results.

-- 
Best regards,
  Vladimir

[PATCH v2] emacs: Add notmuch-update-search-tags

[Vladimir Panteleev]

From: Vladimir Panteleev <git@thecybershadow.net>

Implement an option which, when enabled, causes any tag changes done
from within notmuch-emacs to instantly update matching threads in open
search buffers.

* notmuch.el: Add notmuch-search-update-results.

* notmuch-tag.el: Add notmuch-update-search-tags; invoke
  notmuch-search-update-results from notmuch-tag when
  notmuch-update-search-tags is non-nil.

* T310-emacs.sh: Add test.
---
 This update now includes a test as well.

 Speaking of which, I had a fun time trying to figure out why my test
 didn't work before I discovered that notmuch-tag-deleted-formats is
 reset in test-lib.el. That took quite a bit of debugging; I think it
 would be good to fix this inconsistency to avoid other contributors
 wasting time on this in the future.

 emacs/notmuch-tag.el | 24 ++++++++++++++++++++++++
 emacs/notmuch.el     | 25 +++++++++++++++++++++++++
 test/T310-emacs.sh   | 18 ++++++++++++++++++
 3 files changed, 67 insertions(+)

diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el
index 0500927d..3dda7115 100644
--- a/emacs/notmuch-tag.el
+++ b/emacs/notmuch-tag.el
@@ -364,6 +364,23 @@ the messages that were tagged"
   :options '(notmuch-hl-line-mode)
   :group 'notmuch-hooks)
 
+(defcustom notmuch-update-search-tags nil
+  "Update open `notmuch-search' buffers after tags of a message are modified.
+
+When non-nil, instantly update any matching results in open
+`notmuch-search' buffers, so that all tag changes are immediately
+reflected.
+
+Note that this does not cause the list of search results to be
+updated, but only the display of the currently shown search
+results.  If a tag change causes a search query to include or
+exclude results that were respectively absent or present before,
+they will not be added or removed - `notmuch-refresh-this-buffer'
+must be manually invoked to do so."
+  :type 'boolean
+  :group 'notmuch-search
+  :group 'notmuch-tag)
+
 (defvar notmuch-select-tag-history nil
   "Variable to store minibuffer history for
 `notmuch-select-tag-with-completion' function.")
@@ -477,6 +494,13 @@ notmuch-after-tag-hook will be run."
       (let ((batch-op (concat (mapconcat #'notmuch-hex-encode tag-changes " ")
 			      " -- " query)))
 	(notmuch-call-notmuch-process :stdin-string batch-op "tag" "--batch")))
+    (when notmuch-update-search-tags
+      (let ((results (notmuch-call-notmuch-sexp
+		      "search" "--format=sexp" "--format-version=4" query)))
+	(dolist (buffer (buffer-list))
+	  (when (eq (buffer-local-value 'major-mode buffer) 'notmuch-search-mode)
+	    (with-current-buffer buffer
+	      (notmuch-search-update-results results))))))
     (run-hooks 'notmuch-after-tag-hook)))
 
 (defun notmuch-tag-change-list (tags &optional reverse)
diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 44402f8a..9dd9e661 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -662,6 +662,31 @@ of the result."
 			  (min init-point (- new-end 1)))))
 	(goto-char new-point)))))
 
+(defun notmuch-search-update-results (results)
+  "Replace results with threads matching RESULTS in-place and redraw them.
+
+The :orig-tags property is copied over from the old result so
+that tag changes are displayed correctly."
+  (let ((pos (point-min))
+	(results-alist ; Convert list to alist (keyed by :thread).
+	 (mapcar (lambda (result) (cons (plist-get result :thread) result))
+		 results)))
+    (while pos
+      (let (orig-result thread new-result orig-tags final-result)
+	(and ; All of these variables need to be non-nil.
+	 ;; The original search result as it appears in the buffer.
+	 (setq orig-result (get-text-property pos 'notmuch-search-result))
+	 ;; The result's thread ID.
+	 (setq thread (plist-get orig-result :thread))
+	 ;; The matching updated result we received, if any.
+	 (setq new-result (assoc-default thread results-alist))
+	 ;; The original tags of the old result.
+	 (setq orig-tags (plist-get orig-result :orig-tags))
+	 ;; Result with :orig-tags copied from the old result.
+	 (setq final-result (plist-put new-result :orig-tags orig-tags))
+	 (notmuch-search-update-result final-result pos)))
+      (setq pos (next-single-property-change pos 'notmuch-search-result)))))
+
 (defun notmuch-search-process-sentinel (proc msg)
   "Add a message to let user know when \"notmuch search\" exits"
   (let ((buffer (process-buffer proc))
diff --git a/test/T310-emacs.sh b/test/T310-emacs.sh
index fde11790..a4ca09c5 100755
--- a/test/T310-emacs.sh
+++ b/test/T310-emacs.sh
@@ -128,6 +128,24 @@ test_emacs "(notmuch-search \"$os_x_darwin_thread\")
 output=$(notmuch search $os_x_darwin_thread | notmuch_search_sanitize)
 test_expect_equal "$output" "thread:XXX   2009-11-18 [4/4] Jjgod Jiang, Alexander Botero-Lowry; [notmuch] Mac OS X/Darwin compatibility issues (inbox unread)"
 
+test_begin_subtest "Show new tag in search view with notmuch-update-search-tags"
+test_emacs "(let ((notmuch-update-search-tags t)
+		  (notmuch-tag-added-formats '((\".*\" (concat \"new:\" tag)))))
+	      (notmuch-search \"$os_x_darwin_thread\")
+	      (notmuch-test-wait)
+	      (notmuch-tag \"$os_x_darwin_thread\" '(\"+update-search-tags\"))
+	      (test-output))"
+test_expect_equal "$(cat OUTPUT)" $'  2009-11-18 [4/4]   Jjgod Jiang, Alexander Botero-Lowry      [notmuch] Mac OS X/Darwin compatibility issues (inbox unread new:update-search-tags)\nEnd of search results.'
+
+test_begin_subtest "Keep deleted tag in search view with notmuch-update-search-tags"
+test_emacs "(let ((notmuch-update-search-tags t)
+		  (notmuch-tag-deleted-formats '((\".*\" (concat \"deleted:\" tag)))))
+	      (notmuch-search \"$os_x_darwin_thread\")
+	      (notmuch-test-wait)
+	      (notmuch-tag \"$os_x_darwin_thread\" '(\"-update-search-tags\"))
+	      (test-output))"
+test_expect_equal "$(cat OUTPUT)" $'  2009-11-18 [4/4]   Jjgod Jiang, Alexander Botero-Lowry      [notmuch] Mac OS X/Darwin compatibility issues (inbox unread deleted:update-search-tags)\nEnd of search results.'
+
 test_begin_subtest "Add tag (large query)"
 # We use a long query to force us into batch mode and use a funny tag
 # that requires escaping for batch tagging.
-- 
2.14.1

Re: [PATCH v2] emacs: Add notmuch-update-search-tags

[Mark Walters]

Hi

On Sat, 26 Aug 2017, Vladimir Panteleev <notmuch@thecybershadow.net> wrote:
> From: Vladimir Panteleev <git@thecybershadow.net>
>
> Implement an option which, when enabled, causes any tag changes done
> from within notmuch-emacs to instantly update matching threads in open
> search buffers.

I like the idea, and provided it only marks the tags as changed in the
normal way, I think we could even default to on.

However, I am concerned that this version does other things as
well. Note these comments are from reading the patch not from testing,
so please point out if I am wrong.

1) Suppose a new message arrives in the thread. Then triggering a tag
change in the show buffer, (eg notmuch automatically removing an unread
tag) pulls that message into original search buffer.

2) If that new message has some tags that aren't in the thread they will
get marked as "added tags"

3) At least in theory I think the thread may no longer exist -- it could
have merged with another thread and taken the other threads id.

I think 3 is probably rare enough not to be real concern (though we
should make sure we don't actually give an error). But 1 and 2 seem
undesirable.

Rather than refreshing the result it might make sense to fetch the tags
for the actual messages in that thread (as it was when the search buffer
was loaded) by using notmuch-search-find-stable-query.

So something like the following,

- in each notmuch-search buffer

- see if thread-id thread is in that buffer (or perhaps even if there is a thread
  which contains message id in that buffer?)

- If yes, fetch a new list of tags for the messages that buffer thinks are
  in that thread (using notmuch-search-find-stable-query).

- Update tags appropriately.

If we do that then I think the only change in these buffers is the tag
updates, and exactly as if you changed those tags in the search buffer
itself, the threads shown etc don't change.

What do you think?

Best wishes

Mark

Re: [PATCH v2] emacs: Add notmuch-update-search-tags

[Vladimir Panteleev]

On 2017-08-26 10:23, Mark Walters wrote:
> 1) Suppose a new message arrives in the thread. Then triggering a tag
> change in the show buffer, (eg notmuch automatically removing an unread
> tag) pulls that message into original search buffer.
> 
> 2) If that new message has some tags that aren't in the thread they will
> get marked as "added tags"

Yes, AFAICT this is correct.

> I think 3 is probably rare enough not to be real concern (though we
> should make sure we don't actually give an error). But 1 and 2 seem
> undesirable.

If the thread is gone, then it won't be in the results of the tag query, 
therefore it simply won't be updated.

> If we do that then I think the only change in these buffers is the tag
> updates, and exactly as if you changed those tags in the search buffer
> itself, the threads shown etc don't change.
> 
> What do you think?
I think it's doable in theory, just not efficiently.

In order to fetch correct results for the messages currently displayed 
in the search buffer, we need to pass those exact message IDs to 
notmuch. If a tagging operation affects many messages from a large 
search buffer, doing one notmuch search invocation per thread can add up 
to a lot of notmuch invocations.

It may be possible to optimize this down to one batch search query per 
notmuch-search buffer - however, this results in a large search query. 
Not only would one take a while to execute, but the resulting query can 
become too large to be passed as a command-line parameter, and "notmuch 
search" does not seem to have a --batch switch to read queries from 
standard input.

Even if the above were to work sufficiently well, it is still necessary 
to do one notmuch invocation per search buffer, as individual search 
buffers may capture results from different points in time. In the patch 
above just one additional invocation in total is necessary.

So, I don't see a way to do this in a sufficiently efficient way. 
Performance does matter here, as notmuch will perform tag updates in 
situations where a user shouldn't expect significant delays - e.g. 
holding down the "down arrow" key in a notmuch-show buffer with some 
unread messages will cause notmuch to remove the "unread" tag as point 
travels over each unread message.

What do you think?

-- 
Best regards,
  Vladimir

Re: [PATCH v2] emacs: Add notmuch-update-search-tags

[Mark Walters]

Hi

OK I have now actually tested it, and I have read the patch more
carefully, but I am afraid I still have concerns.

The key problem is the patch assumes that the display of a thread in a
search buffer depends only on the thread. And this is not true as the
number of matching messages is displayed eg [10/19], and the author list
is split into matching authors | unmatching authors.

However, when the tags in a thread are changed the patch makes all
search buffers containing that thread update to look the same. Indeed,
if you change a tag on a single message I think the count will always
update to be [1/??] as there is only one message matching the tag-change
query.

I think you could get round this by modifying your code only slightly --
rather than calling notmuch-search-update-result in
notmuch-search-update-results, *just* update the tags, using something
like notmuch-search-set-tags. (I have just tried this and it seems to work.)

This is not perfect, as this will show tags of newly arrived messages in
the thread, but that might well be acceptable.

And this still keeps it to one call to the database, which avoids your
(completely correct) performance concerns.


> It may be possible to optimize this down to one batch search query per 
> notmuch-search buffer - however, this results in a large search query. 
> Not only would one take a while to execute, but the resulting query can 
> become too large to be passed as a command-line parameter, and "notmuch 
> search" does not seem to have a --batch switch to read queries from 
> standard input.

This points to my next concern -- this is already a problem in the
current patch. If you go into a moderately large search buffer, and do
something like * +foo (to tag all the messages with foo), the tag step
works because is uses the --batch switch to tag, but your search query
doesn't.

The options here are to:  add --batch to search handling, or  just
decide not to do display the tag updates for large queries.

Incidentally there also seems to a bug in the current that the first
thread in a search buffer doesn't seem to get updated. I think you are
using too low level functions -- things like
notmuch-search-foreach-result might do exactly what you need.

Finally, in the longer term, do you wanting to do this for tree and show
buffers too?

[An alternative approach, which I don't think I like but I mention in
case others do -- we could only propagate tag changes to parent
buffers. So updating a tag in a show buffer only updates the single
search buffer that called it. This might avoid the large query problem
as show buffers probably don't have large queries.]

Best wishes

Mark

Re: [PATCH v2] emacs: Add notmuch-update-search-tags

[David Bremner]

Vladimir Panteleev <notmuch@thecybershadow.net> writes:

>  Speaking of which, I had a fun time trying to figure out why my test
>  didn't work before I discovered that notmuch-tag-deleted-formats is
>  reset in test-lib.el. That took quite a bit of debugging; I think it
>  would be good to fix this inconsistency to avoid other contributors
>  wasting time on this in the future.

Yes, I think you're probably right, although I haven't looked at the
resulting work to update all of relevant tests. There's probably a few
other places where we've accumulated "technical debt" by doing the easy
thing in the test suite.

Maybe I'm overthinking this, but it seems like we'd need some way to
recognize various faces. There is a package called faceup that is
targeted at roughly this problem.  I know that racket-mode uses it to
test highlighting. Perhaps that's an dependancy we can tolerate for the
test suite, at least optionally. Or perhaps some simpler approach can
work since we can choose the faces as well.

d

Re: [PATCH v2] emacs: Add notmuch-update-search-tags

[Vladimir Panteleev]

On 2017-08-29 01:18, David Bremner wrote:
> Maybe I'm overthinking this, but it seems like we'd need some way to
> recognize various faces. There is a package called faceup that is
> targeted at roughly this problem.  I know that racket-mode uses it to
> test highlighting. Perhaps that's an dependancy we can tolerate for the
> test suite, at least optionally. Or perhaps some simpler approach can
> work since we can choose the faces as well.

FWIW, I have had good results with using htmlfontify, which is 
distributed with Emacs.

Example from the d-mode package:

https://github.com/Emacs-D-Mode-Maintainers/Emacs-D-Mode/blob/a97c92ced57224287a84e7fc48ba9aac6b2afc08/d-mode-test.el#L270-L292

https://github.com/Emacs-D-Mode-Maintainers/Emacs-D-Mode/blob/a97c92ced57224287a84e7fc48ba9aac6b2afc08/tests/fonts.d.html

-- 
Best regards,
  Vladimir

Re: [PATCH v2] emacs: Add notmuch-update-search-tags

[David Bremner]

Vladimir Panteleev <notmuch@thecybershadow.net> writes:

> On 2017-08-29 01:18, David Bremner wrote:
>> Maybe I'm overthinking this, but it seems like we'd need some way to
>> recognize various faces. There is a package called faceup that is
>> targeted at roughly this problem.  I know that racket-mode uses it to
>> test highlighting. Perhaps that's an dependancy we can tolerate for the
>> test suite, at least optionally. Or perhaps some simpler approach can
>> work since we can choose the faces as well.
>
> FWIW, I have had good results with using htmlfontify, which is 
> distributed with Emacs.
>
> Example from the d-mode package:
>
> https://github.com/Emacs-D-Mode-Maintainers/Emacs-D-Mode/blob/a97c92ced57224287a84e7fc48ba9aac6b2afc08/d-mode-test.el#L270-L292
>
> https://github.com/Emacs-D-Mode-Maintainers/Emacs-D-Mode/blob/a97c92ced57224287a84e7fc48ba9aac6b2afc08/tests/fonts.d.html
>
> -- 
> Best regards,
>   Vladimir

That also sounds like a viable approach, with the advantage of not
needing an add-on package.

d