hl-line fixes

hl-line fixes

[David Bremner]

This is an expanded version of

     id:20240809114506.1081791-1-david@tethera.net

It includes a fairly complete set of tests, and covers tree and
unthreaded views as well as search view.

[PATCH 1/6] emacs: add defcustom to control hl-line mode

[David Bremner]

Currently the presence of hl-line highlighting is controlled
implicitely by hooks. In future commits it will be migrated to use
this variable.
---
 emacs/notmuch.el | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 2a73ffa5..934ec894 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -133,6 +133,11 @@ there will be called at other points of notmuch execution."
   :group 'notmuch-search
   :group 'notmuch-hooks)
 
+(defcustom notmuch-hl-line t
+  "Use hl-line-mode to highlight current thread / message"
+  :type 'boolean
+  :group 'notmuch)
+
 ;;; Mime Utilities
 
 (defun notmuch-foreach-mime-part (function mm-handle)
-- 
2.43.0

[PATCH 2/6] test/emacs: add tests for hl-line-mode integration

[David Bremner]

Most of the known broken tests replicate (my intepretation of) the bug
reported at id:87fsfuuxwn.fsf@thinkbox (or some unreported, but
probably related bugs in tree/unthreaded view). The last 3 broken
tests are just unimplimented planned functionality.
---
 test/T312-emacs-hl-line.sh | 170 +++++++++++++++++++++++++++++++++++++
 1 file changed, 170 insertions(+)
 create mode 100755 test/T312-emacs-hl-line.sh

diff --git a/test/T312-emacs-hl-line.sh b/test/T312-emacs-hl-line.sh
new file mode 100755
index 00000000..4cceed30
--- /dev/null
+++ b/test/T312-emacs-hl-line.sh
@@ -0,0 +1,170 @@
+#!/usr/bin/env bash
+
+test_description="emacs hl-line-mode"
+. $(dirname "$0")/test-lib.sh || exit 1
+. $NOTMUCH_SRCDIR/test/test-lib-emacs.sh || exit 1
+
+EXPECTED=$NOTMUCH_SRCDIR/test/emacs.expected-output
+
+test_require_emacs
+add_email_corpus
+
+test_begin_subtest "line 1, search"
+test_emacs_expect_t '(let ((notmuch-hl-line t))
+			(notmuch-search "tag:inbox")
+			(notmuch-test-wait)
+			(notmuch-test-expect-equal
+			 (let ((start (overlay-start hl-line-overlay))
+			       (end (overlay-end hl-line-overlay)))
+			   (list start (> end start)))
+			 (list 1 t)))'
+
+test_begin_subtest "line 1, tree"
+test_subtest_known_broken
+test_emacs_expect_t '(let ((notmuch-hl-line t))
+			(notmuch-tree "tag:inbox")
+			(notmuch-test-wait)
+			(notmuch-test-expect-equal
+			 (let ((start (overlay-start hl-line-overlay))
+			       (end (overlay-end hl-line-overlay)))
+			   (list start (> end start)))
+			 (list 1 t)))'
+
+test_begin_subtest "line 1, unthreaded"
+test_subtest_known_broken
+test_emacs_expect_t '(let ((notmuch-hl-line t))
+			(notmuch-tree "tag:inbox")
+			(notmuch-test-wait)
+			(notmuch-test-expect-equal
+			 (let ((start (overlay-start hl-line-overlay))
+			       (end (overlay-end hl-line-overlay)))
+			   (list start (> end start)))
+			 (list 1 t)))'
+
+test_begin_subtest "line 1, search, refresh"
+test_emacs_expect_t '(let ((notmuch-hl-line t))
+			(notmuch-search "tag:inbox")
+			(notmuch-test-wait)
+			(notmuch-refresh-this-buffer)
+			(notmuch-test-wait)
+			(notmuch-test-expect-equal (overlay-start hl-line-overlay) 1))'
+
+test_begin_subtest "line 1, tree, refresh"
+test_subtest_known_broken
+test_emacs_expect_t '(let ((notmuch-hl-line t))
+			(notmuch-tree "tag:inbox")
+			(notmuch-test-wait)
+			(notmuch-refresh-this-buffer)
+			(notmuch-test-wait)
+			(notmuch-test-expect-equal
+			 (let ((start (overlay-start hl-line-overlay))
+			       (end (overlay-end hl-line-overlay)))
+			   (list start (> end start)))
+			 (list 1 t)))'
+
+test_begin_subtest "line 1, unthreaded, refresh"
+test_subtest_known_broken
+test_emacs_expect_t '(let ((notmuch-hl-line t))
+			(notmuch-tree "tag:inbox")
+			(notmuch-test-wait)
+			(notmuch-refresh-this-buffer)
+			(notmuch-test-wait)
+			(notmuch-test-expect-equal
+			 (let ((start (overlay-start hl-line-overlay))
+			       (end (overlay-end hl-line-overlay)))
+			   (list start (> end start)))
+			 (list 1 t)))'
+
+
+test_begin_subtest "line 12, notmuch-search"
+test_emacs_expect_t '(let ((notmuch-hl-line t))
+			(notmuch-search "tag:inbox")
+			(notmuch-test-wait)
+			(forward-line 11)
+			(notmuch-post-command)
+			(notmuch-test-expect-equal
+			   (line-number-at-pos (overlay-start hl-line-overlay)) 12))'
+
+test_begin_subtest "line 12, tree"
+test_emacs_expect_t '(let ((notmuch-hl-line t))
+			(notmuch-tree "tag:inbox")
+			(notmuch-test-wait)
+			(forward-line 11)
+			(notmuch-post-command)
+			(notmuch-test-expect-equal
+			   (line-number-at-pos (overlay-start hl-line-overlay)) 12))'
+
+test_begin_subtest "line 12, unthreaded"
+test_emacs_expect_t '(let ((notmuch-hl-line t))
+			(notmuch-unthreaded "tag:inbox")
+			(notmuch-test-wait)
+			(forward-line 11)
+			(notmuch-post-command)
+			(notmuch-test-expect-equal
+			   (line-number-at-pos (overlay-start hl-line-overlay)) 12))'
+
+test_begin_subtest "line 12, search, refresh"
+test_subtest_known_broken
+test_emacs_expect_t '(let ((notmuch-hl-line t))
+			(notmuch-search "tag:inbox")
+			(notmuch-test-wait)
+			(forward-line 11)
+			(notmuch-post-command)
+			(notmuch-refresh-this-buffer)
+			(notmuch-test-wait)
+			(notmuch-test-expect-equal
+			   (line-number-at-pos (overlay-start hl-line-overlay)) 12))'
+
+test_begin_subtest "line 12, tree, refresh"
+test_subtest_known_broken
+test_emacs_expect_t '(let ((notmuch-hl-line t))
+			(notmuch-tree "tag:inbox")
+			(notmuch-test-wait)
+			(forward-line 11)
+			(notmuch-post-command)
+			(notmuch-refresh-this-buffer)
+			(notmuch-test-wait)
+			(notmuch-test-expect-equal
+			   (line-number-at-pos (overlay-start hl-line-overlay)) 12))'
+
+test_begin_subtest "line 12, unthreaded, refresh"
+test_subtest_known_broken
+test_emacs_expect_t '(let ((notmuch-hl-line t))
+			(notmuch-tree "tag:inbox")
+			(notmuch-test-wait)
+			(forward-line 11)
+			(notmuch-post-command)
+			(notmuch-refresh-this-buffer)
+			(notmuch-test-wait)
+			(notmuch-test-expect-equal
+			   (line-number-at-pos (overlay-start hl-line-overlay)) 12))'
+
+test_begin_subtest "hl-line disabled, search"
+test_subtest_known_broken
+test_emacs_expect_t '(let ((notmuch-hl-line nil))
+			(notmuch-search "tag:inbox")
+			(notmuch-test-wait)
+			(notmuch-test-expect-equal
+			   (or hl-line-mode hl-line-overlay)
+			   nil))'
+
+test_begin_subtest "hl-line disabled, tree"
+test_subtest_known_broken
+test_emacs_expect_t '(let ((notmuch-hl-line nil))
+			(notmuch-tree "tag:inbox")
+			(notmuch-test-wait)
+			(notmuch-test-expect-equal
+			   (or hl-line-mode hl-line-overlay)
+			   nil))'
+
+test_begin_subtest "hl-line disabled, unthreaded"
+test_subtest_known_broken
+test_emacs_expect_t '(let ((notmuch-hl-line nil))
+			(notmuch-unthreaded "tag:inbox")
+			(notmuch-test-wait)
+			(notmuch-test-expect-equal
+			   (or hl-line-mode hl-line-overlay)
+			   nil))'
+
+test_done
+
-- 
2.43.0

[PATCH 3/6] emacs: replace use of hook to draw hl-line in search mode

[David Bremner]

In the thread at id:87fsfuuxwn.fsf@thinkbox, a bug is discussed where
the point and hl-line overlay get out of sync, leading the user to
open the wrong message. As far as I can tell this is caused by
notmuch-hl-mode being invoked too early.

This change bypasses the logic preventing notmuch-search-hook being
called only once, so that the overlay is updated later after after the
buffer is full(er).

This change may lead to the overlay being updated multiple times; if
this is annoying we can add a similar buffer local variable to ensure
it is only called once.

The extra logic to check notmuch-search-target-line reduces the
flicker somewhat by not highlighting the first line every time.
---
 emacs/notmuch.el           | 8 ++++++--
 test/T312-emacs-hl-line.sh | 2 --
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 934ec894..2d7e0422 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -126,10 +126,9 @@ there will be called at other points of notmuch execution."
   :type 'file
   :group 'notmuch)
 
-(defcustom notmuch-search-hook '(notmuch-hl-line-mode)
+(defcustom notmuch-search-hook nil
   "List of functions to call when notmuch displays the search results."
   :type 'hook
-  :options '(notmuch-hl-line-mode)
   :group 'notmuch-search
   :group 'notmuch-hooks)
 
@@ -930,6 +929,11 @@ sets the :orig-tag property."
 	(notmuch-sexp-parse-partial-list 'notmuch-search-append-result
 					 results-buf))
       (with-current-buffer results-buf
+	(when (and notmuch-hl-line
+		   (>=
+		    (line-number-at-pos (point-max) t)
+		    (or notmuch-search-target-line -1)))
+	  (notmuch-hl-line-mode))
 	(notmuch--search-hook-wrapper)))))
 
 ;;; Commands (and some helper functions used by them)
diff --git a/test/T312-emacs-hl-line.sh b/test/T312-emacs-hl-line.sh
index 4cceed30..4697b671 100755
--- a/test/T312-emacs-hl-line.sh
+++ b/test/T312-emacs-hl-line.sh
@@ -104,7 +104,6 @@ test_emacs_expect_t '(let ((notmuch-hl-line t))
 			   (line-number-at-pos (overlay-start hl-line-overlay)) 12))'
 
 test_begin_subtest "line 12, search, refresh"
-test_subtest_known_broken
 test_emacs_expect_t '(let ((notmuch-hl-line t))
 			(notmuch-search "tag:inbox")
 			(notmuch-test-wait)
@@ -140,7 +139,6 @@ test_emacs_expect_t '(let ((notmuch-hl-line t))
 			   (line-number-at-pos (overlay-start hl-line-overlay)) 12))'
 
 test_begin_subtest "hl-line disabled, search"
-test_subtest_known_broken
 test_emacs_expect_t '(let ((notmuch-hl-line nil))
 			(notmuch-search "tag:inbox")
 			(notmuch-test-wait)
-- 
2.43.0

[PATCH 4/6] emacs/tree: condition hl-line-mode on notmuch-hl-line

[David Bremner]

It isn't clear that this call to hl-line-mode will survive the coming
re-organization to stop relying on hooks, but incrementally this at
least makes the disabling behviour consistent.
---
 emacs/notmuch-tree.el      | 3 ++-
 test/T312-emacs-hl-line.sh | 2 --
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/emacs/notmuch-tree.el b/emacs/notmuch-tree.el
index 2332f020..24a9970f 100644
--- a/emacs/notmuch-tree.el
+++ b/emacs/notmuch-tree.el
@@ -1091,7 +1091,8 @@ Complete list of currently available key bindings:
 
 \\{notmuch-tree-mode-map}"
   (setq notmuch-buffer-refresh-function #'notmuch-tree-refresh-view)
-  (hl-line-mode 1)
+  (when notmuch-hl-line
+    (hl-line-mode 1))
   (setq buffer-read-only t)
   (setq truncate-lines t)
   (when notmuch-tree-outline-enabled (notmuch-tree-outline-mode 1)))
diff --git a/test/T312-emacs-hl-line.sh b/test/T312-emacs-hl-line.sh
index 4697b671..dd27db0e 100755
--- a/test/T312-emacs-hl-line.sh
+++ b/test/T312-emacs-hl-line.sh
@@ -147,7 +147,6 @@ test_emacs_expect_t '(let ((notmuch-hl-line nil))
 			   nil))'
 
 test_begin_subtest "hl-line disabled, tree"
-test_subtest_known_broken
 test_emacs_expect_t '(let ((notmuch-hl-line nil))
 			(notmuch-tree "tag:inbox")
 			(notmuch-test-wait)
@@ -156,7 +155,6 @@ test_emacs_expect_t '(let ((notmuch-hl-line nil))
 			   nil))'
 
 test_begin_subtest "hl-line disabled, unthreaded"
-test_subtest_known_broken
 test_emacs_expect_t '(let ((notmuch-hl-line nil))
 			(notmuch-unthreaded "tag:inbox")
 			(notmuch-test-wait)
-- 
2.43.0

[PATCH 5/6] emacs/tree: call notmuch-hl-line-mode from tree-sentinel

[David Bremner]

There is a a perceptible gap between when the tree shows up and when
the hl-line is visible, but this is better than the previous state
where the line did not show up at all until the user moved the cursor.
---
 emacs/notmuch-tree.el      | 2 ++
 test/T312-emacs-hl-line.sh | 6 ------
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/emacs/notmuch-tree.el b/emacs/notmuch-tree.el
index 24a9970f..481b0b34 100644
--- a/emacs/notmuch-tree.el
+++ b/emacs/notmuch-tree.el
@@ -1122,6 +1122,8 @@ object, and with the tree results buffer as the current buffer.")
 		(unless (= exit-status 0)
 		  (insert (format " (process returned %d)" exit-status)))
 		(insert "\n"))))
+	  (when (and notmuch-hl-line (= exit-status 0))
+	    (notmuch-hl-line-mode))
 	  (run-hook-with-args 'notmuch-tree-process-exit-functions proc))))))
 
 (defun notmuch-tree-process-filter (proc string)
diff --git a/test/T312-emacs-hl-line.sh b/test/T312-emacs-hl-line.sh
index dd27db0e..3402811c 100755
--- a/test/T312-emacs-hl-line.sh
+++ b/test/T312-emacs-hl-line.sh
@@ -20,7 +20,6 @@ test_emacs_expect_t '(let ((notmuch-hl-line t))
 			 (list 1 t)))'
 
 test_begin_subtest "line 1, tree"
-test_subtest_known_broken
 test_emacs_expect_t '(let ((notmuch-hl-line t))
 			(notmuch-tree "tag:inbox")
 			(notmuch-test-wait)
@@ -31,7 +30,6 @@ test_emacs_expect_t '(let ((notmuch-hl-line t))
 			 (list 1 t)))'
 
 test_begin_subtest "line 1, unthreaded"
-test_subtest_known_broken
 test_emacs_expect_t '(let ((notmuch-hl-line t))
 			(notmuch-tree "tag:inbox")
 			(notmuch-test-wait)
@@ -50,7 +48,6 @@ test_emacs_expect_t '(let ((notmuch-hl-line t))
 			(notmuch-test-expect-equal (overlay-start hl-line-overlay) 1))'
 
 test_begin_subtest "line 1, tree, refresh"
-test_subtest_known_broken
 test_emacs_expect_t '(let ((notmuch-hl-line t))
 			(notmuch-tree "tag:inbox")
 			(notmuch-test-wait)
@@ -63,7 +60,6 @@ test_emacs_expect_t '(let ((notmuch-hl-line t))
 			 (list 1 t)))'
 
 test_begin_subtest "line 1, unthreaded, refresh"
-test_subtest_known_broken
 test_emacs_expect_t '(let ((notmuch-hl-line t))
 			(notmuch-tree "tag:inbox")
 			(notmuch-test-wait)
@@ -115,7 +111,6 @@ test_emacs_expect_t '(let ((notmuch-hl-line t))
 			   (line-number-at-pos (overlay-start hl-line-overlay)) 12))'
 
 test_begin_subtest "line 12, tree, refresh"
-test_subtest_known_broken
 test_emacs_expect_t '(let ((notmuch-hl-line t))
 			(notmuch-tree "tag:inbox")
 			(notmuch-test-wait)
@@ -127,7 +122,6 @@ test_emacs_expect_t '(let ((notmuch-hl-line t))
 			   (line-number-at-pos (overlay-start hl-line-overlay)) 12))'
 
 test_begin_subtest "line 12, unthreaded, refresh"
-test_subtest_known_broken
 test_emacs_expect_t '(let ((notmuch-hl-line t))
 			(notmuch-tree "tag:inbox")
 			(notmuch-test-wait)
-- 
2.43.0

[PATCH 6/6] emacs/tree: add call to notmuch-hl-line-mode from process-filter

[David Bremner]

This removes the visual gap/stutter between when the screen fills and
when the hl-line "cursor" is drawn.  It is not obviously how to
robustly test this, since it the observable effect is purely a matter
of timing.
---
 emacs/notmuch-tree.el | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/emacs/notmuch-tree.el b/emacs/notmuch-tree.el
index 481b0b34..301ffd6e 100644
--- a/emacs/notmuch-tree.el
+++ b/emacs/notmuch-tree.el
@@ -1139,7 +1139,10 @@ object, and with the tree results buffer as the current buffer.")
 	  (goto-char (point-max))
 	  (insert string))
 	(notmuch-sexp-parse-partial-list 'notmuch-tree-insert-forest-thread
-					 results-buf)))))
+					 results-buf))
+      (with-current-buffer results-buf
+	(when notmuch-hl-line
+	  (notmuch-hl-line-mode))))))
 
 (defun notmuch-tree-worker (basic-query &optional query-context target
 					open-target unthreaded oldest-first
-- 
2.43.0