bug#22479: 25.0.50; isearch and query-replace histories do not remember if we were looking for symbols

bug#22479: 25.0.50; isearch and query-replace histories do not remember if we were looking for symbols

[Dima Kogan]

Hi. It is possible to look for a symbol, and for this information to be
lost when repeating a search. For instance, say I have this buffer:

=======
a b c
ab bc
=======

1. I put the point on the first 'a'

2. M-s . (search for symbol at point). This find a symbol 'a', so the
   'a' in 'ab' doesn't match

3. C-g (quit the search)

4. C-s C-s (repeat previous search). Here emacs remembers we looked for
   'a', but not that it was a symbol, so it finds the 'a' in 'ab' even
   though it should not



A similar sequence is possible with query-replace:

1. Point on 'a'

2. M-s .

3. C-M-%, 'asdf' to replace the symbol 'a' with 'asdf'

4. C-g (quit before replacing anything)

5. C-M-% (repeat last replacement. The symbol-ness of 'a' was forgotten)


The simplest way to fix this would probably be to treat these as regex
searches wrapped in \_<...\_>. The downside is that to access the
history we'd have to do C-M-s C-M-s instead of C-s C-s, but maybe that
is ok. Thoughts?

bug#22479: 25.0.50; isearch and query-replace histories do not remember if we were looking for symbols

[Juri Linkov]

> Hi. It is possible to look for a symbol, and for this information to be
> lost when repeating a search. For instance, say I have this buffer:
>
> =======
> a b c
> ab bc
> =======
>
> 1. I put the point on the first 'a'
>
> 2. M-s . (search for symbol at point). This find a symbol 'a', so the
>    'a' in 'ab' doesn't match
>
> 3. C-g (quit the search)
>
> 4. C-s C-s (repeat previous search). Here emacs remembers we looked for
>    'a', but not that it was a symbol, so it finds the 'a' in 'ab' even
>    though it should not
>
>
>
> A similar sequence is possible with query-replace:
>
> 1. Point on 'a'
>
> 2. M-s .
>
> 3. C-M-%, 'asdf' to replace the symbol 'a' with 'asdf'
>
> 4. C-g (quit before replacing anything)
>
> 5. C-M-% (repeat last replacement. The symbol-ness of 'a' was forgotten)
>
>
> The simplest way to fix this would probably be to treat these as regex
> searches wrapped in \_<...\_>. The downside is that to access the
> history we'd have to do C-M-s C-M-s instead of C-s C-s, but maybe that
> is ok. Thoughts?

Thanks for the test cases.  We started designing the customizable variables
for this feature in bug#11378, and reached no final implementation.
What do you think about the proposal in bug#11378?

Since the original subject of bug#11378 was quite different,
I propose to close bug#11378, and leave this bug#22479 open
to implement this feature here.

bug#22479: 25.0.50; isearch and query-replace histories do not remember if we were looking for symbols

[Stephen Berman]

On Wed, 27 Jan 2016 14:27:53 -0800 Dima Kogan <dima@secretsauce.net> wrote:

> Hi. It is possible to look for a symbol, and for this information to be
> lost when repeating a search. For instance, say I have this buffer:
>
> =======
> a b c
> ab bc
> =======
>
> 1. I put the point on the first 'a'
>
> 2. M-s . (search for symbol at point). This find a symbol 'a', so the
>    'a' in 'ab' doesn't match
>
> 3. C-g (quit the search)
>
> 4. C-s C-s (repeat previous search). Here emacs remembers we looked for
>    'a', but not that it was a symbol, so it finds the 'a' in 'ab' even
>    though it should not

I can't reproduce this with -Q in emacs-25 (revision 5293d1b); doing
step 4 displays the message "[No previous search string]".  But if prior
to step 1 I do `C-s b RET C-a' and then continue with your recipe, doing
step 4 makes "b" the search string.  So I guess you had searched for "a"
before doing `M-s .'.

>
> A similar sequence is possible with query-replace:
>
> 1. Point on 'a'
>
> 2. M-s .
>
> 3. C-M-%, 'asdf' to replace the symbol 'a' with 'asdf'
>
> 4. C-g (quit before replacing anything)
>
> 5. C-M-% (repeat last replacement. The symbol-ness of 'a' was forgotten)

This I can reproduce with -Q.

Steve Berman

bug#22479: 25.0.50; isearch and query-replace histories do not remember if we were looking for symbols

[Dima Kogan]

Stephen Berman <stephen.berman@gmx.net> writes:

> On Wed, 27 Jan 2016 14:27:53 -0800 Dima Kogan <dima@secretsauce.net> wrote:
>
>> Hi. It is possible to look for a symbol, and for this information to be
>> lost when repeating a search. For instance, say I have this buffer:
>>
>> =======
>> a b c
>> ab bc
>> =======
>>
>> 1. I put the point on the first 'a'
>>
>> 2. M-s . (search for symbol at point). This find a symbol 'a', so the
>>    'a' in 'ab' doesn't match
>>
>> 3. C-g (quit the search)
>>
>> 4. C-s C-s (repeat previous search). Here emacs remembers we looked for
>>    'a', but not that it was a symbol, so it finds the 'a' in 'ab' even
>>    though it should not
>
> I can't reproduce this with -Q in emacs-25 (revision 5293d1b); doing
> step 4 displays the message "[No previous search string]".  But if prior
> to step 1 I do `C-s b RET C-a' and then continue with your recipe, doing
> step 4 makes "b" the search string.  So I guess you had searched for "a"
> before doing `M-s .'.

Hi. I want to say the recipe was working exactly as written, but I now
see what you say. In any case, if you C-s after the M-s . then the
recipe works

bug#22479: 25.0.50; isearch and query-replace histories do not remember if we were looking for symbols

[Dima Kogan]

On January 27, 2016 3:08:20 PM PST, Juri Linkov <juri@linkov.net> wrote:
>> Hi. It is possible to look for a symbol, and for this information to
>be
>> lost when repeating a search.
>> The simplest way to fix this would probably be to treat these as
>regex
>> searches wrapped in \_<...\_>. The downside is that to access the
>> history we'd have to do C-M-s C-M-s instead of C-s C-s, but maybe
>that
>> is ok. Thoughts?
>
>Thanks for the test cases.  We started designing the customizable
>variables
>for this feature in bug#11378, and reached no final implementation.
>What do you think about the proposal in bug#11378?
>
>Since the original subject of bug#11378 was quite different,
>I propose to close bug#11378, and leave this bug#22479 open
>to implement this feature here.

Hi. #11378 doesn't talk about keeping the meta-data in the history generically. It mentions remembering the state for the last search, but you'd still have the issue in this bug for older searches. So unless I missed it (very possible since it was a very long thread), that bug doesn't touch on this. We can propose new things, however :)

I don't know how much complexity we want here. Each history item can be a richer structure than just the search string. Too much?

bug#22479: 25.0.50; isearch and query-replace histories do not remember if we were looking for symbols

[Juri Linkov]

> Hi. #11378 doesn't talk about keeping the meta-data in the history
> generically. It mentions remembering the state for the last search, but
> you'd still have the issue in this bug for older searches. So unless
> I missed it (very possible since it was a very long thread), that bug
> doesn't touch on this. We can propose new things, however :)
>
> I don't know how much complexity we want here. Each history item can
> be a richer structure than just the search string. Too much?

Yes, this is a harder problem.  We have to remember meta-data for
every search history element.  There are several possibilities:

1. Changing the current format of ‘search-ring’ and ‘regexp-search-ring’
   is not backward-compatible: for external packages, for desktop.el,
   for savehist.el.

   Also note that distinction between these two variables already
   remembers one particular search parameter: either an ordinary or
   regexp search.  So we don't need to remember the regexp search
   parameter alongside with the value of the search search.
   OTOH, by remembering it with the value, we can obsolete
   ‘regexp-search-ring’.

2. Adding more such variables for other search parameters:
   ‘word-search-ring’, ‘symbol-search-ring’, ‘char-fold-search-ring’.
   A big mess ensues...

3. Using an additional variable with only meta-data requires keeping it
   in sync with the values in ‘search-ring’ and ‘regexp-search-ring’.

4. Adding meta-data by text-properties to the strings in
   ‘search-ring’ and ‘regexp-search-ring’ poses a problem of saving
   the values in the desktop and restoring in another session.

5. More ideas? ;)

bug#22479: 25.0.50; isearch and query-replace histories do not remember if we were looking for symbols

[Drew Adams]

> > I don't know how much complexity we want here. Each history item can
> > be a richer structure than just the search string. Too much?

Yes, too much (IMHO).  Trying to solve a non-problem or a minor problem.

(Why stop with search for a symbol?  There are a zillion things
that one could potentially record about the current search state,
from the current mode and currently active minor modes, all current
variable settings, key bindings, etc. to the current phase of the
moon.  Who is to say just what the user considered the search
context to be: what s?he was looking for and why?)

People have been using Isearch for decades without this bothering
them.  And it's not as if it is difficult for a user to reestablish
such a state (symbol search) interactively.

> Yes, this is a harder problem.  We have to remember meta-data for
> every search history element.  There are several possibilities:
...
> 5. More ideas? ;)

Yes, do nothing. ;-)  Move on to real problems...

bug#22479: 25.0.50; isearch and query-replace histories do not remember if we were looking for symbols

[Dima Kogan]

Drew Adams <drew.adams@oracle.com> writes:

>> > I don't know how much complexity we want here. Each history item can
>> > be a richer structure than just the search string. Too much?
>
> Yes, too much (IMHO).  Trying to solve a non-problem or a minor problem.

I won't claim that this is the most pressing thing we have to work on,
but to me, an editor is a large number of small, nice things. So I do
this it's worthwhile to fix this


> People have been using Isearch for decades without this bothering
> them.  And it's not as if it is difficult for a user to reestablish
> such a state (symbol search) interactively.

I only started using the symbol-finding functionality of isearch
recently, and it's clearly underbaked, and that bothers me.


> 5. More ideas? ;)

- We can retain the two histories: regex, non-regex

- Any fancy (word, symbol, etc) searches can be stored into the regex
  history, bookmarked with the appropriate regexen ("\_<", "\_>" for
  instance)

- When reading the history to present to the user, these can be parsed
  out. So for instance when the user looks through the symbol-search
  history by hitting

  M-s _
  C-s

  emacs can look through the regex history, only selecting entries in
  \_<...\_>. The C-M-s history can either include all of these, or show
  only unbookmarked entries; both would be ok, I think.

This should be sufficiently compatible with external packages. I can do
the implementation if this sounds reasonable.

bug#22479: 25.0.50; isearch and query-replace histories do not remember if we were looking for symbols

[Juri Linkov]

>> 5. More ideas? ;)
>
> - We can retain the two histories: regex, non-regex
>
> - Any fancy (word, symbol, etc) searches can be stored into the regex
>   history, bookmarked with the appropriate regexen ("\_<", "\_>" for
>   instance)
>
> - When reading the history to present to the user, these can be parsed
>   out. So for instance when the user looks through the symbol-search
>   history by hitting
>
>   M-s _
>   C-s
>
>   emacs can look through the regex history, only selecting entries in
>   \_<...\_>. The C-M-s history can either include all of these, or show
>   only unbookmarked entries; both would be ok, I think.

This approach is compromising but unreliable (doesn't distinguish
between the case when the user uses the symbol search M-s _
or types \_<...\_> directly in the regexp search) and insufficient
(need to remember other search parameters like case-fold and lax-whitespace).

bug#22479: 25.0.50; isearch and query-replace histories do not remember if we were looking for symbols

[Dima Kogan]

Juri Linkov <juri@linkov.net> writes:

>>> 5. More ideas? ;)
>>
>> - We can retain the two histories: regex, non-regex
>>
>> - Any fancy (word, symbol, etc) searches can be stored into the regex
>>   history, bookmarked with the appropriate regexen ("\_<", "\_>" for
>>   instance)
>>
>> - When reading the history to present to the user, these can be parsed
>>   out. So for instance when the user looks through the symbol-search
>>   history by hitting
>>
>>   M-s _
>>   C-s
>>
>>   emacs can look through the regex history, only selecting entries in
>>   \_<...\_>. The C-M-s history can either include all of these, or show
>>   only unbookmarked entries; both would be ok, I think.
>
> This approach is compromising but unreliable

Indeed it is a compromise. It feels to me to be much better than what we
have now, and the issues aren't too significant... I think...


> (doesn't distinguish between the case when the user uses the symbol
> search M-s _ or types \_<...\_> directly in the regexp search)

True, but I think this is OK. The symbol-search history would include
M-s _ searches AND those C-M-s searches where the intent was to look for
symbols; I think this is fine. The regex-search history would include
all symbol searches also; maybe that is less fine, but it isn't crazy.
The text search history would include none of these; this is GOOD, and
is not what we do now.


> (need to remember other search parameters like case-fold and
> lax-whitespace).

Hmmm. This clearly requires metadata so some of your other, more
intrusive proposals would be required to handle this. The above proposal
would work for word and symbol searches. I think fixing that would be
worth it. Do yall just want to leave it alone?

bug#22479: 25.0.50; isearch and query-replace histories do not remember if we were looking for symbols

[Juri Linkov]

>>>> 5. More ideas? ;)
>>>
>>> - We can retain the two histories: regex, non-regex
>>>
>>> - Any fancy (word, symbol, etc) searches can be stored into the regex
>>>   history, bookmarked with the appropriate regexen ("\_<", "\_>" for
>>>   instance)
>>>
>>> - When reading the history to present to the user, these can be parsed
>>>   out. So for instance when the user looks through the symbol-search
>>>   history by hitting
>>>
>>>   M-s _
>>>   C-s
>>>
>>>   emacs can look through the regex history, only selecting entries in
>>>   \_<...\_>. The C-M-s history can either include all of these, or show
>>>   only unbookmarked entries; both would be ok, I think.
>>
>> This approach is compromising but unreliable
>
> Indeed it is a compromise. It feels to me to be much better than what we
> have now, and the issues aren't too significant... I think...
>
>> (doesn't distinguish between the case when the user uses the symbol
>> search M-s _ or types \_<...\_> directly in the regexp search)
>
> True, but I think this is OK. The symbol-search history would include
> M-s _ searches AND those C-M-s searches where the intent was to look for
> symbols; I think this is fine. The regex-search history would include
> all symbol searches also; maybe that is less fine, but it isn't crazy.
> The text search history would include none of these; this is GOOD, and
> is not what we do now.

How then you would deconstruct an original search string from a
char-fold regexp?  Almost impossible with such garbage-looking
regexp like you demonstrated in bug#22520.

>> (need to remember other search parameters like case-fold and
>> lax-whitespace).
>
> Hmmm. This clearly requires metadata so some of your other, more
> intrusive proposals would be required to handle this.

Might be not too intrusive given that we already have such metadata
in isearch-cmds.  This means extending the isearch-cmds structure
to search-ring, and maybe renaming it to isearch-ring.

bug#22479: 25.0.50; isearch and query-replace histories do not remember if we were looking for symbols

[Juri Linkov]

>>>> 5. More ideas? ;)
>>>
>>> - We can retain the two histories: regex, non-regex

One additional question: what do you expect ‘M-s _ M-p M-p M-p ...’ to do?
Should it browse the previous search strings only from the currently active
search mode (in this case symbol search)?  Or while browsing previous elements,
should it change the search mode, e.g. when a previous search string
was saved to the search-ring by word mode, then toggle word mode
from symbol mode after typing 'M-p'?

bug#22479: 25.0.50; isearch and query-replace histories do not remember if we were looking for symbols

[Dima Kogan]

Juri Linkov <juri@linkov.net> writes:

>>>>> 5. More ideas? ;)
>>>>
>>>> - We can retain the two histories: regex, non-regex
>
> One additional question: what do you expect ‘M-s _ M-p M-p M-p ...’ to do?
> Should it browse the previous search strings only from the currently active
> search mode (in this case symbol search)?  Or while browsing previous elements,
> should it change the search mode, e.g. when a previous search string
> was saved to the search-ring by word mode, then toggle word mode
> from symbol mode after typing 'M-p'?

I was thinking that M-p would retain whatever mode we're in. So in that
example, we'd cycle only through symbol searches. Since there are
multiple different modes that combine combinatorially, maybe this is not
an ideal way to do it, but that's what I'm thinking now.

This conversation has been useful, and I'm now digging through
isearch.el to see what the quickest (and least intrusive) approach to a
patch would be.

Thanks

bug#22479: 25.0.50; isearch and query-replace histories do not remember if we were looking for symbols

[Dima Kogan]

Juri Linkov <juri@linkov.net> writes:

> Yes, this is a harder problem.  We have to remember meta-data for
> every search history element.  There are several possibilities:
>
> 4. Adding meta-data by text-properties to the strings in
>    ‘search-ring’ and ‘regexp-search-ring’ poses a problem of saving
>    the values in the desktop and restoring in another session.

This feels like the best solution. I looked at desktop.el, and it
doesn't save the properties, as you say. But it could be made to. Do you
think some external packages would get broken by this change?

bug#22479: 25.0.50; isearch and query-replace histories do not remember if we were looking for symbols

[Juri Linkov]

>> Yes, this is a harder problem.  We have to remember meta-data for
>> every search history element.  There are several possibilities:
>>
>> 4. Adding meta-data by text-properties to the strings in
>>    ‘search-ring’ and ‘regexp-search-ring’ poses a problem of saving
>>    the values in the desktop and restoring in another session.
>
> This feels like the best solution. I looked at desktop.el, and it
> doesn't save the properties, as you say. But it could be made to. Do you
> think some external packages would get broken by this change?

In addition to desktop.el, other external packages save histories as well
like savehist.el.

bug#22479: 25.0.50; isearch and query-replace histories do not remember if we were looking for symbols

[Juri Linkov]

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

>> Yes, this is a harder problem.  We have to remember meta-data for
>> every search history element.  There are several possibilities:
>>
>> 4. Adding meta-data by text-properties to the strings in
>>    ‘search-ring’ and ‘regexp-search-ring’ poses a problem of saving
>>    the values in the desktop and restoring in another session.
>
> This feels like the best solution. I looked at desktop.el, and it
> doesn't save the properties, as you say. But it could be made to. Do you
> think some external packages would get broken by this change?

This seems to be the least radical change as concluded in
https://lists.gnu.org/archive/html/emacs-devel/2018-03/msg00296.html
The patch makes the search mode to be remembered in isearch as well as
in query-replace.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: isearch-text-properties.2.patch --]
[-- Type: text/x-diff, Size: 6383 bytes --]

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 84b121a..56a44d8 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -1103,7 +1111,9 @@ isearch-done
 
   (if (and (> (length isearch-string) 0) (not nopush))
       ;; Update the ring data.
-      (isearch-update-ring isearch-string isearch-regexp))
+      (isearch-update-ring isearch-string isearch-regexp
+                           `(regexp-function ,isearch-regexp-function
+                             case-fold-search ,isearch-case-fold-search)))
 
   (let ((isearch-mode-end-hook-quit (and nopush (not edit))))
     (run-hooks 'isearch-mode-end-hook))
@@ -1119,13 +1129,14 @@ isearch-done
 
   (and (not edit) isearch-recursive-edit (exit-recursive-edit)))
 
-(defun isearch-update-ring (string &optional regexp)
+(defun isearch-update-ring (string &optional regexp properties)
   "Add STRING to the beginning of the search ring.
 REGEXP if non-nil says use the regexp search ring."
   (add-to-history
    (if regexp 'regexp-search-ring 'search-ring)
-   string
-   (if regexp regexp-search-ring-max search-ring-max)))
+   (if properties (apply 'propertize string properties) string)
+   (if regexp regexp-search-ring-max search-ring-max)
+   t))
 
 ;; Switching buffers should first terminate isearch-mode.
 ;; ;; For Emacs 19, the frame switch event is handled.
@@ -1834,7 +1845,11 @@ isearch-query-replace
 	;; `exit-recursive-edit' in `isearch-done' that terminates
 	;; the execution of this command when it is non-nil.
 	;; We call `exit-recursive-edit' explicitly at the end below.
-	(isearch-recursive-edit nil))
+	(isearch-recursive-edit nil)
+        (isearch-string-propertized
+         (propertize isearch-string
+		     'isearch-regexp-function isearch-regexp-function
+		     'isearch-case-fold-search isearch-case-fold-search)))
     (isearch-done nil t)
     (isearch-clean-overlays)
     (if (and isearch-other-end
@@ -1847,12 +1862,12 @@ isearch-query-replace
 			 (< (mark) (point))))))
         (goto-char isearch-other-end))
     (set query-replace-from-history-variable
-         (cons isearch-string
+         (cons isearch-string-propertized
                (symbol-value query-replace-from-history-variable)))
     (perform-replace
-     isearch-string
+     isearch-string-propertized
      (query-replace-read-to
-      isearch-string
+      isearch-string-propertized
       (concat "Query replace"
               (isearch--describe-regexp-mode (or delimited isearch-regexp-function) t)
 	      (if backward " backward" "")
@@ -2556,7 +2575,13 @@ isearch-ring-adjust1
 		      length)))
       (setq isearch-string (nth yank-pointer ring)
 	    isearch-message (mapconcat 'isearch-text-char-description
-				       isearch-string "")))))
+				       isearch-string ""))
+      (when (memq 'regexp-function (text-properties-at 0 isearch-string))
+        (setq isearch-regexp-function
+              (get-text-property 0 'regexp-function isearch-string)))
+      (when (memq 'case-fold-search (text-properties-at 0 isearch-string))
+        (setq isearch-case-fold-search
+              (get-text-property 0 'case-fold-search isearch-string))))))
 
 (defun isearch-ring-adjust (advance)
   ;; Helper for isearch-ring-advance and isearch-ring-retreat
@@ -2772,11 +2797,16 @@ isearch-search-fun
 
 (defun isearch--lax-regexp-function-p ()
   "Non-nil if next regexp-function call should be lax."
+  ;; FIXME: maybe simpler to use this:
+  ;; (or (memq this-command '(isearch-printing-char isearch-del-char))
+  ;;     isearch-yank-flag)
   (not (or isearch-nonincremental
            (null (car isearch-cmds))
            (eq (length isearch-string)
                (length (isearch--state-string
-                        (car isearch-cmds)))))))
+                        (car isearch-cmds))))
+           ;; Search string comes from the history with text properties
+           (memq 'regexp-function (text-properties-at 0 isearch-string)))))
 
 (defun isearch-search-fun-default ()
   "Return default functions to use for the search."
diff --git a/lisp/replace.el b/lisp/replace.el
index c28c9b3..ac37bd7 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -301,7 +301,9 @@ query-replace-read-args
 	 (to (if (consp from) (prog1 (cdr from) (setq from (car from)))
 	       (query-replace-read-to from prompt regexp-flag))))
     (list from to
-	  (and current-prefix-arg (not (eq current-prefix-arg '-)))
+	  (or (and current-prefix-arg (not (eq current-prefix-arg '-)))
+              (and (memq 'isearch-regexp-function (text-properties-at 0 from))
+                   (get-text-property 0 'isearch-regexp-function from)))
 	  (and current-prefix-arg (eq current-prefix-arg '-)))))
 
 (defun query-replace (from-string to-string &optional delimited start end backward region-noncontiguous-p)
@@ -2361,8 +2363,17 @@ perform-replace
          (message
           (if query-flag
               (apply 'propertize
-                     (substitute-command-keys
-                      "Query replacing %s with %s: (\\<query-replace-map>\\[help] for help) ")
+                     (concat "Query replacing "
+                             (if backward "backward " "")
+                             (if delimited-flag
+                                 (or (and (symbolp delimited-flag)
+                                          (get delimited-flag
+                                               'isearch-message-prefix))
+                                     "word ") "")
+                             (if regexp-flag "regexp " "")
+                             "%s with %s: "
+                             (substitute-command-keys
+                              "(\\<query-replace-map>\\[help] for help) "))
                      minibuffer-prompt-properties))))
 
     ;; Unless a single contiguous chunk is selected, operate on multiple chunks.
@@ -2580,13 +2591,13 @@ perform-replace
 			 (with-output-to-temp-buffer "*Help*"
 			   (princ
 			    (concat "Query replacing "
+				    (if backward "backward " "")
 				    (if delimited-flag
 					(or (and (symbolp delimited-flag)
 						 (get delimited-flag
                                                       'isearch-message-prefix))
 					    "word ") "")
 				    (if regexp-flag "regexp " "")
-				    (if backward "backward " "")
 				    from-string " with "
 				    next-replacement ".\n\n"
 				    (substitute-command-keys

bug#22479: 25.0.50; isearch and query-replace histories do not remember if we were looking for symbols

[Juri Linkov]

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

>>> Yes, this is a harder problem.  We have to remember meta-data for
>>> every search history element.  There are several possibilities:
>>>
>>> 4. Adding meta-data by text-properties to the strings in
>>>    ‘search-ring’ and ‘regexp-search-ring’ poses a problem of saving
>>>    the values in the desktop and restoring in another session.
>>
>> This feels like the best solution. I looked at desktop.el, and it
>> doesn't save the properties, as you say. But it could be made to. Do you
>> think some external packages would get broken by this change?
>
> This seems to be the least radical change as concluded in
> https://lists.gnu.org/archive/html/emacs-devel/2018-03/msg00296.html
> The patch makes the search mode to be remembered in isearch as well as
> in query-replace.

As predicted, the solution with using text properties caused more trouble:

1. To be able to use ‘C-s M-p’ and restore a previous symbol isearch
   it was necessary to let-bind minibuffer-allow-text-properties to t
   in isearch-edit-string.

2. To use a previous replacement with text properties required to replace
   ‘substring-no-properties’ with ‘substring’ in ‘query-replace--split-string’.

3. For the same purpose to use e.g. ‘M-% M-p RET’ also required to completely
   rewrite ‘query-replace-descr’ to keep text properties non-destructively
   in the replacement string.

I'm not sure if I found all problems or there will be more trouble,
but at least these fixes were necessary for this patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: isearch-text-properties.3.patch --]
[-- Type: text/x-diff, Size: 9059 bytes --]

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 15a1543..c0f6f92 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -1105,7 +1105,9 @@ isearch-done
 
   (if (and (> (length isearch-string) 0) (not nopush))
       ;; Update the ring data.
-      (isearch-update-ring isearch-string isearch-regexp))
+      (isearch-update-ring isearch-string isearch-regexp
+                           `(isearch-regexp-function  ,isearch-regexp-function
+                             isearch-case-fold-search ,isearch-case-fold-search)))
 
   (let ((isearch-mode-end-hook-quit (and nopush (not edit))))
     (run-hooks 'isearch-mode-end-hook))
@@ -1121,13 +1123,14 @@ isearch-done
 
   (and (not edit) isearch-recursive-edit (exit-recursive-edit)))
 
-(defun isearch-update-ring (string &optional regexp)
+(defun isearch-update-ring (string &optional regexp properties)
   "Add STRING to the beginning of the search ring.
 REGEXP if non-nil says use the regexp search ring."
   (add-to-history
    (if regexp 'regexp-search-ring 'search-ring)
-   string
-   (if regexp regexp-search-ring-max search-ring-max)))
+   (if properties (apply 'propertize string properties) string)
+   (if regexp regexp-search-ring-max search-ring-max)
+   t))
 
 ;; Switching buffers should first terminate isearch-mode.
 ;; ;; For Emacs 19, the frame switch event is handled.
@@ -1342,6 +1345,13 @@ with-isearch-suspended
 		  multi-isearch-file-list multi-isearch-file-list-new
 		  multi-isearch-buffer-list multi-isearch-buffer-list-new)
 
+	    (when (memq 'isearch-regexp-function (text-properties-at 0 isearch-string))
+	      (setq isearch-regexp-function
+		    (get-text-property 0 'isearch-regexp-function isearch-string)))
+	    (when (memq 'isearch-case-fold-search (text-properties-at 0 isearch-string))
+	      (setq isearch-case-fold-search
+		    (get-text-property 0 'isearch-case-fold-search isearch-string)))
+
 	    ;; Restore the minibuffer message before moving point.
             (funcall (or isearch-message-function #'isearch-message) nil t)
 
@@ -1408,7 +1418,9 @@ isearch-edit-string
 	  (history-add-new-input nil)
 	  ;; Binding minibuffer-history-symbol to nil is a work-around
 	  ;; for some incompatibility with gmhist.
-	  (minibuffer-history-symbol))
+	  (minibuffer-history-symbol)
+	  ;; Search string might have meta information on text properties.
+	  (minibuffer-allow-text-properties t))
      (setq isearch-new-string
 	   (read-from-minibuffer
 	    (isearch-message-prefix nil isearch-nonincremental)
@@ -1836,7 +1848,11 @@ isearch-query-replace
 	;; `exit-recursive-edit' in `isearch-done' that terminates
 	;; the execution of this command when it is non-nil.
 	;; We call `exit-recursive-edit' explicitly at the end below.
-	(isearch-recursive-edit nil))
+	(isearch-recursive-edit nil)
+	(isearch-string-propertized
+	 (propertize isearch-string
+		     'isearch-regexp-function isearch-regexp-function
+		     'isearch-case-fold-search isearch-case-fold-search)))
     (isearch-done nil t)
     (isearch-clean-overlays)
     (if (and isearch-other-end
@@ -1849,12 +1865,12 @@ isearch-query-replace
 			 (< (mark) (point))))))
         (goto-char isearch-other-end))
     (set query-replace-from-history-variable
-         (cons isearch-string
+         (cons isearch-string-propertized
                (symbol-value query-replace-from-history-variable)))
     (perform-replace
-     isearch-string
+     isearch-string-propertized
      (query-replace-read-to
-      isearch-string
+      isearch-string-propertized
       (concat "Query replace"
               (isearch--describe-regexp-mode (or delimited isearch-regexp-function) t)
 	      (if backward " backward" "")
@@ -2562,7 +2578,13 @@ isearch-ring-adjust1
 		      length)))
       (setq isearch-string (nth yank-pointer ring)
 	    isearch-message (mapconcat 'isearch-text-char-description
-				       isearch-string "")))))
+				       isearch-string ""))
+      (when (memq 'isearch-regexp-function (text-properties-at 0 isearch-string))
+	(setq isearch-regexp-function
+	      (get-text-property 0 'isearch-regexp-function isearch-string)))
+      (when (memq 'isearch-case-fold-search (text-properties-at 0 isearch-string))
+	(setq isearch-case-fold-search
+	      (get-text-property 0 'isearch-case-fold-search isearch-string))))))
 
 (defun isearch-ring-adjust (advance)
   ;; Helper for isearch-ring-advance and isearch-ring-retreat
@@ -2778,11 +2800,17 @@ isearch-search-fun
 
 (defun isearch--lax-regexp-function-p ()
   "Non-nil if next regexp-function call should be lax."
-  (not (or isearch-nonincremental
-           (null (car isearch-cmds))
-           (eq (length isearch-string)
-               (length (isearch--state-string
-                        (car isearch-cmds)))))))
+  ;; FIXME: maybe simpler to use this:
+  (or (memq this-command '(isearch-printing-char isearch-del-char))
+      isearch-yank-flag)
+  ;; (not (or isearch-nonincremental
+  ;;          (null (car isearch-cmds))
+  ;;          (eq (length isearch-string)
+  ;;              (length (isearch--state-string
+  ;;                       (car isearch-cmds))))
+  ;;          ;; Search string comes from the history with text properties
+  ;;          (memq 'isearch-regexp-function (text-properties-at 0 isearch-string))))
+  )
 
 (defun isearch-search-fun-default ()
   "Return default functions to use for the search."
diff --git a/lisp/replace.el b/lisp/replace.el
index 4916cb1..b943a2e 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -147,16 +147,27 @@ replace-count
 See `replace-regexp' and `query-replace-regexp-eval'.")
 
 (defun query-replace-descr (string)
-  (mapconcat 'isearch-text-char-description string ""))
+  (let ((string (copy-sequence string)))
+    (dotimes (i (length string) string)
+      (let ((c (aref string i)))
+        (cond
+         ((< c ?\s) (add-text-properties
+                     i (1+ i)
+                     `(display ,(propertize (format "^%c" (+ c 64)) 'face 'escape-glyph))
+                     string))
+         ((= c ?\^?) (add-text-properties
+	              i (1+ i)
+                      `(display ,(propertize "^?" 'face 'escape-glyph))
+                      string)))))))
 
 (defun query-replace--split-string (string)
   "Split string STRING at a substring with property `separator'."
   (let* ((length (length string))
          (split-pos (text-property-any 0 length 'separator t string)))
     (if (not split-pos)
-        (substring-no-properties string)
-      (cons (substring-no-properties string 0 split-pos)
-            (substring-no-properties
+        string
+      (cons (substring string 0 split-pos)
+            (substring
              string (or (text-property-not-all
                          (1+ split-pos) length 'separator t string)
                         length)
@@ -301,7 +312,9 @@ query-replace-read-args
 	 (to (if (consp from) (prog1 (cdr from) (setq from (car from)))
 	       (query-replace-read-to from prompt regexp-flag))))
     (list from to
-	  (and current-prefix-arg (not (eq current-prefix-arg '-)))
+	  (or (and current-prefix-arg (not (eq current-prefix-arg '-)))
+              (and (memq 'isearch-regexp-function (text-properties-at 0 from))
+                   (get-text-property 0 'isearch-regexp-function from)))
 	  (and current-prefix-arg (eq current-prefix-arg '-)))))
 
 (defun query-replace (from-string to-string &optional delimited start end backward region-noncontiguous-p)
@@ -2361,8 +2372,17 @@ perform-replace
          (message
           (if query-flag
               (apply 'propertize
-                     (substitute-command-keys
-                      "Query replacing %s with %s: (\\<query-replace-map>\\[help] for help) ")
+                     (concat "Query replacing "
+                             (if backward "backward " "")
+                             (if delimited-flag
+                                 (or (and (symbolp delimited-flag)
+                                          (get delimited-flag
+                                               'isearch-message-prefix))
+                                     "word ") "")
+                             (if regexp-flag "regexp " "")
+                             "%s with %s: "
+                             (substitute-command-keys
+                              "(\\<query-replace-map>\\[help] for help) "))
                      minibuffer-prompt-properties))))
 
     ;; Unless a single contiguous chunk is selected, operate on multiple chunks.
@@ -2580,13 +2600,13 @@ perform-replace
 			 (with-output-to-temp-buffer "*Help*"
 			   (princ
 			    (concat "Query replacing "
+				    (if backward "backward " "")
 				    (if delimited-flag
 					(or (and (symbolp delimited-flag)
 						 (get delimited-flag
                                                       'isearch-message-prefix))
 					    "word ") "")
 				    (if regexp-flag "regexp " "")
-				    (if backward "backward " "")
 				    from-string " with "
 				    next-replacement ".\n\n"
 				    (substitute-command-keys

bug#22479: 25.0.50; isearch and query-replace histories do not remember if we were looking for symbols

[Juri Linkov]

Version: 27.0.50

> I'm not sure if I found all problems or there will be more trouble,
> but at least these fixes were necessary for this patch:

Pushed to master as 99de04e.

bug#22479: 25.0.50; isearch and query-replace histories do not remember if we were looking for symbols

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Thu, 19 Apr 2018 23:32:04 +0300
> Cc: 22479-done@debbugs.gnu.org
> 
> Version: 27.0.50
> 
> > I'm not sure if I found all problems or there will be more trouble,
> > but at least these fixes were necessary for this patch:
> 
> Pushed to master as 99de04e.

Thanks.

This change seems to have some user-visible effects, in which case it
should be in NEWS and in the manual.

bug#22479: 25.0.50; isearch and query-replace histories do not remember if we were looking for symbols

[Juri Linkov]

>> Pushed to master as 99de04e.
>
> Thanks.
>
> This change seems to have some user-visible effects, in which case it
> should be in NEWS and in the manual.

This change remembers the search mode in the history and reuses it in
repeated searches.  This can be mentioned in NEWS, but I have no idea
what to document in the manual.  Maybe this feature should be
configurable, then its customization can be documented in the manual.

bug#22479: 25.0.50; isearch and query-replace histories do not remember if we were looking for symbols

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 22479@debbugs.gnu.org,  dima@secretsauce.net
> Date: Sat, 21 Apr 2018 23:02:54 +0300
> 
> >> Pushed to master as 99de04e.
> >
> > Thanks.
> >
> > This change seems to have some user-visible effects, in which case it
> > should be in NEWS and in the manual.
> 
> This change remembers the search mode in the history and reuses it in
> repeated searches.  This can be mentioned in NEWS, but I have no idea
> what to document in the manual.

The manual can say what NEWS says, for starters.

It also seemed to me that the Isearch prompt has changed a little; if
that is true, maybe that should be documented.

bug#22479: 25.0.50; isearch and query-replace histories do not remember if we were looking for symbols

[Juri Linkov]

>> > This change seems to have some user-visible effects, in which case it
>> > should be in NEWS and in the manual.
>>
>> This change remembers the search mode in the history and reuses it in
>> repeated searches.  This can be mentioned in NEWS, but I have no idea
>> what to document in the manual.
>
> The manual can say what NEWS says, for starters.
>
> It also seemed to me that the Isearch prompt has changed a little; if
> that is true, maybe that should be documented.

Now the prompt has changed a little.  Should this be mentioned in NEWS,
or this fact is self-evident?

bug#22479: 25.0.50; isearch and query-replace histories do not remember if we were looking for symbols

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 22479@debbugs.gnu.org,  dima@secretsauce.net
> Date: Mon, 23 Apr 2018 01:11:18 +0300
> 
> > The manual can say what NEWS says, for starters.
> >
> > It also seemed to me that the Isearch prompt has changed a little; if
> > that is true, maybe that should be documented.
> 
> Now the prompt has changed a little.  Should this be mentioned in NEWS,
> or this fact is self-evident?

I think every user-visible change should be called out in NEWS,
especially if the prompt itself may need some explanation (e.g., due
to abbreviated words, acronyms, etc.).