bug#53126: 29.0.50; [PATCH] Lazy highlight/count when reading query-replace string, etc.

[Augusto Stoffel <arstoffel@gmail.com>]

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

Hi Juri,

Sorry for getting back to this after such a long time.  I've attached a
new patch that hopefully is good to merge, except for adding some NEWS
entry.  Let me know what you think.

Inlined below some further comments.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Display-lazy-highlight-and-match-count-in-when-readi.patch --]
[-- Type: text/x-patch, Size: 12205 bytes --]

From 87ad2a45293e2fa1796a39f368d9ec16c0c4c070 Mon Sep 17 00:00:00 2001
From: Augusto Stoffel <arstoffel@gmail.com>
Date: Sat, 8 Jan 2022 11:08:46 +0100
Subject: [PATCH] Display lazy highlight and match count in when reading
 regexps

* lisp/isearch.el (lazy-count-update-hook): New hook allowing to
display the lazy count in special ways.
(isearch-edit-string): Add lazy highlight and count of matching text.
(isearch-lazy-highlight-new-loop,
isearch-lazy-highlight-buffer-update): Use
`isearch-lazy-count-display-function' instead of hardcoded call to
`isearch-message'.
(minibuffer-lazy-count-format, minibuffer-lazy-highlight-transform,
minibuffer-lazy-highlight--overlay, minibuffer-lazy-highlight--count,
minibuffer-lazy-highlight--after-change,
minibuffer-lazy-highlight--exit, minibuffer-lazy-highlight-setup):
Variables and functions implementing the lazy highlight functionality
while reading from minibuffer.
* lisp/replace.el (query-replace-read-from): Add lazy highlighting.
(replace--region-filter): New function, extracted from
'perform-replace'.
---
 lisp/isearch.el | 84 +++++++++++++++++++++++++++++++++++++++++++++----
 lisp/replace.el | 58 ++++++++++++++++++++++++----------
 2 files changed, 120 insertions(+), 22 deletions(-)

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 8970216398..6076cc4b5e 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -1812,6 +1812,8 @@ isearch-edit-string
 	  (minibuffer-history-symbol)
 	  ;; Search string might have meta information on text properties.
 	  (minibuffer-allow-text-properties t))
+     (when isearch-lazy-highlight
+       (add-hook 'minibuffer-setup-hook #'minibuffer-lazy-highlight-setup))
      (setq isearch-new-string
 	   (read-from-minibuffer
 	    (isearch-message-prefix nil isearch-nonincremental)
@@ -3990,6 +3992,8 @@ isearch-lazy-highlight-error
 (defvar isearch-lazy-count-current nil)
 (defvar isearch-lazy-count-total nil)
 (defvar isearch-lazy-count-hash (make-hash-table))
+(defvar lazy-count-update-hook nil
+  "Hook run after new lazy count results are computed.")
 
 (defun lazy-highlight-cleanup (&optional force procrastinate)
   "Stop lazy highlighting and remove extra highlighting from current buffer.
@@ -4048,7 +4052,7 @@ isearch-lazy-highlight-new-loop
 			         isearch-lazy-highlight-window-end))))))
     ;; something important did indeed change
     (lazy-highlight-cleanup t (not (equal isearch-string ""))) ;stop old timer
-    (when (and isearch-lazy-count isearch-mode (null isearch-message-function))
+    (when isearch-lazy-count
       (when (or (equal isearch-string "")
                 ;; Check if this place was reached by a condition above
                 ;; other than changed window boundaries (that shouldn't
@@ -4067,7 +4071,10 @@ isearch-lazy-highlight-new-loop
         (setq isearch-lazy-count-current nil
               isearch-lazy-count-total nil)
         ;; Delay updating the message if possible, to avoid flicker
-        (when (string-equal isearch-string "") (isearch-message))))
+        (when (string-equal isearch-string "")
+          (when (and isearch-mode (null isearch-message-function))
+            (isearch-message))
+          (run-hooks 'lazy-count-update-hook))))
     (setq isearch-lazy-highlight-window-start-changed nil)
     (setq isearch-lazy-highlight-window-end-changed nil)
     (setq isearch-lazy-highlight-error isearch-error)
@@ -4120,13 +4127,15 @@ isearch-lazy-highlight-new-loop
                                  '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))
+  (when isearch-lazy-count
     ;; Update isearch-lazy-count-current only when it was already set
     ;; at the end of isearch-lazy-highlight-buffer-update
     (when isearch-lazy-count-current
       (setq isearch-lazy-count-current
             (gethash (point) isearch-lazy-count-hash 0))
-      (isearch-message))))
+      (when (and isearch-mode (null isearch-message-function))
+        (isearch-message))
+      (run-hooks 'lazy-count-update-hook))))
 
 (defun isearch-lazy-highlight-search (string bound)
   "Search ahead for the next or previous match, for lazy highlighting.
@@ -4327,16 +4336,79 @@ isearch-lazy-highlight-buffer-update
 		    (setq looping nil
 			  nomore  t))))
 	    (if nomore
-		(when (and isearch-lazy-count isearch-mode (null isearch-message-function))
+		(when isearch-lazy-count
 		  (unless isearch-lazy-count-total
 		    (setq isearch-lazy-count-total 0))
 		  (setq isearch-lazy-count-current
 			(gethash opoint isearch-lazy-count-hash 0))
-		  (isearch-message))
+                  (when (and isearch-mode (null isearch-message-function))
+                    (isearch-message))
+		  (run-hooks 'lazy-count-update-hook))
 	      (setq isearch-lazy-highlight-timer
 		    (run-at-time lazy-highlight-interval nil
 				 'isearch-lazy-highlight-buffer-update)))))))))
 
+;; Reading from minibuffer with lazy highlight and match count
+
+(defcustom minibuffer-lazy-count-format "%s "
+  "Format of the total number of matches for the prompt prefix."
+  :type '(choice (const :tag "Don't display a count" nil)
+                 (string :tag "Display match count" "%s "))
+  :group 'lazy-count
+  :version "29.1")
+
+(defvar minibuffer-lazy-highlight-transform #'identity
+  "Function to transform minibuffer text into a `isearch-string' for highlighting.")
+
+(defvar minibuffer-lazy-highlight--overlay nil
+  "Overlay for minibuffer prompt updates.")
+
+(defun minibuffer-lazy-highlight--count ()
+  "Display total match count in the minibuffer prompt."
+  (when minibuffer-lazy-highlight--overlay
+    (overlay-put minibuffer-lazy-highlight--overlay
+                 'after-string
+                 (and isearch-lazy-count-total
+                      (not isearch-error)
+                      (format minibuffer-lazy-count-format
+                              isearch-lazy-count-total)))))
+
+(defun minibuffer-lazy-highlight--after-change (_beg _end _len)
+  "Update lazy highlight state in minibuffer selected window."
+  (when isearch-lazy-highlight
+    (let ((inhibit-redisplay t) ;; Avoid cursor flickering
+          (string (minibuffer-contents)))
+      (with-minibuffer-selected-window
+        (setq isearch-string (funcall minibuffer-lazy-highlight-transform string))
+        (isearch-lazy-highlight-new-loop)))))
+
+(defun minibuffer-lazy-highlight--exit ()
+  "Unwind changes from `minibuffer-lazy-highlight-setup'."
+  (remove-hook 'after-change-functions
+               #'minibuffer-lazy-highlight--after-change)
+  (remove-hook 'lazy-count-update-hook #'minibuffer-lazy-highlight--count)
+  (remove-hook 'minibuffer-exit-hook #'minibuffer-lazy-highlight--exit)
+  (setq minibuffer-lazy-highlight--overlay nil)
+  (lazy-highlight-cleanup))
+
+(defun minibuffer-lazy-highlight-setup ()
+  "Set up minibuffer for lazy highlight of matches in the original window.
+
+This function is intended to be added to `minibuffer-setup-hook'.
+Note that several other isearch variables influence the lazy
+highlighting, including `isearch-regexp',
+`isearch-lazy-highlight' and `isearch-lazy-count'."
+  (remove-hook 'minibuffer-setup-hook #'minibuffer-lazy-highlight-setup)
+  (add-hook 'after-change-functions
+            #'minibuffer-lazy-highlight--after-change)
+  (add-hook 'lazy-count-update-hook #'minibuffer-lazy-highlight--count)
+  (add-hook 'minibuffer-exit-hook #'minibuffer-lazy-highlight--exit)
+  (setq minibuffer-lazy-highlight--overlay
+        (and minibuffer-lazy-count-format
+             (make-overlay (point-min) (point-min) (current-buffer) t)))
+  (minibuffer-lazy-highlight--after-change nil nil nil))
+
+\f
 (defun isearch-resume (string regexp word forward message case-fold)
   "Resume an incremental search.
 STRING is the string or regexp searched for.
diff --git a/lisp/replace.el b/lisp/replace.el
index 06be597855..bd5a694110 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -254,11 +254,31 @@ query-replace-read-from
                                    (query-replace-descr
                                     (cdar query-replace-defaults)))))
                   (t (format-prompt prompt nil))))
+           ;; Variables controlling lazy highlighting while reading
+           ;; FROM regexp, which is set up below.
+           (isearch-lazy-highlight query-replace-lazy-highlight)
+           (isearch-regexp regexp-flag)
+           (isearch-regexp-function nil)
+           (isearch-case-fold-search case-fold-search) ;; FIXME: the case-folding rule here is complicated...
+           (minibuffer-lazy-highlight-transform
+            (lambda (string)
+              (let ((from (query-replace--split-string string)))
+                (if (consp from) (car from) from))))
 	   (from
 	    ;; The save-excursion here is in case the user marks and copies
 	    ;; a region in order to specify the minibuffer input.
 	    ;; That should not clobber the region for the query-replace itself.
 	    (save-excursion
+              (when query-replace-lazy-highlight
+                (add-hook 'minibuffer-setup-hook #'minibuffer-lazy-highlight-setup)
+                (when (use-region-p)
+                  (letrec ((region-filter (replace--region-filter
+                                           (funcall region-extract-function 'bounds)))
+                           (cleanup (lambda ()
+                                      (remove-function isearch-filter-predicate region-filter)
+                                      (remove-hook 'minibuffer-exit-hook cleanup))))
+                    (add-function :after-while isearch-filter-predicate region-filter)
+                    (add-hook 'minibuffer-exit-hook cleanup))))
               (minibuffer-with-setup-hook
                   (lambda ()
                     (setq-local text-property-default-nonsticky
@@ -2773,6 +2793,26 @@ replace--push-stack
 	       ,search-str ,next-replace)
          ,stack))
 
+(defun replace--region-filter (bounds)
+  "Return a function that decides if a region is inside BOUNDS.
+BOUNDS is a list of cons cells of the form (START . END).  The
+returned function takes as argument two buffer positions, START
+and END."
+  (let ((region-bounds
+         (mapcar (lambda (position)
+                   (cons (copy-marker (car position))
+                         (copy-marker (cdr position))))
+                 bounds)))
+    (lambda (start end)
+      (delq nil (mapcar
+                 (lambda (bounds)
+                   (and
+                    (>= start (car bounds))
+                    (<= start (cdr bounds))
+                    (>= end   (car bounds))
+                    (<= end   (cdr bounds))))
+                 region-bounds)))))
+
 (defun perform-replace (from-string replacements
 		        query-flag regexp-flag delimited-flag
 			&optional repeat-count map start end backward region-noncontiguous-p)
@@ -2857,22 +2897,8 @@ perform-replace
 
     ;; Unless a single contiguous chunk is selected, operate on multiple chunks.
     (when region-noncontiguous-p
-      (let ((region-bounds
-             (mapcar (lambda (position)
-                       (cons (copy-marker (car position))
-                             (copy-marker (cdr position))))
-                     (funcall region-extract-function 'bounds))))
-        (setq region-filter
-              (lambda (start end)
-                (delq nil (mapcar
-                           (lambda (bounds)
-                             (and
-                              (>= start (car bounds))
-                              (<= start (cdr bounds))
-                              (>= end   (car bounds))
-                              (<= end   (cdr bounds))))
-                           region-bounds))))
-        (add-function :after-while isearch-filter-predicate region-filter)))
+      (setq region-filter (replace--region-filter
+                           (funcall region-extract-function 'bounds))))
 
     ;; If region is active, in Transient Mark mode, operate on region.
     (if backward
-- 
2.35.1


[-- Attachment #3: Type: text/plain, Size: 2743 bytes --]


On Mon, 10 Jan 2022 at 21:09, Juri Linkov <juri@linkov.net> wrote:

>> I attached a new patch (still a sketch) that requires no changes in
>> comint.el and simple.el.  Perhaps you will find this approach more
>> acceptable.
>
> Thanks, no changes in other files is certainly a big plus.
>
>>> It would be great to use your new variable with a function
>>> to show replacement counts in perform-replace.  IIUC,
>>> let-binding isearch-lazy-count-display-function to
>>> isearch-read-with-highlight-count will suppress isearch-message?
>>
>> I tried this and it's relatively simple to do, but there is a problem.
>> Suppose you want to replace all "a" with "z", and your buffer has 20
>> "a"s initially.  Then, as you keep hitting "y" to confirm a replacement
>> the count will be
>>
>>    1/20, 1/19, ..., 1/1
>
> This is an interesting question.  I tried in other editors,
> and e.g. in the editor xed that is installed by default,
> this is exactly what is displayed: 1/20, 1/19, ..., 1/1.

Let's do this at a later point, to keep this patch smaller and more
focused.

Note that there is one further fancy feature from anzu that we could add
eventually, namely the preview of the replacement text during replace.
This was requested in some recent bug.  I think it's not so often that
such a thing would be useful, but it can be very handy occasionally.

>> since the number of "a"s decrease, and the point is always at the first
>> of the still-existing ones.  But probably one should count the number of
>> prompts, so
>>
>>    1/20, 2/20, ..., 20/20
>>
>> I think this means `perform-replace' has to implement its own way to
>> display a count.
>
> Maybe this makes more sense, when the users will learn
> what do these numbers mean.
>
>>> I meant using simply
>>>
>>>   (add-hook 'minibuffer-setup-hook 'isearch-read-with-highlight-setup)
>>>
>>> But it seems isearch-read-with-highlight-setup doesn't set
>>> isearch-lazy-count-display-function.

What used to be 'isearch-read-with-highlight-setup' is now
'minibuffer-lazy-highlight-setup'.

Just to make sure I understood you: your suggestion is for this function
to remove itself automagically from the minibuffer setup hook, to
dispense with the need of 'minibuffer-with-setup-hook'?  This seems
handy but unusual, hence my question.

>> I guess this could be done.
>
> Maybe two separate hooks could be defined?  One highlights like
> lazy-highlight, and another counts like lazy-count does:
>
>   (add-hook 'minibuffer-setup-hook 'isearch-read-with-highlight-setup)
>   (add-hook 'minibuffer-setup-hook 'isearch-read-with-count-setup)

The highlight without counting can be achieved by binding a suitable new
variable.  Counting without highlight is not supported by isearch AFAIU.