bug#24580: 25.2.50; query-replace history doesn't work

bug#24580: 25.2.50; query-replace history doesn't work

[Marcin Borkowski]

Hi all,

try this:

- start emacs -Q
- press `a' `C-b', `M-%', `a', `RET', `b', `RET', `!'
- press `M-%', `M-p'

Emacs says "Beginning of history; no preceding item".

Also, while I'm on my usual Emacs (with my customizations etc.), the
history seems to work, but apparently doesn't use
`query-replace-from-to-separator'.  I suspect these two bugs might be
related, but if needed, I may try to pinpoint the latter problem, too.

I'm on GNU Emacs 25.2.50.2 (commit 24388c8).

Best,

-- 
Marcin Borkowski

bug#24580: 25.2.50; query-replace history doesn't work

[Eli Zaretskii]

> From: Marcin Borkowski <mbork@mbork.pl>
> Date: Sat, 01 Oct 2016 08:43:27 +0200
> 
> - start emacs -Q
> - press `a' `C-b', `M-%', `a', `RET', `b', `RET', `!'
> - press `M-%', `M-p'
> 
> Emacs says "Beginning of history; no preceding item".

Mark, this regression was caused by the lexical-binding change in
replace.el.  Could you please take a look?

bug#24580: 25.2.50; query-replace history doesn't work

[Mark Oteiza]

On 01/10/16 at 11:40am, Eli Zaretskii wrote:
> > From: Marcin Borkowski <mbork@mbork.pl>
> > Date: Sat, 01 Oct 2016 08:43:27 +0200
> > 
> > - start emacs -Q
> > - press `a' `C-b', `M-%', `a', `RET', `b', `RET', `!'
> > - press `M-%', `M-p'
> > 
> > Emacs says "Beginning of history; no preceding item".
> 
> Mark, this regression was caused by the lexical-binding change in
> replace.el.  Could you please take a look?

The problem lies (at least) in query-replace-read-from, where
query-replace-from-to is let bound, and later read-from-minibuffer
refers to the symbol 'query-replace-from-to.  At first sight, it looks
like it could be fixed by putting some of the setup done by these
bindings into the minibuffer setup hook.

As for the other problem Marcin mentioned, that seems to be due to the
custom-reevaluate-setting call in the same function.

bug#24580: 25.2.50; query-replace history doesn't work

[Eli Zaretskii]

> Date: Sun, 2 Oct 2016 00:17:15 -0400
> From: Mark Oteiza <mvoteiza@udel.edu>
> Cc: Marcin Borkowski <mbork@mbork.pl>, 24580@debbugs.gnu.org
> 
> > > - start emacs -Q
> > > - press `a' `C-b', `M-%', `a', `RET', `b', `RET', `!'
> > > - press `M-%', `M-p'
> > > 
> > > Emacs says "Beginning of history; no preceding item".
> > 
> > Mark, this regression was caused by the lexical-binding change in
> > replace.el.  Could you please take a look?
> 
> The problem lies (at least) in query-replace-read-from, where
> query-replace-from-to is let bound, and later read-from-minibuffer
> refers to the symbol 'query-replace-from-to.  At first sight, it looks
> like it could be fixed by putting some of the setup done by these
> bindings into the minibuffer setup hook.

Isn't there a less intrusive change to fix this?  A significant change
like the one you propose is more risky, but if there's no safer one, I
guess we should take it.

> As for the other problem Marcin mentioned, that seems to be due to the
> custom-reevaluate-setting call in the same function.

I'm not sure I understand: what's wrong with
custom-reevaluate-setting in this case?

Thanks.

bug#24580: 25.2.50; query-replace history doesn't work

[Mark Oteiza]

On 02/10/16 at 10:02am, Eli Zaretskii wrote:
> > Date: Sun, 2 Oct 2016 00:17:15 -0400
> > From: Mark Oteiza <mvoteiza@udel.edu>
> > Cc: Marcin Borkowski <mbork@mbork.pl>, 24580@debbugs.gnu.org
> > 
> > > > - start emacs -Q
> > > > - press `a' `C-b', `M-%', `a', `RET', `b', `RET', `!'
> > > > - press `M-%', `M-p'
> > > > 
> > > > Emacs says "Beginning of history; no preceding item".
> > > 
> > > Mark, this regression was caused by the lexical-binding change in
> > > replace.el.  Could you please take a look?
> > 
> > The problem lies (at least) in query-replace-read-from, where
> > query-replace-from-to is let bound, and later read-from-minibuffer
> > refers to the symbol 'query-replace-from-to.  At first sight, it looks
> > like it could be fixed by putting some of the setup done by these
> > bindings into the minibuffer setup hook.
> 
> Isn't there a less intrusive change to fix this?  A significant change
> like the one you propose is more risky, but if there's no safer one, I
> guess we should take it.

Perhaps just adding a (defvar query-replace-from-to-history …) will be
sufficient.  That read-from-minibuffer and friends' HIST argument can
only be a symbol is a bit frustrating.

> > As for the other problem Marcin mentioned, that seems to be due to the
> > custom-reevaluate-setting call in the same function.
> 
> I'm not sure I understand: what's wrong with
> custom-reevaluate-setting in this case?

From -Q:

1. (setq-default query-replace-from-to-separator " => ")
2. Do an M-% (Like Marcin's in original recipe) once to get some default in the prompt
3. M-%

I would expect to see (default a => b) in the prompt, but
custom-reevaluate-setting apparently ignores settings done outside of
customize.  The first step instead needs to be

  (custom-set-variables '(query-replace-from-to-separator " => "))

or something with the default for query-replace-from-to-separator should
be changed.

bug#24580: 25.2.50; query-replace history doesn't work

[Mark Oteiza]

On 02/10/16 at 12:51pm, Mark Oteiza wrote:
> On 02/10/16 at 10:02am, Eli Zaretskii wrote:
> > > Date: Sun, 2 Oct 2016 00:17:15 -0400
> > > From: Mark Oteiza <mvoteiza@udel.edu>
> > > Cc: Marcin Borkowski <mbork@mbork.pl>, 24580@debbugs.gnu.org
> > > 
> > > > > - start emacs -Q
> > > > > - press `a' `C-b', `M-%', `a', `RET', `b', `RET', `!'
> > > > > - press `M-%', `M-p'
> > > > > 
> > > > > Emacs says "Beginning of history; no preceding item".
> > > > 
> > > > Mark, this regression was caused by the lexical-binding change in
> > > > replace.el.  Could you please take a look?
> > > 
> > > The problem lies (at least) in query-replace-read-from, where
> > > query-replace-from-to is let bound, and later read-from-minibuffer
> > > refers to the symbol 'query-replace-from-to.  At first sight, it looks
> > > like it could be fixed by putting some of the setup done by these
> > > bindings into the minibuffer setup hook.
> > 
> > Isn't there a less intrusive change to fix this?  A significant change
> > like the one you propose is more risky, but if there's no safer one, I
> > guess we should take it.
> 
> Perhaps just adding a (defvar query-replace-from-to-history …) will be
> sufficient.  That read-from-minibuffer and friends' HIST argument can
> only be a symbol is a bit frustrating.

Surely there is a better way.

diff --git a/lisp/replace.el b/lisp/replace.el
index 4256751..0a373db 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -90,6 +90,9 @@ query-replace-from-to-separator
   :type '(choice string (sexp :tag "Display specification"))
   :version "25.1")
 
+(defvar query-replace-from-to-history nil
+  "???")
+
 (defcustom query-replace-from-history-variable 'query-replace-history
   "History list to use for the FROM argument of `query-replace' commands.
 The value of this variable should be a symbol; that symbol
@@ -174,20 +177,7 @@ query-replace-read-from
 	      (propertize "\0"
 			  'display query-replace-from-to-separator
 			  'separator t)))
-	   (query-replace-from-to-history
-	    (append
-	     (when separator
-	       (mapcar (lambda (from-to)
-			 (concat (query-replace-descr (car from-to))
-				 separator
-				 (query-replace-descr (cdr from-to))))
-		       query-replace-defaults))
-	     (symbol-value query-replace-from-history-variable)))
 	   (minibuffer-allow-text-properties t) ; separator uses text-properties
-	   (prompt
-	    (if (and query-replace-defaults separator)
-		(format "%s (default %s): " prompt (car query-replace-from-to-history))
-	      (format "%s: " prompt)))
 	   (from
 	    ;; The save-excursion here is in case the user marks and copies
 	    ;; a region in order to specify the minibuffer input.
@@ -195,13 +185,26 @@ query-replace-read-from
 	    (save-excursion
               (minibuffer-with-setup-hook
                   (lambda ()
+                    (setq query-replace-from-to-history
+                          (append
+                           (when separator
+                             (mapcar (lambda (from-to)
+                                       (concat (query-replace-descr (car from-to))
+                                               separator
+                                               (query-replace-descr (cdr from-to))))
+                                     query-replace-defaults))
+                           (symbol-value query-replace-from-history-variable)))
                     (setq-local text-property-default-nonsticky
                                 (cons '(separator . t) text-property-default-nonsticky)))
-                (if regexp-flag
-                    (read-regexp prompt nil 'query-replace-from-to-history)
-                  (read-from-minibuffer
-                   prompt nil nil nil 'query-replace-from-to-history
-                   (car (if regexp-flag regexp-search-ring search-ring)) t)))))
+                (let ((prompt
+                       (if (and query-replace-defaults separator)
+                           (format "%s (default %s): " prompt (car query-replace-from-to-history))
+                         (format "%s: " prompt))))
+                  (if regexp-flag
+                      (read-regexp prompt nil 'query-replace-from-to-history)
+                    (read-from-minibuffer
+                     prompt nil nil nil 'query-replace-from-to-history
+                     (car (if regexp-flag regexp-search-ring search-ring)) t))))))
            (to))
       (if (and (zerop (length from)) query-replace-defaults)
 	  (cons (caar query-replace-defaults)

bug#24580: 25.2.50; query-replace history doesn't work

[Eli Zaretskii]

> Date: Sun, 2 Oct 2016 12:51:28 -0400
> From: Mark Oteiza <mvoteiza@udel.edu>
> Cc: mbork@mbork.pl, 24580@debbugs.gnu.org
> 
> > > As for the other problem Marcin mentioned, that seems to be due to the
> > > custom-reevaluate-setting call in the same function.
> > 
> > I'm not sure I understand: what's wrong with
> > custom-reevaluate-setting in this case?
> 
> >From -Q:
> 
> 1. (setq-default query-replace-from-to-separator " => ")
> 2. Do an M-% (Like Marcin's in original recipe) once to get some default in the prompt
> 3. M-%
> 
> I would expect to see (default a => b) in the prompt, but
> custom-reevaluate-setting apparently ignores settings done outside of
> customize.  The first step instead needs to be
> 
>   (custom-set-variables '(query-replace-from-to-separator " => "))
> 
> or something with the default for query-replace-from-to-separator should
> be changed.

OK, but is this a new problem related to turning on lexical-binding?
If so, can you tell why it worked before?

Thanks.

bug#24580: 25.2.50; query-replace history doesn't work

[Eli Zaretskii]

> Date: Sun, 2 Oct 2016 21:08:43 -0400
> From: Mark Oteiza <mvoteiza@udel.edu>
> Cc: mbork@mbork.pl, 24580@debbugs.gnu.org
> 
> > > > The problem lies (at least) in query-replace-read-from, where
> > > > query-replace-from-to is let bound, and later read-from-minibuffer
> > > > refers to the symbol 'query-replace-from-to.  At first sight, it looks
> > > > like it could be fixed by putting some of the setup done by these
> > > > bindings into the minibuffer setup hook.
> > > 
> > > Isn't there a less intrusive change to fix this?  A significant change
> > > like the one you propose is more risky, but if there's no safer one, I
> > > guess we should take it.
> > 
> > Perhaps just adding a (defvar query-replace-from-to-history …) will be
> > sufficient.  That read-from-minibuffer and friends' HIST argument can
> > only be a symbol is a bit frustrating.
> 
> Surely there is a better way.

Isn't this exactly the minibuffer setup hook method that I wanted to
avoid?

Does a simple defvar you suggested before not work?

Thanks.

bug#24580: 25.2.50; query-replace history doesn't work

[Mark Oteiza]

On 03/10/16 at 10:08am, Eli Zaretskii wrote:
> > Date: Sun, 2 Oct 2016 12:51:28 -0400
> > From: Mark Oteiza <mvoteiza@udel.edu>
> > Cc: mbork@mbork.pl, 24580@debbugs.gnu.org
> > 
> > > > As for the other problem Marcin mentioned, that seems to be due to the
> > > > custom-reevaluate-setting call in the same function.
> > > 
> > > I'm not sure I understand: what's wrong with
> > > custom-reevaluate-setting in this case?
> > 
> > >From -Q:
> > 
> > 1. (setq-default query-replace-from-to-separator " => ")
> > 2. Do an M-% (Like Marcin's in original recipe) once to get some default in the prompt
> > 3. M-%
> > 
> > I would expect to see (default a => b) in the prompt, but
> > custom-reevaluate-setting apparently ignores settings done outside of
> > customize.  The first step instead needs to be
> > 
> >   (custom-set-variables '(query-replace-from-to-separator " => "))
> > 
> > or something with the default for query-replace-from-to-separator should
> > be changed.
> 
> OK, but is this a new problem related to turning on lexical-binding?

No.

bug#24580: 25.2.50; query-replace history doesn't work

[Eli Zaretskii]

> Date: Mon, 3 Oct 2016 07:24:07 -0400
> From: Mark Oteiza <mvoteiza@udel.edu>
> Cc: mbork@mbork.pl, 24580@debbugs.gnu.org
> 
> On 03/10/16 at 10:08am, Eli Zaretskii wrote:
> > > Date: Sun, 2 Oct 2016 12:51:28 -0400
> > > From: Mark Oteiza <mvoteiza@udel.edu>
> > > Cc: mbork@mbork.pl, 24580@debbugs.gnu.org
> > > 
> > > > > As for the other problem Marcin mentioned, that seems to be due to the
> > > > > custom-reevaluate-setting call in the same function.
> > > > 
> > > > I'm not sure I understand: what's wrong with
> > > > custom-reevaluate-setting in this case?
> > > 
> > > >From -Q:
> > > 
> > > 1. (setq-default query-replace-from-to-separator " => ")
> > > 2. Do an M-% (Like Marcin's in original recipe) once to get some default in the prompt
> > > 3. M-%
> > > 
> > > I would expect to see (default a => b) in the prompt, but
> > > custom-reevaluate-setting apparently ignores settings done outside of
> > > customize.  The first step instead needs to be
> > > 
> > >   (custom-set-variables '(query-replace-from-to-separator " => "))
> > > 
> > > or something with the default for query-replace-from-to-separator should
> > > be changed.
> > 
> > OK, but is this a new problem related to turning on lexical-binding?
> 
> No.

Thanks, that's what I thought.  But it sounds like the use of
custom-reevaluate-setting is just a means to do something not
necessarily related to Customize, so I think avoiding the use of
custom-reevaluate-setting, and instead using char-displayable-p, at
least when not dumping, would provide a solution.

In any case, this is unrelated to lexical-binding, and is a separate
issue.

bug#24580: 25.2.50; query-replace history doesn't work

[Mark Oteiza]

On 03/10/16 at 10:16am, Eli Zaretskii wrote:
> > Date: Sun, 2 Oct 2016 21:08:43 -0400
> > From: Mark Oteiza <mvoteiza@udel.edu>
> > Cc: mbork@mbork.pl, 24580@debbugs.gnu.org
> > 
> > > > > The problem lies (at least) in query-replace-read-from, where
> > > > > query-replace-from-to is let bound, and later read-from-minibuffer
> > > > > refers to the symbol 'query-replace-from-to.  At first sight, it looks
> > > > > like it could be fixed by putting some of the setup done by these
> > > > > bindings into the minibuffer setup hook.
> > > > 
> > > > Isn't there a less intrusive change to fix this?  A significant change
> > > > like the one you propose is more risky, but if there's no safer one, I
> > > > guess we should take it.
> > > 
> > > Perhaps just adding a (defvar query-replace-from-to-history …) will be
> > > sufficient.  That read-from-minibuffer and friends' HIST argument can
> > > only be a symbol is a bit frustrating.
> > 
> > Surely there is a better way.
> 
> Isn't this exactly the minibuffer setup hook method that I wanted to
> avoid?

Yes

> Does a simple defvar you suggested before not work?

No, unless I erred somehow, which is likely, because what I tried first
(binding minibuffer-history) didn't appear to work either.  However, the
following does appear to solve the issue.

diff --git a/lisp/replace.el b/lisp/replace.el
index 4256751..4fc48d4 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -174,7 +174,7 @@ query-replace-read-from
 	      (propertize "\0"
 			  'display query-replace-from-to-separator
 			  'separator t)))
-	   (query-replace-from-to-history
+	   (minibuffer-history
 	    (append
 	     (when separator
 	       (mapcar (lambda (from-to)
@@ -186,7 +186,7 @@ query-replace-read-from
 	   (minibuffer-allow-text-properties t) ; separator uses text-properties
 	   (prompt
 	    (if (and query-replace-defaults separator)
-		(format "%s (default %s): " prompt (car query-replace-from-to-history))
+		(format "%s (default %s): " prompt (car minibuffer-history))
 	      (format "%s: " prompt)))
 	   (from
 	    ;; The save-excursion here is in case the user marks and copies
@@ -198,9 +198,9 @@ query-replace-read-from
                     (setq-local text-property-default-nonsticky
                                 (cons '(separator . t) text-property-default-nonsticky)))
                 (if regexp-flag
-                    (read-regexp prompt nil 'query-replace-from-to-history)
+                    (read-regexp prompt)
                   (read-from-minibuffer
-                   prompt nil nil nil 'query-replace-from-to-history
+                   prompt nil nil nil nil
                    (car (if regexp-flag regexp-search-ring search-ring)) t)))))
            (to))
       (if (and (zerop (length from)) query-replace-defaults)

bug#24580: 25.2.50; query-replace history doesn't work

[Eli Zaretskii]

> Date: Mon, 3 Oct 2016 20:24:51 -0400
> From: Mark Oteiza <mvoteiza@udel.edu>
> Cc: mbork@mbork.pl, 24580@debbugs.gnu.org
> 
> > Does a simple defvar you suggested before not work?
> 
> No, unless I erred somehow, which is likely, because what I tried first
> (binding minibuffer-history) didn't appear to work either.  However, the
> following does appear to solve the issue.

This looks much safer, so if you are okay with it, please push to
master.

Thanks.

bug#24580: 25.2.50; query-replace history doesn't work

[Mark Oteiza]

On 04/10/16 at 09:36am, Eli Zaretskii wrote:
> > Date: Mon, 3 Oct 2016 20:24:51 -0400
> > From: Mark Oteiza <mvoteiza@udel.edu>
> > Cc: mbork@mbork.pl, 24580@debbugs.gnu.org
> > 
> > > Does a simple defvar you suggested before not work?
> > 
> > No, unless I erred somehow, which is likely, because what I tried first
> > (binding minibuffer-history) didn't appear to work either.  However, the
> > following does appear to solve the issue.
> 
> This looks much safer, so if you are okay with it, please push to
> master.

Done in 6d6c93f4