GNU bug report logs - #20687
25.0.50; `perform-replace' should invoke a key that you have bound in `query-replace-map'

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 20687 in the body.
You can then email your comments to 20687 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 25.0.50; `perform-replace' should invoke a key that you have bound in
 `query-replace-map'
Date: Thu, 28 May 2015 14:12:16 -0700 (PDT)

You can bind any key you like in `query-replace-map'.  But
`perform-replace', in effect, hard-codes the keys that it recognizes.

This is not necessary.  You shbould be able to bind a new key in that
map to some command, and have `perform-replace' invoke that command.

All that's required for this is to add another `cond' clause, just
before the final `t' (otherwise) clause, to do this:

(cond
  ...
  (def (call-interactively def)) ; User-defined key - invoke it.
  (t
   ;; Note: we do not need to treat `exit-prefix'
   ;; specially here, since we reread
   ;; any unrecognized character.
   ...))

It seems silly for the code to be written like it is.  We already look
up the key you press in the q-r keymap.  If we find a DEF that is not
one of those predefined by Emacs then we ignore it?  That makes no sense
(to me).



In GNU Emacs 25.0.50.1 (i686-pc-mingw32)
 of 2014-10-20 on LEG570
Bzr revision: 118168 rgm <at> gnu.org-20141020195941-icp42t8ttcnud09g
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --enable-checking=yes,glyphs CPPFLAGS=-DGLYPH_DEBUG=1'

Message #8 received at 20687 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: 20687 <at> debbugs.gnu.org
Subject: Re: bug#20687: 25.0.50;
 `perform-replace' should invoke a key that you have bound in
 `query-replace-map'
Date: Mon, 01 Jun 2015 23:53:05 +0300

> You can bind any key you like in `query-replace-map'.  But
> `perform-replace', in effect, hard-codes the keys that it recognizes.
>
> This is not necessary.  You shbould be able to bind a new key in that
> map to some command, and have `perform-replace' invoke that command.
>
> All that's required for this is to add another `cond' clause, just
> before the final `t' (otherwise) clause, to do this:
>
> (cond
>   ...
>   (def (call-interactively def)) ; User-defined key - invoke it.
>   (t
>    ;; Note: we do not need to treat `exit-prefix'
>    ;; specially here, since we reread
>    ;; any unrecognized character.
>    ...))
>
> It seems silly for the code to be written like it is.  We already look
> up the key you press in the q-r keymap.  If we find a DEF that is not
> one of those predefined by Emacs then we ignore it?  That makes no sense
> (to me).

Could you please send an example of your custom keybindings in
`query-replace-map' that currently don't work.

Message #11 received at 20687 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 20687 <at> debbugs.gnu.org
Subject: RE: bug#20687: 25.0.50; `perform-replace' should invoke a key that
 you have bound in `query-replace-map'
Date: Mon, 1 Jun 2015 14:11:42 -0700 (PDT)

> Could you please send an example of your custom keybindings in
> `query-replace-map' that currently don't work.

I don't have any custom keybindings in `query-replace-map' that
don't work (in fact, I don't have any custom bindings in that
map at all).

This bug report came from this emacs.StackExchange answer - see
the discussion in the comments.
http://emacs.stackexchange.com/a/12781/105.

The aim here was to add `C' to `query-replace-map', to have it
toggle `case-fold-search'.  But it doesn't matter what key a
user might want to bind to what command during q-r.

The point is that a user can do that (that's what keymaps and
key bindings are for), but currently `perform-replace' refuses to
recognize such a key and its command.

There is no good reason for this, AFAICT.  It should be OK for
a user to do this.  Of course, that doesn't update the doc
string to reflect the new key and its action, but that's all.
At the user level, this should be something that users can do
easily, without needing to perform surgery.

Message #14 received at 20687 <at> debbugs.gnu.org (full text, mbox):

From: Kaushal <kaushal.modi <at> gmail.com>
To: 20687 <at> debbugs.gnu.org
Cc: Drew Adams <drew.adams <at> oracle.com>, juri <at> linkov.net
Subject: 25.0.50; `perform-replace' should invoke a key that you have bound in
 `query-replace-map'
Date: Tue, 2 Jun 2015 09:08:51 -0400

[Message part 1 (text/plain, inline)]

Hi guys,

I tried the fix as Drew suggested and it works great.

Patch:

--- replace.el 2015-06-02 09:04:57.944380000 -0400
+++ replace-editted.el 2015-06-02 09:08:22.038682000 -0400
@@ -2099,12 +2099,11 @@
          ;; Data for the next match.  If a cons, it has the same format as
          ;; (match-data); otherwise it is t if a match is possible at
point.
          (match-again t)
-
          (message
           (if query-flag
               (apply 'propertize
                      (substitute-command-keys
-                      "Query replacing %s with %s:
(\\<query-replace-map>\\[help] for help) ")
+                      "%sQuery replacing %s with %s:
(\\<query-replace-map>\\[help] for help) ")
                      minibuffer-prompt-properties))))

     ;; If region is active, in Transient Mark mode, operate on region.
@@ -2275,6 +2274,8 @@
      nocasify literal))
    next-replacement)))
     (message message
+                             ;; Show whether `case-fold-search' is `t' or
`nil'
+                             (if case-fold-search "[case] " "[CaSe] ")
                              (query-replace-descr from-string)
                              (query-replace-descr
replacement-presentation)))
   (setq key (read-event))
@@ -2404,6 +2405,7 @@
  (replace-dehighlight)
  (save-excursion (recursive-edit))
  (setq replaced t))
+                        (def (call-interactively def)) ; User-defined key
- invoke it.
  ;; Note: we do not need to treat `exit-prefix'
  ;; specially here, since we reread
  ;; any unrecognized character.


Here is then how I add a new binding to the query-replace-map:

;; http://emacs.stackexchange.com/a/12781/115
(defun drew/toggle-case ()
  "Toggle the value of `case-fold-search' between `nil' and non-nil."
  (interactive)
  ;; `case-fold-search' automatically becomes buffer-local when set
  (setq case-fold-search (not case-fold-search)))
(define-key query-replace-map (kbd "C") #'drew/toggle-case)

(
https://github.com/kaushalmodi/.emacs.d/blob/e690fddc7176368b3d25b0d34ec02510ee92503a/setup-files/setup-search.el#L22-L28
)


--
Kaushal Modi

[Message part 2 (text/html, inline)]

Message #17 received at 20687 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: 20687 <at> debbugs.gnu.org
Subject: Re: bug#20687: 25.0.50;
 `perform-replace' should invoke a key that you have bound in
 `query-replace-map'
Date: Wed, 03 Jun 2015 01:01:39 +0300

> The point is that a user can do that (that's what keymaps and
> key bindings are for), but currently `perform-replace' refuses to
> recognize such a key and its command.

I see there are already commands (as opposed to special internal
values such as `act' and `skip') in query-replace-map:

    (define-key map "\C-v" 'scroll-up)
    (define-key map "\M-v" 'scroll-down)
    (define-key map [next] 'scroll-up)
    (define-key map [prior] 'scroll-down)
    (define-key map [?\C-\M-v] 'scroll-other-window)
    (define-key map [M-next] 'scroll-other-window)
    (define-key map [?\C-\M-\S-v] 'scroll-other-window-down)
    (define-key map [M-prior] 'scroll-other-window-down)

These bindings look like real commands intended to be called
interactively, so after enabling this feature in query-replace
they will start doing their job which is good.

The only suggestion I have is to check whether the binding
is a command before trying to call it, i.e.:

diff --git a/lisp/replace.el b/lisp/replace.el
index 1bf1343..503531a 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -2404,6 +2404,8 @@ (defun perform-replace (from-string replacements
 			 (replace-dehighlight)
 			 (save-excursion (recursive-edit))
 			 (setq replaced t))
+                        ((commandp def t)
+                         (call-interactively def))
 			;; Note: we do not need to treat `exit-prefix'
 			;; specially here, since we reread
 			;; any unrecognized character.

Message #20 received at 20687 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Kaushal <kaushal.modi <at> gmail.com>
Cc: 20687 <at> debbugs.gnu.org, Drew Adams <drew.adams <at> oracle.com>
Subject: Re: bug#20687: 25.0.50;
 `perform-replace' should invoke a key that you have bound in
 `query-replace-map'
Date: Wed, 03 Jun 2015 01:02:51 +0300

> I tried the fix as Drew suggested and it works great.
>
> Patch:
>
> --- replace.el 2015-06-02 09:04:57.944380000 -0400
> +++ replace-editted.el 2015-06-02 09:08:22.038682000 -0400
> @@ -2099,12 +2099,11 @@
>           ;; Data for the next match.  If a cons, it has the same format as
>           ;; (match-data); otherwise it is t if a match is possible at
> point.
>           (match-again t)
> -
>           (message
>            (if query-flag
>                (apply 'propertize
>                       (substitute-command-keys
> -                      "Query replacing %s with %s: (\\<query-replace-map>\\[help] for help) ")
> +                      "%sQuery replacing %s with %s: (\\<query-replace-map>\\[help] for help) ")
>                       minibuffer-prompt-properties))))
>
>      ;; If region is active, in Transient Mark mode, operate on region.
> @@ -2275,6 +2274,8 @@
>       nocasify literal))
>     next-replacement)))
>      (message message
> +                             ;; Show whether `case-fold-search' is `t' or  `nil'
> +                             (if case-fold-search "[case] " "[CaSe] ")
>                               (query-replace-descr from-string)
>                               (query-replace-descr

Maybe we should use the same message about case-folding like in isearch?

Message #23 received at 20687 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 20687 <at> debbugs.gnu.org
Subject: RE: bug#20687: 25.0.50; `perform-replace' should invoke a key that
 you have bound in `query-replace-map'
Date: Tue, 2 Jun 2015 15:12:09 -0700 (PDT)

> These bindings look like real commands intended to be called
> interactively, so after enabling this feature in query-replace
> they will start doing their job which is good.
> 
> The only suggestion I have is to check whether the binding
> is a command before trying to call it, i.e.:

Good idea.  Thx.

Message #26 received at 20687 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Juri Linkov <juri <at> linkov.net>, Kaushal <kaushal.modi <at> gmail.com>
Cc: 20687 <at> debbugs.gnu.org
Subject: RE: bug#20687: 25.0.50; `perform-replace' should invoke a key that
 you have bound in `query-replace-map'
Date: Tue, 2 Jun 2015 15:50:51 -0700 (PDT)

> > +                  ;; Show whether `case-fold-search' is `t' or `nil'
> > +                  (if case-fold-search "[case] " "[CaSe] ")
> 
> Maybe we should use the same message about case-folding like in
> isearch?

The msg should somehow indicate that what is involved here is (only)
case-sensitivity wrt FROM (i.e., wrt search, not replacement).  Not
sure what the best way to do that would be.

IOW, there is more than one use of case sensitivity here, unlike
the case for search.  There is what `case-fold-search' controls (the
search), and there is what `case-replace' controls (the replacement).
And then there is what happens for the replacement according to the
case of FROM.

---

BTW, we might consider binding a key to toggle case sensitivity for
search as part of this bug fix (i.e., not just fixing `perform-replace'
so it respects keys that user might bind).  In that case, maybe the
same key we use in Isearch (`M-c') would be a good choice.

---

BTW2, I think that Emacs manual node `Replacement and Case' is confusing.
The first three paragraphs (2/3 of the node), for instance:

 If the first argument of a replace command is all lower case, the
 command ignores case while searching for occurrences to
 replace--provided `case-fold-search' is non-`nil'.  If
 `case-fold-search' is set to `nil', case is always significant in all
 searches.

  An upper-case letter anywhere in the incremental search string makes
  the search case-sensitive.  Thus, searching for `Foo' does not find
  `foo' or `FOO'.  This applies to regular expression search as well as
  to string search.  The effect ceases if you delete the upper-case
  letter from the search string.

  If you set the variable `case-fold-search' to `nil', then all
  letters must match exactly, including case.  This is a per-buffer
  variable; altering the variable normally affects only the current
  buffer, unless you change its default value.  *Note Locals::.  This
  variable applies to nonincremental searches also, including those
  performed by the replace commands (*note Replace::) and the minibuffer
  history matching commands (*note Minibuffer History::).

These paragraphs really say only that the search part of replace commands
acts normally: `case-fold-search' governs.  They should be removed or
changed to say just that.  Leaving them as they are just confuses readers, IMO.

Message #29 received at 20687 <at> debbugs.gnu.org (full text, mbox):

From: Kaushal <kaushal.modi <at> gmail.com>
To: Drew Adams <drew.adams <at> oracle.com>, Juri Linkov <juri <at> linkov.net>
Cc: 20687 <at> debbugs.gnu.org
Subject: Re: bug#20687: 25.0.50; `perform-replace' should invoke a key that
 you have bound in `query-replace-map'
Date: Wed, 03 Jun 2015 03:35:15 +0000

[Message part 1 (text/plain, inline)]

In that case, the patch becomes much more complicated.

We have to modify the query-replace-map in replace.el itself as we cannot
access the internal variables required to printed this message in
isearch-style with (sit-for 1):

(message query-replace-in-progress-message
                                    (query-replace-descr from-string)
                                    (query-replace-descr
replacement-presentation)
                                    query-message-momentary)

Here: from-string, replacement-presentation are internal variables and
cannot be used in a function defined outside that (cond ..) form. So the
earlier approach to define a function externally to toggle the case and to
bind that to query-replace-map from outside does not apply (if we want to
flash the case fold toggle info momentarily as done in isearch). Even the
replacement-presentation was in a (let .. ) form not accessible to the
(cond ..) form and so I moved it to an outer (let ..) form as seen in the
below patch.

I tested this out and the M-c and M-r bindings work great. It now also
gives clear info on what the user should expect after that binding is used.
Please give it a try.

I have still kept this line

 (def (call-interactively def)) ; User-defined key, invoke it.

as it could be useful to bind any other function from outside that does not
need internal variables.

--- replace.el 2015-06-02 23:21:42.631715000 -0400
+++ replace-editted.el 2015-06-02 23:32:47.754001000 -0400
@@ -1834,6 +1834,8 @@
     (define-key map [M-next] 'scroll-other-window)
     (define-key map [?\C-\M-\S-v] 'scroll-other-window-down)
     (define-key map [M-prior] 'scroll-other-window-down)
+    (define-key map "\M-c" 'toggle-query-case)
+    (define-key map "\M-r" 'toggle-replace-preserve-case)
     ;; Binding ESC would prohibit the M-v binding.  Instead, callers
     ;; should check for ESC specially.
     ;; (define-key map "\e" 'exit-prefix)
@@ -2100,12 +2102,14 @@
          ;; (match-data); otherwise it is t if a match is possible at
point.
          (match-again t)

-         (message
+         (query-replace-in-progress-message
           (if query-flag
               (apply 'propertize
                      (substitute-command-keys
-                      "Query replacing %s with %s:
(\\<query-replace-map>\\[help] for help) ")
-                     minibuffer-prompt-properties))))
+                      (concat "Query replacing %s with %s: "
+                              "(\\<query-replace-map>\\[help] for help) %s
"))
+                     minibuffer-prompt-properties)))
+         (query-message-momentary ""))

     ;; If region is active, in Transient Mark mode, operate on region.
     (if backward
@@ -2251,7 +2255,7 @@
                          noedit real-match-data backward)
                         replace-count (1+ replace-count)))
               (undo-boundary)
-              (let (done replaced key def)
+              (let (done replaced key def replacement-presentation)
                 ;; Loop reading commands until one of them sets done,
                 ;; which means it has finished handling this
                 ;; occurrence.  Any command that sets `done' should
@@ -2266,17 +2270,18 @@
                    regexp-flag delimited-flag case-fold-search backward)
                   ;; Bind message-log-max so we don't fill up the message
log
                   ;; with a bunch of identical messages.
-                  (let ((message-log-max nil)
-                        (replacement-presentation
-                         (if query-replace-show-replacement
-                             (save-match-data
-                               (set-match-data real-match-data)
-                               (match-substitute-replacement
next-replacement
-                                                             nocasify
literal))
-                           next-replacement)))
-                    (message message
+                  (let ((message-log-max nil))
+                    (setq replacement-presentation
+                          (if query-replace-show-replacement
+                              (save-match-data
+                                (set-match-data real-match-data)
+                                (match-substitute-replacement
next-replacement
+                                                              nocasify
literal))
+                            next-replacement))
+                    (message query-replace-in-progress-message
                              (query-replace-descr from-string)
-                             (query-replace-descr
replacement-presentation)))
+                             (query-replace-descr replacement-presentation)
+                             query-message-momentary))
                   (setq key (read-event))
                   ;; Necessary in case something happens during read-event
                   ;; that clobbers the match data.
@@ -2404,6 +2409,51 @@
                          (replace-dehighlight)
                          (save-excursion (recursive-edit))
                          (setq replaced t))
+
+                        ((eq def 'toggle-query-case)
+                         (setq case-fold-search (not case-fold-search))
+                         (let ((message-log-max nil)
+                               (query-message-momentary
+                                (concat "["
+                                        (if case-fold-search
+                                            "case insensitive search"
+                                          "Case Sensitive Search")
+                                        "]")))
+                           (message query-replace-in-progress-message
+                                    (query-replace-descr from-string)
+                                    (query-replace-descr
replacement-presentation)
+                                    query-message-momentary)
+                           (sit-for 1)))
+
+                        ((eq def 'toggle-replace-preserve-case)
+                         (let ((message-log-max nil)
+                               (nocasify-value-reason "")
+                               query-message-momentary)
+                           (setq nocasify (not nocasify))
+                           (cond
+                            ((null case-fold-search)
+                             (setq nocasify nil)
+                             (setq nocasify-value-reason ", as
case-fold-search is nil"))
+                            ((null (isearch-no-upper-case-p from-string
regexp-flag))
+                             (setq nocasify nil)
+                             (setq nocasify-value-reason ", as FROM-STRING
has an upper case char."))
+                            ((null (isearch-no-upper-case-p
next-replacement regexp-flag))
+                             (setq nocasify t)
+                             (setq nocasify-value-reason ", as REPLACEMENT
has an upper case char.")))
+                           (setq query-message-momentary
+                                 (concat "[Replaced text case will "
+                                         (if nocasify "NOT " "")
+                                         "be preserved"
+                                         nocasify-value-reason
+                                         "]"))
+                           (message query-replace-in-progress-message
+                                    (query-replace-descr from-string)
+                                    (query-replace-descr
replacement-presentation)
+                                    query-message-momentary)
+                           (sit-for 1.5)))
+
+                        (def (call-interactively def)) ; User-defined key,
invoke it.
+
                         ;; Note: we do not need to treat `exit-prefix'
                         ;; specially here, since we reread
                         ;; any unrecognized character.


On Tue, Jun 2, 2015 at 6:51 PM Drew Adams <drew.adams <at> oracle.com> wrote:

> > > +                  ;; Show whether `case-fold-search' is `t' or `nil'
> > > +                  (if case-fold-search "[case] " "[CaSe] ")
> >
> > Maybe we should use the same message about case-folding like in
> > isearch?
>
> The msg should somehow indicate that what is involved here is (only)
> case-sensitivity wrt FROM (i.e., wrt search, not replacement).  Not
> sure what the best way to do that would be.
>
> IOW, there is more than one use of case sensitivity here, unlike
> the case for search.  There is what `case-fold-search' controls (the
> search), and there is what `case-replace' controls (the replacement).
> And then there is what happens for the replacement according to the
> case of FROM.
>
> ---
>
> BTW, we might consider binding a key to toggle case sensitivity for
> search as part of this bug fix (i.e., not just fixing `perform-replace'
> so it respects keys that user might bind).  In that case, maybe the
> same key we use in Isearch (`M-c') would be a good choice.
>
> ---
>
> BTW2, I think that Emacs manual node `Replacement and Case' is confusing.
> The first three paragraphs (2/3 of the node), for instance:
>
>  If the first argument of a replace command is all lower case, the
>  command ignores case while searching for occurrences to
>  replace--provided `case-fold-search' is non-`nil'.  If
>  `case-fold-search' is set to `nil', case is always significant in all
>  searches.
>
>   An upper-case letter anywhere in the incremental search string makes
>   the search case-sensitive.  Thus, searching for `Foo' does not find
>   `foo' or `FOO'.  This applies to regular expression search as well as
>   to string search.  The effect ceases if you delete the upper-case
>   letter from the search string.
>
>   If you set the variable `case-fold-search' to `nil', then all
>   letters must match exactly, including case.  This is a per-buffer
>   variable; altering the variable normally affects only the current
>   buffer, unless you change its default value.  *Note Locals::.  This
>   variable applies to nonincremental searches also, including those
>   performed by the replace commands (*note Replace::) and the minibuffer
>   history matching commands (*note Minibuffer History::).
>
> These paragraphs really say only that the search part of replace commands
> acts normally: `case-fold-search' governs.  They should be removed or
> changed to say just that.  Leaving them as they are just confuses readers,
> IMO.
>

[Message part 2 (text/html, inline)]

Message #32 received at 20687 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Kaushal <kaushal.modi <at> gmail.com>, Juri Linkov <juri <at> linkov.net>
Cc: 20687 <at> debbugs.gnu.org
Subject: RE: bug#20687: 25.0.50; `perform-replace' should invoke a key that
 you have bound in `query-replace-map'
Date: Tue, 2 Jun 2015 21:39:32 -0700 (PDT)

> I tested this out and the M-c and M-r bindings work great. It now
> also gives clear info on what the user should expect after that
> binding is used. Please give it a try. I have still kept this line
> 
>  (def (call-interactively def)) ; User-defined key, invoke it.
> 
> as it could be useful to bind any other function from outside
> that does not need internal variables.

1. I'm OK with whatever you guys come up with.  Thanks for working
   on this.

2. I tried it only a little.  When I tried `M-r':

   * If the replacement string had uppercase chars then I always
     got the same message, which was very long - too long to read
     in the short time it was displayed.  Could we shorten that
     message, please?  And could we maybe have it logged to
     *Messages*, so that if someone doesn't have time to read it
     s?he can look it up?

   * If the replacement string had no uppercase chars then I always
     got the same message (about case-fold-search being nil).

   What is `M-r' really supposed to do?  I don't see how it is a
   toggle, if repeating it always gives the same message, given
   the same replacement string.  Can you describe what the toggling
   or cycling among states is supposed to do/mean?

3. Wrt this: 

      I have still kept this line
      (def (call-interactively def)) ; User-defined key, invoke it.
      as it could be useful to bind any other function from outside
      that does not need internal variables.

   I think Juri is right, that it should be the following, because
   `lookup-key' can return a number if the key is too long:

   ((commandp def t)          ; User-defined key, invoke it.
    (call-interactively def))

4. If one of you could replace the paragraphs of the doc that I
   mentioned by just a statement that search is controlled by
   `case-fold-search', that would be good. You could then add
   that you can toggle this using `M-c' etc. IOW, (1) those
   paragraphs are useless, and (2) now we have something more
   to say about case sensitivity.

Message #35 received at 20687 <at> debbugs.gnu.org (full text, mbox):

From: Kaushal <kaushal.modi <at> gmail.com>
To: Drew Adams <drew.adams <at> oracle.com>, Juri Linkov <juri <at> linkov.net>
Cc: 20687 <at> debbugs.gnu.org
Subject: Re: bug#20687: 25.0.50; `perform-replace' should invoke a key that
 you have bound in `query-replace-map'
Date: Wed, 03 Jun 2015 05:10:32 +0000

[Message part 1 (text/plain, inline)]

> If the replacement string had uppercase chars then I always
     got the same message, which was very long - too long to read
     in the short time it was displayed.  Could we shorten that
     message, please?
Yes, I am looking for more ideas to get a better, shorter message.

>  And could we maybe have it logged to
     *Messages*, so that if someone doesn't have time to read it
     s?he can look it up?
Only for the messages where toggling is not possible, the message can be
logged to *Messages*. Sounds good?

> If the replacement string had no uppercase chars then I always
     got the same message (about case-fold-search being nil).
The toggling is not unconditional. Toggling case-replace/nocasify is very
picky!
So I had to put that (cond ..) statement there to handle the picky
scenarios where toggling cannot happen even if the user wanted to.

For the above case, nocasify will stay t regardless of the value of
case-replace IF the user has set case-fold-search to nil.
So the user will first need to do M-c (toggle case-fold-search to t) and
then do M-r. That too will not work IF the user has used upper case letter
in the search/regexp string or the replacement string.

This is the ideal case for M-r to always toggle nocasify
1. case-fold-search is t
2. all lower case in search/regexp string
3. all lower case in replacement string

>   What is `M-r' really supposed to do?  I don't see how it is a
   toggle, if repeating it always gives the same message, given
   the same replacement string.  Can you describe what the toggling
   or cycling among states is supposed to do/mean?

As described above, we cannot unconditionally toggle nocasify.. it depends
on a bunch of conditions to be right.

>   I think Juri is right, that it should be the following, because
   `lookup-key' can return a number if the key is too long:

   ((commandp def t)          ; User-defined key, invoke it.
    (call-interactively def))

I agree. Will make the change.

> If one of you could replace the paragraphs of the doc that I
   mentioned by just a statement that search is controlled by
   `case-fold-search', that would be good. You could then add
   that you can toggle this using `M-c' etc. IOW, (1) those
   paragraphs are useless, and (2) now we have something more
   to say about case sensitivity.

Case fold toggling is also a bit picky but the results are obvious, and M-c
can force toggle case-fold-search.

But default, search-upper-case is t. So if the user has a string with an
upper case in the search field of query-replace, case-fold-search will be
set to nil automatically (even if it is `t` by default). Then M-r will not
work in the beginning. User can, though, use M-c to toggle case-fold-search
first and then M-r if they wish.

I found the current documentation useful while working on this patch and
testing it out. But I will give it a one more read to try to improve it.

On Wed, Jun 3, 2015 at 12:39 AM Drew Adams <drew.adams <at> oracle.com> wrote:

> > I tested this out and the M-c and M-r bindings work great. It now
> > also gives clear info on what the user should expect after that
> > binding is used. Please give it a try. I have still kept this line
> >
> >  (def (call-interactively def)) ; User-defined key, invoke it.
> >
> > as it could be useful to bind any other function from outside
> > that does not need internal variables.
>
> 1. I'm OK with whatever you guys come up with.  Thanks for working
>    on this.
>
> 2. I tried it only a little.  When I tried `M-r':
>
>    * If the replacement string had uppercase chars then I always
>      got the same message, which was very long - too long to read
>      in the short time it was displayed.  Could we shorten that
>      message, please?  And could we maybe have it logged to
>      *Messages*, so that if someone doesn't have time to read it
>      s?he can look it up?
>
>    * If the replacement string had no uppercase chars then I always
>      got the same message (about case-fold-search being nil).
>
>    What is `M-r' really supposed to do?  I don't see how it is a
>    toggle, if repeating it always gives the same message, given
>    the same replacement string.  Can you describe what the toggling
>    or cycling among states is supposed to do/mean?
>
> 3. Wrt this:
>
>       I have still kept this line
>       (def (call-interactively def)) ; User-defined key, invoke it.
>       as it could be useful to bind any other function from outside
>       that does not need internal variables.
>
>    I think Juri is right, that it should be the following, because
>    `lookup-key' can return a number if the key is too long:
>
>    ((commandp def t)          ; User-defined key, invoke it.
>     (call-interactively def))
>
> 4. If one of you could replace the paragraphs of the doc that I
>    mentioned by just a statement that search is controlled by
>    `case-fold-search', that would be good. You could then add
>    that you can toggle this using `M-c' etc. IOW, (1) those
>    paragraphs are useless, and (2) now we have something more
>    to say about case sensitivity.
>

[Message part 2 (text/html, inline)]

Message #38 received at 20687 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Juri Linkov <juri <at> linkov.net>
Cc: 20687 <at> debbugs.gnu.org, Drew Adams <drew.adams <at> oracle.com>,
 Kaushal <kaushal.modi <at> gmail.com>
Subject: Re: bug#20687: 25.0.50; `perform-replace' should invoke a key that
 you have bound in `query-replace-map'
Date: Thu, 17 Sep 2020 20:11:22 +0200

Juri Linkov <juri <at> linkov.net> writes:

> I see there are already commands (as opposed to special internal
> values such as `act' and `skip') in query-replace-map:
>
>     (define-key map "\C-v" 'scroll-up)
>     (define-key map "\M-v" 'scroll-down)
>     (define-key map [next] 'scroll-up)
>     (define-key map [prior] 'scroll-down)
>     (define-key map [?\C-\M-v] 'scroll-other-window)
>     (define-key map [M-next] 'scroll-other-window)
>     (define-key map [?\C-\M-\S-v] 'scroll-other-window-down)
>     (define-key map [M-prior] 'scroll-other-window-down)
>
> These bindings look like real commands intended to be called
> interactively, so after enabling this feature in query-replace
> they will start doing their job which is good.

[...]

> +                        ((commandp def t)
> +                         (call-interactively def))

I'm not sure I quite understand Kaushal's proposed patch here, and how
it relates to the problem Drew describes, but Juri's patch here seems
like the right thing, at least?  As far as I can tell, it was never
applied.

So I've applied it to Emacs 28, and I'm closing this bug report.  If
there are other related things to be done in this area, it might be
better to open a separate bug report and restate what the requested
feature is.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.