bug#72147: 30.0.60; 30.0.60; Misleading regexp highlighting in 'Q' dired command

bug#72147: 30.0.60; 30.0.60; Misleading regexp highlighting in 'Q' dired command

[Visuwesh]

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

When using the Q command in dired to query-replace-regexp across marked
files in dired, the regexp typed in the first prompt highlights text in
the _dired_ buffer.

To reproduce,

        1. emacs -Q
        2. C-x d ~
        3. Q -

Notice how the dashes in the permission fields are highlighted.  This is
misleading and even confusing.  It would be better to turn off regexp
highlighting for this command.  The attached patch does the job on my
end.  Let binding the variable this way does not turn off
lazy-highlighting during the replacement.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Don-t-highlight-regexp-matches-when-using-dired-Q-co.patch --]
[-- Type: text/x-diff, Size: 1239 bytes --]

From 35f06221023133708f228167f88eb5350ba84dde Mon Sep 17 00:00:00 2001
From: Visuwesh <visuweshm@gmail.com>
Date: Wed, 17 Jul 2024 09:11:04 +0530
Subject: [PATCH] Don't highlight regexp matches when using dired Q command

* lisp/dired-aux.el (dired-do-query-replace-regexp): Turn off
lazy highlighting when reading the regexp from the user to avoid
highlighting regexp matches in the dired buffer.
---
 lisp/dired-aux.el | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index e06e36aaa75..8029074d5a4 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -3801,9 +3801,10 @@ dired-do-query-replace-regexp
 If you exit the query-replace loop (\\[keyboard-quit], RET or q), you can
 resume the query replace with the command \\[fileloop-continue]."
   (interactive
-   (let ((common
-	  (query-replace-read-args
-	   "Query replace regexp in marked files" t t)))
+   (let* ((query-replace-lazy-highlight)
+          (common
+	   (query-replace-read-args
+	    "Query replace regexp in marked files" t t)))
      (list (nth 0 common) (nth 1 common) (nth 2 common)))
    dired-mode)
   (dolist (file (dired-get-marked-files nil nil #'dired-nondirectory-p nil t))
-- 
2.45.2


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


In GNU Emacs 30.0.60 (build 5, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.18.0, Xaw scroll bars) of 2024-07-10 built on astatine
Repository revision: 5cf8d60e0dec5606a94749d17a5d678455b7a437
Repository branch: emacs-30
Windowing system distributor 'The X.Org Foundation', version 11.0.12101011
System Description: Debian GNU/Linux trixie/sid

Configured using:
 'configure --with-sound=alsa --with-x-toolkit=lucid --without-xaw3d
 --without-gconf --without-libsystemd --with-cairo'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
LCMS2 LIBOTF LIBSELINUX LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG RSVG
SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS WEBP X11 XDBE XIM
XINPUT2 XPM LUCID ZLIB

Important settings:
  value of $LC_MONETARY: ta_IN.UTF-8
  value of $LC_NUMERIC: ta_IN.UTF-8
  value of $LANG: en_GB.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  minibuffer-regexp-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util text-property-search time-date subr-x mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
cl-loaddefs cl-lib sendmail rfc2047 rfc2045 ietf-drums mm-util
mail-prsvr mail-utils rmc iso-transl tooltip cconv eldoc paren electric
uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel
term/x-win x-win term/common-win x-dnd touch-screen tool-bar dnd fontset
image regexp-opt fringe tabulated-list replace newcomment text-mode
lisp-mode prog-mode register page tab-bar menu-bar rfn-eshadow isearch
easymenu timer select scroll-bar mouse jit-lock font-lock syntax
font-core term/tty-colors frame minibuffer nadvice seq simple cl-generic
indonesian philippine cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button loaddefs
theme-loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads dbusbind inotify lcms2
dynamic-setting system-font-setting font-render-setting cairo x-toolkit
xinput2 x multi-tty move-toolbar make-network-process emacs)

Memory information:
((conses 16 41209 13476) (symbols 48 5821 0) (strings 32 14271 1663)
 (string-bytes 1 317312) (vectors 16 9302)
 (vector-slots 8 110789 8365) (floats 8 23 3) (intervals 56 256 1)
 (buffers 984 10))

bug#72147: 30.0.60; 30.0.60; Misleading regexp highlighting in 'Q' dired command

[Juri Linkov]

> When using the Q command in dired to query-replace-regexp across marked
> files in dired, the regexp typed in the first prompt highlights text in
> the _dired_ buffer.
>
> To reproduce,
>
>         1. emacs -Q
>         2. C-x d ~
>         3. Q -
>
> Notice how the dashes in the permission fields are highlighted.  This is
> misleading and even confusing.  It would be better to turn off regexp
> highlighting for this command.  The attached patch does the job on my
> end.  Let binding the variable this way does not turn off
> lazy-highlighting during the replacement.
>
> @@ -3801,9 +3801,10 @@ dired-do-query-replace-regexp
>    (interactive
> -   (let ((common
> -	  (query-replace-read-args
> -	   "Query replace regexp in marked files" t t)))
> +   (let* ((query-replace-lazy-highlight)
> +          (common
> +	   (query-replace-read-args
> +	    "Query replace regexp in marked files" t t)))

Thanks, I tried your patch, but it has no effect since
by default Q is bound to dired-do-find-regexp-and-replace.
Shouldn't query-replace-lazy-highlight be added to all
functions in dired that use query-replace-read-args?

bug#72147: 30.0.60; 30.0.60; Misleading regexp highlighting in 'Q' dired command

[Visuwesh]

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

[வெள்ளி ஜூலை 19, 2024] Juri Linkov wrote:

>> When using the Q command in dired to query-replace-regexp across marked
>> files in dired, the regexp typed in the first prompt highlights text in
>> the _dired_ buffer.
>>
>> To reproduce,
>>
>>         1. emacs -Q
>>         2. C-x d ~
>>         3. Q -
>>
>> Notice how the dashes in the permission fields are highlighted.  This is
>> misleading and even confusing.  It would be better to turn off regexp
>> highlighting for this command.  The attached patch does the job on my
>> end.  Let binding the variable this way does not turn off
>> lazy-highlighting during the replacement.
>>
>> @@ -3801,9 +3801,10 @@ dired-do-query-replace-regexp
>>    (interactive
>> -   (let ((common
>> -	  (query-replace-read-args
>> -	   "Query replace regexp in marked files" t t)))
>> +   (let* ((query-replace-lazy-highlight)
>> +          (common
>> +	   (query-replace-read-args
>> +	    "Query replace regexp in marked files" t t)))
>
> Thanks, I tried your patch, but it has no effect since
> by default Q is bound to dired-do-find-regexp-and-replace.

Ah, I guess I didn't test properly in emacs -Q.  I ended up mixing my
own config and emacs -Q.  My bad.

> Shouldn't query-replace-lazy-highlight be added to all
> functions in dired that use query-replace-read-args?

Yes, you're right.  I have attached a patch that does it.

BTW, is there a reason why xref--query-replace-1 let-binds
query-replace-lazy-highlight to nil?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Don-t-highlight-matches-in-dired-query-replace-comma.patch --]
[-- Type: text/x-diff, Size: 2295 bytes --]

From b2c7a4deafd505c18a0907fb86b86763f007e85c Mon Sep 17 00:00:00 2001
From: Visuwesh <visuweshm@gmail.com>
Date: Wed, 17 Jul 2024 09:11:04 +0530
Subject: [PATCH] Don't highlight matches in dired query-replace commands

* lisp/dired-aux.el (dired-do-query-replace-regexp)
(dired-do-replace-regexp-as-diff)
(dired-do-find-regexp-and-replace): Turn off lazy highlighting
when reading the regexp from the user to avoid highlighting
regexp matches in the dired buffer. (bug#72147)
---
 lisp/dired-aux.el | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index e06e36aaa75..decd9702eaf 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -3801,9 +3801,10 @@ dired-do-query-replace-regexp
 If you exit the query-replace loop (\\[keyboard-quit], RET or q), you can
 resume the query replace with the command \\[fileloop-continue]."
   (interactive
-   (let ((common
-	  (query-replace-read-args
-	   "Query replace regexp in marked files" t t)))
+   (let* ((query-replace-lazy-highlight)
+          (common
+	   (query-replace-read-args
+	    "Query replace regexp in marked files" t t)))
      (list (nth 0 common) (nth 1 common) (nth 2 common)))
    dired-mode)
   (dolist (file (dired-get-marked-files nil nil #'dired-nondirectory-p nil t))
@@ -3825,9 +3826,10 @@ dired-do-replace-regexp-as-diff
 The replacements are displayed in the buffer *replace-diff* that
 you can later apply as a patch after reviewing the changes."
   (interactive
-   (let ((common
-          (query-replace-read-args
-           "Replace regexp as diff in marked files" t t)))
+   (let* ((query-replace-lazy-highlight)
+          (common
+           (query-replace-read-args
+            "Replace regexp as diff in marked files" t t)))
      (list (nth 0 common) (nth 1 common) (nth 2 common))))
   (dired-post-do-command)
   (multi-file-replace-regexp-as-diff
@@ -3901,7 +3903,8 @@ dired-do-find-regexp-and-replace
 Also see `query-replace' for user options that affect how this
 function works."
   (interactive
-   (let ((common
+   (let* ((query-replace-lazy-highlight)
+          (common
           (query-replace-read-args
            "Query replace regexp in marked files" t t)))
      (list (nth 0 common) (nth 1 common)))
--
2.45.2

bug#72147: 30.0.60; 30.0.60; Misleading regexp highlighting in 'Q' dired command

[Juri Linkov]

close 72147 31.0.50
thanks

>> Shouldn't query-replace-lazy-highlight be added to all
>> functions in dired that use query-replace-read-args?
>
> Yes, you're right.  I have attached a patch that does it.

Thanks for the patch, now pushed to master.

> BTW, is there a reason why xref--query-replace-1 let-binds
> query-replace-lazy-highlight to nil?

I guess for the same reason why you let-bind it to nil in Dired,
i.e. in Xref it's called in an irrelevant buffer too.

bug#72147: 30.0.60; 30.0.60; Misleading regexp highlighting in 'Q' dired command

[Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Juri Linkov <juri@linkov.net> writes:

> Thanks for the patch, now pushed to master.

Wouldn't it be better to add a new optional argument NO-HIGHLIGHT to
`query-replace-read-args', for stylistic reasons?


Michael.

bug#72147: 30.0.60; 30.0.60; Misleading regexp highlighting in 'Q' dired command

[Juri Linkov]

>> Thanks for the patch, now pushed to master.
>
> Wouldn't it be better to add a new optional argument NO-HIGHLIGHT to
> `query-replace-read-args', for stylistic reasons?

It's not clear how such arg should be combined with
`query-replace-lazy-highlight' in `query-replace-read-args':

(defun query-replace-read-args (prompt regexp-flag &optional noerror no-highlight)
  ...
  (from (minibuffer-with-setup-hook
            (minibuffer-lazy-highlight-setup
             :highlight query-replace-lazy-highlight

with `and'?  With `or'?  With `xor'?
`(and query-replace-lazy-highlight (not no-highlight))'?

bug#72147: 30.0.60; 30.0.60; Misleading regexp highlighting in 'Q' dired command

[Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Juri Linkov <juri@linkov.net> writes:

> >> Thanks for the patch, now pushed to master.
> >
> > Wouldn't it be better to add a new optional argument NO-HIGHLIGHT to
> > `query-replace-read-args', for stylistic reasons?
>
> It's not clear how such arg should be combined with
> `query-replace-lazy-highlight' in `query-replace-read-args':
>
> (defun query-replace-read-args (prompt regexp-flag &optional noerror no-highlight)
>   ...
>   (from (minibuffer-with-setup-hook
>             (minibuffer-lazy-highlight-setup
>              :highlight query-replace-lazy-highlight
>
> with `and'?  With `or'?  With `xor'?
> `(and query-replace-lazy-highlight (not no-highlight))'?

The installed patch binds query-replace-lazy-highlight to nil and
shadows the old binding.  This would correspond to

  (and query-replace-lazy-highlight (not no-highlight))

which looks reasonable in this case: we just an argument to inhibit
highlighting.  Because we never want highlighting in some cases (like
here).


Michael.

bug#72147: 30.0.60; 30.0.60; Misleading regexp highlighting in 'Q' dired command

[Juri Linkov]

>> >> Thanks for the patch, now pushed to master.
>> >
>> > Wouldn't it be better to add a new optional argument NO-HIGHLIGHT to
>> > `query-replace-read-args', for stylistic reasons?
>>
>> It's not clear how such arg should be combined with
>> `query-replace-lazy-highlight' in `query-replace-read-args':
>>
>> (defun query-replace-read-args (prompt regexp-flag &optional noerror no-highlight)
>>   ...
>>   (from (minibuffer-with-setup-hook
>>             (minibuffer-lazy-highlight-setup
>>              :highlight query-replace-lazy-highlight
>>
>> with `and'?  With `or'?  With `xor'?
>> `(and query-replace-lazy-highlight (not no-highlight))'?
>
> The installed patch binds query-replace-lazy-highlight to nil and
> shadows the old binding.  This would correspond to
>
>   (and query-replace-lazy-highlight (not no-highlight))
>
> which looks reasonable in this case: we just an argument to inhibit
> highlighting.  Because we never want highlighting in some cases (like
> here).

Agreed, let's add a new argument.

bug#72147: 30.0.60; 30.0.60; Misleading regexp highlighting in 'Q' dired command

[Visuwesh]

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

[வியாழன் ஜூலை 25, 2024] Juri Linkov wrote:

>>> > Wouldn't it be better to add a new optional argument NO-HIGHLIGHT to
>>> > `query-replace-read-args', for stylistic reasons?
>>>
>>> It's not clear how such arg should be combined with
>>> `query-replace-lazy-highlight' in `query-replace-read-args':
>>>
>>> (defun query-replace-read-args (prompt regexp-flag &optional noerror no-highlight)
>>>   ...
>>>   (from (minibuffer-with-setup-hook
>>>             (minibuffer-lazy-highlight-setup
>>>              :highlight query-replace-lazy-highlight
>>>
>>> with `and'?  With `or'?  With `xor'?
>>> `(and query-replace-lazy-highlight (not no-highlight))'?
>>
>> The installed patch binds query-replace-lazy-highlight to nil and
>> shadows the old binding.  This would correspond to
>>
>>   (and query-replace-lazy-highlight (not no-highlight))
>>
>> which looks reasonable in this case: we just an argument to inhibit
>> highlighting.  Because we never want highlighting in some cases (like
>> here).
>
> Agreed, let's add a new argument.

So how about the attached patch?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-new-optional-argument-NO-HIGHLIGHT-to-query-repl.patch --]
[-- Type: text/x-diff, Size: 3468 bytes --]

From 17a6b4e86887235fcd5ab324ffab1d26eb90612f Mon Sep 17 00:00:00 2001
From: Visuwesh <visuweshm@gmail.com>
Date: Sun, 28 Jul 2024 10:27:33 +0530
Subject: [PATCH] Add new optional argument NO-HIGHLIGHT to
 query-replace-read-args

* lisp/replace.el (query-replace-read-args): Add new optional
argument to disable regexp highlighting.

* lisp/dired-aux.el (dired-do-query-replace-regexp)
(dired-do-replace-regexp-as-diff)
(dired-do-find-regexp-and-replace): Revert last change, and use
the new argument instead.  (bug#72147)
---
 lisp/dired-aux.el | 19 ++++++++-----------
 lisp/replace.el   |  4 ++--
 2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index decd9702eaf..cd948bd7dd9 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -3801,10 +3801,9 @@ dired-do-query-replace-regexp
 If you exit the query-replace loop (\\[keyboard-quit], RET or q), you can
 resume the query replace with the command \\[fileloop-continue]."
   (interactive
-   (let* ((query-replace-lazy-highlight)
-          (common
-	   (query-replace-read-args
-	    "Query replace regexp in marked files" t t)))
+   (let ((common
+	  (query-replace-read-args
+	   "Query replace regexp in marked files" t t t)))
      (list (nth 0 common) (nth 1 common) (nth 2 common)))
    dired-mode)
   (dolist (file (dired-get-marked-files nil nil #'dired-nondirectory-p nil t))
@@ -3826,10 +3825,9 @@ dired-do-replace-regexp-as-diff
 The replacements are displayed in the buffer *replace-diff* that
 you can later apply as a patch after reviewing the changes."
   (interactive
-   (let* ((query-replace-lazy-highlight)
-          (common
-           (query-replace-read-args
-            "Replace regexp as diff in marked files" t t)))
+   (let ((common
+          (query-replace-read-args
+           "Replace regexp as diff in marked files" t t t)))
      (list (nth 0 common) (nth 1 common) (nth 2 common))))
   (dired-post-do-command)
   (multi-file-replace-regexp-as-diff
@@ -3903,10 +3901,9 @@ dired-do-find-regexp-and-replace
 Also see `query-replace' for user options that affect how this
 function works."
   (interactive
-   (let* ((query-replace-lazy-highlight)
-          (common
+   (let ((common
           (query-replace-read-args
-           "Query replace regexp in marked files" t t)))
+           "Query replace regexp in marked files" t t t)))
      (list (nth 0 common) (nth 1 common)))
    dired-mode)
   (require 'xref)
diff --git a/lisp/replace.el b/lisp/replace.el
index 01a892bbba7..2285b19b519 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -352,7 +352,7 @@ query-replace-read-to
        to))
    regexp-flag))
 
-(defun query-replace-read-args (prompt regexp-flag &optional noerror)
+(defun query-replace-read-args (prompt regexp-flag &optional noerror no-highlight)
   (unless noerror
     (barf-if-buffer-read-only))
   (save-mark-and-excursion
@@ -364,7 +364,7 @@ query-replace-read-args
                       :filter (when (use-region-p)
                                 (replace--region-filter
                                  (funcall region-extract-function 'bounds)))
-                      :highlight query-replace-lazy-highlight
+                      :highlight (and query-replace-lazy-highlight (not no-highlight))
                       :regexp regexp-flag
                       :regexp-function (or replace-regexp-function
                                            delimited-flag
-- 
2.45.2

bug#72147: 30.0.60; 30.0.60; Misleading regexp highlighting in 'Q' dired command

[Juri Linkov]

>>> The installed patch binds query-replace-lazy-highlight to nil and
>>> shadows the old binding.  This would correspond to
>>>
>>>   (and query-replace-lazy-highlight (not no-highlight))
>>>
>>> which looks reasonable in this case: we just an argument to inhibit
>>> highlighting.  Because we never want highlighting in some cases (like
>>> here).
>>
>> Agreed, let's add a new argument.
>
> So how about the attached patch?

Thanks, now pushed to master.

PS: It seems adding a new argument is not possible in xref.el
because it's used in older versions via an ELPA package.