bug#12526: 24.2.50; `query-replace-interactive' not always respected

bug#12526: 24.2.50; `query-replace-interactive' not always respected

[Dani Moncayo]

From emacs -Q: M-< C-s t h i s M-%

Expected result: the query-replace command ask me for both the "from"
and the "to" strings.  (note that the variable
`query-replace-interactive' is nil).

Observed result: the command uses the last search string ("this") as
the "from" string and ask me only for the "to" string.


In GNU Emacs 24.2.50.1 (i386-mingw-nt6.1.7601)
 of 2012-09-23 on DANI-PC
Bzr revision: 110171 rgm@gnu.org-20120923184055-lgm2en2gxt2lkqr1
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --with-gcc (4.7) --no-opt --enable-checking --cflags
 -I../../libs/libxpm-3.5.8/include -I../../libs/libxpm-3.5.8/src
 -I../../libs/libpng-1.4.10 -I../../libs/zlib-1.2.6
 -I../../libs/giflib-4.1.4-1/include -I../../libs/jpeg-6b-4/include
 -I../../libs/tiff-3.8.2-1/include
 -I../../libs/libxml2-2.7.8-w32-bin/include/libxml2
 -I../../libs/gnutls-3.0.16/include
 -I../../libs/libiconv-1.14-2-mingw32-dev/include'

Important settings:
  value of $LANG: ESN
  locale-coding-system: cp1252
  default enable-multibyte-characters: t


-- 
Dani Moncayo

bug#12526: 24.2.50; `query-replace-interactive' not always respected

[Stefan Monnier]

>> From emacs -Q: M-< C-s t h i s M-%
> Expected result: the query-replace command ask me for both the "from"
> and the "to" strings.  (note that the variable
> `query-replace-interactive' is nil).
> Observed result: the command uses the last search string ("this") as
> the "from" string and ask me only for the "to" string.

That's a feature independent from query-replace-interactive, indeed.
If you want to be prompted, then terminate your search (e.g. with RET)
before hitting M-%.


        Stefan

bug#12526: 24.2.50; `query-replace-interactive' not always respected

[Dani Moncayo]

On Thu, Sep 27, 2012 at 3:41 PM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>>> From emacs -Q: M-< C-s t h i s M-%
>> Expected result: the query-replace command ask me for both the "from"
>> and the "to" strings.  (note that the variable
>> `query-replace-interactive' is nil).
>> Observed result: the command uses the last search string ("this") as
>> the "from" string and ask me only for the "to" string.
>
> That's a feature independent from query-replace-interactive, indeed.
> If you want to be prompted, then terminate your search (e.g. with RET)
> before hitting M-%.

Ah, thanks.

Is that feature documented somewhere?
Is there a way to disable that feature?

-- 
Dani Moncayo

bug#12526: 24.2.50; `query-replace-interactive' not always respected

[Dani Moncayo]

>> That's a feature independent from query-replace-interactive, indeed.
>> If you want to be prompted, then terminate your search (e.g. with RET)
>> before hitting M-%.
>
> Ah, thanks.
>
> Is that feature documented somewhere?

BTW: I think it should be documented (at least) in the docstring of
query-replace.

> Is there a way to disable that feature?



-- 
Dani Moncayo

bug#12526: 24.2.50; `query-replace-interactive' not always respected

[Dani Moncayo]

>> That's a feature independent from query-replace-interactive, indeed.
>> If you want to be prompted, then terminate your search (e.g. with RET)
>> before hitting M-%.
>
> Ah, thanks.
>
> Is that feature documented somewhere?
> Is there a way to disable that feature?

What about (slightly) altering the current semantics of
`query-replace-interactive' so that the "from" string in query-replace
is taken from:
* if "nil" --> from the minibuffer.
* if "t" --> from the last search string (if any), else from the minibuffer.
* if "any other non-nil value" --> from the last search string, if
`query-replace' was invoked from an I-search session, and else from
the minibuffer.

-- 
Dani Moncayo

bug#12526: 24.2.50; `query-replace-interactive' not always respected

[Juri Linkov]

>> That's a feature independent from query-replace-interactive, indeed.
>> If you want to be prompted, then terminate your search (e.g. with RET)
>> before hitting M-%.
>
> Ah, thanks.
>
> Is that feature documented somewhere?

Yes, it is documented in the Info node
(info "(emacs) Special Isearch")

> Is there a way to disable that feature?

Yes, you can easily disable this feature
by adding this keybinding to your init file:

(define-key isearch-mode-map [?\M-%]
  (lambda ()
    (interactive)
    (isearch-exit)  ;; I'm too lazy to terminate search with RET.
    (call-interactively 'query-replace)))

bug#12526: 24.2.50; `query-replace-interactive' not always respected

[Dani Moncayo]

>> Is that feature documented somewhere?
>
> Yes, it is documented in the Info node
> (info "(emacs) Special Isearch")

Ok, but definitely the docstring of query-replace should be updated,
because currently it is misleading:
  If `query-replace-interactive' is non-nil, the last incremental search
  string is used as FROM-STRING--you don't have to specify it with the
  minibuffer.

This paragraph is incomplete (and therefore misleading) because it
doesn't explain the behavior I complained about.

Also, the docstring of `query-replace-interactive' should be update
for the same reason.

>> Is there a way to disable that feature?
>
> Yes, you can easily disable this feature
> by adding this keybinding to your init file:
>
> (define-key isearch-mode-map [?\M-%]
>   (lambda ()
>     (interactive)
>     (isearch-exit)  ;; I'm too lazy to terminate search with RET.
>     (call-interactively 'query-replace)))


This is an ugly solution, IMO.  I don't want to define a new command
and bind M-% to it;

I'd like to set some user-option in my init file so that query-replace
never assumes nothing about the "from" string.

BTW: I think that a much better approach would be to add the last
search string to the list of default arguments, so that, after an
Isearch, I could do `M-% M-n' if I wanted to retrieve that last search
string.

-- 
Dani Moncayo

bug#12526: 24.2.50; `query-replace-interactive' not always respected

[Juri Linkov]

>>> Is that feature documented somewhere?
>>
>> Yes, it is documented in the Info node
>> (info "(emacs) Special Isearch")
>
> Ok, but definitely the docstring of query-replace should be updated,
> because currently it is misleading:
>   If `query-replace-interactive' is non-nil, the last incremental search
>   string is used as FROM-STRING--you don't have to specify it with the
>   minibuffer.

Thanks for the suggestion.  I fixed this docstring in the patch below.

> Also, the docstring of `query-replace-interactive' should be update
> for the same reason.

Since it's easy to do `C-s C-s M-%' to replace the last search string,
the option `query-replace-interactive' is useless now and can be
declared obsolete.  Another reason to remove it is your suggestion below
to use `M-% M-n' as its replacement.

>> (define-key isearch-mode-map [?\M-%]
>>   (lambda ()
>>     (interactive)
>>     (isearch-exit)  ;; I'm too lazy to terminate search with RET.
>>     (call-interactively 'query-replace)))
>
> This is an ugly solution, IMO.  I don't want to define a new command
> and bind M-% to it;

If you don't want to define a new command,
then you can add just one line to your init file:

(define-key isearch-mode-map [?\M-%] 'isearch-other-meta-char)

> I'd like to set some user-option in my init file so that query-replace
> never assumes nothing about the "from" string.

If you don't want to use `define-key' for configuration purposes,
we might consider using an user option, but not `query-replace-interactive'
that is irrelevant for isearch.

Such an option has been discussed but not yet implemented.
To get an idea how this option could work, please see bug#9706
and its related thread on emacs-devel.  With this new option,
you will be able to define any command (not only `query-replace')
to bypass its isearch keybinding to its original command.

> BTW: I think that a much better approach would be to add the last
> search string to the list of default arguments, so that, after an
> Isearch, I could do `M-% M-n' if I wanted to retrieve that last search
> string.

Two weeks ago I added the last search string to `C-M-% M-n'.
But it's still missing for `M-% M-n'.  Thank you for noticing this omission.
This is fixed in the patch below.  This is another reason
to declare `query-replace-interactive' obsolete because now
there are two functional replacements for it as is documented
in the fixed docstrings.

Also I verified that "Query replace (default foo -> bar): RET"
still works correctly and empty input redoes the last replace.

=== modified file 'lisp/replace.el'
--- lisp/replace.el	2012-09-25 04:13:02 +0000
+++ lisp/replace.el	2012-10-03 23:38:00 +0000
@@ -62,6 +62,10 @@ (defvar query-replace-defaults nil
 (defvar query-replace-interactive nil
   "Non-nil means `query-replace' uses the last search string.
 That becomes the \"string to replace\".")
+(make-obsolete-variable 'query-replace-interactive
+			"use `M-n' in the minibuffer
+that asks for the string to replace, or run `query-replace'
+from Isearch by using a key sequence like `C-s C-s M-%'." "24.3")
 
 (defcustom query-replace-from-history-variable 'query-replace-history
   "History list to use for the FROM argument of `query-replace' commands.
@@ -125,8 +129,6 @@ (defun query-replace-read-from (prompt r
   "Query and return the `from' argument of a query-replace operation.
 The return value can also be a pair (FROM . TO) indicating that the user
 wants to replace FROM with TO."
-  (if query-replace-interactive
-      (car (if regexp-flag regexp-search-ring search-ring))
     (let* ((history-add-new-input nil)
 	   (prompt
 	    (if query-replace-defaults
@@ -142,7 +144,8 @@ (defun query-replace-read-from (prompt r
 	      (if regexp-flag
 		  (read-regexp prompt nil query-replace-from-history-variable)
 		(read-from-minibuffer
-		 prompt nil nil nil query-replace-from-history-variable nil t)))))
+	       prompt nil nil nil query-replace-from-history-variable
+	       (car (if regexp-flag regexp-search-ring search-ring)) t)))))
       (if (and (zerop (length from)) query-replace-defaults)
 	  (cons (car query-replace-defaults)
 		(query-replace-compile-replacement
@@ -158,7 +161,7 @@ (defun query-replace-read-from (prompt r
 		((string= match "\\t")
 		 (message "Note: `\\t' here doesn't match a tab; to do that, just type TAB")))
 	       (sit-for 2)))
-	from))))
+      from)))
 
 (defun query-replace-compile-replacement (to regexp-flag)
   "Maybe convert a regexp replacement TO to Lisp.
@@ -231,9 +234,11 @@ (defun query-replace (from-string to-str
 In Transient Mark mode, if the mark is active, operate on the contents
 of the region.  Otherwise, operate from point to the end of the buffer.
 
-If `query-replace-interactive' is non-nil, the last incremental search
-string is used as FROM-STRING--you don't have to specify it with the
-minibuffer.
+Use \\<minibuffer-local-map>\\[next-history-element] \
+to pull the last incremental search string to the minibuffer
+that reads FROM-STRING, or start `query-replace' from
+incremental search with a key sequence like `C-s C-s M-%'
+to use its current search string as the string to replace.
 
 Matching is independent of case if `case-fold-search' is non-nil and
 FROM-STRING has no uppercase letters.  Replacement transfers the case
@@ -279,9 +284,11 @@ (defun query-replace-regexp (regexp to-s
 In Transient Mark mode, if the mark is active, operate on the contents
 of the region.  Otherwise, operate from point to the end of the buffer.
 
-If `query-replace-interactive' is non-nil, the last incremental search
-regexp is used as REGEXP--you don't have to specify it with the
-minibuffer.
+Use \\<minibuffer-local-map>\\[next-history-element] \
+to pull the last incremental search regexp to the minibuffer
+that reads REGEXP, or start `query-replace-regexp' from
+incremental search with a key sequence like `C-M-s C-M-s C-M-%'
+to use its current search regexp as the regexp to replace.
 
 Matching is independent of case if `case-fold-search' is non-nil and
 REGEXP has no uppercase letters.  Replacement transfers the case
@@ -364,9 +371,9 @@ (defun query-replace-regexp-eval (regexp
 In Transient Mark mode, if the mark is active, operate on the contents
 of the region.  Otherwise, operate from point to the end of the buffer.
 
-If `query-replace-interactive' is non-nil, the last incremental search
-regexp is used as REGEXP--you don't have to specify it with the
-minibuffer.
+Use \\<minibuffer-local-map>\\[next-history-element] \
+to pull the last incremental search regexp to the minibuffer
+that reads REGEXP.
 
 Preserves case in each replacement if `case-replace' and `case-fold-search'
 are non-nil and REGEXP has no uppercase letters.
@@ -417,19 +424,16 @@ (defun map-query-replace-regexp (regexp
 
 Non-interactively, TO-STRINGS may be a list of replacement strings.
 
-If `query-replace-interactive' is non-nil, the last incremental search
-regexp is used as REGEXP--you don't have to specify it with the minibuffer.
+Use \\<minibuffer-local-map>\\[next-history-element] \
+to pull the last incremental search regexp to the minibuffer
+that reads REGEXP.
 
 A prefix argument N says to use each replacement string N times
 before rotating to the next.
 Fourth and fifth arg START and END specify the region to operate on."
   (interactive
-   (let* ((from (if query-replace-interactive
-		    (car regexp-search-ring)
-		  (read-from-minibuffer "Map query replace (regexp): "
-					nil nil nil
-					query-replace-from-history-variable
-					nil t)))
+   (let* ((from (read-regexp "Map query replace (regexp): " nil
+			     query-replace-from-history-variable))
 	  (to (read-from-minibuffer
 	       (format "Query replace %s with (space-separated strings): "
 		       (query-replace-descr from))
@@ -475,9 +479,9 @@ (defun replace-string (from-string to-st
 only matches surrounded by word boundaries.
 Fourth and fifth arg START and END specify the region to operate on.
 
-If `query-replace-interactive' is non-nil, the last incremental search
-string is used as FROM-STRING--you don't have to specify it with the
-minibuffer.
+Use \\<minibuffer-local-map>\\[next-history-element] \
+to pull the last incremental search string to the minibuffer
+that reads FROM-STRING.
 
 This function is usually the wrong thing to use in a Lisp program.
 What you probably want is a loop like this:
@@ -540,8 +544,9 @@ (defun replace-regexp (regexp to-string
 text, TO-STRING is actually made a list instead of a string.
 Use \\[repeat-complex-command] after this command for details.
 
-If `query-replace-interactive' is non-nil, the last incremental search
-regexp is used as REGEXP--you don't have to specify it with the minibuffer.
+Use \\<minibuffer-local-map>\\[next-history-element] \
+to pull the last incremental search regexp to the minibuffer
+that reads REGEXP.
 
 This function is usually the wrong thing to use in a Lisp program.
 What you probably want is a loop like this:

bug#12526: 24.2.50; `query-replace-interactive' not always respected

[Dani Moncayo]

>>>> Is that feature documented somewhere?
>>>
>>> Yes, it is documented in the Info node
>>> (info "(emacs) Special Isearch")
>>
>> Ok, but definitely the docstring of query-replace should be updated,
>> because currently it is misleading:
>>   If `query-replace-interactive' is non-nil, the last incremental search
>>   string is used as FROM-STRING--you don't have to specify it with the
>>   minibuffer.
>
> Thanks for the suggestion.  I fixed this docstring in the patch below.

Good, thanks.

>> Also, the docstring of `query-replace-interactive' should be update
>> for the same reason.
>
> Since it's easy to do `C-s C-s M-%' to replace the last search string,
> the option `query-replace-interactive' is useless now and can be
> declared obsolete.  Another reason to remove it is your suggestion below
> to use `M-% M-n' as its replacement.

As I said before, I think it is unsuitable to assume that the user
always wants to use the last search string as "from" string in
query-replace (when query-replace is invoked from Isearch).  It is
much better to let the user decide, i.e., to always prompt for the
"from" string in the minibuffer.  If the user wants to use the last
search string, it is pretty easy to type `M-n'.  At least, please,
make this behavior optional.

>> I'd like to set some user-option in my init file so that query-replace
>> never assumes nothing about the "from" string.
>
> If you don't want to use `define-key' for configuration purposes,
> we might consider using an user option, but not `query-replace-interactive'
> that is irrelevant for isearch.
>
> Such an option has been discussed but not yet implemented.
> To get an idea how this option could work, please see bug#9706
> and its related thread on emacs-devel.  With this new option,
> you will be able to define any command (not only `query-replace')
> to bypass its isearch keybinding to its original command.

I fail to see exactly how that is related to the problem discussed
here, but I think that the proper fix here is simpler (see my previous
comment).

>> BTW: I think that a much better approach would be to add the last
>> search string to the list of default arguments, so that, after an
>> Isearch, I could do `M-% M-n' if I wanted to retrieve that last search
>> string.
>
> Two weeks ago I added the last search string to `C-M-% M-n'.
> But it's still missing for `M-% M-n'.  Thank you for noticing this omission.
> This is fixed in the patch below.  This is another reason
> to declare `query-replace-interactive' obsolete because now
> there are two functional replacements for it as is documented
> in the fixed docstrings.
>
> Also I verified that "Query replace (default foo -> bar): RET"
> still works correctly and empty input redoes the last replace.

Ok, thank you for working on this.

-- 
Dani Moncayo

bug#12526: 24.2.50; `query-replace-interactive' not always respected

[Juri Linkov]

> As I said before, I think it is unsuitable to assume that the user
> always wants to use the last search string as "from" string in
> query-replace (when query-replace is invoked from Isearch).

`query-replace' is not invoked from Isearch.
`isearch-query-replace' is invoked from Isearch.
So your wish is about invoking `query-replace'
instead of `isearch-query-replace' from Isearch.

Your wish is not different from other similar desires e.g.:
"I want `M-e' in Isearch to exit Isearch and call `forward-sentence'
instead of editing the search string" or "I want `C-y' in Isearch
to exit Isearch and call `yank' instead of pulling string from the kill ring
into the search string" and the same can be said for other Isearch
keybindings too.

All of them could be solved at once by implementing a new feature
that will bypass Isearch keybindings to original commands.

bug#12526: 24.2.50; `query-replace-interactive' not always respected

[Dani Moncayo]

On Thu, Oct 4, 2012 at 11:09 AM, Juri Linkov <juri@jurta.org> wrote:
>> As I said before, I think it is unsuitable to assume that the user
>> always wants to use the last search string as "from" string in
>> query-replace (when query-replace is invoked from Isearch).
>
> `query-replace' is not invoked from Isearch.
> `isearch-query-replace' is invoked from Isearch.

Ah, right.  I didn't notice it.  I thought that M-% wasn't rebound in Isearch.

> So your wish is about invoking `query-replace'
> instead of `isearch-query-replace' from Isearch.

I think so.  What is the difference between these two commands, apart
from the one involving the "from" string?

> Your wish is not different from other similar desires e.g.:
> "I want `M-e' in Isearch to exit Isearch and call `forward-sentence'
> instead of editing the search string" or "I want `C-y' in Isearch
> to exit Isearch and call `yank' instead of pulling string from the kill ring
> into the search string" and the same can be said for other Isearch
> keybindings too.
>
> All of them could be solved at once by implementing a new feature
> that will bypass Isearch keybindings to original commands.

Ok, thanks for clarifying this.

-- 
Dani Moncayo

bug#12526: 24.2.50; `query-replace-interactive' not always respected

[Drew Adams]

FWIW, in my local version, `query-replace-read-(from|to)' provides defaults (in
order):

a. The secondary selection, if option `search/replace-2nd-sel-as-default-flag'
is non-nil.

b. Using the (function) value of option `search/replace-default-fn'.  This is a
thing-at-point function such as `non-nil-symbol-name-nearest-point' or
`word-at-point'.  If it returns a list of values then they are all available as
defaults.

c. Using `completing-read', if option `replace-w-completion-flag' is non-nil,
with default `query-replace-from-history-variable'.

d. The usual way, from the history list.

That gives the user a fair number of choices.

bug#12526: 24.2.50; `query-replace-interactive' not always respected

[Juri Linkov]

>> So your wish is about invoking `query-replace'
>> instead of `isearch-query-replace' from Isearch.
>
> I think so.  What is the difference between these two commands, apart
> from the one involving the "from" string?

Another difference is that `isearch-query-replace' invokes replacements
using the current Isearch state (case-sensitivity used in the current search,
word delimiters, lax whitespace, etc.), so replacements will replace exactly
the same matches as visited by Isearch.

bug#12526: 24.2.50; `query-replace-interactive' not always respected

[Juri Linkov]

> b. Using the (function) value of option `search/replace-default-fn'.  This is a
> thing-at-point function such as `non-nil-symbol-name-nearest-point' or
> `word-at-point'.  If it returns a list of values then they are all available as
> defaults.

I intended to do something like this after the feature freeze is over.
But let's do this in a separate feature request.