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

[Augusto Stoffel]

On Tue, 15 Mar 2022 at 19:24, Juri Linkov <juri@linkov.net> wrote:

>> 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.
>
> Please see comments for your latest patch below:
>
>> @@ -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))
>
> Since this does both highlighting and counting, shouldn't the condition be
>
>         (when (and isearch-lazy-highlight isearch-lazy-count))
>
> Or maybe new separate customizable options are needed, e.g.
> 'minibuffer-lazy-highlight' and 'minibuffer-lazy-count'?

isearch-edit-string already has the behavior your expect: if
isearch-lazy-count is nil but isearch-lazy-highlight is t, then the
matches are highlighted but the count is not displayed.

This happens because the new lazy-count-update-hook is only run when
isearch-lazy-highlight is non-nil.

I see no need for a customization variable, since the user already gets
exactly what they asked for.

>> @@ -2350,7 +2352,9 @@ isearch-query-replace
>>      (isearch-recursive-edit nil)
>>      (isearch-string-propertized
>>           (isearch-string-propertize isearch-string)))
>> -    (isearch-done nil t)
>> +    (let ((lazy-highlight-cleanup (and lazy-highlight-cleanup
>> +                                       (not query-replace-lazy-highlight))))
>> +      (isearch-done nil t))
>
> Is this some optimization?  It seems it's intended to leave
> some existing highlighting?  Is this to avoid double highlighting?
>
> Also maybe this condition could use a new variable as well.

The weird let-binding for lazy-highlight-cleanup is indeed an
optimization.  Without it, you can see the lazy highlights briefly flash
off an on again after you hit RET to confirm the replacement string.

(Same remark explains why condition-case is used instead of a
unwind-protect in query-replace-read-args).

>> @@ -4048,7 +4056,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
>> ...
>> @@ -4067,7 +4075,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))
>> ...
>> @@ -4120,13 +4131,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
> 
> The problem is that when these conditions 'isearch-mode (null 
> isearch-message-function)'
> are removed, now this shows wrong counts in the minibuffer history search
> (e.g. 'M-! C-r s C-r C-r ...') and the shell history search
> (e.g. 'M-x shell RET M-r s C-r C-r ...').  Before this change
> counting was disabled in the history search because it shows wrong numbers.
>

Okay, so this means we should bind isearch-lazy-count to nil in these
commands, do you agree?  It has always looked like a hack to me to check
for (null isearch-message-function).

>> +(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)))))
>> ...
>> +  (setq minibuffer-lazy-highlight--overlay
>> +        (and minibuffer-lazy-count-format
>> +             (make-overlay (point-min) (point-min) (current-buffer) t)))
>
> For some reasons the package lisp/mb-depth.el uses 'after-string'
> instead of 'before-string', and (make-overlay (point-min) (1+ (point-min)))
> instead of (make-overlay (point-min) (point-min)),
> so maybe better to do the same?
>

Okay, but do you know the reasons?  I've changed to before-string, but I
don't like to make the overlay 1 char longer than it has to be :-P

>> @@ -365,14 +372,44 @@ query-replace-read-args
>> +    (condition-case error
>> +        (let (;; Variables controlling lazy highlighting while reading
>> +              ;; FROM and TO.
>> +              (lazy-highlight-cleanup nil)
>> +              (isearch-lazy-highlight query-replace-lazy-highlight)
>> +              (isearch-regexp regexp-flag)
>> +              (isearch-regexp-function nil)
>
> Highlighting is still incorrect for word replacement ('C-u M-%')
> and for non-nil 'replace-char-fold'.  To handle these cases correctly,
> 'replace-highlight' uses:
>
>           (isearch-regexp-function (or replace-regexp-function
>                                        delimited-flag
>                                        (and replace-char-fold
>                                             (not regexp-flag)
>                                             #'char-fold-to-regexp)))

Okay, fixed this.  (BTW, where is replace-regexp-function used?  It's
not set anywhere in Emacs, and it's not a defcustom either.)

>> @@ -2857,22 +2914,8 @@ perform-replace
>>      (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))))
>
> I wonder why (add-function :after-while isearch-filter-predicate 
> region-filter)
> is removed?

Oops, that was a very serious typo.  I've fixed it now.

Here are the changes I applied on top of my previous patch:

0001-Changes-after-Juri-s-comments.patch
Description: Text Data

And this is the aggregated patch for the entire feature:

0001-Display-lazy-highlight-and-match-count-in-when-readi.patch
Description: Text Data