Patch to avoid flicker in Isearch with lazy count

Patch to avoid flicker in Isearch with lazy count

[Augusto Stoffel]

[-- Attachment #1: Type: text/plain, Size: 1223 bytes --]

Setting ‘lazy-highlight-initial-delay’ to 0 (which should be a
reasonable default on a modern machine) causes some flickering in the
search buffer, and a lot of flickering in the echo are when
‘isearch-lazy-count’ is on.

The attached patch addresses these problems.  It also makes the
desired effect of setting ‘lazy-highlight-initial-delay’ to 0 the
default, even though that variable is unchanged.

Flicker in the search buffer happens because the highlighting of
matches happens after typing just one letter, and there are
potentially many of those.  It is fixed by adding a new variable,
‘lazy-highlight-delay-max-chars’.  Lazy highlighting kicks in
immediately if the search string is longer than that number.  I set
the default value to 2, but maybe 1 is even better.

Flicker in the echo area happens even with the default value of
‘lazy-highlight-initial-delay’, if less pronouncedly, and is due to
too frequent updates to the echo area message.  This is solved by
keeping the lazy count in the echo area of out of date for the
fraction of a second where the lazy stuff is waiting to kick in.

Please let me know if this works well and can be merged.

Best,
Augusto


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Reduce-flicker-in-Isearch-mode.patch --]
[-- Type: text/x-patch, Size: 5869 bytes --]

From aaef7a6438aaa74a5978e5e9d4b5e95910c2b15e Mon Sep 17 00:00:00 2001
From: Augusto Stoffel <arstoffel@gmail.com>
Date: Wed, 27 Jan 2021 16:09:38 +0100
Subject: [PATCH] Reduce flicker in Isearch mode

Lazy highlighting now happens immediately (only) if the search string
is longer than the value of the new custom variable
`lazy-highlight-delay-max-chars`.  Also avoid updating the lazy count
in the echo area too often.
* isearch.el (lazy-highlight-delay-max-chars): new defcustom
* isearch.el (isearch-lazy-highlight-with-timer): new function,
factors out a repeated snippet of code.
* isearch.el (isearch-lazy-highlight-new-loop,
isearch-lazy-highlight-update, isearch-lazy-highlight-buffer-update):
use the new `isearch-lazy-highlight-with-timer` function.
* isearch.el (isearch-lazy-highlight-new-loop): avoid a call to
`isearch-message` that is quickly succeed by a second echo area
update, thus causing flicker.
---
 lisp/isearch.el | 48 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 16 deletions(-)

diff --git a/lisp/isearch.el b/lisp/isearch.el
index a86678572c..ca0fd1434a 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -352,12 +352,22 @@ lazy-highlight-cleanup
   :group 'lazy-highlight)
 
 (defcustom lazy-highlight-initial-delay 0.25
-  "Seconds to wait before beginning to lazily highlight all matches."
+  "Seconds to wait before beginning to lazily highlight all matches.
+This setting only has effect if the current search string is at most
+`lazy-highlight-initial-delay' character long."
   :type 'number
   :group 'lazy-highlight)
 
+(defcustom lazy-highlight-delay-max-chars 2
+  "Size of a search string above which lazy highlight happens immediately."
+  :type 'number
+  :group 'lazy-highlight
+  :version "28.1")
+
 (defcustom lazy-highlight-interval 0 ; 0.0625
-  "Seconds between lazily highlighting successive matches."
+  "Seconds between lazily highlighting successive matches.
+This setting only has effect if the current search string is at most
+`lazy-highlight-initial-delay' character long."
   :type 'number
   :group 'lazy-highlight)
 
@@ -3357,7 +3367,7 @@ isearch-lazy-count-format
              (not isearch-error)
              (not isearch-suspended))
         (format format-string
-                (if isearch-forward
+                (if isearch-lazy-highlight-forward
                     isearch-lazy-count-current
                   (if (eq isearch-lazy-count-current 0)
                       0
@@ -3858,6 +3868,15 @@ lazy-highlight-cleanup
     (cancel-timer isearch-lazy-highlight-timer)
     (setq isearch-lazy-highlight-timer nil)))
 
+(defun isearch-lazy-highlight-with-timer (delay function)
+  "Arrange for FUNCTION to be called in idle time, possibly with DELAY.
+There is no delay if the current search string is longer than
+`lazy-highlight-delay-max-chars'."
+  (setq isearch-lazy-highlight-timer
+        (run-with-idle-timer
+         (if (> (length isearch-string) lazy-highlight-delay-max-chars) 0 delay)
+         nil function)))
+
 (defun isearch-lazy-highlight-new-loop (&optional beg end)
   "Cleanup any previous `lazy-highlight' loop and begin a new one.
 BEG and END specify the bounds within which highlighting should occur.
@@ -3917,7 +3936,8 @@ isearch-lazy-highlight-new-loop
         (clrhash isearch-lazy-count-hash)
         (setq isearch-lazy-count-current nil
               isearch-lazy-count-total nil)
-        (isearch-message)))
+        ;; Delay updating the message if possible, to avoid flicker
+        (when (string-equal isearch-string "") (isearch-message))))
     (setq isearch-lazy-highlight-window-start-changed nil)
     (setq isearch-lazy-highlight-window-end-changed nil)
     (setq isearch-lazy-highlight-error isearch-error)
@@ -3961,9 +3981,8 @@ isearch-lazy-highlight-new-loop
 		    (1- (length isearch-lazy-highlight-last-string)))
 		 (point-min))))
     (unless (equal isearch-string "")
-      (setq isearch-lazy-highlight-timer
-            (run-with-idle-timer lazy-highlight-initial-delay nil
-                                 'isearch-lazy-highlight-start))))
+      (isearch-lazy-highlight-with-timer lazy-highlight-initial-delay
+                                         'isearch-lazy-highlight-start)))
   ;; Update the current match number only in isearch-mode and
   ;; unless isearch-mode is used specially with isearch-message-function
   (when (and isearch-lazy-count isearch-mode (null isearch-message-function))
@@ -4101,12 +4120,10 @@ isearch-lazy-highlight-update
 		  (if isearch-lazy-highlight-forward
 		      (setq isearch-lazy-highlight-end (point-min))
 		    (setq isearch-lazy-highlight-start (point-max)))
-		  (setq isearch-lazy-highlight-timer
-			(run-at-time lazy-highlight-interval nil
-				     'isearch-lazy-highlight-buffer-update)))
-	      (setq isearch-lazy-highlight-timer
-		    (run-at-time lazy-highlight-interval nil
-				 'isearch-lazy-highlight-update)))))))))
+	          (isearch-lazy-highlight-with-timer
+	           lazy-highlight-interval 'isearch-lazy-highlight-buffer-update))
+	      (isearch-lazy-highlight-with-timer
+               lazy-highlight-interval 'isearch-lazy-highlight-update))))))))
 
 (defun isearch-lazy-highlight-buffer-update ()
   "Update highlighting of other matches in the full buffer."
@@ -4180,9 +4197,8 @@ isearch-lazy-highlight-buffer-update
 		  (setq isearch-lazy-count-current
 			(gethash opoint isearch-lazy-count-hash 0))
 		  (isearch-message))
-	      (setq isearch-lazy-highlight-timer
-		    (run-at-time lazy-highlight-interval nil
-				 'isearch-lazy-highlight-buffer-update)))))))))
+	      (isearch-lazy-highlight-with-timer
+	       lazy-highlight-interval 'isearch-lazy-highlight-buffer-update))))))))
 
 (defun isearch-resume (string regexp word forward message case-fold)
   "Resume an incremental search.
-- 
2.29.2

Re: Patch to avoid flicker in Isearch with lazy count

[Juri Linkov]

> Setting ‘lazy-highlight-initial-delay’ to 0 (which should be a
> reasonable default on a modern machine) causes some flickering in the
> search buffer, and a lot of flickering in the echo are when
> ‘isearch-lazy-count’ is on.
>
> The attached patch addresses these problems.

Thanks for the patch.

> It also makes the desired effect of setting ‘lazy-highlight-initial-delay’
> to 0 the default, even though that variable is unchanged.

Does this mean that for users who already customized
‘lazy-highlight-initial-delay’ to 0, now it's recommended to
revert such customization, and leave it at its default non-zero value?
So after typing just one letter an initial delay is recommended,
and after typing more letters no delay.  Then this should be mentioned
in etc/NEWS.

> Flicker in the search buffer happens because the highlighting of
> matches happens after typing just one letter, and there are
> potentially many of those.  It is fixed by adding a new variable,
> ‘lazy-highlight-delay-max-chars’.  Lazy highlighting kicks in
> immediately if the search string is longer than that number.  I set
> the default value to 2, but maybe 1 is even better.
>
> Flicker in the echo area happens even with the default value of
> ‘lazy-highlight-initial-delay’, if less pronouncedly, and is due to
> too frequent updates to the echo area message.  This is solved by
> keeping the lazy count in the echo area of out of date for the
> fraction of a second where the lazy stuff is waiting to kick in.

Please see more comments:

>  (defcustom lazy-highlight-initial-delay 0.25
> -  "Seconds to wait before beginning to lazily highlight all matches."
> +  "Seconds to wait before beginning to lazily highlight all matches.
> +This setting only has effect if the current search string is at most
> +`lazy-highlight-initial-delay' character long."

Typo?  Maybe lazy-highlight-delay-max-chars?

>  (defcustom lazy-highlight-interval 0 ; 0.0625
> -  "Seconds between lazily highlighting successive matches."
> +  "Seconds between lazily highlighting successive matches.
> +This setting only has effect if the current search string is at most
> +`lazy-highlight-initial-delay' character long."

The same typo?

> +(defun isearch-lazy-highlight-with-timer (delay function)
> +  "Arrange for FUNCTION to be called in idle time, possibly with DELAY.
> +There is no delay if the current search string is longer than
> +`lazy-highlight-delay-max-chars'."
> +  (setq isearch-lazy-highlight-timer
> +        (run-with-idle-timer
> +         (if (> (length isearch-string) lazy-highlight-delay-max-chars) 0 delay)
> +         nil function)))

Currently, run-with-idle-timer is used for lazy-highlight-initial-delay,
and run-at-time for lazy-highlight-interval.  But you replaced all
with run-with-idle-timer.  Any reason not to use run-at-time
for lazy-highlight-interval to reduce delays for quickly finishing
the already started lazily highlighting successive matches?

Re: Patch to avoid flicker in Isearch with lazy count

[Augusto Stoffel]

Hi Juri,

On Wed, 27 Jan 2021 at 19:48, Juri Linkov <juri@linkov.net> wrote:

> Does this mean that for users who already customized
> ‘lazy-highlight-initial-delay’ to 0, now it's recommended to
> revert such customization, and leave it at its default non-zero value?

That's right.  I'll make sure to add a NEWS entry and fix those typos in
the next iteration of the patch.

> Currently, run-with-idle-timer is used for lazy-highlight-initial-delay,
> and run-at-time for lazy-highlight-interval.  But you replaced all
> with run-with-idle-timer.  Any reason not to use run-at-time
> for lazy-highlight-interval to reduce delays for quickly finishing
> the already started lazily highlighting successive matches?

That was just to avoid repeating a piece of code at several places.  It
makes no difference with the default ‘lazy-highlight-interval’ of 0,
right?  But it might actually be better to make
‘lazy-highlight-delay-max-chars’ to affect only
‘lazy-highlight-initial-delay’ and not ‘lazy-highlight-interval’.  What
do you think?

Best,
Augusto

Re: Patch to avoid flicker in Isearch with lazy count

[Juri Linkov]

>> Currently, run-with-idle-timer is used for lazy-highlight-initial-delay,
>> and run-at-time for lazy-highlight-interval.  But you replaced all
>> with run-with-idle-timer.  Any reason not to use run-at-time
>> for lazy-highlight-interval to reduce delays for quickly finishing
>> the already started lazily highlighting successive matches?
>
> That was just to avoid repeating a piece of code at several places.  It
> makes no difference with the default ‘lazy-highlight-interval’ of 0,
> right?

I'm not sure if there is a difference between run-at-time and run-with-idle-timer
with the delay 0.  In practice maybe the difference is negligible.

> But it might actually be better to make
> ‘lazy-highlight-delay-max-chars’ to affect only
> ‘lazy-highlight-initial-delay’ and not ‘lazy-highlight-interval’.
> What do you think?

Definitely, ‘lazy-highlight-delay-max-chars’ should not affect
‘lazy-highlight-interval’ because with pauses between highlighting
portions of matches, when lazy-highlight-buffer-max-at-a-time is 20,
this could create a situation when only first 20 matches are counted,
and during the delay before processing the next portion of matches,
the users might think this is the final total of all existing matches,
so this is misleading for users.

Re: Patch to avoid flicker in Isearch with lazy count

[Augusto Stoffel]

On Thu, 28 Jan 2021 at 11:06, Juri Linkov <juri@linkov.net> wrote:

> Definitely, ‘lazy-highlight-delay-max-chars’ should not affect
> ‘lazy-highlight-interval’ because with pauses between highlighting
> portions of matches, when lazy-highlight-buffer-max-at-a-time is 20,
> this could create a situation when only first 20 matches are counted,
> and during the delay before processing the next portion of matches,
> the users might think this is the final total of all existing matches,
> so this is misleading for users.

Good, I'll make it so.  By the way, why is the default
‘lazy-highlight-buffer-max-at-a-time’ (20) so low?  The overhead of
creating a timer for every 20 matches is pretty substantial.  On my
machine, a value like 500 or 1000 makes the lazy count finish visibly
faster, and doesn't affect responsiveness even in huge files.

Re: Patch to avoid flicker in Isearch with lazy count

[Juri Linkov]

>> Definitely, ‘lazy-highlight-delay-max-chars’ should not affect
>> ‘lazy-highlight-interval’ because with pauses between highlighting
>> portions of matches, when lazy-highlight-buffer-max-at-a-time is 20,
>> this could create a situation when only first 20 matches are counted,
>> and during the delay before processing the next portion of matches,
>> the users might think this is the final total of all existing matches,
>> so this is misleading for users.
>
> Good, I'll make it so.  By the way, why is the default
> ‘lazy-highlight-buffer-max-at-a-time’ (20) so low?  The overhead of
> creating a timer for every 20 matches is pretty substantial.  On my
> machine, a value like 500 or 1000 makes the lazy count finish visibly
> faster, and doesn't affect responsiveness even in huge files.

For historical reasons, when computers were slow.  Maybe now it's time
to bump up the default value.

Re: Patch to avoid flicker in Isearch with lazy count

[Augusto Stoffel]

[-- Attachment #1: Type: text/plain, Size: 496 bytes --]

Hi Juri,

I have attached a new patch addressing the issues we discussed.  I have
also changed the name of the new variable a bit, I hope it's a bit more
self-explanatory now.

I have also tested increasing ‘lazy-highlight-buffer-max-at-a-time’ to
1000.  I can't see any loss in responsiveness, except when the old value
of 20 also makes the editor freeze (for instance when searching for
".*a.*a$" in very large buffer).  This is probably a reasonable default.

Best,
Augusto


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Reduce-flicker-in-Isearch-mode.patch --]
[-- Type: text/x-patch, Size: 5082 bytes --]

From 2562d8b3b804729310590b1d689ce5a7c6ff2ea0 Mon Sep 17 00:00:00 2001
From: Augusto Stoffel <arstoffel@gmail.com>
Date: Wed, 27 Jan 2021 16:09:38 +0100
Subject: [PATCH] Reduce flicker in Isearch mode

Lazy highlighting now happens immediately when the search string is at
least as long as the value of the new custom variable
`lazy-highlight-delay-no-delay-length`.  Also avoid updating the lazy
count in the echo area too often.
* isearch.el (lazy-highlight-delay-no-delay-length): new defcustom
* isearch.el (lazy-lazy-count-format): avoid a momentarily incorrect
count when reversing search direction.
* isearch.el (isearch-lazy-highlight-new-loop): avoid a call to
`isearch-message` that is quickly succeed by a second echo area
update, thus causing flicker.
* isearch.el (isearch-lazy-highlight-new-loop):
start lazy highlight immediately if appropriate.
* etc/NEWS: Announce the change.
* lisp/iserch.el: Document `lazy-highlight-delay-no-delay-length'
---
 doc/emacs/search.texi |  7 +++++++
 etc/NEWS              |  7 +++++++
 lisp/isearch.el       | 23 +++++++++++++++++++----
 3 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/doc/emacs/search.texi b/doc/emacs/search.texi
index 637867b811..f3c42bcea7 100644
--- a/doc/emacs/search.texi
+++ b/doc/emacs/search.texi
@@ -2027,6 +2027,13 @@ Search Customizations
 @item lazy-highlight-initial-delay
 @vindex lazy-highlight-initial-delay
 Time in seconds to wait before highlighting visible matches.
+Applies only if the search string is less than
+@code{lazy-highlight-no-delay-length} characters long.
+
+@item lazy-highlight-no-delay-length
+@vindex lazy-highlight-no-delay-length
+For search strings at least as long as the value of this variable,
+lazy highlighting of matches starts immediately.
 
 @item lazy-highlight-interval
 @vindex lazy-highlight-interval
diff --git a/etc/NEWS b/etc/NEWS
index e038076e96..becda4bc6b 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -226,6 +226,13 @@ C-M-<return> instead of <C-M-return>.  Either variant can be used as
 input; functions such as 'kbd' and 'read-kbd-macro' accept both styles
 as equivalent (they have done so for a long time).
 
++++
+** New user option 'lazy-highlight-no-delay-length'.
+Lazy highlighting of matches in Isearch now starts immediately if the
+search string is at least this long.  'lazy-highlight-initial-delay'
+still applies for shorter search strings, which avoids flicker in the
+search buffer due to too many matches being highlighted.
+
 \f
 * Editing Changes in Emacs 28.1
 
diff --git a/lisp/isearch.el b/lisp/isearch.el
index a86678572c..a1c393c9ee 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -352,10 +352,20 @@ lazy-highlight-cleanup
   :group 'lazy-highlight)
 
 (defcustom lazy-highlight-initial-delay 0.25
-  "Seconds to wait before beginning to lazily highlight all matches."
+  "Seconds to wait before beginning to lazily highlight all matches.
+This setting only has effect when the search string is less than
+`lazy-highlight-no-delay-length' characters long."
   :type 'number
   :group 'lazy-highlight)
 
+(defcustom lazy-highlight-no-delay-length 3
+  "For search strings at least this long, lazy highlight starts immediately.
+For shorter search strings, `lazy-highlight-initial-delay'
+applies."
+  :type 'number
+  :group 'lazy-highlight
+  :version "28.1")
+
 (defcustom lazy-highlight-interval 0 ; 0.0625
   "Seconds between lazily highlighting successive matches."
   :type 'number
@@ -3357,7 +3367,7 @@ isearch-lazy-count-format
              (not isearch-error)
              (not isearch-suspended))
         (format format-string
-                (if isearch-forward
+                (if isearch-lazy-highlight-forward
                     isearch-lazy-count-current
                   (if (eq isearch-lazy-count-current 0)
                       0
@@ -3917,7 +3927,8 @@ isearch-lazy-highlight-new-loop
         (clrhash isearch-lazy-count-hash)
         (setq isearch-lazy-count-current nil
               isearch-lazy-count-total nil)
-        (isearch-message)))
+        ;; Delay updating the message if possible, to avoid flicker
+        (when (string-equal isearch-string "") (isearch-message))))
     (setq isearch-lazy-highlight-window-start-changed nil)
     (setq isearch-lazy-highlight-window-end-changed nil)
     (setq isearch-lazy-highlight-error isearch-error)
@@ -3962,7 +3973,11 @@ isearch-lazy-highlight-new-loop
 		 (point-min))))
     (unless (equal isearch-string "")
       (setq isearch-lazy-highlight-timer
-            (run-with-idle-timer lazy-highlight-initial-delay nil
+            (run-with-idle-timer (if (>= (length isearch-string)
+                                         lazy-highlight-no-delay-length)
+                                     0
+                                   lazy-highlight-initial-delay)
+                                 nil
                                  'isearch-lazy-highlight-start))))
   ;; Update the current match number only in isearch-mode and
   ;; unless isearch-mode is used specially with isearch-message-function
-- 
2.29.2

Re: Patch to avoid flicker in Isearch with lazy count

[Juri Linkov]

> I have attached a new patch addressing the issues we discussed.  I have
> also changed the name of the new variable a bit, I hope it's a bit more
> self-explanatory now.

Thanks for the patch, everything looks good.  Now pushed to master.

> I have also tested increasing ‘lazy-highlight-buffer-max-at-a-time’ to
> 1000.  I can't see any loss in responsiveness, except when the old value
> of 20 also makes the editor freeze (for instance when searching for
> ".*a.*a$" in very large buffer).  This is probably a reasonable default.

Indeed, there are corner cases that can make isearch unresponsive.
Feel free to send more patches when your find better defaults.

Re: Patch to avoid flicker in Isearch with lazy count

[Stefan Kangas]

Juri Linkov <juri@linkov.net> writes:

> > I have attached a new patch addressing the issues we discussed.  I have
> > also changed the name of the new variable a bit, I hope it's a bit more
> > self-explanatory now.
>
> Thanks for the patch, everything looks good.  Now pushed to master.

[Sorry for coming back to this late, it comes from looking over NEWS
in preparation of the upcoming release of Emacs 28.1.]

If the variable 'lazy-highlight-no-delay-length' only applies to
isearch, shouldn't it rather be named
'isearch-lazy-highlight-no-delay-length'?

If it does not only relate to isearch, perhaps this should be clarified in NEWS?
The current text is:

    ** New user option 'lazy-highlight-no-delay-length'.
    Lazy highlighting of matches in Isearch now starts immediately if the
    search string is at least this long.  'lazy-highlight-initial-delay'
    still applies for shorter search strings, which avoids flicker in the
    search buffer due to too many matches being highlighted.

I see that there are several variables starting with 'lazy-highlight',
and I know next to nothing about this, so please let me know what you
think.

Re: Patch to avoid flicker in Isearch with lazy count

[Juri Linkov]

> If the variable 'lazy-highlight-no-delay-length' only applies to
> isearch, shouldn't it rather be named
> 'isearch-lazy-highlight-no-delay-length'?
>
> If it does not only relate to isearch, perhaps this should be clarified in NEWS?
> The current text is:
>
>     ** New user option 'lazy-highlight-no-delay-length'.
>     Lazy highlighting of matches in Isearch now starts immediately if the
>     search string is at least this long.  'lazy-highlight-initial-delay'
>     still applies for shorter search strings, which avoids flicker in the
>     search buffer due to too many matches being highlighted.
>
> I see that there are several variables starting with 'lazy-highlight',
> and I know next to nothing about this, so please let me know what you
> think.

Indeed, like other variables starting with 'lazy-highlight',
'lazy-highlight-no-delay-length' is not applied only to Isearch.
But still Isearch is its primary consumer, so it's ok to mention only
Isearch in NEWS.