Simple isearch concerns

Simple isearch concerns

[Ergus]

Hi:

After some time I returned to isearch and I am concerned about some
details and I am not sure if they are issues or design choices. IMHO
they look more like issues giving limited experience to the users; but
probably I am wrong ...

1) Are there any option to move the cursor to the start of the candidate
after the search? I see that there is an isearch-other-end. But it is
not a custom but an internal variable. Now I am using a hook but it
seems a bit hacky.

2) I want to disable wrapping and I see that there is an
`isearch-wrap-function` but it is a variable not a custom and it is not
documented in detail what is expected to be there.

3) In general many of the customizable variables in isearch are defined
with defvar so it is difficult to distinguish between internal and user
exposed ones. Just to mention some examples:
isearch-push-state-function, isearch-filter-predicate, the hooks,

I am wondering if one of the main maintainers is planning to do some
refactor and cleanup to the isearch code because it looks like it is
using some old things and the code has evolved into a sort of labyrinth
(for example it is not using the minor-mode functions, but manually
implementing a similar behavior), there are many useless conditions that
could be replaced with substitutions, redundant variables and so on...

Are there any WIP related?
Best,
Ergus

Re: Simple isearch concerns

[Thierry Volpiatto]

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


Ergus <spacibba@aol.com> writes:

> Are there any WIP related?

Don't know, but there is external packages, mine is
https://github.com/thierryvolpiatto/isearch-light.


-- 
Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 686 bytes --]

Re: Simple isearch concerns

[Manuel Uberti]

On 03/04/21 02:15, Ergus wrote:
> Are there any WIP related?

Along with the package mentioned by Thierry there is CTRLF[1].

[1] https://github.com/raxod502/ctrlf

-- 
Manuel Uberti
www.manueluberti.eu

Re: Simple isearch concerns

[Daniel Martín]

Manuel Uberti <manuel.uberti@inventati.org> writes:

> On 03/04/21 02:15, Ergus wrote:
>> Are there any WIP related?
>
> Along with the package mentioned by Thierry there is CTRLF[1].
>
> [1] https://github.com/raxod502/ctrlf

CTRLF is a solid package, but has a crucial difference, in my opinion,
with respect to isearch.  The Readme cites this as an isearch
disadvantange: "If you type any command that is not bound in Isearch,
then you exit your search and run that command".  In my case, and
perhaps I'm the only one, this is one of the features of isearch that I
cannot live without: It's very simple to end a search.  But I don't
oppose having the package in ELPA for people that like or are used to
the way search in web browsers work.

Re: Simple isearch concerns

[Zhiwei Chen]

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


On Apr 3, 2021, at 6:37 PM, Daniel Martín <mardani29@yahoo.es<mailto:mardani29@yahoo.es>> wrote:

"If you type any command that is not bound in Isearch,
then you exit your search and run that command".  In my case, and
perhaps I'm the only one, this is one of the features of isearch that I
cannot live without: It's very simple to end a search.  But I don't
oppose having the package in ELPA for people that like or are used to
the way search in web browsers work.


Actually this feature can cause inconvenience in some cases.

`isearch-occur’ (bound to `M-s o’ in 'isearch-mode-map`) when `isearch’, even a buffer is created, `isearch’  doesn’t exit though.

In vanilla Emacs, the cursor is in the original buffer. But if you use shackle or customize `display-buffer-alist’ directly, the cursor
can be refocused to the new created one. Typing in the *Occur* buffer triggers `isearch-printing-char’ which is counterintuitive.

--
Zhiwei Chen


[-- Attachment #2: Type: text/html, Size: 2300 bytes --]

Re: Simple isearch concerns

[Stefan Monnier]

I'm not completely sure I understand the behavior you describe, but
since you consider it counter-intuitive, it sounds like you might want
to `M-x report-emacs-bug`.


        Stefan


Zhiwei Chen [2021-04-06 07:12:57] wrote:

> On Apr 3, 2021, at 6:37 PM, Daniel Martín <mardani29@yahoo.es<mailto:mardani29@yahoo.es>> wrote:
>
> "If you type any command that is not bound in Isearch,
> then you exit your search and run that command".  In my case, and
> perhaps I'm the only one, this is one of the features of isearch that I
> cannot live without: It's very simple to end a search.  But I don't
> oppose having the package in ELPA for people that like or are used to
> the way search in web browsers work.
>
>
> Actually this feature can cause inconvenience in some cases.
>
> `isearch-occur’ (bound to `M-s o’ in 'isearch-mode-map`) when `isearch’,
> even a buffer is created, `isearch’  doesn’t exit though.
>
> In vanilla Emacs, the cursor is in the original buffer. But if you use
> shackle or customize `display-buffer-alist’ directly, the cursor
> can be refocused to the new created one. Typing in the *Occur* buffer
> triggers `isearch-printing-char’ which is counterintuitive.
>
> --
> Zhiwei Chen

Re: Simple isearch concerns

[Gregory Heytings]

>
> I'm not completely sure I understand the behavior you describe, but 
> since you consider it counter-intuitive, it sounds like you might want 
> to `M-x report-emacs-bug`.
>

IIUC, what Zhiwei Chen described is what happens with:

(define-key isearch-mode-map (kbd "M-s o") #'(lambda () (interactive) (call-interactively 'isearch-occur) (pop-to-buffer "*Occur*")))

Point moves to the occur buffer after you type M-s o, yet isearch has not 
ended, so pressing "n", "p", "q", ... adds these characters to the search 
string instead of acting in the occur buffer.

I'm not sure this is a bug, but it should be possible to avoid that 
behavior by checking e.g. in isearch-pre-command-hook that point is still 
in the buffer in which isearch started.

Re: Simple isearch concerns

[Gregory Heytings]

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


>> I'm not completely sure I understand the behavior you describe, but 
>> since you consider it counter-intuitive, it sounds like you might want 
>> to `M-x report-emacs-bug`.
>
> IIUC, what Zhiwei Chen described is what happens with:
>
> (define-key isearch-mode-map (kbd "M-s o") #'(lambda () (interactive) 
> (call-interactively 'isearch-occur) (pop-to-buffer "*Occur*")))
>
> Point moves to the occur buffer after you type M-s o, yet isearch has 
> not ended, so pressing "n", "p", "q", ... adds these characters to the 
> search string instead of acting in the occur buffer.
>
> I'm not sure this is a bug, but it should be possible to avoid that 
> behavior by checking e.g. in isearch-pre-command-hook that point is 
> still in the buffer in which isearch started.
>

And here is a micro-patch to fix that behavior.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=0001-Terminate-isearch-when-point-has-moved-to-another-bu.patch, Size: 1506 bytes --]

From 86af0d656e6ef13c92d7eb4ec6bdfc5f204b4101 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Tue, 6 Apr 2021 14:14:04 +0000
Subject: [PATCH] Terminate isearch when point has moved to another buffer

* lisp/isearch.el (isearch-post-command-hook): Terminate isearch
when the command just executed has moved point to another buffer.
---
 lisp/isearch.el | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 943e24aa56..0f0985ba83 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -1209,7 +1209,8 @@ used to set the value of `isearch-regexp-function'."
 	;; Save the original value of `minibuffer-message-timeout', and
 	;; set it to nil so that isearch's messages don't get timed out.
 	isearch-original-minibuffer-message-timeout minibuffer-message-timeout
-	minibuffer-message-timeout nil)
+	minibuffer-message-timeout nil
+	isearch-buffer (current-buffer))
 
   (if (local-variable-p 'tool-bar-map)
       (setq isearch-tool-bar-old-map tool-bar-map))
@@ -3012,6 +3013,9 @@ See more for options in `search-exit-option'."
            (goto-char isearch-pre-move-point))
          (isearch-search-and-update)))
      (setq isearch-pre-move-point nil))
+  ;; Terminate the search if point has moved to another buffer.
+  (unless (eq isearch-buffer (current-buffer))
+    (isearch-cancel))
   (force-mode-line-update))
 
 (defun isearch-quote-char (&optional count)
-- 
2.30.2

Re: Simple isearch concerns

[Juri Linkov]

> And here is a micro-patch to fix that behavior.
>
> +  ;; Terminate the search if point has moved to another buffer.
> +  (unless (eq isearch-buffer (current-buffer))
> +    (isearch-cancel))

Have you tested this with 'multi-isearch-buffers'?

Re: Simple isearch concerns

[Gregory Heytings]

>> And here is a micro-patch to fix that behavior.
>>
>> +  ;; Terminate the search if point has moved to another buffer.
>> +  (unless (eq isearch-buffer (current-buffer))
>> +    (isearch-cancel))
>
> Have you tested this with 'multi-isearch-buffers'?
>

No, because I did not know that multi-isearch-buffers exists.  I see that 
it uses multi-isearch-current-buffer in a similar way than isearch-buffer, 
but currently I don't know anything else about multi-isearch-buffers, so I 
cannot try to fix this.

Re: Simple isearch concerns

[Gregory Heytings]

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


>>> And here is a micro-patch to fix that behavior.
>>> 
>>> +  ;; Terminate the search if point has moved to another buffer.
>>> +  (unless (eq isearch-buffer (current-buffer))
>>> +    (isearch-cancel))
>> 
>> Have you tested this with 'multi-isearch-buffers'?
>
> No, because I did not know that multi-isearch-buffers exists.  I see 
> that it uses multi-isearch-current-buffer in a similar way than 
> isearch-buffer, but currently I don't know anything else about 
> multi-isearch-buffers, so I cannot try to fix this.
>

Okay, I looked a bit closer at multi-isearch, and it seems that the 
attached patch works with mutli-isearch, too.

[-- Attachment #2: Type: text/x-diff, Size: 1053 bytes --]

From 2a000d2eee25ca0ccbb5f0c638c7c9bd0a653808 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Wed, 7 Apr 2021 10:16:24 +0000
Subject: [PATCH] Terminate isearch when point has moved to another buffer

* lisp/isearch.el (isearch-post-command-hook): Terminate isearch
when the command just executed has moved point to another buffer.
---
 lisp/isearch.el | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lisp/isearch.el b/lisp/isearch.el
index a828c569aa..a93e7a9576 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -3040,6 +3040,9 @@ See more for options in `search-exit-option'."
            (goto-char isearch-pre-move-point))
          (isearch-search-and-update)))
      (setq isearch-pre-move-point nil))
+  ;; Terminate the search if point has moved to another buffer.
+  (unless (eq isearch--current-buffer (current-buffer))
+    (with-current-buffer isearch--current-buffer (isearch-exit)))
   (force-mode-line-update))
 
 (defun isearch-quote-char (&optional count)
-- 
2.30.2

Re: Simple isearch concerns

[Juri Linkov]

> Okay, I looked a bit closer at multi-isearch, and it seems that the
> attached patch works with mutli-isearch, too.
>
> +  ;; Terminate the search if point has moved to another buffer.
> +  (unless (eq isearch--current-buffer (current-buffer))
> +    (with-current-buffer isearch--current-buffer (isearch-exit)))

When in your original test case '(pop-to-buffer "*Occur*")'
is replaced with '(kill-current-buffer)':

  (define-key isearch-mode-map (kbd "M-s o")
    #'(lambda ()
        (interactive)
        (call-interactively 'isearch-occur)
        (kill-current-buffer)))

then it fails with:

  Error in post-command-hook (isearch-post-command-hook): (error "Selecting deleted buffer")

All other uses of 'isearch--current-buffer' have a check
for a live buffer, e.g.

  (unless (eq (current-buffer) isearch--current-buffer)
    (when (buffer-live-p isearch--current-buffer)
      (with-current-buffer isearch--current-buffer

Re: Simple isearch concerns

[Gregory Heytings]

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


>
> When in your original test case '(pop-to-buffer "*Occur*")' is replaced 
> with '(kill-current-buffer)':
>
>  (define-key isearch-mode-map (kbd "M-s o")
>    #'(lambda ()
>        (interactive)
>        (call-interactively 'isearch-occur)
>        (kill-current-buffer)))
>
> then it fails with:
>
>  Error in post-command-hook (isearch-post-command-hook): (error "Selecting deleted buffer")
>
> All other uses of 'isearch--current-buffer' have a check for a live 
> buffer, e.g.
>
>  (unless (eq (current-buffer) isearch--current-buffer)
>    (when (buffer-live-p isearch--current-buffer)
>      (with-current-buffer isearch--current-buffer
>

Indeed, updated patch attached.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=0001-Terminate-isearch-when-point-has-moved-to-another-bu.patch, Size: 1110 bytes --]

From bffbde3ad05e677ca45af5c0f7d18c10181c061f Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Wed, 7 Apr 2021 16:58:51 +0000
Subject: [PATCH] Terminate isearch when point has moved to another buffer

* lisp/isearch.el (isearch-post-command-hook): Terminate isearch
when the command just executed has moved point to another buffer.
---
 lisp/isearch.el | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lisp/isearch.el b/lisp/isearch.el
index a828c569aa..2e76e08f54 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -3040,6 +3040,10 @@ See more for options in `search-exit-option'."
            (goto-char isearch-pre-move-point))
          (isearch-search-and-update)))
      (setq isearch-pre-move-point nil))
+  ;; Terminate the search if point has moved to another buffer.
+  (unless (eq isearch--current-buffer (current-buffer))
+    (when (buffer-live-p isearch--current-buffer)
+      (with-current-buffer isearch--current-buffer (isearch-exit))))
   (force-mode-line-update))
 
 (defun isearch-quote-char (&optional count)
-- 
2.30.2

Re: Simple isearch concerns

[Juri Linkov]

> Indeed, updated patch attached.
>
> diff --git a/lisp/isearch.el b/lisp/isearch.el
> index a828c569aa..2e76e08f54 100644
> --- a/lisp/isearch.el
> +++ b/lisp/isearch.el
> @@ -3040,6 +3040,10 @@ See more for options in `search-exit-option'."
>             (goto-char isearch-pre-move-point))
>           (isearch-search-and-update)))
>       (setq isearch-pre-move-point nil))
> +  ;; Terminate the search if point has moved to another buffer.
> +  (unless (eq isearch--current-buffer (current-buffer))
> +    (when (buffer-live-p isearch--current-buffer)
> +      (with-current-buffer isearch--current-buffer (isearch-exit))))

Thanks, now pushed.  Let's hope it doesn't break some other modes.

Re: Simple isearch concerns

[Zhiwei Chen]

> On Apr 9, 2021, at 3:08 AM, Juri Linkov <juri@linkov.net> wrote:
> 
>> Indeed, updated patch attached.
>> 
>> diff --git a/lisp/isearch.el b/lisp/isearch.el
>> index a828c569aa..2e76e08f54 100644
>> --- a/lisp/isearch.el
>> +++ b/lisp/isearch.el
>> @@ -3040,6 +3040,10 @@ See more for options in `search-exit-option'."
>>            (goto-char isearch-pre-move-point))
>>          (isearch-search-and-update)))
>>      (setq isearch-pre-move-point nil))
>> +  ;; Terminate the search if point has moved to another buffer.
>> +  (unless (eq isearch--current-buffer (current-buffer))
>> +    (when (buffer-live-p isearch--current-buffer)
>> +      (with-current-buffer isearch--current-buffer (isearch-exit))))
> 
> Thanks, now pushed.  Let's hope it doesn't break some other modes.


(defun display-buffer-select (buffer alist)
  (let ((window (display-buffer-below-selected buffer alist)))
    (when (window-live-p window)
      (select-window window))))

(setq display-buffer-alist '(("\\*Occur\\*" (display-buffer-select))))

It failed to work when buffer is auto selected via `display-buffer-alist’. When the point moves to the *Occur* buffer, typing “n”, “p” still sends “n”, “p” to isearch.

Re: Simple isearch concerns

[Juri Linkov]

> Actually this feature can cause inconvenience in some cases.
>
> `isearch-occur’ (bound to `M-s o’ in 'isearch-mode-map`) when `isearch’,
> even a buffer is created, `isearch’  doesn’t exit though.
>
> In vanilla Emacs, the cursor is in the original buffer. But if you use
> shackle or customize `display-buffer-alist’ directly, the cursor
> can be refocused to the new created one. Typing in the *Occur* buffer
> triggers `isearch-printing-char’ which is counterintuitive.

As discussed in bug#47894, the right solution for you would be to use
such customization that helps isearch-occur to exit isearch explicitly:

#+begin_src emacs-lisp
(advice-add 'isearch-occur :after
            (lambda (&rest _args)
              (isearch-done nil t)
              (isearch-clean-overlays)))
#+end_src

Re: Simple isearch concerns

[Zhiwei Chen]

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


On Apr 22, 2021, at 1:51 AM, Juri Linkov <juri@linkov.net<mailto:juri@linkov.net>> wrote:

Actually this feature can cause inconvenience in some cases.

`isearch-occur’ (bound to `M-s o’ in 'isearch-mode-map`) when `isearch’,
even a buffer is created, `isearch’  doesn’t exit though.

In vanilla Emacs, the cursor is in the original buffer. But if you use
shackle or customize `display-buffer-alist’ directly, the cursor
can be refocused to the new created one. Typing in the *Occur* buffer
triggers `isearch-printing-char’ which is counterintuitive.

As discussed in bug#47894, the right solution for you would be to use
such customization that helps isearch-occur to exit isearch explicitly:

#+begin_src emacs-lisp
(advice-add 'isearch-occur :after
           (lambda (&rest _args)
             (isearch-done nil t)
             (isearch-clean-overlays)))
#+end_src


tags 47894 fixed
close 47894 28.0.50
quit

>> Isearch help commands solve the second problem by simply using
>> isearch--display-help-action that inhibits displaying other buffers
>> in the same window.
>>
>> Instead of let-binding display-buffer-overriding-action
>> in all isearch help commands, we could set it temporarily
>> like we already temporarily set overriding-terminal-local-map:

> Actually, I withdraw my patches.  Better not to make any assumptions
> about possible ways how isearch is used because of existence of
> such complex cases like with input methods in this bug report
> (fixed and closed).

> By default, most commands that want to display a buffer
> in another window, exit isearch automatically,
> so there is no problem.  Only isearch help commands
> and isearch-occur don't exit isearch before displaying
> another buffer, thus they need to be treated individually.

isearch-occur is such a special command. Even there is no customization for
`display-buffer-alist', user feels that point is in the original buffer, but
typing "n" changes isearch string rather than insert in the original buffer.
Currently only those commands prefixed with C- and M- will cancel isearch. So I propose to
make `isearch-occur' cancel (exit?) isearch too.

--
Zhiwei Chen


[-- Attachment #2: Type: text/html, Size: 4034 bytes --]

Re: Simple isearch concerns

[Philip Kaludercic]

Manuel Uberti <manuel.uberti@inventati.org> writes:

> On 03/04/21 02:15, Ergus wrote:
>> Are there any WIP related?
>
> Along with the package mentioned by Thierry there is CTRLF[1].

I tried this package a few months ago, but still do not understand what
it does differently from isearch.

> [1] https://github.com/raxod502/ctrlf

-- 
	Philip K.

Re: Simple isearch concerns

[Gregory Heytings]

>> Along with the package mentioned by Thierry there is CTRLF[1].
>>
>> [1] https://github.com/raxod502/ctrlf
>
> I tried this package a few months ago, but still do not understand what 
> it does differently from isearch.
>

This is explained on [1].  I'm not sure I understand all arguments, but 
what I understood is:

"If you type any command that is not bound in Isearch, then you exit your 
search and run that command. This means editing your search query is 
awkward and unintuitive."  If you type, say, C-t during an isearch, 
isearch exits and transpose-chars is executed.

"Another issue is predictability. The behavior of C-g is hard to predict 
because it depends on not only what you have typed in what order, but also 
on the buffer contents and the state of the current search."  When isearch 
has found a match, C-g aborts isearch and returns to the point you were. 
But if isearch did not find a match, the first C-g removes the failing 
characters, and you have to type C-g again to abort isearch.

"The handling of wraparound exacerbates problems of predictability: the 
number of times you must type C-s to move to the next match is 
unpredictable"  If point is after the last occurrence of the string you 
isearch when you type C-s, a single C-s wraps around; otherwise you have 
to type C-s two times to wrap around.

"and the minibuffer prompt changes in several different and confusing ways 
during this process."  The prompt changes and can admittedly become a bit 
confusing, the longest one I'm aware of is the 61 character long "Failing 
overwrapped case-sensitive regexp I-search backward: ".

Re: Simple isearch concerns

[Philip Kaludercic]

Gregory Heytings <gregory@heytings.org> writes:

>>> Along with the package mentioned by Thierry there is CTRLF[1].
>>>
>>> [1] https://github.com/raxod502/ctrlf
>>
>> I tried this package a few months ago, but still do not understand
>> what it does differently from isearch.
>>
>
> This is explained on [1].  I'm not sure I understand all arguments,
> but what I understood is:

I too have read the README and have installed the package, but even
then, I cannot really notice any specific difference. Something *feels*
different, but I cannot pinpoint what it is.

I am probably used to isearch on some more intuitive than conscious
level to really say what the difference is.

-- 
	Philip K.

Re: Simple isearch concerns

[Gregory Heytings]

>>>> Along with the package mentioned by Thierry there is CTRLF[1].
>>>>
>>>> [1] https://github.com/raxod502/ctrlf
>>>
>>> I tried this package a few months ago, but still do not understand 
>>> what it does differently from isearch.
>>
>> This is explained on [1].  I'm not sure I understand all arguments, but 
>> what I understood is:
>
> I too have read the README and have installed the package, but even 
> then, I cannot really notice any specific difference. Something *feels* 
> different, but I cannot pinpoint what it is.
>

You may have seen that what followed the above sentence were some concrete 
examples of these differences.

Re: Simple isearch concerns

[Philip Kaludercic]

Gregory Heytings <gregory@heytings.org> writes:

>>>>> Along with the package mentioned by Thierry there is CTRLF[1].
>>>>>
>>>>> [1] https://github.com/raxod502/ctrlf
>>>>
>>>> I tried this package a few months ago, but still do not understand
>>>> what it does differently from isearch.
>>>
>>> This is explained on [1].  I'm not sure I understand all arguments,
>>> but what I understood is:
>>
>> I too have read the README and have installed the package, but even
>> then, I cannot really notice any specific difference. Something
>> *feels* different, but I cannot pinpoint what it is.
>>
>
> You may have seen that what followed the above sentence were some
> concrete examples of these differences.

I tried all of them, but without paying attention, I wouldn't notice if
I'm using isearch or ctrlf (except for the N/M overlay).

Either way, it is not that important.

-- 
	Philip K.

Re: Simple isearch concerns

[Dmitry Gutov]

On 03.04.2021 21:36, Philip Kaludercic wrote:
> I tried all of them, but without paying attention, I wouldn't notice if
> I'm using isearch or ctrlf (except for the N/M overlay).

ctrlf doesn't exit search when some unrelated command is used, and or 
doesn't warn before doing wraparound.

I'm too used to the current isearch behavior on these two fronts to 
switch, but otherwise ctrlf seemed solid.

Re: Simple isearch concerns

[Ergus]

On Sat, Apr 03, 2021 at 10:36:23PM +0300, Dmitry Gutov wrote:
>On 03.04.2021 21:36, Philip Kaludercic wrote:
>
>ctrlf doesn't exit search when some unrelated command is used, and or 
>doesn't warn before doing wraparound.
>
I thing it shouldn't be very hard to provide these behavior with
customs.

The wraparound is trivial to implement and don't exiting seems simple to
do too.

But IMO it is better if the isearch code is cleaned a bit before adding
more code there.

Re: Simple isearch concerns

[Juri Linkov]

>> ctrlf doesn't exit search when some unrelated command is used, and or
>> doesn't warn before doing wraparound.
>>
> I thing it shouldn't be very hard to provide these behavior with
> customs.
>
> The wraparound is trivial to implement and don't exiting seems simple to
> do too.

Not exiting was already implemented a long time ago: just customize
search-exit-option to nil.

> But IMO it is better if the isearch code is cleaned a bit before adding
> more code there.

Good luck rewriting isearch.el while keeping all its smallest features unchanged.
Last time when I slightly changed the behavior of C-y in isearch it caused
a thread with hundreds of messages.

Re: Simple isearch concerns

[Ergus]

Yes. IMO the difference with CTRF is minimal; probably nothing that
couldn't be implemented in isearch with a couple of custom variables and
bindings more or less without too much effort.

In any case; my initial question was more related with the fact that
isearch seems to be a bit outdated compared with the rest of the emacs
code. Needing a hook to go to the other end instead of having a simple
custom, or rebinding a variable that is not in customs to something
undocumented like #'ignore is not what a user expects from an editor at
least not in 200... sorry 2021.

Details like the defvars instead of customs or the lack of some basic
options, or the fact that it is not using any modern api like
define-minor-mode, easy-mmode-defmap, makes me think that maybe some
good lisper should make some cleanup there, simplify the code, convert
some defvars in documented customs and remove many redundant variables,
code and checks.

It is a very basic and important part of emacs, that every user will use
for sure and relying on external packages for something too basic is not
a good idea in my opinion (and a bad symptom).

I am not asking for any change in default behavior or complex extension,
or functionality. And I don't try to do this myself because a key
functionality like this should be in hands of the professionals that
know all the details.

Should this go into a bug report as a feature request?


On Sat, Apr 03, 2021 at 06:37:06PM +0200, Philip Kaludercic wrote:
>Gregory Heytings <gregory@heytings.org> writes:
>
>>>> Along with the package mentioned by Thierry there is CTRLF[1].
>>>>
>>>> [1] https://github.com/raxod502/ctrlf
>>>
>>> I tried this package a few months ago, but still do not understand
>>> what it does differently from isearch.
>>>
>>
>> This is explained on [1].  I'm not sure I understand all arguments,
>> but what I understood is:
>
>I too have read the README and have installed the package, but even
>then, I cannot really notice any specific difference. Something *feels*
>different, but I cannot pinpoint what it is.
>
>I am probably used to isearch on some more intuitive than conscious
>level to really say what the difference is.
>
>-- 
>	Philip K.
>

Re: Simple isearch concerns

[Augusto Stoffel]

On Sat,  3 Apr 2021 at 19:45, Ergus <spacibba@aol.com> wrote:

> Yes. IMO the difference with CTRF is minimal; probably nothing that
> couldn't be implemented in isearch with a couple of custom variables and
> bindings more or less without too much effort.

FWIW, I've worked on something like that a while ago:

https://github.com/astoff/isearch-mb/

It's a minor mode that makes Isearch read the search string from the
minibuffer instead of hijacking typed characters.  This makes it easier
to edit the search string and harder to quit the search.

It has some advantages over Ctrlf, such as not hanging on large buffers
and retaining the fancy wrapping of Info-mode.  Alas, it doesn't work
out of the box with every third-party Isearch extension (e.g.,
consult-isearch, loccur-isearch or pdf-tools).

If there's interest to bring something like isearch-mb into Emacs, I'd
suggest to integrate it into `isearch-edit-string'.  Thus, among other
things, `C-s' and `C-r' would search without quitting the minibuffer.

Re: Simple isearch concerns

[Juri Linkov]

> FWIW, I've worked on something like that a while ago:
>
> https://github.com/astoff/isearch-mb/
>
> It's a minor mode that makes Isearch read the search string from the
> minibuffer instead of hijacking typed characters.  This makes it easier
> to edit the search string and harder to quit the search.
>
> It has some advantages over Ctrlf, such as not hanging on large buffers
> and retaining the fancy wrapping of Info-mode.  Alas, it doesn't work
> out of the box with every third-party Isearch extension (e.g.,
> consult-isearch, loccur-isearch or pdf-tools).
>
> If there's interest to bring something like isearch-mb into Emacs, I'd
> suggest to integrate it into `isearch-edit-string'.  Thus, among other
> things, `C-s' and `C-r' would search without quitting the minibuffer.

Nice idea.  Like TAB typed in the minibuffer updates the Completions buffer,
C-s typed in the isearch-edit-string minibuffer could update the matches
in the searched buffer.

There is already the value 'edit' of 'search-exit-option' that can
activate the editing minibuffer, but what is missing is a way to
redirect C-s in the editing minibuffer back to the original buffer.

Re: Simple isearch concerns

[Augusto Stoffel]

On Fri, 23 Apr 2021 at 01:24, Juri Linkov <juri@linkov.net> wrote:

> Nice idea.  Like TAB typed in the minibuffer updates the Completions buffer,
> C-s typed in the isearch-edit-string minibuffer could update the matches
> in the searched buffer.

Do you think this feature could make it into isearch.el, or would make
more sense as an external package?  You can take a look at the git repo
linked in my previous message to get a sense of how the implementation
would look like.

In isearch.el itself, one would have to introduce a new macro
`(with-current-search-buffer ...)' and wrap the body of every relevant
interactive command in it (similar to the `isearch-mb--with-buffer'
advice in my package sketch).  Then there's also the case of commands
start or end a search, which need special treatment.

I'm not sure this can be made to work perfectly in every use case, or
not break some third party packages and user configurations.  It
certainly can be made to work well enough for those who find the normal
Isearch a bit too eccentric.

Re: Simple isearch concerns

[Juri Linkov]

>> Nice idea.  Like TAB typed in the minibuffer updates the Completions buffer,
>> C-s typed in the isearch-edit-string minibuffer could update the matches
>> in the searched buffer.
>
> Do you think this feature could make it into isearch.el, or would make
> more sense as an external package?  You can take a look at the git repo
> linked in my previous message to get a sense of how the implementation
> would look like.

If its implementation becomes more hackish, then it would make
more sense as an external package.  Otherwise, if we can find
an elegant way to naturally integrate it with isearch.el,
then this feature could make it into isearch.el.

As the first step in this direction, we could try to get rid of the
macro ‘with-isearch-suspended’.  It was a hackish solution to
temporarily exit isearch before reading a string from the minibuffer.

To be able to do the same in a nicer way, we need to turn isearch-mode
into buffer-local.  Its current global status caused too much trouble
in the past.  For example, see our recent attempt to forcefully terminate
isearch when it switched to another buffer.  With buffer-local isearch-mode,
there will be no need to exit isearch before switching to another buffer.
You could even have simultaneously active isearch modes in different buffers,
and freely switch to the minibuffer without exiting buffer-local isearch.

Is such thing achievable?  There is no problem to make hooks used by isearch
buffer-local by adding the arg LOCAL to add-hook:

  (add-hook 'pre-command-hook 'isearch-pre-command-hook nil t)
  (add-hook 'post-command-hook 'isearch-post-command-hook nil t)
  (add-hook 'mouse-leave-buffer-hook 'isearch-mouse-leave-buffer nil t)
  (add-hook 'kbd-macro-termination-hook 'isearch-done nil t)

But trying to make buffer-local overriding-terminal-local-map with:

  (setq-local overriding-terminal-local-map isearch-mode-map)

fails with an error:

  Debugger entered--Lisp error: (error "Symbol overriding-terminal-local-map may not be buffer-local")
    make-local-variable(overriding-terminal-local-map)
    (set (make-local-variable 'overriding-terminal-local-map) isearch-mode-map)
    isearch-mode(t nil nil nil)
    isearch-forward(nil 1)

> In isearch.el itself, one would have to introduce a new macro
> `(with-current-search-buffer ...)' and wrap the body of every relevant
> interactive command in it (similar to the `isearch-mb--with-buffer'
> advice in my package sketch).  Then there's also the case of commands
> start or end a search, which need special treatment.

This is similar to what currently does isearch-yank-char-in-minibuffer.
It would be nice to replace it with a new macro that will
redirect all normal isearch commands to the original buffer.

RE: [External] : Re: Simple isearch concerns

[Drew Adams]

> As the first step in this direction, we could try to get rid of the
> macro ‘with-isearch-suspended’.  It was a hackish solution to
> temporarily exit isearch before reading a string from the minibuffer.
> 
> To be able to do the same in a nicer way, we need to turn isearch-mode
> into buffer-local. ...
> With buffer-local isearch-mode, there will be no need to exit
> isearch before switching to another buffer.

Maybe not, but `with-isearch-suspended' is much more general
than that use case.  It's not just about switching (temporarily)
to another buffer.  In particular, it's not just about pausing
to read some input, though that's by far the most common use.

It's about pausing Isearch temporarily for _any_ purpose.
And (my version) provides a catch for throwing to end a
subsidiary activity.

I use it for lots of things, including (a) non-modifying
actions on a hook invoked after visiting and highlighting
each search hit (in `isearch-update') and (b) recursive edits.

Re: Simple isearch concerns

[Augusto Stoffel]

On Sun, 25 Apr 2021 at 20:24, Juri Linkov <juri@linkov.net> wrote:

> If its implementation becomes more hackish, then it would make
> more sense as an external package.  Otherwise, if we can find
> an elegant way to naturally integrate it with isearch.el,
> then this feature could make it into isearch.el.

This can be decided at any time, since an implementation exists.

It's not really that hacky.  But I don't know if it breaks some obscure
functionality or extension of Isearch.  However, contrary to my original
statement, it now works with all Isearch extensions I know/care about
(Embark, Consult, Loccur, Anzu's query-replace, pdf-tools), possibly
after applying an advice.

> But trying to make buffer-local overriding-terminal-local-map with:
>
>   (setq-local overriding-terminal-local-map isearch-mode-map)
>
> fails with an error:
>
>   Debugger entered--Lisp error: (error "Symbol overriding-terminal-local-map may not be buffer-local")
>     make-local-variable(overriding-terminal-local-map)
>     (set (make-local-variable 'overriding-terminal-local-map) isearch-mode-map)
>     isearch-mode(t nil nil nil)
>     isearch-forward(nil 1)

Is this overriding map there just so that `isearch-mode-map' takes
precedence over other minor mode maps?  If so, couldn't this be achieved
by manipulating minor-mode-map-alist to put Isearch's map at the top?
Maybe that can be done buffer-locally.

Re: Simple isearch concerns

[Juri Linkov]

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

>>   (setq-local overriding-terminal-local-map isearch-mode-map)
>>
>> fails with an error:
>>
>>   Debugger entered--Lisp error: (error "Symbol overriding-terminal-local-map may not be buffer-local")
>>     make-local-variable(overriding-terminal-local-map)
>>     (set (make-local-variable 'overriding-terminal-local-map) isearch-mode-map)
>>     isearch-mode(t nil nil nil)
>>     isearch-forward(nil 1)
>
> Is this overriding map there just so that `isearch-mode-map' takes
> precedence over other minor mode maps?  If so, couldn't this be achieved
> by manipulating minor-mode-map-alist to put Isearch's map at the top?
> Maybe that can be done buffer-locally.

According to (info "(elisp) Searching Keymaps") the next keymap to try is
`overriding-local-map`.  It can be set buffer-locally.  And indeed,
with the following patch you can set a few commands to not exit isearch:

  (put 'other-window 'isearch-scroll t)
  (put 'eval-expression 'isearch-scroll t)

Then switching to another buffer with 'C-x o', or switching
to the minibuffer with 'M-:' leaves the local isearch active
in the original buffer.  After switching back to the original buffer,
or after exiting the minibuffer, isearch is still active
and can be used to continue searching.

Later this could be handled depending on a new option 'isearch-buffer-local':


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

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 9f3cfd70fb..11c2798b76 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -1295,11 +1295,11 @@ isearch-mode
   (setq	isearch-mode " Isearch")  ;; forward? regexp?
   (force-mode-line-update)
 
-  (setq overriding-terminal-local-map isearch-mode-map)
+  (setq-local overriding-local-map isearch-mode-map)
   (run-hooks 'isearch-mode-hook)
   ;; Remember the initial map possibly modified
   ;; by external packages in isearch-mode-hook.  (Bug#16035)
-  (setq isearch--saved-overriding-local-map overriding-terminal-local-map)
+  (setq isearch--saved-overriding-local-map overriding-local-map)
 
   ;; Pushing the initial state used to be before running isearch-mode-hook,
   ;; but a hook might set `isearch-push-state-function' used in
@@ -1308,10 +1308,10 @@ isearch-mode
 
   (isearch-update)
 
-  (add-hook 'pre-command-hook 'isearch-pre-command-hook)
-  (add-hook 'post-command-hook 'isearch-post-command-hook)
-  (add-hook 'mouse-leave-buffer-hook 'isearch-mouse-leave-buffer)
-  (add-hook 'kbd-macro-termination-hook 'isearch-done)
+  (add-hook 'pre-command-hook 'isearch-pre-command-hook nil t)
+  (add-hook 'post-command-hook 'isearch-post-command-hook nil t)
+  (add-hook 'mouse-leave-buffer-hook 'isearch-mouse-leave-buffer nil t)
+  (add-hook 'kbd-macro-termination-hook 'isearch-done nil t)
 
   ;; isearch-mode can be made modal (in the sense of not returning to
   ;; the calling function until searching is completed) by entering
@@ -1406,10 +1406,10 @@ isearch-done
                                      ,isearch-message
                                      ',isearch-case-fold-search)))
 
-  (remove-hook 'pre-command-hook 'isearch-pre-command-hook)
-  (remove-hook 'post-command-hook 'isearch-post-command-hook)
-  (remove-hook 'mouse-leave-buffer-hook 'isearch-mouse-leave-buffer)
-  (remove-hook 'kbd-macro-termination-hook 'isearch-done)
+  (remove-hook 'pre-command-hook 'isearch-pre-command-hook t)
+  (remove-hook 'post-command-hook 'isearch-post-command-hook t)
+  (remove-hook 'mouse-leave-buffer-hook 'isearch-mouse-leave-buffer t)
+  (remove-hook 'kbd-macro-termination-hook 'isearch-done t)
   (when (buffer-live-p isearch--current-buffer)
     (with-current-buffer isearch--current-buffer
       (setq isearch--current-buffer nil)
@@ -1417,7 +1417,7 @@ isearch-done
 
   ;; Called by all commands that terminate isearch-mode.
   ;; If NOPUSH is non-nil, we don't push the string on the search ring.
-  (setq overriding-terminal-local-map nil)
+  (setq overriding-local-map nil)
   ;; (setq pre-command-hook isearch-old-pre-command-hook) ; for lemacs
   (setq minibuffer-message-timeout isearch-original-minibuffer-message-timeout)
   (isearch-dehighlight)
@@ -2599,7 +2599,7 @@ isearch-mouse-2
 is bound to outside of Isearch."
   (interactive "e")
   (let ((w (posn-window (event-start click)))
-        (binding (let ((overriding-terminal-local-map nil)
+        (binding (let ((overriding-local-map nil)
                        ;; Key search depends on mode (bug#47755)
                        (isearch-mode nil))
                    (key-binding (this-command-keys-vector) t))))
@@ -3018,7 +3018,7 @@ isearch-pre-command-hook
     (cond
      ;; Don't exit Isearch if we're in the middle of some
      ;; `set-transient-map' thingy like `universal-argument--mode'.
-     ((not (eq overriding-terminal-local-map isearch--saved-overriding-local-map)))
+     ((not (eq overriding-local-map isearch--saved-overriding-local-map)))
      ;; Don't exit Isearch for isearch key bindings.
      ((or (commandp (lookup-key isearch-mode-map key nil))
           (commandp

Re: Simple isearch concerns

[Juri Linkov]

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

>> Is this overriding map there just so that `isearch-mode-map' takes
>> precedence over other minor mode maps?  If so, couldn't this be achieved
>> by manipulating minor-mode-map-alist to put Isearch's map at the top?
>> Maybe that can be done buffer-locally.
>
> According to (info "(elisp) Searching Keymaps") the next keymap to try is
> `overriding-local-map`.  It can be set buffer-locally.

Actually, overriding-local-map can't be used, because it prioritizes
global-map over local maps, so e.g. in Dired typing C-o exits isearch
and calls open-line instead of dired-display-file.

According to (info "(elisp) Searching Keymaps"), the next available keymap is
emulation-mode-map-alists.  The following patch added a single line:

  (add-to-list 'emulation-mode-map-alists `((isearch-mode . ,isearch-mode-map)))

then removed all mentions of overriding-terminal-local-map,
removed all 156 lines of the monstrous macro with-isearch-suspended,
added a single line to the end of isearch-edit-string:

  (isearch-search-and-update)

and then M-e edits the search string without exiting isearch.  And it's
trivial to use isearch commands without exiting the minibuffer with e.g.

  (with-minibuffer-selected-window (isearch-repeat-forward))

And everything works fine.  It's too good to be true,
but I don't see where's the catch.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: isearch-emulation-mode-map-alists.patch --]
[-- Type: text/x-diff, Size: 7674 bytes --]

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 9f3cfd70fb..bbf091fd0e 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -668,6 +668,8 @@ isearch-mode-map
     map)
   "Keymap for `isearch-mode'.")
 
+(add-to-list 'emulation-mode-map-alists `((isearch-mode . ,isearch-mode-map)))
+
 (easy-menu-define isearch-menu-bar-map  isearch-mode-map
   "Menu for `isearch-mode'."
   '("Isearch"
@@ -952,8 +954,6 @@ isearch-hidden
 ;; The value of input-method-function when isearch is invoked.
 (defvar isearch-input-method-function nil)
 
-(defvar isearch--saved-overriding-local-map nil)
-
 ;; Minor-mode-alist changes - kind of redundant with the
 ;; echo area, but if isearching in multiple windows, it can be useful.
 ;; Also, clicking the mode-line indicator pops up
@@ -1295,11 +1295,7 @@ isearch-mode
   (setq	isearch-mode " Isearch")  ;; forward? regexp?
   (force-mode-line-update)
 
-  (setq overriding-terminal-local-map isearch-mode-map)
   (run-hooks 'isearch-mode-hook)
-  ;; Remember the initial map possibly modified
-  ;; by external packages in isearch-mode-hook.  (Bug#16035)
-  (setq isearch--saved-overriding-local-map overriding-terminal-local-map)
 
   ;; Pushing the initial state used to be before running isearch-mode-hook,
   ;; but a hook might set `isearch-push-state-function' used in
@@ -1308,10 +1304,10 @@ isearch-mode
 
   (isearch-update)
 
-  (add-hook 'pre-command-hook 'isearch-pre-command-hook)
-  (add-hook 'post-command-hook 'isearch-post-command-hook)
-  (add-hook 'mouse-leave-buffer-hook 'isearch-mouse-leave-buffer)
-  (add-hook 'kbd-macro-termination-hook 'isearch-done)
+  (add-hook 'pre-command-hook 'isearch-pre-command-hook nil t)
+  (add-hook 'post-command-hook 'isearch-post-command-hook nil t)
+  (add-hook 'mouse-leave-buffer-hook 'isearch-mouse-leave-buffer nil t)
+  (add-hook 'kbd-macro-termination-hook 'isearch-done nil t)
 
   ;; isearch-mode can be made modal (in the sense of not returning to
   ;; the calling function until searching is completed) by entering
@@ -1406,10 +1402,11 @@ isearch-done
                                      ,isearch-message
                                      ',isearch-case-fold-search)))
 
-  (remove-hook 'pre-command-hook 'isearch-pre-command-hook)
-  (remove-hook 'post-command-hook 'isearch-post-command-hook)
-  (remove-hook 'mouse-leave-buffer-hook 'isearch-mouse-leave-buffer)
-  (remove-hook 'kbd-macro-termination-hook 'isearch-done)
+  (remove-hook 'pre-command-hook 'isearch-pre-command-hook t)
+  (remove-hook 'post-command-hook 'isearch-post-command-hook t)
+  (remove-hook 'mouse-leave-buffer-hook 'isearch-mouse-leave-buffer t)
+  (remove-hook 'kbd-macro-termination-hook 'isearch-done t)
+
   (when (buffer-live-p isearch--current-buffer)
     (with-current-buffer isearch--current-buffer
       (setq isearch--current-buffer nil)
@@ -1417,7 +1414,6 @@ isearch-done
 
   ;; Called by all commands that terminate isearch-mode.
   ;; If NOPUSH is non-nil, we don't push the string on the search ring.
-  (setq overriding-terminal-local-map nil)
   ;; (setq pre-command-hook isearch-old-pre-command-hook) ; for lemacs
   (setq minibuffer-message-timeout isearch-original-minibuffer-message-timeout)
   (isearch-dehighlight)
@@ -1792,32 +1788,32 @@ isearch-edit-string
 \\[isearch-reverse-exit-minibuffer] to resume isearching backward.
 \\[isearch-complete-edit] to complete the search string using the search ring."
   (interactive)
-  (with-isearch-suspended
-   (let* ((message-log-max nil)
-	  ;; Don't add a new search string to the search ring here
-	  ;; in `read-from-minibuffer'. It should be added only
-	  ;; by `isearch-update-ring' called from `isearch-done'.
-	  (history-add-new-input nil)
-	  ;; Binding minibuffer-history-symbol to nil is a work-around
-	  ;; for some incompatibility with gmhist.
-	  (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)
-	    (cons isearch-string (1+ (or (isearch-fail-pos)
-					 (length isearch-string))))
-	    minibuffer-local-isearch-map nil
-	    (if isearch-regexp
-		(cons 'regexp-search-ring
-		      (1+ (or regexp-search-ring-yank-pointer -1)))
-	      (cons 'search-ring
-		    (1+ (or search-ring-yank-pointer -1))))
-	    nil t)
-	   isearch-new-message
-	   (mapconcat 'isearch-text-char-description
-		      isearch-new-string "")))))
+  (let* ((message-log-max nil)
+	 ;; Don't add a new search string to the search ring here
+	 ;; in `read-from-minibuffer'. It should be added only
+	 ;; by `isearch-update-ring' called from `isearch-done'.
+	 (history-add-new-input nil)
+	 ;; Binding minibuffer-history-symbol to nil is a work-around
+	 ;; for some incompatibility with gmhist.
+	 (minibuffer-history-symbol)
+	 ;; Search string might have meta information on text properties.
+	 (minibuffer-allow-text-properties t))
+    (setq isearch-string
+	  (read-from-minibuffer
+	   (isearch-message-prefix nil isearch-nonincremental)
+	   (cons isearch-string (1+ (or (isearch-fail-pos)
+					(length isearch-string))))
+	   minibuffer-local-isearch-map nil
+	   (if isearch-regexp
+	       (cons 'regexp-search-ring
+		     (1+ (or regexp-search-ring-yank-pointer -1)))
+	     (cons 'search-ring
+		   (1+ (or search-ring-yank-pointer -1))))
+	   nil t)
+	  isearch-message
+	  (mapconcat 'isearch-text-char-description
+		     isearch-string ""))
+    (isearch-search-and-update)))
 
 (defun isearch-nonincremental-exit-minibuffer ()
   (interactive)
@@ -2599,7 +2595,7 @@ isearch-mouse-2
 is bound to outside of Isearch."
   (interactive "e")
   (let ((w (posn-window (event-start click)))
-        (binding (let ((overriding-terminal-local-map nil)
+        (binding (let (
                        ;; Key search depends on mode (bug#47755)
                        (isearch-mode nil))
                    (key-binding (this-command-keys-vector) t))))
@@ -2715,16 +2711,16 @@ isearch-char-by-name
 Completion is available like in `read-char-by-name' used by `insert-char'.
 With argument, add COUNT copies of the character."
   (interactive "p")
-  (with-isearch-suspended
-   (let ((char (read-char-by-name "Add character to search (Unicode name or hex): ")))
-     (when char
-       (let ((string (if (and (integerp count) (> count 1))
-			 (make-string count char)
-		       (char-to-string char))))
-	 (setq isearch-new-string (concat isearch-string string)
-	       isearch-new-message (concat isearch-message
-					   (mapconcat 'isearch-text-char-description
-						      string ""))))))))
+  (let ((char (read-char-by-name "Add character to search (Unicode name or hex): ")))
+    (when char
+      (let ((string (if (and (integerp count) (> count 1))
+			(make-string count char)
+		      (char-to-string char))))
+	(setq isearch-string (concat isearch-string string)
+	      isearch-message (concat isearch-message
+				      (mapconcat 'isearch-text-char-description
+						 string ""))))))
+  (isearch-search-and-update))
 
 (defun isearch-search-and-update ()
   "Do the search and update the display."
@@ -3016,9 +3012,6 @@ isearch-pre-command-hook
   (let* ((key (this-single-command-keys))
 	 (main-event (aref key 0)))
     (cond
-     ;; Don't exit Isearch if we're in the middle of some
-     ;; `set-transient-map' thingy like `universal-argument--mode'.
-     ((not (eq overriding-terminal-local-map isearch--saved-overriding-local-map)))
      ;; Don't exit Isearch for isearch key bindings.
      ((or (commandp (lookup-key isearch-mode-map key nil))
           (commandp

Re: Simple isearch concerns

[Augusto Stoffel]

On Thu, 29 Apr 2021 at 19:29, Juri Linkov <juri@linkov.net> wrote:

>>> Is this overriding map there just so that `isearch-mode-map' takes
>>> precedence over other minor mode maps?  If so, couldn't this be achieved
>>> by manipulating minor-mode-map-alist to put Isearch's map at the top?
>>> Maybe that can be done buffer-locally.
>>
>> According to (info "(elisp) Searching Keymaps") the next keymap to try is
>> `overriding-local-map`.  It can be set buffer-locally.
>
> Actually, overriding-local-map can't be used, because it prioritizes
> global-map over local maps, so e.g. in Dired typing C-o exits isearch
> and calls open-line instead of dired-display-file.
>
> According to (info "(elisp) Searching Keymaps"), the next available keymap is
> emulation-mode-map-alists.  The following patch added a single line:
>
>   (add-to-list 'emulation-mode-map-alists `((isearch-mode . ,isearch-mode-map)))
>
> then removed all mentions of overriding-terminal-local-map,
> removed all 156 lines of the monstrous macro with-isearch-suspended,
> added a single line to the end of isearch-edit-string:
>
>   (isearch-search-and-update)
>
> and then M-e edits the search string without exiting isearch.  And it's
> trivial to use isearch commands without exiting the minibuffer with e.g.
>
>   (with-minibuffer-selected-window (isearch-repeat-forward))
>
> And everything works fine.  It's too good to be true,
> but I don't see where's the catch.
>

This seems promising, and actually not too different from the experiment
I mentioned a few messages back: https://github.com/astoff/isearch-mb/

I've been using it for some time without problems.

Re: Simple isearch concerns

[Juri Linkov]

>> Actually, overriding-local-map can't be used, because it prioritizes
>> global-map over local maps, so e.g. in Dired typing C-o exits isearch
>> and calls open-line instead of dired-display-file.
>>
>> According to (info "(elisp) Searching Keymaps"), the next available keymap is
>> emulation-mode-map-alists.  The following patch added a single line:
>>
>>   (add-to-list 'emulation-mode-map-alists `((isearch-mode . ,isearch-mode-map)))
>>
>> then removed all mentions of overriding-terminal-local-map,
>> removed all 156 lines of the monstrous macro with-isearch-suspended,
>> added a single line to the end of isearch-edit-string:
>>
>>   (isearch-search-and-update)
>>
>> and then M-e edits the search string without exiting isearch.  And it's
>> trivial to use isearch commands without exiting the minibuffer with e.g.
>>
>>   (with-minibuffer-selected-window (isearch-repeat-forward))
>>
>> And everything works fine.  It's too good to be true,
>> but I don't see where's the catch.
>
> This seems promising, and actually not too different from the experiment
> I mentioned a few messages back: https://github.com/astoff/isearch-mb/
>
> I've been using it for some time without problems.

Thanks for the experiment that demonstrates how it's possible
to implement this feature for all Emacs versions.

Now the question is whether it's worth to have this feature
in isearch.el where adding it will allow simplifications.

What would be good customization options for that?  I imagine
a new boolean option 'isearch-buffer-local'.  Maybe later
it would require a new value to support even multiple isearch
sessions in different buffers.  In this case, all state variables
like isearch-cmds, isearch-lazy-highlight-overlays etc. could be made
buffer-local.

Re: Simple isearch concerns

[Augusto Stoffel]

> Thanks for the experiment that demonstrates how it's possible
> to implement this feature for all Emacs versions.
>
> Now the question is whether it's worth to have this feature
> in isearch.el where adding it will allow simplifications.

Agreed.  I don't have a formed opinion yet.  If it's decided against
changing isearch.el in this direction, I would like to submit isearch-mb
as an ELPA package.

> What would be good customization options for that?  I imagine
> a new boolean option 'isearch-buffer-local'.  Maybe later
> it would require a new value to support even multiple isearch
> sessions in different buffers.  In this case, all state variables
> like isearch-cmds, isearch-lazy-highlight-overlays etc. could be made
> buffer-local.

I don't understand why `isearch-buffer-local' should be a user
customization, since it doesn't change the external behavior of Isearch,
but simply improves the implementation.  It might break third-party code
that hacks into isearch.el, though.

Re: Simple isearch concerns

[Juri Linkov]

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

>> What would be good customization options for that?  I imagine
>> a new boolean option 'isearch-buffer-local'.  Maybe later
>> it would require a new value to support even multiple isearch
>> sessions in different buffers.  In this case, all state variables
>> like isearch-cmds, isearch-lazy-highlight-overlays etc. could be made
>> buffer-local.
>
> I don't understand why `isearch-buffer-local' should be a user
> customization, since it doesn't change the external behavior of Isearch,
> but simply improves the implementation.  It might break third-party code
> that hacks into isearch.el, though.

Indeed, we need to care about third-party code as well.
So we could add a new option, then ask the users to enable it
to see what effect it has on user customizations, then after
the next release its default could be changed to t:


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

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 9f3cfd70fb..5c71519054 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -195,6 +195,11 @@ isearch-repeat-on-direction-change
                  (const :tag "Move to another match" t))
   :version "28.1")
 
+(defcustom isearch-buffer-local nil
+  "Whether isearch should be buffer-local."
+  :type 'boolean
+  :version "28.1")
+
 (defvar isearch-mode-hook nil
   "Function(s) to call after starting up an incremental search.")
 
@@ -1295,11 +1300,14 @@ isearch-mode
   (setq	isearch-mode " Isearch")  ;; forward? regexp?
   (force-mode-line-update)
 
-  (setq overriding-terminal-local-map isearch-mode-map)
+  (if isearch-buffer-local
+      (add-to-list 'emulation-mode-map-alists `((isearch-mode . ,isearch-mode-map)))
+    (setq overriding-terminal-local-map isearch-mode-map))
   (run-hooks 'isearch-mode-hook)
-  ;; Remember the initial map possibly modified
-  ;; by external packages in isearch-mode-hook.  (Bug#16035)
-  (setq isearch--saved-overriding-local-map overriding-terminal-local-map)
+  (unless isearch-buffer-local
+    ;; Remember the initial map possibly modified
+    ;; by external packages in isearch-mode-hook.  (Bug#16035)
+    (setq isearch--saved-overriding-local-map overriding-terminal-local-map))
 
   ;; Pushing the initial state used to be before running isearch-mode-hook,
   ;; but a hook might set `isearch-push-state-function' used in
@@ -1308,10 +1316,10 @@ isearch-mode
 
   (isearch-update)
 
-  (add-hook 'pre-command-hook 'isearch-pre-command-hook)
-  (add-hook 'post-command-hook 'isearch-post-command-hook)
-  (add-hook 'mouse-leave-buffer-hook 'isearch-mouse-leave-buffer)
-  (add-hook 'kbd-macro-termination-hook 'isearch-done)
+  (add-hook 'pre-command-hook 'isearch-pre-command-hook nil isearch-buffer-local)
+  (add-hook 'post-command-hook 'isearch-post-command-hook nil isearch-buffer-local)
+  (add-hook 'mouse-leave-buffer-hook 'isearch-mouse-leave-buffer nil isearch-buffer-local)
+  (add-hook 'kbd-macro-termination-hook 'isearch-done nil isearch-buffer-local)
 
   ;; isearch-mode can be made modal (in the sense of not returning to
   ;; the calling function until searching is completed) by entering
@@ -1406,10 +1414,11 @@ isearch-done
                                      ,isearch-message
                                      ',isearch-case-fold-search)))
 
-  (remove-hook 'pre-command-hook 'isearch-pre-command-hook)
-  (remove-hook 'post-command-hook 'isearch-post-command-hook)
-  (remove-hook 'mouse-leave-buffer-hook 'isearch-mouse-leave-buffer)
-  (remove-hook 'kbd-macro-termination-hook 'isearch-done)
+  (remove-hook 'pre-command-hook 'isearch-pre-command-hook isearch-buffer-local)
+  (remove-hook 'post-command-hook 'isearch-post-command-hook isearch-buffer-local)
+  (remove-hook 'mouse-leave-buffer-hook 'isearch-mouse-leave-buffer isearch-buffer-local)
+  (remove-hook 'kbd-macro-termination-hook 'isearch-done isearch-buffer-local)
+
   (when (buffer-live-p isearch--current-buffer)
     (with-current-buffer isearch--current-buffer
       (setq isearch--current-buffer nil)
@@ -1630,12 +1639,19 @@ with-isearch-suspended
 `isearch-new-string', `isearch-new-message', `isearch-new-forward',
 `isearch-new-regexp-function', `isearch-new-case-fold',
 `isearch-new-nonincremental'."
+  `(if isearch-buffer-local
+       (let ((isearch-new-string isearch-string)
+             (isearch-new-message isearch-message))
+         (progn ,@body)
+         (setq isearch-string isearch-new-string
+               isearch-message isearch-new-message)
+         (let ((isearch-yank-flag t)) (isearch-search-and-update)))
   ;; This code is very hairy for several reasons, explained in the code.
   ;; Mainly, isearch-mode must be terminated while editing and then restarted.
   ;; If there were a way to catch any change of buffer from the minibuffer,
   ;; this could be simplified greatly.
   ;; Editing doesn't back up the search point.  Should it?
-  `(condition-case nil
+   (condition-case nil
       (progn
 	(let ((isearch-new-nonincremental isearch-nonincremental)
 
@@ -1779,7 +1795,7 @@ with-isearch-suspended
     (quit  ; handle abort-recursive-edit
      (setq isearch-suspended nil)
      (isearch-abort)  ;; outside of let to restore outside global values
-     )))
+     ))))
 
 (defvar minibuffer-history-symbol) ;; from external package gmhist.el
 
@@ -3018,7 +3060,8 @@ isearch-pre-command-hook
     (cond
      ;; Don't exit Isearch if we're in the middle of some
      ;; `set-transient-map' thingy like `universal-argument--mode'.
-     ((not (eq overriding-terminal-local-map isearch--saved-overriding-local-map)))
+     ((unless isearch-buffer-local
+        (not (eq overriding-terminal-local-map isearch--saved-overriding-local-map))))
      ;; Don't exit Isearch for isearch key bindings.
      ((or (commandp (lookup-key isearch-mode-map key nil))
           (commandp

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


And here is a patch for updating the search from the minibuffer. This allows
to implement https://lists.gnu.org/archive/html/emacs-devel/2020-01/msg00447.html
to control the search from the minibuffer.  But I'm not sure if the same option
isearch-buffer-local should enable this mode:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: isearch-edit-string-after-change.patch --]
[-- Type: text/x-diff, Size: 3610 bytes --]

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 9f3cfd70fb..5c71519054 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -1803,21 +1819,33 @@ isearch-edit-string
 	  (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)
-	    (cons isearch-string (1+ (or (isearch-fail-pos)
-					 (length isearch-string))))
-	    minibuffer-local-isearch-map nil
-	    (if isearch-regexp
-		(cons 'regexp-search-ring
-		      (1+ (or regexp-search-ring-yank-pointer -1)))
-	      (cons 'search-ring
-		    (1+ (or search-ring-yank-pointer -1))))
-	    nil t)
-	   isearch-new-message
-	   (mapconcat 'isearch-text-char-description
-		      isearch-new-string "")))))
+     (minibuffer-with-setup-hook
+         (lambda ()
+           (when isearch-buffer-local
+             (add-hook 'after-change-functions
+                       (lambda (_ _ _)
+                         (let ((new-string (minibuffer-contents)))
+                           (with-minibuffer-selected-window
+                             (setq isearch-string new-string
+                                   isearch-message (mapconcat 'isearch-text-char-description
+		                                              isearch-string ""))
+                             (let ((isearch-yank-flag t)) (isearch-search-and-update)))))
+                       nil t)))
+       (setq isearch-new-string
+	     (read-from-minibuffer
+	      (isearch-message-prefix nil isearch-nonincremental)
+	      (cons isearch-string (1+ (or (isearch-fail-pos)
+					   (length isearch-string))))
+	      minibuffer-local-isearch-map nil
+	      (if isearch-regexp
+		  (cons 'regexp-search-ring
+		        (1+ (or regexp-search-ring-yank-pointer -1)))
+	        (cons 'search-ring
+		      (1+ (or search-ring-yank-pointer -1))))
+	      nil t)
+	     isearch-new-message
+	     (mapconcat 'isearch-text-char-description
+		        isearch-new-string ""))))))
 
 (defun isearch-nonincremental-exit-minibuffer ()
   (interactive)
@@ -1830,14 +1858,28 @@ isearch-nonincremental-exit-minibuffer
 (defun isearch-forward-exit-minibuffer ()
   "Resume isearching forward from the minibuffer that edits the search string."
   (interactive)
-  (setq isearch-new-forward t isearch-new-nonincremental nil)
-  (exit-minibuffer))
+  (if isearch-buffer-local
+      (let ((new-string (minibuffer-contents)))
+        (with-minibuffer-selected-window
+          (setq isearch-string new-string
+                isearch-message (mapconcat 'isearch-text-char-description
+		                           isearch-string ""))
+          (isearch-repeat-forward)))
+    (setq isearch-new-forward t isearch-new-nonincremental nil)
+    (exit-minibuffer)))
 
 (defun isearch-reverse-exit-minibuffer ()
   "Resume isearching backward from the minibuffer that edits the search string."
   (interactive)
-  (setq isearch-new-forward nil isearch-new-nonincremental nil)
-  (exit-minibuffer))
+  (if isearch-buffer-local
+      (let ((new-string (minibuffer-contents)))
+        (with-minibuffer-selected-window
+          (setq isearch-string new-string
+                isearch-message (mapconcat 'isearch-text-char-description
+		                           isearch-string ""))
+          (isearch-repeat-backward)))
+    (setq isearch-new-forward nil isearch-new-nonincremental nil)
+    (exit-minibuffer)))
 
 (defun isearch-cancel ()
   "Terminate the search and go back to the starting point."

Re: Simple isearch concerns

[Augusto Stoffel]

Hi Juri,

I tested your patch a bit and it seem to work well.  I have a few
comments below.

1. Since you probably want to remove the ugly `with-isearch-suspended'
   eventually, the local mode will not only be on by default, it will be
   the only alternative.  Should then `isearch-buffer-local' be
   double-dashed, so it too can be removed?

2. In edit mode, when I delete a character, the search doesn't backtrack
   to the barrier.  It's the same undesirable behavior described in
   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=46469.  In isearch-mb,
   the post-command-hook contains

       (goto-char isearch-barrier)
       (setq isearch-adjusted t isearch-success t)

   for to fix that.

3. The after change hook should call `isearch-update' instead of
   `isearch-search-and-update' after `previous-history-element' and few
   other commands.  In isearch-mb I use a symbol property to keep track
   of this.

4. Every time one types a character in the minibuffer, there is a
   visible flicker in the cursor.  In isearch-mb I've let-bound
   `inhibit-redisplay' to t in a couple of places and it solved this
   problem.  I wonder if there's a better solution.

5. The command `isearch-forward-exit-minibuffer' should eventually be
   removed, and instead `isearch-repeat-forward' should work in both
   cases (this will probably be implemented with the help of a new
   macro).  The reason for this is that there is at least a dozen other
   Isearch commands that make sense in the minibuffer map as well
   (toggling modes, quitting and entering query-replace, and so on), and
   it would be annoying to write a minibuffer variant for each of them.

6. Why is there the option to specify an `isearch-new-message' in
   `with-isearch-suspended'?  I've never seen any valid value other than
   `(mapconcat 'isearch-text-char-description isearch-string "")', which,
   by the way, repeats at several places in isearch.el

On Fri, 30 Apr 2021 at 19:41, Juri Linkov <juri@linkov.net> wrote:

> And here is a patch for updating the search from the minibuffer. This allows
> to implement https://lists.gnu.org/archive/html/emacs-devel/2020-01/msg00447.html
> to control the search from the minibuffer.  But I'm not sure if the same option
> isearch-buffer-local should enable this mode:

I still don't understand why Isearch has to be so stringent about
backward compatibility of key sequences.  But I don't care about the
edit mode per se.  I only care about an alternative mode where `C-s'
immediately goes to the minibuffer, and exiting the minibuffer ends the
search.

Re: Simple isearch concerns

[Juri Linkov]

> 1. Since you probably want to remove the ugly `with-isearch-suspended'
>    eventually, the local mode will not only be on by default, it will be
>    the only alternative.  Should then `isearch-buffer-local' be
>    double-dashed, so it too can be removed?

It could be removed in later releases, indeed.  But OTOH, until
it's removed, naming a user option with double-dashes is not allowed.

> 2. In edit mode, when I delete a character, the search doesn't backtrack
>    to the barrier.  It's the same undesirable behavior described in
>    https://debbugs.gnu.org/cgi/bugreport.cgi?bug=46469.  In isearch-mb,
>    the post-command-hook contains
>
>        (goto-char isearch-barrier)
>        (setq isearch-adjusted t isearch-success t)
>
>    for to fix that.

Thanks, this should be added to the patch.

> 3. The after change hook should call `isearch-update' instead of
>    `isearch-search-and-update' after `previous-history-element' and few
>    other commands.  In isearch-mb I use a symbol property to keep track
>    of this.

Thanks, it seems many solutions could be taken from isearch-mb to isearch.el.

> 4. Every time one types a character in the minibuffer, there is a
>    visible flicker in the cursor.  In isearch-mb I've let-bound
>    `inhibit-redisplay' to t in a couple of places and it solved this
>    problem.  I wonder if there's a better solution.

Maybe a flicker is caused by isearch-message?

> 5. The command `isearch-forward-exit-minibuffer' should eventually be
>    removed, and instead `isearch-repeat-forward' should work in both
>    cases (this will probably be implemented with the help of a new
>    macro).  The reason for this is that there is at least a dozen other
>    Isearch commands that make sense in the minibuffer map as well
>    (toggling modes, quitting and entering query-replace, and so on), and
>    it would be annoying to write a minibuffer variant for each of them.

This is the part where I don't yet have an idea how to handle
all isearch commands from the minibuffer without using a macro
to implement their duplicates to be callable from the minibuffer.

> 6. Why is there the option to specify an `isearch-new-message' in
>    `with-isearch-suspended'?  I've never seen any valid value other than
>    `(mapconcat 'isearch-text-char-description isearch-string "")', which,
>    by the way, repeats at several places in isearch.el

This is just to handle existing uses of with-isearch-suspended that
currently set isearch-new-message, i.e. this could be changed later.

>> And here is a patch for updating the search from the minibuffer. This allows
>> to implement https://lists.gnu.org/archive/html/emacs-devel/2020-01/msg00447.html
>> to control the search from the minibuffer.  But I'm not sure if the same option
>> isearch-buffer-local should enable this mode:
>
> I still don't understand why Isearch has to be so stringent about
> backward compatibility of key sequences.  But I don't care about the
> edit mode per se.  I only care about an alternative mode where `C-s'
> immediately goes to the minibuffer, and exiting the minibuffer ends the
> search.

In such a new mode isearch-edit-string could be called at the end of
isearch-mode.

Re: Simple isearch concerns

[Augusto Stoffel]

On Mon,  3 May 2021 at 01:18, Juri Linkov <juri@linkov.net> wrote:

>> 1. Since you probably want to remove the ugly `with-isearch-suspended'
>>    eventually, the local mode will not only be on by default, it will be
>>    the only alternative.  Should then `isearch-buffer-local' be
>>    double-dashed, so it too can be removed?
>
> It could be removed in later releases, indeed.  But OTOH, until
> it's removed, naming a user option with double-dashes is not allowed.
>
>> 2. In edit mode, when I delete a character, the search doesn't backtrack
>>    to the barrier.  It's the same undesirable behavior described in
>>    https://debbugs.gnu.org/cgi/bugreport.cgi?bug=46469.  In isearch-mb,
>>    the post-command-hook contains
>>
>>        (goto-char isearch-barrier)
>>        (setq isearch-adjusted t isearch-success t)
>>
>>    for to fix that.
>
> Thanks, this should be added to the patch.
>
>> 3. The after change hook should call `isearch-update' instead of
>>    `isearch-search-and-update' after `previous-history-element' and few
>>    other commands.  In isearch-mb I use a symbol property to keep track
>>    of this.
>
> Thanks, it seems many solutions could be taken from isearch-mb to
> isearch.el.

Actually, since I thought this thing through already, I would be happy
to provide a patch implementing the minibuffer-controlled isearch.

May I suggest that you merge just the `isearch-buffer-local' change at
this point, and then wait for my patch doing the minibuffer part?

>
>> 4. Every time one types a character in the minibuffer, there is a
>>    visible flicker in the cursor.  In isearch-mb I've let-bound
>>    `inhibit-redisplay' to t in a couple of places and it solved this
>>    problem.  I wonder if there's a better solution.
>
> Maybe a flicker is caused by isearch-message?

That's not it, since isearch-mb overrides `isearch-message' completely,
and still requires inhibiting the redisplay.  It seems to be related to
selecting a different window.

>
>> 5. The command `isearch-forward-exit-minibuffer' should eventually be
>>    removed, and instead `isearch-repeat-forward' should work in both
>>    cases (this will probably be implemented with the help of a new
>>    macro).  The reason for this is that there is at least a dozen other
>>    Isearch commands that make sense in the minibuffer map as well
>>    (toggling modes, quitting and entering query-replace, and so on), and
>>    it would be annoying to write a minibuffer variant for each of them.
>
> This is the part where I don't yet have an idea how to handle
> all isearch commands from the minibuffer without using a macro
> to implement their duplicates to be callable from the minibuffer.

I don't think there's another way.  Do you see this as a big problem?

>
>> 6. Why is there the option to specify an `isearch-new-message' in
>>    `with-isearch-suspended'?  I've never seen any valid value other than
>>    `(mapconcat 'isearch-text-char-description isearch-string "")', which,
>>    by the way, repeats at several places in isearch.el
>
> This is just to handle existing uses of with-isearch-suspended that
> currently set isearch-new-message, i.e. this could be changed later.

Actually, in the patch attached to
https://lists.gnu.org/archive/html/emacs-devel/2021-04/msg01359.html
(which by the way, do you think we can merge it?) I added an
`isearch-set-string' function to encapsulate this.

This function would be useful to implement the minibuffer-controlled
mode, as well as for third-party packages and user tweaks.

>
>>> And here is a patch for updating the search from the minibuffer. This allows
>>> to implement https://lists.gnu.org/archive/html/emacs-devel/2020-01/msg00447.html
>>> to control the search from the minibuffer.  But I'm not sure if the same option
>>> isearch-buffer-local should enable this mode:
>>
>> I still don't understand why Isearch has to be so stringent about
>> backward compatibility of key sequences.  But I don't care about the
>> edit mode per se.  I only care about an alternative mode where `C-s'
>> immediately goes to the minibuffer, and exiting the minibuffer ends the
>> search.
>
> In such a new mode isearch-edit-string could be called at the end of
> isearch-mode.

True.  `RET' and `C-g', among a few other things, should behave
differently in the two cases, but that's a detail.

Re: Simple isearch concerns

[Juri Linkov]

>>> 3. The after change hook should call `isearch-update' instead of
>>>    `isearch-search-and-update' after `previous-history-element' and few
>>>    other commands.  In isearch-mb I use a symbol property to keep track
>>>    of this.
>>
>> Thanks, it seems many solutions could be taken from isearch-mb to
>> isearch.el.
>
> Actually, since I thought this thing through already, I would be happy
> to provide a patch implementing the minibuffer-controlled isearch.

It would be nice if your experience with the minibuffer-controlled isearch
could help to bring the same feature to isearch.el.

> May I suggest that you merge just the `isearch-buffer-local' change at
> this point, and then wait for my patch doing the minibuffer part?

Testing shows that the `isearch-buffer-local' change works fine,
with only one deficiency: as (info "(elisp) Searching Keymaps") says
get-char-property takes precedence over emulation-mode-map-alists.

This means that typing isearch characters when point is e.g.
on a Gnus attachment line, typing a character like 'o' bound
to 'gnus-mime-save-part' will exit isearch and run this command.

I see no way to workaround this limitation.

> Actually, in the patch attached to
> https://lists.gnu.org/archive/html/emacs-devel/2021-04/msg01359.html
> (which by the way, do you think we can merge it?)

Before merging I tried to check if it covers everything discussed in
bug#11378.

Re: Simple isearch concerns

[Juri Linkov]

>> May I suggest that you merge just the `isearch-buffer-local' change at
>> this point, and then wait for my patch doing the minibuffer part?
>
> Testing shows that the `isearch-buffer-local' change works fine,
> with only one deficiency: as (info "(elisp) Searching Keymaps") says
> get-char-property takes precedence over emulation-mode-map-alists.

Actually, there are more problems: buffer-local design doesn't work for
multi-buffer isearch.

The current design is not without flaws either: when multi-buffer
isearch visits another buffer, it removes the isearch menu, because
isearch-mode is already buffer-local.  And isearch-mode can't be globalized,
because both minor-mode-alist and minor-mode-map-alist already contain
isearch keys that should not be active globally in other buffers.

While rewriting isearch for a new design (with a new option for backward-compatibility),
three basic aspects should be taken into consideration - mode, keymap, hooks:

1. mode - currently buffer-local and can't be globalized,
   so multi-buffer isearch should be fixed anyway to enable
   isearch-mode when visiting a new buffer and disable afterwards.

2. keymap - the current global overriding-terminal-local-map
   can be replaced by the global emulation-mode-map-alists
   that depends on the buffer-local value of isearch-mode.

   After fixing multi-buffer isearch to enable isearch-mode
   in visited buffers, keymap in emulation-mode-map-alists
   will enable isearch keys in the next buffer.

   Also a new feature should be designed to give precedence to
   some modes in emulation-mode-map-alists over get-char-property.

3. hooks - currently global hooks when turned into buffer-local
   should be enabled/disabled when multi-buffer isearch visits
   the next buffer.

4. additionally to support multiple isearch sessions in different buffers
   at the same time, isearch state variables could be buffer-local.

Re: Simple isearch concerns

[Juri Linkov]

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

> The current design is not without flaws either: when multi-buffer
> isearch visits another buffer, it removes the isearch menu, because
> isearch-mode is already buffer-local.

This patch fixes the problem.  It disables isearch-mode in the previous
buffer, and enables in the next buffer.

Later for isearch-buffer-local, the same function could remove hooks
in the previous buffer, and add buffer-local hooks in the next buffer.


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

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 5c71519054..095f8ba145 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -3549,9 +3552,8 @@ isearch-search-string
     (when pos1
       ;; When using multiple buffers isearch, switch to the new buffer here,
       ;; because `save-excursion' above doesn't allow doing it inside funcall.
-      (if (and multi-isearch-next-buffer-current-function
-	       (buffer-live-p multi-isearch-current-buffer))
-	  (switch-to-buffer multi-isearch-current-buffer))
+      (when multi-isearch-next-buffer-current-function
+        (multi-isearch-switch-buffer))
       (goto-char pos1)
       pos1)))
 
diff --git a/lisp/misearch.el b/lisp/misearch.el
index 335efb9516..338880f25f 100644
--- a/lisp/misearch.el
+++ b/lisp/misearch.el
@@ -190,10 +190,10 @@ multi-isearch-wrap
   (if (or (null multi-isearch-pause)
 	  (and multi-isearch-pause multi-isearch-current-buffer))
       (progn
-	(switch-to-buffer
-	 (setq multi-isearch-current-buffer
-	       (funcall multi-isearch-next-buffer-current-function
-			(current-buffer) t)))
+	(setq multi-isearch-current-buffer
+	      (funcall multi-isearch-next-buffer-current-function
+		       (current-buffer) t))
+	(multi-isearch-switch-buffer)
 	(goto-char (if isearch-forward (point-min) (point-max))))
     (setq multi-isearch-current-buffer (current-buffer))
     (setq isearch-wrapped nil)))
@@ -208,8 +208,18 @@ multi-isearch-push-state
 (defun multi-isearch-pop-state (_cmd buffer)
   "Restore the multiple buffers search state in BUFFER.
 Switch to the buffer restored from the search status stack."
-  (unless (equal buffer (current-buffer))
-    (switch-to-buffer (setq multi-isearch-current-buffer buffer))))
+  (unless (eq buffer (current-buffer))
+    (setq multi-isearch-current-buffer buffer)
+    (multi-isearch-switch-buffer)))
+
+;;;###autoload
+(defun multi-isearch-switch-buffer ()
+  "Switch to the next buffer in multi-buffer search."
+  (when (and (buffer-live-p multi-isearch-current-buffer)
+             (not (eq multi-isearch-current-buffer (current-buffer))))
+    (setq isearch-mode nil)
+    (switch-to-buffer multi-isearch-current-buffer)
+    (setq isearch-mode " M-Isearch")))
 
 \f
 ;;; Global multi-buffer search invocations

Re: Simple isearch concerns

[Augusto Stoffel]

On Fri,  7 May 2021 at 20:14, Juri Linkov <juri@linkov.net> wrote:

>> The current design is not without flaws either: when multi-buffer
>> isearch visits another buffer, it removes the isearch menu, because
>> isearch-mode is already buffer-local.
>
> This patch fixes the problem.  It disables isearch-mode in the previous
> buffer, and enables in the next buffer.

Do you think the `with-isearch-window-quitting-edit' macro from the
patch I sent in the "Controlling Isearch from the minibuffer" thread
will be sufficient to make this work in conjunction with the
minibuffer-controlled mode?

Re: Simple isearch concerns

[Juri Linkov]

>>> The current design is not without flaws either: when multi-buffer
>>> isearch visits another buffer, it removes the isearch menu, because
>>> isearch-mode is already buffer-local.
>>
>> This patch fixes the problem.  It disables isearch-mode in the previous
>> buffer, and enables in the next buffer.
>
> Do you think the `with-isearch-window-quitting-edit' macro from the
> patch I sent in the "Controlling Isearch from the minibuffer" thread
> will be sufficient to make this work in conjunction with the
> minibuffer-controlled mode?

Using multi-buffer isearch with isearch-from-minibuffer adds a new
dimension to complexity, but could be simplified when all isearch commands
will be redirected from the minibuffer to the original buffer.

The main problem is that currently quitting the minibuffer restores
the original window configuration, so quitting isearch-edit-string
will restore the original buffer state before isearch was started.

RE: [External] : Re: Simple isearch concerns

[Drew Adams]

Apologies for not following this thread.  It seems to be
quite wide in its scope (but I could easily just be
misunderstanding).

Is there really a focus on somehow changing just `M-e'
(edit the search pattern, with searching suspended while
doing that)?  Somehow I've gotten the impression that
much more seems to be under discussion (i.e., at stake).

What really is the _concrete problem_ that you're trying
to solve, for which you think (IIUC) that Isearch should
use the minibuffer (aside from the current uses it makes
of the minibuffer, which are really outside searching)?

Somehow, this kinda feels like an X-Y problem.

https://meta.stackexchange.com/questions/66377/what-is-the-xy-problem

And the OP seems to say (again, IIUC) that though he's
used Isearch for years he's never really understood the
C-g behavior (and DEL perhaps).  Is there an Isearch
problem or a learning/doc problem?
___

Those are just comments/questions from impressions I've
had while perusing some of the posts in the thread.
They might reflect gross misunderstanding (or rather,
lack of understanding) of what the problem and the
proposed solution really are.

Here, at least, is a concrete question:

If the minibuffer becomes the buffer that will be current
when using Isearch (I thought I saw that as a proposal),
will it still be possible to use Isearch in the minibuffer,
i.e., to search text in the minibuffer, when the minibuffer
is being used normally (not by Isearch)?

That's important, IMO: being able to use Isearch in any
minibuffer, to search its text.

Re: [External] : Re: Simple isearch concerns

[Juri Linkov]

> Apologies for not following this thread.  It seems to be
> quite wide in its scope (but I could easily just be
> misunderstanding).
>
> Is there really a focus on somehow changing just `M-e'
> (edit the search pattern, with searching suspended while
> doing that)?  Somehow I've gotten the impression that
> much more seems to be under discussion (i.e., at stake).
>
> What really is the _concrete problem_ that you're trying
> to solve, for which you think (IIUC) that Isearch should
> use the minibuffer (aside from the current uses it makes
> of the minibuffer, which are really outside searching)?

When you type 'C-f' in a web browser, it activates the "minibuffer".
Then you can type a search string.  As soon as you type a character
in the minibuffer, it immediately updates the search state
in the original window.  You can edit the search string
while the minibuffer is active, and the search results are
updated immediately.  Also each browser window has own search.

There are no plans to change the current default isearch behavior,
but still users might want to have an option to have a search
like in browsers.

RE: [External] : Re: Simple isearch concerns

[Drew Adams]

> > What really is the _concrete problem_ that you're trying
> > to solve, for which you think (IIUC) that Isearch should
> > use the minibuffer (aside from the current uses it makes
> > of the minibuffer, which are really outside searching)?
> 
> When you type 'C-f' in a web browser, it activates the "minibuffer".

Sorry, I don't follow you.  Do you mean some Emacs
web browser, such as EWW?  Or does your quoting the
word mean that you're trying to make some kind of
(hand-waving) analogy?  I guess I should assume the
latter.

> Then you can type a search string.  As soon as you type a character
> in the minibuffer, it immediately updates the search state
> in the original window.  You can edit the search string
> while the minibuffer is active, and the search results are
> updated immediately.

Mentally quoting each occurrence of "minibuffer"
there, and trying to imagine your analogy to using
a common web browser such as Chrome, what's the
essential difference from Emacs behavior that you're
trying to point out?

`C-s' in Emacs does that as well, no?  As soon as
you type a char in the "minibuffer" (but just as for
a web browser, it's not really a minibuffer)...  You
can edit the search string while the "minibuffer" is
active, and the search results are updated
immediately.  No?

> Also each browser window has own search.

There, I can see a difference.  But I see no
connection with a minibuffer.

> There are no plans to change the current default isearch behavior,
> but still users might want to have an option to have a search
> like in browsers.

What does "search like in browsers" mean in this
context?  And what's the improvement that that
offers?  And what does it have to do with changing
Isearch to use the minibuffer?

Re: [External] : Re: Simple isearch concerns

[Juri Linkov]

>> You can edit the search string while the minibuffer is active,
>> and the search results are updated immediately.
>
> `C-s' in Emacs does that as well, no?  As soon as
> you type a char in the "minibuffer" (but just as for
> a web browser, it's not really a minibuffer)...  You
> can edit the search string while the "minibuffer" is
> active, and the search results are updated
> immediately.  No?

No, the search in Emacs has no feature quoted above:
"You can edit the search string while the minibuffer is active,
and the search results are updated immediately."

RE: [External] : Re: Simple isearch concerns

[Drew Adams]

> >> You can edit the search string while the minibuffer is active,
> >> and the search results are updated immediately.
> >
> > `C-s' in Emacs does that as well, no?  As soon as
> > you type a char in the "minibuffer" (but just as for
> > a web browser, it's not really a minibuffer)...  You
> > can edit the search string while the "minibuffer" is
> > active, and the search results are updated
> > immediately.  No?
> 
> No, the search in Emacs has no feature quoted above:
> "You can edit the search string while the minibuffer
> is active, and the search results are updated immediately."

I disagree.  Augusto posed an analogy with web browsers,
which have no minibuffer.  He put "minibuffer" in quotes,
as did I.

Isearch certainly lets you delete chars from the end of
the search pattern, and add chars there.  That's
completely general, and it lets you edit the search
pattern while searching.

I put "minibuffer" in quotes, since clearly web browsers
don't have Emacs minibuffers.  For Isearch, the equivalent
to "while the minibuffer is active", is "while searching".
While isearching you certainly can edit the Isearch search
pattern.

The fact that Isearch doesn't use the minibuffer while
searching doesn't change anything in the given description
of the behavior: edit the search pattern while searching.

But if you specify using the minibuffer _not_ as a means
to do something different - in particular, something we
can't do now - but only as a _requirement_, then clearly
it's a vacuous truth that the proposed feature offers
something new.  In that case, the new thing is only that
it uses the minibuffer!  A vacuous QED.
___

I mentioned the Isearch+ feature that you misunderstood
as just the `edit' value of `search-exit-option' for a
reason in this context.

Besides deleting off the end of the search pattern
(which, as I say, is completely general), with option
`isearchp-initiate-edit-commands' you can, e.g., use
`C-b' not to exit search but to move the cursor to the
left in the search pattern.  Then type chars to insert
them there.  And so on.  All you need to do is hit
`C-s' again, to search with the edited pattern.

IOW, yes, minibuffer for editing search patterns.  But
no, not minibuffer all the time.  Only when you want
to do something other than add/remove chars at the end
of the pattern.  And end-modifying actions are the most
common search-pattern editing actions, _by far_.

Re: [External] : Re: Simple isearch concerns

[Yuri Khan]

On Tue, 11 May 2021 at 04:31, Juri Linkov <juri@linkov.net> wrote:

> When you type 'C-f' in a web browser, it activates the "minibuffer".
> Then you can type a search string.  As soon as you type a character
> in the minibuffer, it immediately updates the search state
> in the original window.  You can edit the search string
> while the minibuffer is active, and the search results are
> updated immediately.  Also each browser window has own search.

That makes sense for a web browser, because a web browser is typically
used only for viewing content and not editing. It typically also has
no point (except in form fields and editable content). (There is also
an optional accessibility feature that gives you a point which you can
move around the page and select text with the keyboard.)

The typical use case in a browser is to enter a search term and jump
through its occurrences. As soon as you have found the one you want,
you just keep reading from that point.

Because browsing the Web is mostly a mouse-driven activity, and only a
few keys are involved, a browser can afford giving all text editing
keys to the search bar, including Home/←/→/End to move the insertion
point within the search box and any text selection and cut/copy/paste
keys.

On the other hand, in a text editor such as Emacs, the use case is
different. You want to find an occurrence and then work with buffer
text in its neighborhood, and you want to switch from searching to
editing with as little friction as possible — preferably none. Isearch
supports exactly this use case, by making any unbound key terminate
the search and then perform its usual action, and making an exception
for Backspace so you can correct small typing mistakes. This comes at
the price of making editing the search string itself a bit less
convenient — one has to press M-e for that.

Re: [External] : Re: Simple isearch concerns

[Augusto Stoffel]

On Tue, 11 May 2021 at 13:20, Yuri Khan <yuri.v.khan@gmail.com> wrote:

> On Tue, 11 May 2021 at 04:31, Juri Linkov <juri@linkov.net> wrote:
>
>> When you type 'C-f' in a web browser, it activates the "minibuffer".
>> Then you can type a search string.  As soon as you type a character
>> in the minibuffer, it immediately updates the search state
>> in the original window.  You can edit the search string
>> while the minibuffer is active, and the search results are
>> updated immediately.  Also each browser window has own search.
>
> That makes sense for a web browser, because a web browser is typically
> used only for viewing content and not editing. It typically also has
> no point (except in form fields and editable content). (There is also
> an optional accessibility feature that gives you a point which you can
> move around the page and select text with the keyboard.)
>
> The typical use case in a browser is to enter a search term and jump
> through its occurrences. As soon as you have found the one you want,
> you just keep reading from that point.
>
> Because browsing the Web is mostly a mouse-driven activity, and only a
> few keys are involved, a browser can afford giving all text editing
> keys to the search bar, including Home/←/→/End to move the insertion
> point within the search box and any text selection and cut/copy/paste
> keys.
>
> On the other hand, in a text editor such as Emacs, the use case is
> different. You want to find an occurrence and then work with buffer
> text in its neighborhood, and you want to switch from searching to
> editing with as little friction as possible — preferably none. Isearch
> supports exactly this use case, by making any unbound key terminate
> the search and then perform its usual action, and making an exception
> for Backspace so you can correct small typing mistakes. This comes at
> the price of making editing the search string itself a bit less
> convenient — one has to press M-e for that.

The approach Isearch uses to read the search string is fine, but, unlike
you seem to be claiming, it has no fundamental advantage over using the
minibuffer.

In the minibuffer-based Isearch, one could easily make every movement
command (or any random subset of them) quit the minibuffer first.  In
fact, the patch already adds this behavior for `isearch-query-replace'
and a few other commands.

Re: [External] : Re: Simple isearch concerns

[Juri Linkov]

>> When you type 'C-f' in a web browser, it activates the "minibuffer".
>> Then you can type a search string.  As soon as you type a character
>> in the minibuffer, it immediately updates the search state
>> in the original window.  You can edit the search string
>> while the minibuffer is active, and the search results are
>> updated immediately.  Also each browser window has own search.
>
> That makes sense for a web browser, because a web browser is typically
> used only for viewing content and not editing. It typically also has
> no point (except in form fields and editable content). (There is also
> an optional accessibility feature that gives you a point which you can
> move around the page and select text with the keyboard.)

Such a feature as Caret Browsing (F7) in Firefox is quite convenient
to reduce dependence on mouse.

> Because browsing the Web is mostly a mouse-driven activity, and only a
> few keys are involved, a browser can afford giving all text editing
> keys to the search bar, including Home/←/→/End to move the insertion
> point within the search box and any text selection and cut/copy/paste
> keys.

I never use the mouse for searching in web browsers because there are
Isearch-like keys: C-g to repeat forward, and C-S-g to repeat backward.
And additionally the search string can be edited.  Even in a browser
Home/End are frequently used keys outside of the search to move
to the beginning/end of the window, but there is no problem to use
these keys to edit the search string in search mode.

So when Home/←/→/End by default quit Isearch, this is fine, but this
doesn't preclude from having an option to use Home/←/→/End to edit
the search string, and RET/C-g to quit the search minibuffer.

Re: Simple isearch concerns

[Juri Linkov]

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

> Testing shows that the `isearch-buffer-local' change works fine,
> with only one deficiency: as (info "(elisp) Searching Keymaps") says
> get-char-property takes precedence over emulation-mode-map-alists.
>
> This means that typing isearch characters when point is e.g.
> on a Gnus attachment line, typing a character like 'o' bound
> to 'gnus-mime-save-part' will exit isearch and run this command.
>
> I see no way to workaround this limitation.

Here is a patch that allows minor mode keys to take precedence over
char-property keys, by adding a special property to mode symbol, e.g.

  (put 'isearch-mode 'overriding-keymap t)

indicated in the decision tree by NEW:

     (or (if overriding-terminal-local-map
             (FIND-IN overriding-terminal-local-map))
         (if overriding-local-map
             (FIND-IN overriding-local-map)
           (or ;; => NEW: modes with overriding-keymap
               (FIND-IN (get-char-property (point) 'keymap))
               (FIND-IN-ANY emulation-mode-map-alists)
               (FIND-IN-ANY minor-mode-overriding-map-alist)
               (FIND-IN-ANY minor-mode-map-alist)
               (if (get-text-property (point) 'local-map)
                   (FIND-IN (get-char-property (point) 'local-map))
                 (FIND-IN (current-local-map)))))
         (FIND-IN (current-global-map)))


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

diff --git a/src/keymap.c b/src/keymap.c
index fb8eceaec1..c3801cca67 100644
--- a/src/keymap.c
+++ b/src/keymap.c
@@ -1331,7 +1331,7 @@ silly_event_symbol_error (Lisp_Object c)
    list, let the key sequence be read, and hope some other piece of
    code signals the error.  */
 ptrdiff_t
-current_minor_maps (Lisp_Object **modeptr, Lisp_Object **mapptr)
+current_minor_maps (Lisp_Object **modeptr, Lisp_Object **mapptr, int overriding)
 {
   ptrdiff_t i = 0;
   Lisp_Object alist, assoc, var, val;
@@ -1358,7 +1358,10 @@ current_minor_maps (Lisp_Object **modeptr, Lisp_Object **mapptr)
 	if ((assoc = XCAR (alist), CONSP (assoc))
 	    && (var = XCAR (assoc), SYMBOLP (var))
 	    && (val = find_symbol_value (var), !EQ (val, Qunbound))
-	    && !NILP (val))
+	    && !NILP (val)
+	    && (overriding == 0
+		|| (overriding == 1 && NILP (Fget (var, Qoverriding_keymap)))
+		|| (overriding == 2 && !NILP (Fget (var, Qoverriding_keymap)))))
 	  {
 	    Lisp_Object temp;
 
@@ -1556,7 +1559,7 @@ DEFUN ("current-active-maps", Fcurrent_active_maps, Scurrent_active_maps,
 	keymaps = Fcons (local_map, keymaps);
 
       /* Now put all the minor mode keymaps on the list.  */
-      nmaps = current_minor_maps (0, &maps);
+      nmaps = current_minor_maps (0, &maps, 1);
 
       for (int i = --nmaps; i >= 0; i--)
 	if (!NILP (maps[i]))
@@ -1565,6 +1568,12 @@ DEFUN ("current-active-maps", Fcurrent_active_maps, Scurrent_active_maps,
       if (!NILP (keymap))
 	keymaps = Fcons (keymap, keymaps);
 
+      /* Now put all the overriding minor mode keymaps on the list.  */
+      nmaps = current_minor_maps (0, &maps, 2);
+      for (int i = --nmaps; i >= 0; i--)
+	if (!NILP (maps[i]))
+	  keymaps = Fcons (maps[i], keymaps);
+
       if (!NILP (olp) && !NILP (otlp))
 	keymaps = Fcons (otlp, keymaps);
     }
@@ -1658,7 +1667,7 @@ DEFUN ("minor-mode-key-binding", Fminor_mode_key_binding, Sminor_mode_key_bindin
   (Lisp_Object key, Lisp_Object accept_default)
 {
   Lisp_Object *modes, *maps;
-  int nmaps = current_minor_maps (&modes, &maps);
+  int nmaps = current_minor_maps (&modes, &maps, 0);
   Lisp_Object binding = Qnil;
 
   int j;
@@ -1719,7 +1728,7 @@ DEFUN ("current-minor-mode-maps", Fcurrent_minor_mode_maps, Scurrent_minor_mode_
   (void)
 {
   Lisp_Object *maps;
-  int nmaps = current_minor_maps (0, &maps);
+  int nmaps = current_minor_maps (0, &maps, 0);
 
   return Flist (nmaps, maps);
 }
@@ -2713,7 +2722,7 @@ DEFUN ("describe-buffer-bindings", Fdescribe_buffer_bindings, Sdescribe_buffer_b
 	 minor modes correctly.  */
       Fset_buffer (buffer);
 
-      int nmaps = current_minor_maps (&modes, &maps);
+      int nmaps = current_minor_maps (&modes, &maps, 0);
       Fset_buffer (outbuf);
 
       start1 = get_local_map (BUF_PT (XBUFFER (buffer)),
@@ -3124,6 +3133,7 @@ syms_of_keymap (void)
   DEFSYM (Qdescribe_map_tree, "describe-map-tree");
 
   DEFSYM (Qkeymap_canonicalize, "keymap-canonicalize");
+  DEFSYM (Qoverriding_keymap, "overriding-keymap");
 
   /* Now we are ready to set up this property, so we can
      create char tables.  */
diff --git a/src/keymap.h b/src/keymap.h
index f417301c8f..b01698d394 100644
--- a/src/keymap.h
+++ b/src/keymap.h
@@ -36,7 +36,7 @@ #define KEYMAPP(m) (!NILP (get_keymap (m, false, false)))
 extern char *push_key_description (EMACS_INT, char *);
 extern Lisp_Object access_keymap (Lisp_Object, Lisp_Object, bool, bool, bool);
 extern Lisp_Object get_keymap (Lisp_Object, bool, bool);
-extern ptrdiff_t current_minor_maps (Lisp_Object **, Lisp_Object **);
+extern ptrdiff_t current_minor_maps (Lisp_Object **, Lisp_Object **, int);
 extern void initial_define_lispy_key (Lisp_Object, const char *, const char *);
 extern void syms_of_keymap (void);
 
diff --git a/src/keyboard.c b/src/keyboard.c
index 47b5e59024..a64d99e4c3 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -7395,7 +7395,7 @@ menu_bar_items (Lisp_Object old)
 	   properties may not work reliable, as they are only
 	   recognized when the menu-bar (or mode-line) is updated,
 	   which does not normally happen after every command.  */
-	ptrdiff_t nminor = current_minor_maps (NULL, &tmaps);
+	ptrdiff_t nminor = current_minor_maps (NULL, &tmaps, 0);
 	SAFE_NALLOCA (maps, 1, nminor + 4);
 	nmaps = 0;
 	Lisp_Object tem = KVAR (current_kboard, Voverriding_terminal_local_map);
@@ -7948,7 +7948,7 @@ tab_bar_items (Lisp_Object reuse, int *nitems)
 	 properties may not work reliably, as they are only
 	 recognized when the tab-bar (or mode-line) is updated,
 	 which does not normally happen after every command.  */
-      ptrdiff_t nminor = current_minor_maps (NULL, &tmaps);
+      ptrdiff_t nminor = current_minor_maps (NULL, &tmaps, 0);
       SAFE_NALLOCA (maps, 1, nminor + 4);
       nmaps = 0;
       Lisp_Object tem = KVAR (current_kboard, Voverriding_terminal_local_map);
@@ -8332,7 +8332,7 @@ tool_bar_items (Lisp_Object reuse, int *nitems)
 	 properties may not work reliably, as they are only
 	 recognized when the tool-bar (or mode-line) is updated,
 	 which does not normally happen after every command.  */
-      ptrdiff_t nminor = current_minor_maps (NULL, &tmaps);
+      ptrdiff_t nminor = current_minor_maps (NULL, &tmaps, 0);
       SAFE_NALLOCA (maps, 1, nminor + 4);
       nmaps = 0;
       Lisp_Object tem = KVAR (current_kboard, Voverriding_terminal_local_map);

Re: Simple isearch concerns

[Gregory Heytings]

>> Along with the package mentioned by Thierry there is CTRLF[1].
>>
>> [1] https://github.com/raxod502/ctrlf
>
> I tried this package a few months ago, but still do not understand what 
> it does differently from isearch.
>

And it provides a few commands that isearch doesn't have: move to 
first/last match, move to first match above/below window, center current 
match in window.

Re: Simple isearch concerns

[Daniel Martín]

Gregory Heytings <gregory@heytings.org> writes:

>
> And it provides a few commands that isearch doesn't have: move to
> first/last match, move to first match above/below window, center
> current match in window.

Move to first/last match in isearch is bound to M-s M-< and M-s M->.

You can also scroll without exiting an isearch (and use C-l, for
example) if you set `isearch-allow-scroll' to non-`nil'.

Re: Simple isearch concerns

[Gregory Heytings]

>> And it provides a few commands that isearch doesn't have: move to 
>> first/last match, move to first match above/below window, center 
>> current match in window.
>
> Move to first/last match in isearch is bound to M-s M-< and M-s M->.
>

I forgot these two ones indeed, but M-< and M-> are more natural than M-s 
M-< and M-s M->.

>
> You can also scroll without exiting an isearch (and use C-l, for 
> example) if you set `isearch-allow-scroll' to non-`nil'.
>

That doesn't do what ctrlf does, even with isearch-allow-scroll set to t, 
the match doesn't change, whereas with ctrlf C-v and M-v change the match 
to the first one above/below the window.

Note that I'm not saying that ctrlf is superior to isearch, I'm only 
indicating what the differences between isearch and ctrlf are.

Re: Simple isearch concerns

[Ergus]

On Sat, Apr 03, 2021 at 12:29:42PM +0000, Gregory Heytings wrote:
>
>>>Along with the package mentioned by Thierry there is CTRLF[1].
>>>
>>>[1] https://github.com/raxod502/ctrlf
>>
>>I tried this package a few months ago, but still do not understand 
>>what it does differently from isearch.
>>
>
>And it provides a few commands that isearch doesn't have: move to 
>first/last match, 

Isearch provides M-s M-< and M-s M->; I only bind them in
isearch-mode-map to M-< and M-> and it works as expected.

>move to first match above/below window, 
This one is missing.

>center current match in window.
C-l works when isearch-allow-scroll is non-nil

Re: Simple isearch concerns

[Daniel Martín]

Ergus <spacibba@aol.com> writes:

>
> 1) Are there any option to move the cursor to the start of the candidate
> after the search? I see that there is an isearch-other-end. But it is
> not a custom but an internal variable. Now I am using a hook but it
> seems a bit hacky.

You can create your own command that exits the search and moves the
point to the other end of the match.  See
https://www.emacswiki.org/emacs/ZapToISearch (`isearch-exit-other-end').

>
> 2) I want to disable wrapping and I see that there is an
> `isearch-wrap-function` but it is a variable not a custom and it is not
> documented in detail what is expected to be there.

Did you try to set it to something that is not `nil' but that will still
do nothing, like #'ignore?

Re: Simple isearch concerns

[Juri Linkov]

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

> 1) Are there any option to move the cursor to the start of the candidate
> after the search? I see that there is an isearch-other-end. But it is
> not a custom but an internal variable. Now I am using a hook but it
> seems a bit hacky.

I agree that isearch-mode-end-hook should be customizable.
Then initially we could provide at least one choice -
a hook to move to isearch-other-end on exit:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: isearch-move-to-other-end.patch --]
[-- Type: text/x-diff, Size: 1287 bytes --]

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 943e24aa56..c924b73607 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -178,10 +178,13 @@ isearch-mode-hook
 (defvar isearch-update-post-hook nil
   "Function(s) to call after isearch has found matches in the buffer.")
 
-(defvar isearch-mode-end-hook nil
+(defcustom isearch-mode-end-hook nil
   "Function(s) to call after terminating an incremental search.
 When these functions are called, `isearch-mode-end-hook-quit'
-is non-nil if the user quits the search.")
+is non-nil if the user quits the search."
+  :type 'hook
+  :options '(isearch-exit-other-end)
+  :version "28.1")
 
 (defvar isearch-mode-end-hook-quit nil
   "Non-nil while running `isearch-mode-end-hook' if the user quits the search.")
@@ -1417,6 +1420,15 @@ isearch-done
 
   (and (not edit) isearch-recursive-edit (exit-recursive-edit)))
 
+(defun isearch-exit-other-end ()
+  "Move point to the beginning of the found search string.
+Can be used in `isearch-mode-end-hook' or interactively to exit isearch."
+  (interactive)
+  (when isearch-mode
+    (isearch-exit))
+  (when isearch-other-end
+    (goto-char isearch-other-end)))
+
 (defvar isearch-mouse-commands '(mouse-minor-mode-menu)
   "List of mouse commands that are allowed during Isearch.")
 

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


> 2) I want to disable wrapping and I see that there is an
> `isearch-wrap-function` but it is a variable not a custom and it is not
> documented in detail what is expected to be there.

isearch-wrap-function could be customizable in the same way with 3 options:
pause before wrapping (default), no pause, no warp at all.

> 3) In general many of the customizable variables in isearch are defined
> with defvar so it is difficult to distinguish between internal and user
> exposed ones. Just to mention some examples:
> isearch-push-state-function, isearch-filter-predicate, the hooks,

And these as well.

> Are there any WIP related?

There is no WIP until you create a feature request.

Re: Simple isearch concerns

[Stefan Monnier]

> -(defvar isearch-mode-end-hook nil
> +(defcustom isearch-mode-end-hook nil
>    "Function(s) to call after terminating an incremental search.
>  When these functions are called, `isearch-mode-end-hook-quit'
> -is non-nil if the user quits the search.")
> +is non-nil if the user quits the search."
> +  :type 'hook
> +  :options '(isearch-exit-other-end)
> +  :version "28.1")

FWIW, I can't imagine why someone would like that option.  I can see it
being useful *sometimes*, but I think it'd be harmful at least as often
as it is useful.


        Stefan

Re: Simple isearch concerns

[Ergus]

On Sun, Apr 04, 2021 at 07:27:13PM -0400, Stefan Monnier wrote:
>> -(defvar isearch-mode-end-hook nil
>> +(defcustom isearch-mode-end-hook nil
>>    "Function(s) to call after terminating an incremental search.
>>  When these functions are called, `isearch-mode-end-hook-quit'
>> -is non-nil if the user quits the search.")
>> +is non-nil if the user quits the search."
>> +  :type 'hook
>> +  :options '(isearch-exit-other-end)
>> +  :version "28.1")
>
>FWIW, I can't imagine why someone would like that option.  I can see it
>being useful *sometimes*, but I think it'd be harmful at least as often
>as it is useful.
>
>
>        Stefan
>

Hi Stefan:

Swiper provides this option and after trying it I found it very
useful and some other users seems they do too. 

Specially when the idea is to select a word C-M-SPC after it is found is
very handy. Another use case is when the word looks like my_var or
my-var it requires M-b twice to go to the beginning and then select.

In any case, if you think this option is not desirable for any reason,
another alternative may be to add a command in the isearch map (bind to
C-RET, or M-RET) to end the search and go to the other end... something
like isearch-exit-go-other-end. This alternative is actually a bit more
interesting; and maybe equivalent to pressing C-r before RET... but
faster?

RE: [External] : Re: Simple isearch concerns

[Drew Adams]

> >FWIW, I can't imagine why someone would like that option.  I can see it
> >being useful *sometimes*, but I think it'd be harmful at least as often
> >as it is useful.
>
> Swiper provides this option and after trying it I found it very
> useful and some other users seems they do too.
> 
> Specially when the idea is to select a word C-M-SPC after it is found is
> very handy. Another use case is when the word looks like my_var or
> my-var it requires M-b twice to go to the beginning and then select.
> 
> In any case, if you think this option is not desirable for any reason,
> another alternative may be to add a command in the isearch map (bind to
> C-RET, or M-RET) to end the search and go to the other end... something
> like isearch-exit-go-other-end. This alternative is actually a bit more
> interesting; and maybe equivalent to pressing C-r before RET... but
> faster?

FWIW, Since 2006 `isearch+.el' provides something
similar, but which I think is maybe a bit more useful.

* Option `isearchp-set-region-flag' non-`nil' means
  automatically set the region around the last search
  target.

* `M-s M-SPC' toggles `isearchp-set-region-flag'
  during isearch.

* Command `isearchp-set-region-around-search-target'
  manually (anytime - not bound to a key by defaut)
  sets the region around the last search target.

Is it more useful to just go to the other end of the
search hit or to select the search hit?  Depends;
anyway, either allows for the other pretty easily.

Re: [External] : Re: Simple isearch concerns

[Juri Linkov]

> FWIW, Since 2006 `isearch+.el' provides something
> similar, but which I think is maybe a bit more useful.
>
> * Option `isearchp-set-region-flag' non-`nil' means
>   automatically set the region around the last search
>   target.

This is already easy to do with the hook:

(add-hook 'isearch-mode-end-hook
          (lambda ()
            (when transient-mark-mode
              (push-mark isearch-other-end t 'activate))))

RE: [External] : Re: Simple isearch concerns

[Drew Adams]

> > FWIW, Since 2006 `isearch+.el' provides something
> > similar, but which I think is maybe a bit more useful.
> >
> > * Option `isearchp-set-region-flag' non-`nil' means
> >   automatically set the region around the last search
> >   target.
> 
> This is already easy to do with the hook:
> 
> (add-hook 'isearch-mode-end-hook
>           (lambda ()
>             (when transient-mark-mode
>               (push-mark isearch-other-end t 'activate))))

Yes, of course that's possible.  But it's less
convenient, especially without a key for toggling it.

Re: Simple isearch concerns

[Stefan Monnier]

> Swiper provides this option and after trying it I found it very
> useful and some other users seems they do too.

I strongly suspect that swiper does not provide *exactly* that option.
E.g. does Swiper's option also cause C-r foo RET to place point at the *end*
of "foo" rather than its beginning?

> Specially when the idea is to select a word C-M-SPC after it is found
> is very handy.  Another use case is when the word looks like my_var or
> my-var it requires M-b twice to go to the beginning and then select.

As I said I have no doubt that it's sometimes useful, but that's not
the question.

> In any case, if you think this option is not desirable for any reason,
> another alternative may be to add a command in the isearch map (bind to
> C-RET, or M-RET) to end the search and go to the other end... something
> like isearch-exit-go-other-end. This alternative is actually a bit more
> interesting; and maybe equivalent to pressing C-r before RET... but
> faster?

Yes, that sounds a lot more useful.


        Stefan

Re: Simple isearch concerns

[Juri Linkov]

>> Swiper provides this option and after trying it I found it very
>> useful and some other users seems they do too.
>
> I strongly suspect that swiper does not provide *exactly* that option.
> E.g. does Swiper's option also cause C-r foo RET to place point at the *end*
> of "foo" rather than its beginning?

The really useful command that I used for a long time
has a check for isearch-forward:

              (if (and isearch-forward isearch-other-end)
                  (goto-char isearch-other-end))

that doesn't place point at the end for C-r foo RET.

>> In any case, if you think this option is not desirable for any reason,
>> another alternative may be to add a command in the isearch map (bind to
>> C-RET, or M-RET) to end the search and go to the other end... something
>> like isearch-exit-go-other-end. This alternative is actually a bit more
>> interesting; and maybe equivalent to pressing C-r before RET... but
>> faster?
>
> Yes, that sounds a lot more useful.

And I bound the useful command above to C-RET because such feature is needed
only occasionally.

Re: Simple isearch concerns

[Basil L. Contovounesios]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Swiper provides this option and after trying it I found it very
>> useful and some other users seems they do too.
>
> I strongly suspect that swiper does not provide *exactly* that option.
> E.g. does Swiper's option also cause C-r foo RET to place point at the *end*
> of "foo" rather than its beginning?

Just for completeness:

The original swiper and swiper-backward commands either always leave
point at the end of the match (by default), or always at the start
(after customising swiper-goto-start-of-match), regardless of direction.

Later came swiper-isearch and swiper-isearch-backward, for handling
multiple matches per line.  AFAICT the former always leaves point at the
end, and the latter always at the start.

AFAIK neither of these interfaces was originally designed with a way of
customising where to leave point in mind; it was an afterthought, and
naturally there are open bug reports about their behaviour.

-- 
Basil

RE: [External] : Re: Simple isearch concerns

[Drew Adams]

> >> Swiper provides this option and after trying it I found it very
> >> useful and some other users seems they do too.
> >
> > I strongly suspect that swiper does not provide *exactly* that option.
> > E.g. does Swiper's option also cause C-r foo RET to place point at the
> > *end* of "foo" rather than its beginning?
> 
> Just for completeness:
> 
> The original swiper and swiper-backward commands either always leave
> point at the end of the match (by default), or always at the start
> (after customising swiper-goto-start-of-match), regardless of direction.
> 
> Later came swiper-isearch and swiper-isearch-backward, for handling
> multiple matches per line.  AFAICT the former always leaves point at the
> end, and the latter always at the start.
> 
> AFAIK neither of these interfaces was originally designed with a way of
> customising where to leave point in mind; it was an afterthought, and
> naturally there are open bug reports about their behaviour.

FWIW, with isearch+.el forward and backward search
mirror each other in this regard.  Point isn't moved,
and the search hit is selected as the active region.

So point remains in "front" of mark in the direction
of search (which is helpful for repeated search, for
example).

The main use case is wanting to act on the search hit,
not just to move to its other end.  Having mark at the
other end gives you easy access to it, as a freebie.

And yes, I think that if someone really wants to just
move point to the other end of the hit then `C-r RET'
is just fine for that.

I definitely wouldn't advise wasting a key binding
such as `C-RET' on that.  But that's just me, becoming
a bindings conservationist in my old age.

Re: Simple isearch concerns

[Ergus]

On Mon, Apr 05, 2021 at 01:48:20AM +0300, Juri Linkov wrote:
>
>> Are there any WIP related?
>
>There is no WIP until you create a feature request.

Done! bug#47599

Re: Simple isearch concerns

[Juri Linkov]

> 1) Are there any option to move the cursor to the start of the candidate
> after the search? I see that there is an isearch-other-end. But it is
> not a custom but an internal variable. Now I am using a hook but it
> seems a bit hacky.

As you see from the discussion, adding a defcustom makes little sense,
because this feature might be needed only occasionally, so better
to be bound to a special key, e.g. C-RET to exit and move to other-end,
S-RET to exit and mark the found string as a region.

> 2) I want to disable wrapping and I see that there is an
> `isearch-wrap-function` but it is a variable not a custom and it is not
> documented in detail what is expected to be there.

isearch-wrap-function can't be a defcustom too, because it's impossible
to implement automatic wrapping by a wrap function.

> 3) In general many of the customizable variables in isearch are defined
> with defvar so it is difficult to distinguish between internal and user
> exposed ones. Just to mention some examples:
> isearch-push-state-function, isearch-filter-predicate, the hooks,

Most of these functions are intended to be modified by different modes
using add-function, so again there is nothing to customize here, sorry.

RE: [External] : Re: Simple isearch concerns

[Drew Adams]

> > 1) Are there any option to move the cursor to the start of the candidate
> > after the search? I see that there is an isearch-other-end. But it is
> > not a custom but an internal variable. Now I am using a hook but it
> > seems a bit hacky.
> 
> As you see from the discussion, adding a defcustom makes little sense,
> because this feature might be needed only occasionally, so better
> to be bound to a special key, e.g. C-RET to exit and move to other-end,
> S-RET to exit and mark the found string as a region.

FWIW, I don't see that from "the discussion".  I see
that that's what you proposed.  I described something
different, and I opposed binding keys for what you
describe.

For something that you yourself claim might be needed
only occasionally, there's no reason to add a key for
that, let alone two keys.

In particular, I'm not in favor of adding keys for
exiting + performing an action.  Better to add keys
for toggling some search behavior, and leave exiting
keys as they are.

There are many keys to exit search, and there's no
reason to suppose that they shouldn't perform the
particular action you have in mind, and instead that
only some other dedicated key should perform that
action.

In general, it's better to have a variable that
defines the action, and a key that toggles it.

That way, users can themselves decide whether or not
to continue having it be in effect - they can switch
it on/off on the fly anytime during search.  And they
can continue to use any of the keys they usually use
to exit the search, and have those keys also effect
the action.

Re: [External] : Re: Simple isearch concerns

[Juri Linkov]

>> As you see from the discussion, adding a defcustom makes little sense,
>> because this feature might be needed only occasionally, so better
>> to be bound to a special key, e.g. C-RET to exit and move to other-end,
>> S-RET to exit and mark the found string as a region.
>
> In particular, I'm not in favor of adding keys for
> exiting + performing an action.

I meant that these keys can easily be bound by users.

> Better to add keys for toggling some search behavior, and leave
> exiting keys as they are.

In bug#39512 we already developed a command bound to 'M-s M-.'
to pull the active region to the search string.

Now we could add a command that sets the region from the search string.

Re: [External] : Re: Simple isearch concerns

[Ergus]

On Tue, Apr 06, 2021 at 12:35:52AM +0300, Juri Linkov wrote:
>>> As you see from the discussion, adding a defcustom makes little sense,
>>> because this feature might be needed only occasionally, so better
>>> to be bound to a special key, e.g. C-RET to exit and move to other-end,
>>> S-RET to exit and mark the found string as a region.
>>
>> In particular, I'm not in favor of adding keys for
>> exiting + performing an action.
>
>I meant that these keys can easily be bound by users.
>
>> Better to add keys for toggling some search behavior, and leave
>> exiting keys as they are.
>
>In bug#39512 we already developed a command bound to 'M-s M-.'
>to pull the active region to the search string.
>
>Now we could add a command that sets the region from the search string.

Is it possible to make this in a way that some commands could be bound
in a way that the prefix M-s won't be needed sometimes when isearch is
active? (I mean a custom option of course)

That could be more handy for some users. Because `M-s C-RET` is not
faster than `C-r RET`.

The same applies to `M-s M-<` and `M-s M->`. And maybe for some future
commands like the ones Gregory mentioned some days ago like go to
candidate outside visible the page and so on in M-v and C-v.

Maybe we can add a map with some extra commands and then bind the map
in the isearch-mode-map with and without prefix conditionally? Does it
makes sense?.

Re: [External] : Re: Simple isearch concerns

[Juri Linkov]

> Is it possible to make this in a way that some commands could be bound
> in a way that the prefix M-s won't be needed sometimes when isearch is
> active? (I mean a custom option of course)
>
> That could be more handy for some users. Because `M-s C-RET` is not
> faster than `C-r RET`.
>
> The same applies to `M-s M-<` and `M-s M->`. And maybe for some future
> commands like the ones Gregory mentioned some days ago like go to
> candidate outside visible the page and so on in M-v and C-v.
>
> Maybe we can add a map with some extra commands and then bind the map
> in the isearch-mode-map with and without prefix conditionally? Does it
> makes sense?.

I already bound them to shorter keys because `M-s M-<`
is harder to type:

  (define-key isearch-mode-map "\M-<" 'isearch-beginning-of-buffer)
  (define-key isearch-mode-map "\M->" 'isearch-end-of-buffer)

The problem is that by default most keys including `M-<`
should exit isearch.

But maybe when search-exit-option is customized to nil,
or when isearch-allow-scroll is customized to 'unlimited',
or when the third option added by Gregory isearch-allow-match-scroll
is customized to t, then remap motion commands to navigate
isearch matches.

Should then forward-char be remapped to isearch-repeat to go to the next
match?  Then also remap next-line to go to the next line with matches, etc.

The logic should be quite simple: isearch-allow-scroll could have
a new option similar to 'unlimited'.  Let's name it 'sync'.
It should allow typing a motion key, then afterwards it will call
isearch-repeat to find the next match after moving to new location.

Re: [External] : Re: Simple isearch concerns

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Tue, 06 Apr 2021 22:11:13 +0300
> Cc: Drew Adams <drew.adams@oracle.com>,
>  "emacs-devel@gnu.org" <emacs-devel@gnu.org>
> 
> Should then forward-char be remapped to isearch-repeat to go to the next
> match?  Then also remap next-line to go to the next line with matches, etc.

Please don't, these commands should exit the search.

Re: [External] : Re: Simple isearch concerns

[Gregory Heytings]

>> Should then forward-char be remapped to isearch-repeat to go to the 
>> next match?  Then also remap next-line to go to the next line with 
>> matches, etc.
>
> Please don't, these commands should exit the search.
>

Yes, I also think these commands should exit the search by default. 
Users who would like to have that behavior can do so easily with my patch:

(put 'next-line 'isearch-match-scroll 'isearch-repeat-forward)

Re: [External] : Re: Simple isearch concerns

[Juri Linkov]

>>> Should then forward-char be remapped to isearch-repeat to go to the next
>>> match?  Then also remap next-line to go to the next line with
>>> matches, etc.
>>
>> Please don't, these commands should exit the search.
>
> Yes, I also think these commands should exit the search by default. Users
> who would like to have that behavior can do so easily with my patch:
>
> (put 'next-line 'isearch-match-scroll 'isearch-repeat-forward)

Then 'isearch-match-scroll' has nothing to do with scrolling.
It's just a duplicate way to remap commands, i.e. this provides
exactly the same result:

  (define-key isearch-mode-map [remap next-line] 'isearch-repeat-forward)

Re: [External] : Re: Simple isearch concerns

[Gregory Heytings]

>>>> Should then forward-char be remapped to isearch-repeat to go to the 
>>>> next match?  Then also remap next-line to go to the next line with 
>>>> matches, etc.
>>>
>>> Please don't, these commands should exit the search.
>>
>> Yes, I also think these commands should exit the search by default. 
>> Users who would like to have that behavior can do so easily with my 
>> patch:
>>
>> (put 'next-line 'isearch-match-scroll 'isearch-repeat-forward)
>
> Then 'isearch-match-scroll' has nothing to do with scrolling.
>

It was you who suggested that change, you asked: "Should then forward-char 
be remapped to isearch-repeat to go to the next match?"  Or am I 
misunderstanding something?

Re: [External] : Re: Simple isearch concerns

[Gregory Heytings]

>
> The logic should be quite simple: isearch-allow-scroll could have a new 
> option similar to 'unlimited'.  Let's name it 'sync'. It should allow 
> typing a motion key, then afterwards it will call isearch-repeat to find 
> the next match after moving to new location.
>

I don't think this would be TRT, because my guess is that most users will 
want to continue to exit isearch with C-f / C-b / C-n / C-p / C-a / C-e. 
Exiting isearch with M-<, M->, C-v and M-v makes much less sense, so 
rebinding these keys with a single user option makes sense (as I do in my 
patch).  IMO it is better to create such an option, and to let the users 
who would like to rebind more keys do so in their init file.

RE: [External] : Re: Simple isearch concerns

[Drew Adams]

> >> As you see from the discussion, adding a defcustom makes little sense,
> >> because this feature might be needed only occasionally, so better
> >> to be bound to a special key, e.g. C-RET to exit and move to other-end,
> >> S-RET to exit and mark the found string as a region.
> >
> > In particular, I'm not in favor of adding keys for
> > exiting + performing an action.
> 
> I meant that these keys can easily be bound by users.

I see; good.  I misunderstood, in that case.

> > Better to add keys for toggling some search behavior,
> > and leave exiting keys as they are.
> 
> In bug#39512 we already developed a command bound to
> 'M-s M-.'to pull the active region to the search string.
> 
> Now we could add a command that sets the region from the search string.

I suggest a toggle for that: select or not.

Re: Simple isearch concerns

[Ergus]

On Mon, Apr 05, 2021 at 11:37:30PM +0300, Juri Linkov wrote:
>> 1) Are there any option to move the cursor to the start of the candidate
>> after the search? I see that there is an isearch-other-end. But it is
>> not a custom but an internal variable. Now I am using a hook but it
>> seems a bit hacky.
>
>As you see from the discussion, adding a defcustom makes little sense,
>because this feature might be needed only occasionally, so better
>to be bound to a special key, e.g. C-RET to exit and move to other-end,
>S-RET to exit and mark the found string as a region.
>
Ok for me.

>> 2) I want to disable wrapping and I see that there is an
>> `isearch-wrap-function` but it is a variable not a custom and it is not
>> documented in detail what is expected to be there.
>
>isearch-wrap-function can't be a defcustom too, because it's impossible
>to implement automatic wrapping by a wrap function.
>
Sorry I don't get this one, on yesterday you gave what was IMO the best
proposal. Provide 3 options for: disable wrapping, pause and just
wrap. Are these really so hard to provide?

* Any way if there is a reason for this; then I won't argue about that.


>> 3) In general many of the customizable variables in isearch are defined
>> with defvar so it is difficult to distinguish between internal and user
>> exposed ones. Just to mention some examples:
>> isearch-push-state-function, isearch-filter-predicate, the hooks,
>
>Most of these functions are intended to be modified by different modes
>using add-function, so again there is nothing to customize here, sorry.
>
I use some of them as simple hooks. But again (go to *)

Re: Simple isearch concerns

[Juri Linkov]

>>> 2) I want to disable wrapping and I see that there is an
>>> `isearch-wrap-function` but it is a variable not a custom and it is not
>>> documented in detail what is expected to be there.
>>
>>isearch-wrap-function can't be a defcustom too, because it's impossible
>>to implement automatic wrapping by a wrap function.
>>
> Sorry I don't get this one, on yesterday you gave what was IMO the best
> proposal. Provide 3 options for: disable wrapping, pause and just
> wrap. Are these really so hard to provide?

Indeed, this would be nice to have.  Now the wrapping option was implemented
in bug#47599.

> * Any way if there is a reason for this; then I won't argue about that.

My point was that this required adding a completely new separate option,
not turning the existing hook into defcustom.

>>> 3) In general many of the customizable variables in isearch are defined
>>> with defvar so it is difficult to distinguish between internal and user
>>> exposed ones. Just to mention some examples:
>>> isearch-push-state-function, isearch-filter-predicate, the hooks,
>>
>>Most of these functions are intended to be modified by different modes
>>using add-function, so again there is nothing to customize here, sorry.
>>
> I use some of them as simple hooks. But again (go to *)

The existing hooks are intended to be used by modes,
and indeed they are used by modes that directly change the values
of these variable.  When the users will try to customize them,
this might have unintended consequences.

Re: Simple isearch concerns

[Gregory Heytings]

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


>> 1) Are there any option to move the cursor to the start of the 
>> candidate after the search? I see that there is an isearch-other-end. 
>> But it is not a custom but an internal variable. Now I am using a hook 
>> but it seems a bit hacky.
>
> As you see from the discussion, adding a defcustom makes little sense, 
> because this feature might be needed only occasionally, so better to be 
> bound to a special key, e.g. C-RET to exit and move to other-end, S-RET 
> to exit and mark the found string as a region.
>

Note that neither C-RET nor S-RET are available in a terminal.

What I consider to be the most useful feature of CTRLF, namely the 
possibility to scroll among the matches, has not much been discussed so 
far, alas.  I attach a short patch which implements that feature in 
isearch.  I've been using it for a day, and I'm convinced it's a 
must-have, it makes navigating among the search matches much faster. 
WDYT?

[-- Attachment #2: Type: text/x-diff, Size: 6087 bytes --]

From a1dd7f1f93f0a2b74c7f9e43d97f5b5f09e16b69 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Tue, 6 Apr 2021 00:17:39 +0000
Subject: [PATCH] New user option to scroll isearch matches

* lisp/isearch.el (isearch-allow-match-scroll): New user option.
(isearch-scroll-up, isearch-scroll-down): New commands to scroll
the current search match, that complement the existing
isearch-beginning-of-buffer and isearch-end-of-buffer commands.
(isearch-pre-command-hook): Handle the new option and commands.

* etc/NEWS: Mention the new user option.

* doc/emacs/search.texi: Document the new user option.
---
 doc/emacs/search.texi |  8 ++++++++
 etc/NEWS              |  8 ++++++++
 lisp/isearch.el       | 47 ++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/doc/emacs/search.texi b/doc/emacs/search.texi
index f3c42bcea7..6c161d4c58 100644
--- a/doc/emacs/search.texi
+++ b/doc/emacs/search.texi
@@ -587,6 +587,14 @@ Not Exiting Isearch
 an incremental search.  This feature is disabled if
 @code{isearch-allow-scroll} is @code{nil} (which it is by default).
 
+@vindex isearch-allow-match-scroll
+  Likewise, if you change the variable @code{isearch-allow-match-scroll}
+to a non-@code{nil} value, this enables the use of the keyboard scrolling
+commands @kbd{C-v},  @kbd{M-v}, @kbd{M-<} and @kbd{M->} to move
+respectively to the first first occurrence of the current search
+string in the buffer, the last one, the first one after the current
+window, and the last one before the current window.
+
 @item Motion Commands
 @cindex motion commands, during incremental search
 When @code{isearch-yank-on-move} is customized to @code{shift},
diff --git a/etc/NEWS b/etc/NEWS
index c8400ba8c2..411ea72e7c 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -367,6 +367,14 @@ trying to be non-destructive.
 This command opens a new buffer called "*Memory Report*" and gives a
 summary of where Emacs is using memory currently.
 
++++
+** New user option 'isearch-allow-match-scroll'.
+When this option is set, the commands 'beginning-of-buffer',
+'end-of-buffer', 'scroll-up-command' and 'scroll-down-command' move
+respectively to the first first occurrence of the current search
+string in the buffer, the last one, the first one after the current
+window, and the last one before the current window.
+
 ** Outline
 
 +++
diff --git a/lisp/isearch.el b/lisp/isearch.el
index 943e24aa56..bdb2f147ab 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -1942,6 +1942,23 @@ isearch-end-of-buffer
     (goto-char (point-max))
     (isearch-repeat 'backward arg)))
 
+(defun isearch-scroll-up ()
+  "Go to the first occurrence of the current search string after window.
+Move point to the end of the window, and search forward."
+  (interactive)
+  (setq isearch-just-started t)
+  (goto-char (window-end))
+  (isearch-repeat 'forward))
+
+(defun isearch-scroll-down ()
+  "Go to the first occurrence of the current search string before window.
+Move point to the beginning of the window, recenter it, and search backward."
+  (interactive)
+  (setq isearch-just-started t)
+  (goto-char (window-start))
+  (recenter nil t)
+  (isearch-repeat 'backward))
+
 \f
 ;;; Toggles for `isearch-regexp-function' and `search-default-mode'.
 (defmacro isearch-define-mode-toggle (mode key function &optional docstring &rest body)
@@ -2840,12 +2857,35 @@ isearch-allow-scroll
 However, you cannot scroll far enough that the current match is
 no longer visible (is off screen).  But if the value is `unlimited'
 that limitation is removed and you can scroll any distance off screen.
-If nil, scrolling commands exit Isearch mode."
+If nil, scrolling commands exit Isearch mode.
+See also the related option `isearch-allow-match-scroll'."
   :type '(choice (const :tag "Scrolling exits Isearch" nil)
                  (const :tag "Scrolling with current match on screen" t)
                  (const :tag "Scrolling with current match off screen" unlimited))
   :group 'isearch)
 
+(put 'beginning-of-buffer 'isearch-match-scroll t)
+(put 'end-of-buffer 'isearch-match-scroll t)
+(put 'scroll-up-command 'isearch-match-scroll t)
+(put 'scroll-down-command 'isearch-match-scroll t)
+
+(put 'beginning-of-buffer 'isearch-match-scroll-command 'isearch-beginning-of-buffer)
+(put 'end-of-buffer 'isearch-match-scroll-command 'isearch-end-of-buffer)
+(put 'scroll-up-command 'isearch-match-scroll-command 'isearch-scroll-up)
+(put 'scroll-down-command 'isearch-match-scroll-command 'isearch-scroll-down)
+
+(defcustom isearch-allow-match-scroll nil
+  "Whether scrolling to another match is allowed during incremental search.
+If non-nil, the four scrolling commands `beginning-of-buffer',
+`end-of-buffer', `scroll-up-command' and `scroll-down-command' move
+respectively to the first first occurrence of the current search string in
+the buffer, the last one, the first one after the current window, and the
+last one before the current window.
+See also the related option `isearch-allow-scroll'."
+  :type '(choice (const :tag "Off" nil)
+                 (const tag "On" t))
+  :group 'isearch)
+
 (defcustom isearch-allow-prefix t
   "Whether prefix arguments are allowed during incremental search.
 If non-nil, entering a prefix argument will not terminate the
@@ -2947,6 +2987,11 @@ isearch-pre-command-hook
      ;; Optionally edit the search string instead of exiting.
      ((eq search-exit-option 'edit)
       (setq this-command 'isearch-edit-string))
+     ;; Handle match scrolling functions
+     ((and isearch-allow-match-scroll
+           (symbolp this-command)
+           (eq (get this-command 'isearch-match-scroll) t))
+      (setq this-command (get this-command 'isearch-match-scroll-command)))
      ;; Handle a scrolling function or prefix argument.
      ((or (and isearch-allow-prefix
                (memq this-command '(universal-argument universal-argument-more
-- 
2.30.2

Re: Simple isearch concerns

[Gregory Heytings]

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


>>> 1) Are there any option to move the cursor to the start of the 
>>> candidate after the search? I see that there is an isearch-other-end. 
>>> But it is not a custom but an internal variable. Now I am using a hook 
>>> but it seems a bit hacky.
>> 
>> As you see from the discussion, adding a defcustom makes little sense, 
>> because this feature might be needed only occasionally, so better to be 
>> bound to a special key, e.g. C-RET to exit and move to other-end, S-RET 
>> to exit and mark the found string as a region.
>
> Note that neither C-RET nor S-RET are available in a terminal.
>
> What I consider to be the most useful feature of CTRLF, namely the 
> possibility to scroll among the matches, has not much been discussed so 
> far, alas.  I attach a short patch which implements that feature in 
> isearch.  I've been using it for a day, and I'm convinced it's a 
> must-have, it makes navigating among the search matches much faster. 
> WDYT?
>

Sorry, there was a typo in the previous patch, I attach a corrected one.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=0001-New-user-option-to-scroll-isearch-matches.patch, Size: 6087 bytes --]

From a1dd7f1f93f0a2b74c7f9e43d97f5b5f09e16b69 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Tue, 6 Apr 2021 00:17:39 +0000
Subject: [PATCH] New user option to scroll isearch matches

* lisp/isearch.el (isearch-allow-match-scroll): New user option.
(isearch-scroll-up, isearch-scroll-down): New commands to scroll
the current search match, that complement the existing
isearch-beginning-of-buffer and isearch-end-of-buffer commands.
(isearch-pre-command-hook): Handle the new option and commands.

* etc/NEWS: Mention the new user option.

* doc/emacs/search.texi: Document the new user option.
---
 doc/emacs/search.texi |  8 ++++++++
 etc/NEWS              |  8 ++++++++
 lisp/isearch.el       | 47 ++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/doc/emacs/search.texi b/doc/emacs/search.texi
index f3c42bcea7..6c161d4c58 100644
--- a/doc/emacs/search.texi
+++ b/doc/emacs/search.texi
@@ -587,6 +587,14 @@ Not Exiting Isearch
 an incremental search.  This feature is disabled if
 @code{isearch-allow-scroll} is @code{nil} (which it is by default).
 
+@vindex isearch-allow-match-scroll
+  Likewise, if you change the variable @code{isearch-allow-match-scroll}
+to a non-@code{nil} value, this enables the use of the keyboard scrolling
+commands @kbd{M-<}, @kbd{M->}, @kbd{C-v} and @kbd{M-v}, to move
+respectively to the first first occurrence of the current search
+string in the buffer, the last one, the first one after the current
+window, and the last one before the current window.
+
 @item Motion Commands
 @cindex motion commands, during incremental search
 When @code{isearch-yank-on-move} is customized to @code{shift},
diff --git a/etc/NEWS b/etc/NEWS
index c8400ba8c2..411ea72e7c 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -367,6 +367,14 @@ trying to be non-destructive.
 This command opens a new buffer called "*Memory Report*" and gives a
 summary of where Emacs is using memory currently.
 
++++
+** New user option 'isearch-allow-match-scroll'.
+When this option is set, the commands 'beginning-of-buffer',
+'end-of-buffer', 'scroll-up-command' and 'scroll-down-command' move
+respectively to the first first occurrence of the current search
+string in the buffer, the last one, the first one after the current
+window, and the last one before the current window.
+
 ** Outline
 
 +++
diff --git a/lisp/isearch.el b/lisp/isearch.el
index 943e24aa56..bdb2f147ab 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -1942,6 +1942,23 @@ isearch-end-of-buffer
     (goto-char (point-max))
     (isearch-repeat 'backward arg)))
 
+(defun isearch-scroll-up ()
+  "Go to the first occurrence of the current search string after window.
+Move point to the end of the window, and search forward."
+  (interactive)
+  (setq isearch-just-started t)
+  (goto-char (window-end))
+  (isearch-repeat 'forward))
+
+(defun isearch-scroll-down ()
+  "Go to the first occurrence of the current search string before window.
+Move point to the beginning of the window, recenter it, and search backward."
+  (interactive)
+  (setq isearch-just-started t)
+  (goto-char (window-start))
+  (recenter nil t)
+  (isearch-repeat 'backward))
+
 \f
 ;;; Toggles for `isearch-regexp-function' and `search-default-mode'.
 (defmacro isearch-define-mode-toggle (mode key function &optional docstring &rest body)
@@ -2840,12 +2857,35 @@ isearch-allow-scroll
 However, you cannot scroll far enough that the current match is
 no longer visible (is off screen).  But if the value is `unlimited'
 that limitation is removed and you can scroll any distance off screen.
-If nil, scrolling commands exit Isearch mode."
+If nil, scrolling commands exit Isearch mode.
+See also the related option `isearch-allow-match-scroll'."
   :type '(choice (const :tag "Scrolling exits Isearch" nil)
                  (const :tag "Scrolling with current match on screen" t)
                  (const :tag "Scrolling with current match off screen" unlimited))
   :group 'isearch)
 
+(put 'beginning-of-buffer 'isearch-match-scroll t)
+(put 'end-of-buffer 'isearch-match-scroll t)
+(put 'scroll-up-command 'isearch-match-scroll t)
+(put 'scroll-down-command 'isearch-match-scroll t)
+
+(put 'beginning-of-buffer 'isearch-match-scroll-command 'isearch-beginning-of-buffer)
+(put 'end-of-buffer 'isearch-match-scroll-command 'isearch-end-of-buffer)
+(put 'scroll-up-command 'isearch-match-scroll-command 'isearch-scroll-up)
+(put 'scroll-down-command 'isearch-match-scroll-command 'isearch-scroll-down)
+
+(defcustom isearch-allow-match-scroll nil
+  "Whether scrolling to another match is allowed during incremental search.
+If non-nil, the four scrolling commands `beginning-of-buffer',
+`end-of-buffer', `scroll-up-command' and `scroll-down-command' move
+respectively to the first first occurrence of the current search string in
+the buffer, the last one, the first one after the current window, and the
+last one before the current window.
+See also the related option `isearch-allow-scroll'."
+  :type '(choice (const :tag "Off" nil)
+                 (const tag "On" t))
+  :group 'isearch)
+
 (defcustom isearch-allow-prefix t
   "Whether prefix arguments are allowed during incremental search.
 If non-nil, entering a prefix argument will not terminate the
@@ -2947,6 +2987,11 @@ isearch-pre-command-hook
      ;; Optionally edit the search string instead of exiting.
      ((eq search-exit-option 'edit)
       (setq this-command 'isearch-edit-string))
+     ;; Handle match scrolling functions
+     ((and isearch-allow-match-scroll
+           (symbolp this-command)
+           (eq (get this-command 'isearch-match-scroll) t))
+      (setq this-command (get this-command 'isearch-match-scroll-command)))
      ;; Handle a scrolling function or prefix argument.
      ((or (and isearch-allow-prefix
                (memq this-command '(universal-argument universal-argument-more
-- 
2.30.2

Re: Simple isearch concerns

[Juri Linkov]

> Sorry, there was a typo in the previous patch, I attach a corrected one.

Thanks, please see the comments below:

> +(put 'beginning-of-buffer 'isearch-match-scroll t)
> +(put 'end-of-buffer 'isearch-match-scroll t)
> +(put 'scroll-up-command 'isearch-match-scroll t)
> +(put 'scroll-down-command 'isearch-match-scroll t)
> +
> +(put 'beginning-of-buffer 'isearch-match-scroll-command 'isearch-beginning-of-buffer)
> +(put 'end-of-buffer 'isearch-match-scroll-command 'isearch-end-of-buffer)
> +(put 'scroll-up-command 'isearch-match-scroll-command 'isearch-scroll-up)
> +(put 'scroll-down-command 'isearch-match-scroll-command 'isearch-scroll-down)

Why such unnecessary duplication?  When 'beginning-of-buffer' has the
property 'isearch-match-scroll-command' this means it has a non-nil value.
There is no need to duplicate this fact with isearch-match-scroll=t.

> +  :type '(choice (const :tag "Off" nil)
> +                 (const tag "On" t))
                           ===
                           :tag

Re: Simple isearch concerns

[Gregory Heytings]

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


Thanks for reviewing, again!

>> +(put 'beginning-of-buffer 'isearch-match-scroll t)
>> +(put 'end-of-buffer 'isearch-match-scroll t)
>> +(put 'scroll-up-command 'isearch-match-scroll t)
>> +(put 'scroll-down-command 'isearch-match-scroll t)
>> +
>> +(put 'beginning-of-buffer 'isearch-match-scroll-command 'isearch-beginning-of-buffer)
>> +(put 'end-of-buffer 'isearch-match-scroll-command 'isearch-end-of-buffer)
>> +(put 'scroll-up-command 'isearch-match-scroll-command 'isearch-scroll-up)
>> +(put 'scroll-down-command 'isearch-match-scroll-command 'isearch-scroll-down)
>
> Why such unnecessary duplication?  When 'beginning-of-buffer' has the 
> property 'isearch-match-scroll-command' this means it has a non-nil 
> value. There is no need to duplicate this fact with 
> isearch-match-scroll=t.
>

Indeed, see the attached simplified patch.

>> +  :type '(choice (const :tag "Off" nil)
>> +                 (const tag "On" t))
>                           ===
>                           :tag
>

Fixed.

[-- Attachment #2: Type: text/x-diff, Size: 5983 bytes --]

From 740fbab328d2043c55edede5cd737207cc24c14c Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Tue, 6 Apr 2021 19:30:16 +0000
Subject: [PATCH] New user option to scroll isearch matches

* lisp/isearch.el (isearch-allow-match-scroll): New user option.
(isearch-scroll-up, isearch-scroll-down): New commands to scroll
the current search match, that complement the existing
isearch-beginning-of-buffer and isearch-end-of-buffer commands.
(isearch-pre-command-hook): Handle the new option and commands.

* etc/NEWS: Mention the new user option.

* doc/emacs/search.texi: Document the new user option.
---
 doc/emacs/search.texi |  8 ++++++++
 etc/NEWS              |  8 ++++++++
 lisp/isearch.el       | 42 +++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/doc/emacs/search.texi b/doc/emacs/search.texi
index f3c42bcea7..2ae7fd24b2 100644
--- a/doc/emacs/search.texi
+++ b/doc/emacs/search.texi
@@ -587,6 +587,14 @@ or the selected window and frame.  The command must not itself attempt
 an incremental search.  This feature is disabled if
 @code{isearch-allow-scroll} is @code{nil} (which it is by default).
 
+@vindex isearch-allow-match-scroll
+  Likewise, if you change the variable @code{isearch-allow-match-scroll}
+to a non-@code{nil} value, this enables the use of the keyboard scrolling
+commands @kbd{M-<}, @kbd{M->}, @kbd{C-v} and @kbd{M-v}, to move
+respectively to the first first occurrence of the current search
+string in the buffer, the last one, the first one after the current
+window, and the last one before the current window.
+
 @item Motion Commands
 @cindex motion commands, during incremental search
 When @code{isearch-yank-on-move} is customized to @code{shift},
diff --git a/etc/NEWS b/etc/NEWS
index c8400ba8c2..411ea72e7c 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -367,6 +367,14 @@ trying to be non-destructive.
 This command opens a new buffer called "*Memory Report*" and gives a
 summary of where Emacs is using memory currently.
 
++++
+** New user option 'isearch-allow-match-scroll'.
+When this option is set, the commands 'beginning-of-buffer',
+'end-of-buffer', 'scroll-up-command' and 'scroll-down-command' move
+respectively to the first first occurrence of the current search
+string in the buffer, the last one, the first one after the current
+window, and the last one before the current window.
+
 ** Outline
 
 +++
diff --git a/lisp/isearch.el b/lisp/isearch.el
index 943e24aa56..ed7b62d2ee 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -1942,6 +1942,23 @@ type \\[isearch-repeat-backward] with a numeric argument."
     (goto-char (point-max))
     (isearch-repeat 'backward arg)))
 
+(defun isearch-scroll-up ()
+  "Go to the first occurrence of the current search string after window.
+Move point to the end of the window, and search forward."
+  (interactive)
+  (setq isearch-just-started t)
+  (goto-char (window-end))
+  (isearch-repeat 'forward))
+
+(defun isearch-scroll-down ()
+  "Go to the first occurrence of the current search string before window.
+Move point to the beginning of the window, recenter it, and search backward."
+  (interactive)
+  (setq isearch-just-started t)
+  (goto-char (window-start))
+  (recenter nil t)
+  (isearch-repeat 'backward))
+
 \f
 ;;; Toggles for `isearch-regexp-function' and `search-default-mode'.
 (defmacro isearch-define-mode-toggle (mode key function &optional docstring &rest body)
@@ -2840,12 +2857,30 @@ If non-nil, scrolling commands can be used in Isearch mode.
 However, you cannot scroll far enough that the current match is
 no longer visible (is off screen).  But if the value is `unlimited'
 that limitation is removed and you can scroll any distance off screen.
-If nil, scrolling commands exit Isearch mode."
+If nil, scrolling commands exit Isearch mode.
+See also the related option `isearch-allow-match-scroll'."
   :type '(choice (const :tag "Scrolling exits Isearch" nil)
                  (const :tag "Scrolling with current match on screen" t)
                  (const :tag "Scrolling with current match off screen" unlimited))
   :group 'isearch)
 
+(put 'beginning-of-buffer 'isearch-match-scroll 'isearch-beginning-of-buffer)
+(put 'end-of-buffer 'isearch-match-scroll 'isearch-end-of-buffer)
+(put 'scroll-up-command 'isearch-match-scroll 'isearch-scroll-up)
+(put 'scroll-down-command 'isearch-match-scroll 'isearch-scroll-down)
+
+(defcustom isearch-allow-match-scroll nil
+  "Whether scrolling to another match is allowed during incremental search.
+If non-nil, the four scrolling commands `beginning-of-buffer',
+`end-of-buffer', `scroll-up-command' and `scroll-down-command' move
+respectively to the first first occurrence of the current search string in
+the buffer, the last one, the first one after the current window, and the
+last one before the current window.
+See also the related option `isearch-allow-scroll'."
+  :type '(choice (const :tag "Off" nil)
+                 (const :tag "On" t))
+  :group 'isearch)
+
 (defcustom isearch-allow-prefix t
   "Whether prefix arguments are allowed during incremental search.
 If non-nil, entering a prefix argument will not terminate the
@@ -2947,6 +2982,11 @@ See more for options in `search-exit-option'."
      ;; Optionally edit the search string instead of exiting.
      ((eq search-exit-option 'edit)
       (setq this-command 'isearch-edit-string))
+     ;; Handle match scrolling functions.
+     ((and isearch-allow-match-scroll
+           (symbolp this-command)
+           (get this-command 'isearch-match-scroll))
+      (setq this-command (get this-command 'isearch-match-scroll)))
      ;; Handle a scrolling function or prefix argument.
      ((or (and isearch-allow-prefix
                (memq this-command '(universal-argument universal-argument-more
-- 
2.30.2

Re: Simple isearch concerns

[Juri Linkov]

> +(put 'beginning-of-buffer 'isearch-match-scroll 'isearch-beginning-of-buffer)
> +(put 'end-of-buffer 'isearch-match-scroll 'isearch-end-of-buffer)
> +(put 'scroll-up-command 'isearch-match-scroll 'isearch-scroll-up)
> +(put 'scroll-down-command 'isearch-match-scroll 'isearch-scroll-down)
> ...
> +     ((and isearch-allow-match-scroll
> +           (symbolp this-command)
> +           (get this-command 'isearch-match-scroll))
> +      (setq this-command (get this-command 'isearch-match-scroll)))

As noted in another message, this is basically the same as:

  (define-key isearch-mode-map [remap beginning-of-buffer] 'isearch-beginning-of-buffer)
  (define-key isearch-mode-map [remap end-of-buffer] 'isearch-end-of-buffer)
  (define-key isearch-mode-map [remap scroll-up-command] 'isearch-scroll-up)
  (define-key isearch-mode-map [remap scroll-down-command] 'isearch-scroll-down)

Re: Simple isearch concerns

[Gregory Heytings]

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


>> +(put 'beginning-of-buffer 'isearch-match-scroll 'isearch-beginning-of-buffer)
>> +(put 'end-of-buffer 'isearch-match-scroll 'isearch-end-of-buffer)
>> +(put 'scroll-up-command 'isearch-match-scroll 'isearch-scroll-up)
>> +(put 'scroll-down-command 'isearch-match-scroll 'isearch-scroll-down)
>> ...
>> +     ((and isearch-allow-match-scroll
>> +           (symbolp this-command)
>> +           (get this-command 'isearch-match-scroll))
>> +      (setq this-command (get this-command 'isearch-match-scroll)))
>
> As noted in another message, this is basically the same as:
>
>  (define-key isearch-mode-map [remap beginning-of-buffer] 'isearch-beginning-of-buffer)
>  (define-key isearch-mode-map [remap end-of-buffer] 'isearch-end-of-buffer)
>  (define-key isearch-mode-map [remap scroll-up-command] 'isearch-scroll-up)
>  (define-key isearch-mode-map [remap scroll-down-command] 'isearch-scroll-down)
>

I'm not sure I understand what you mean by this.  Indeed this can be done 
that way, too, but in that case it isn't a user option anymore, which can 
be easily set and toggled.

I attach an updated patch, with an improved documentation (including the 
typo spotted by Howard Melman).

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=0001-New-user-option-to-scroll-isearch-matches.patch, Size: 6914 bytes --]

From 3e1c5ced298f9091b4551f3ea7313ca29b3f7884 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Wed, 7 Apr 2021 17:15:02 +0000
Subject: [PATCH] New user option to scroll isearch matches

* lisp/isearch.el (isearch-allow-match-scroll): New user option.
(isearch-scroll-up, isearch-scroll-down): New commands to scroll
the current search match, that complement the existing
isearch-beginning-of-buffer and isearch-end-of-buffer commands.
(isearch-pre-command-hook): Handle the new option and commands.

* etc/NEWS: Mention the new user option.

* doc/emacs/search.texi: Document the new user option.
---
 doc/emacs/search.texi | 21 +++++++++++++++++++++
 etc/NEWS              |  8 ++++++++
 lisp/isearch.el       | 42 +++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 70 insertions(+), 1 deletion(-)

diff --git a/doc/emacs/search.texi b/doc/emacs/search.texi
index f3c42bcea7..2993377f04 100644
--- a/doc/emacs/search.texi
+++ b/doc/emacs/search.texi
@@ -587,6 +587,14 @@ or the selected window and frame.  The command must not itself attempt
 an incremental search.  This feature is disabled if
 @code{isearch-allow-scroll} is @code{nil} (which it is by default).
 
+@vindex isearch-allow-match-scroll
+  Likewise, if you change the variable @code{isearch-allow-match-scroll}
+to a non-@code{nil} value, this enables the use of the keyboard scrolling
+commands @kbd{M-<}, @kbd{M->}, @kbd{C-v} and @kbd{M-v}, to move
+respectively to the first occurrence of the current search string in
+the buffer, the last one, the first one after the current window,
+and the last one before the current window.
+
 @item Motion Commands
 @cindex motion commands, during incremental search
 When @code{isearch-yank-on-move} is customized to @code{shift},
@@ -598,6 +606,19 @@ When @code{isearch-yank-on-move} is @code{t}, you can extend the
 search string without using the shift key for cursor motion commands,
 but it applies only for certain motion command that have the
 @code{isearch-move} property on their symbols.
+
+@cindex motion commands, during incremental search, change
+  When @code{isearch-allow-match-scroll} is non-@code{nil}, it
+is also possible to change the effect of motion commands during
+incremental search, by modifying the @code{isearch-match-scroll}
+property of their symbols.  For example, to make @kbd{C-f} and
+@kbd{C-b} move to the next and previous match, you can put the
+following lines in your init file (@pxref{Init File}):
+
+@example
+(put 'forward-char 'isearch-match-scroll 'isearch-repeat-forward)
+(put 'backward-char 'isearch-match-scroll 'isearch-repeat-backward)
+@end example
 @end table
 
 @node Isearch Minibuffer
diff --git a/etc/NEWS b/etc/NEWS
index d3a8748ded..7411e3de10 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -367,6 +367,14 @@ trying to be non-destructive.
 This command opens a new buffer called "*Memory Report*" and gives a
 summary of where Emacs is using memory currently.
 
++++
+** New user option 'isearch-allow-match-scroll'.
+When this option is set, the commands 'beginning-of-buffer',
+'end-of-buffer', 'scroll-up-command' and 'scroll-down-command' move
+respectively to the first occurrence of the current search string
+in the buffer, the last one, the first one after the current window,
+and the last one before the current window.
+
 ** Outline
 
 +++
diff --git a/lisp/isearch.el b/lisp/isearch.el
index a828c569aa..96ed179c84 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -1970,6 +1970,23 @@ type \\[isearch-repeat-backward] with a numeric argument."
     (goto-char (point-max))
     (isearch-repeat 'backward arg)))
 
+(defun isearch-scroll-up ()
+  "Go to the first occurrence of the current search string after window.
+Move point to the end of the window, and search forward."
+  (interactive)
+  (setq isearch-just-started t)
+  (goto-char (window-end))
+  (isearch-repeat 'forward))
+
+(defun isearch-scroll-down ()
+  "Go to the first occurrence of the current search string before window.
+Move point to the beginning of the window, recenter it, and search backward."
+  (interactive)
+  (setq isearch-just-started t)
+  (goto-char (window-start))
+  (recenter nil t)
+  (isearch-repeat 'backward))
+
 \f
 ;;; Toggles for `isearch-regexp-function' and `search-default-mode'.
 (defmacro isearch-define-mode-toggle (mode key function &optional docstring &rest body)
@@ -2868,12 +2885,30 @@ If non-nil, scrolling commands can be used in Isearch mode.
 However, you cannot scroll far enough that the current match is
 no longer visible (is off screen).  But if the value is `unlimited'
 that limitation is removed and you can scroll any distance off screen.
-If nil, scrolling commands exit Isearch mode."
+If nil, scrolling commands exit Isearch mode.
+See also the related option `isearch-allow-match-scroll'."
   :type '(choice (const :tag "Scrolling exits Isearch" nil)
                  (const :tag "Scrolling with current match on screen" t)
                  (const :tag "Scrolling with current match off screen" unlimited))
   :group 'isearch)
 
+(put 'beginning-of-buffer 'isearch-match-scroll 'isearch-beginning-of-buffer)
+(put 'end-of-buffer 'isearch-match-scroll 'isearch-end-of-buffer)
+(put 'scroll-up-command 'isearch-match-scroll 'isearch-scroll-up)
+(put 'scroll-down-command 'isearch-match-scroll 'isearch-scroll-down)
+
+(defcustom isearch-allow-match-scroll nil
+  "Whether scrolling to another match is allowed during incremental search.
+If non-nil, the four scrolling commands `beginning-of-buffer',
+`end-of-buffer', `scroll-up-command' and `scroll-down-command' move
+respectively to the first first occurrence of the current search string in
+the buffer, the last one, the first one after the current window, and the
+last one before the current window.
+See also the related option `isearch-allow-scroll'."
+  :type '(choice (const :tag "Off" nil)
+                 (const :tag "On" t))
+  :group 'isearch)
+
 (defcustom isearch-allow-prefix t
   "Whether prefix arguments are allowed during incremental search.
 If non-nil, entering a prefix argument will not terminate the
@@ -2975,6 +3010,11 @@ See more for options in `search-exit-option'."
      ;; Optionally edit the search string instead of exiting.
      ((eq search-exit-option 'edit)
       (setq this-command 'isearch-edit-string))
+     ;; Handle match scrolling functions.
+     ((and isearch-allow-match-scroll
+           (symbolp this-command)
+           (get this-command 'isearch-match-scroll))
+      (setq this-command (get this-command 'isearch-match-scroll)))
      ;; Handle a scrolling function or prefix argument.
      ((or (and isearch-allow-prefix
                (memq this-command '(universal-argument universal-argument-more
-- 
2.30.2

Re: Simple isearch concerns

[Juri Linkov]

>>> +(put 'beginning-of-buffer 'isearch-match-scroll 'isearch-beginning-of-buffer)
>>> +(put 'end-of-buffer 'isearch-match-scroll 'isearch-end-of-buffer)
>>> +(put 'scroll-up-command 'isearch-match-scroll 'isearch-scroll-up)
>>> +(put 'scroll-down-command 'isearch-match-scroll 'isearch-scroll-down)
>>> ...
>>> +     ((and isearch-allow-match-scroll
>>> +           (symbolp this-command)
>>> +           (get this-command 'isearch-match-scroll))
>>> +      (setq this-command (get this-command 'isearch-match-scroll)))
>>
>> As noted in another message, this is basically the same as:
>>
>>  (define-key isearch-mode-map [remap beginning-of-buffer] 'isearch-beginning-of-buffer)
>>  (define-key isearch-mode-map [remap end-of-buffer] 'isearch-end-of-buffer)
>>  (define-key isearch-mode-map [remap scroll-up-command] 'isearch-scroll-up)
>>  (define-key isearch-mode-map [remap scroll-down-command] 'isearch-scroll-down)
>
> I'm not sure I understand what you mean by this.  Indeed this can be done
> that way, too, but in that case it isn't a user option anymore, which can
> be easily set and toggled.

If you want a separate option, this is fine.  But then there is no need
to duplicate the key remapping feature.  To make the new feature more powerful,
you could attach to command symbol properties a function that
defines where to move point before searching the next match.

This is similar to how isearch-yank-internal funcalls its arg 'jumpform',
and how its callers like isearch-yank-char uses

  (lambda () (forward-char arg) (point))

or isearch-yank-word uses

  (lambda () (forward-word arg) (point))

to define where to move point before using it as the buffer position.

So command symbol properties could be defined using the same logic:

  (put 'beginning-of-buffer 'isearch-match-scroll 'beginning-of-buffer)
  (put 'end-of-buffer 'isearch-match-scroll 'end-of-buffer)
  (put 'scroll-up-command 'isearch-match-scroll (lambda () (goto-char (window-end))))
  (put 'scroll-down-command 'isearch-match-scroll (lambda () (goto-char (window-start))))

where the symbol property is called like (funcall 'beginning-of-buffer)
before repeating the search.

Then there is no need to add such ad-hoc commands as isearch-scroll-up
and isearch-scroll-down.

And instead of

  (setq this-command (get this-command 'isearch-match-scroll))

isearch-pre-command-hook could contain the same code
that is currently duplicated in several commands:

  (setq isearch-just-started t)
  (goto-char (window-end))  ;; only this line needs to be
                            ;; replaced with (funcall jumpform)
  (isearch-repeat 'forward)

Whereas it would be easy to handle more commands like below:

>>>>> Should then forward-char be remapped to isearch-repeat to go to the
>>>>> next match?  Then also remap next-line to go to the next line with
>>>>> matches, etc.
>>>>
>>>> Please don't, these commands should exit the search.
>>>
>>> Yes, I also think these commands should exit the search by
>>> default. Users who would like to have that behavior can do so easily
>>> with my patch:
>>>
>>> (put 'next-line 'isearch-match-scroll 'isearch-repeat-forward)
>>
>> Then 'isearch-match-scroll' has nothing to do with scrolling.
>
> It was you who suggested that change, you asked: "Should then forward-char
> be remapped to isearch-repeat to go to the next match?"  Or am
> I misunderstanding something?

Actually, the 'isearch-match-scroll' above doesn't do what is needed.
It just goes to the next match, whereas the need was to go to the next line
before searching for a new match.

With the logic above, this is easy to define like:

  (put 'next-line 'isearch-match-scroll (lambda () (forward-line 1)))

Re: Simple isearch concerns

[Gregory Heytings]

Thanks for your detailed comments!  I think I see what you mean, but I see 
a problem in your proposed solution:

>
> And instead of
>
>  (setq this-command (get this-command 'isearch-match-scroll))
>
> isearch-pre-command-hook could contain the same code
> that is currently duplicated in several commands:
>
>  (setq isearch-just-started t)
>  (goto-char (window-end))  ;; only this line needs to be
>                            ;; replaced with (funcall jumpform)
>  (isearch-repeat 'forward)
>

I'm not sure this would work, because it's not always (isearch-repeat 
'forward).  For example with M-v it would be 'backward, regardless of the 
current direction of the search.  So I guess it means that another symbol 
property would be necessary.

With C-p it's not clear (to me) if it should depend on the current 
direction of the search or not.  Should C-s C-p change the direction to a 
backward search, or move to previous line and restart a forward search?

Re: Simple isearch concerns

[Juri Linkov]

>> isearch-pre-command-hook could contain the same code
>> that is currently duplicated in several commands:
>>
>>  (setq isearch-just-started t)
>>  (goto-char (window-end))  ;; only this line needs to be
>>                            ;; replaced with (funcall jumpform)
>>  (isearch-repeat 'forward)
>>
>
> I'm not sure this would work, because it's not always (isearch-repeat
> 'forward).  For example with M-v it would be 'backward, regardless of the
> current direction of the search.  So I guess it means that another symbol
> property would be necessary.

A function attached to a symbol could return its search direction.
Indeed, the definitions will be slightly longer, but still this is
better than defining new commands.  So this could look like:

  (put 'beginning-of-buffer 'isearch-match-scroll (lambda () (goto-char (point-min))) 'forward)
  (put 'end-of-buffer 'isearch-match-scroll (lambda () (goto-char (point-max)) 'backward))
  (put 'scroll-up-command 'isearch-match-scroll (lambda () (goto-char (window-end)) 'forward))
  (put 'scroll-down-command 'isearch-match-scroll (lambda () (goto-char (window-start)) 'backward))

> With C-p it's not clear (to me) if it should depend on the current
> direction of the search or not.  Should C-s C-p change the direction to
> a backward search, or move to previous line and restart a forward search?

The same question applies to scroll-up-command and scroll-down-command, e.g.
in a forward search using scroll-up-command should change the direction?

Maybe this preference should be specified by the same your new option
isearch-allow-match-scroll, so when its value is t, then the enabled scrolling
doesn't change the direction.  But it does this when its value is e.g.
'change-direction'.

Re: Simple isearch concerns

[Gregory Heytings]

>> I'm not sure this would work, because it's not always (isearch-repeat 
>> 'forward).  For example with M-v it would be 'backward, regardless of 
>> the current direction of the search.  So I guess it means that another 
>> symbol property would be necessary.
>
> A function attached to a symbol could return its search direction. 
> Indeed, the definitions will be slightly longer, but still this is 
> better than defining new commands.  So this could look like:
>
>  (put 'beginning-of-buffer 'isearch-match-scroll (lambda () (goto-char (point-min))) 'forward)
>  (put 'end-of-buffer 'isearch-match-scroll (lambda () (goto-char (point-max)) 'backward))
>  (put 'scroll-up-command 'isearch-match-scroll (lambda () (goto-char (window-end)) 'forward))
>  (put 'scroll-down-command 'isearch-match-scroll (lambda () (goto-char (window-start)) 'backward))
>

Thank you!  Indeed that should work, I'll try to do that.

>> With C-p it's not clear (to me) if it should depend on the current 
>> direction of the search or not.  Should C-s C-p change the direction to 
>> a backward search, or move to previous line and restart a forward 
>> search?
>
> The same question applies to scroll-up-command and scroll-down-command, 
> e.g. in a forward search using scroll-up-command should change the 
> direction?
>

For scroll-up-command and scroll-down-command that would not make sense: 
it would mean that you start searching at window-end and search backward.

Re: Simple isearch concerns

[Juri Linkov]

>>> With C-p it's not clear (to me) if it should depend on the current
>>> direction of the search or not.  Should C-s C-p change the direction to
>>> a backward search, or move to previous line and restart
>>> a forward search?
>>
>> The same question applies to scroll-up-command and scroll-down-command,
>> e.g. in a forward search using scroll-up-command should change
>> the direction?
>
> For scroll-up-command and scroll-down-command that would not make sense: it
> would mean that you start searching at window-end and search backward.

Surely it should search forward after moving to window-end.
But the question is: when the search direction was backward initially
before calling scroll-up-command, should it keep the same
backward direction, or should it change the direction to forward
(since internally it used the forward search after moving to window-end).

Re: Simple isearch concerns

[Gregory Heytings]

>>>> With C-p it's not clear (to me) if it should depend on the current 
>>>> direction of the search or not.  Should C-s C-p change the direction 
>>>> to a backward search, or move to previous line and restart a forward 
>>>> search?
>>>
>>> The same question applies to scroll-up-command and 
>>> scroll-down-command, e.g. in a forward search using scroll-up-command 
>>> should change the direction?
>>
>> For scroll-up-command and scroll-down-command that would not make 
>> sense: it would mean that you start searching at window-end and search 
>> backward.
>
> Surely it should search forward after moving to window-end. But the 
> question is: when the search direction was backward initially before 
> calling scroll-up-command, should it keep the same backward direction, 
> or should it change the direction to forward (since internally it used 
> the forward search after moving to window-end).
>

Do I understand correctly that your remark is about the internal variable 
'isearch-forward', and whether its value should change when using "fast 
forward/backward" searches?  If so, would that make a difference in 
practice?  IIUC it is changed by isearch-beginning-of-buffer and 
isearch-end-of-buffer.

Re: Simple isearch concerns

[Juri Linkov]

>> Surely it should search forward after moving to window-end. But the
>> question is: when the search direction was backward initially before
>> calling scroll-up-command, should it keep the same backward direction, or
>> should it change the direction to forward (since internally it used the
>> forward search after moving to window-end).
>
> Do I understand correctly that your remark is about the internal variable
> 'isearch-forward', and whether its value should change when using "fast
> forward/backward" searches?  If so, would that make a difference in
> practice?  IIUC it is changed by isearch-beginning-of-buffer and
> isearch-end-of-buffer.

isearch-beginning-of-buffer changes the direction to 'forward'
because it makes no sense to keep it 'backward' - there are no matches
before the first match.

But I agree that when the user called scroll-up-command
then it makes more sense to change the direction to 'forward'
to be able to navigate the matches forward from the top of the next screen
(as opposed to keeping the backward direction to be able to quickly
go back that is less useful).

Re: Simple isearch concerns

[Gregory Heytings]

>> Do I understand correctly that your remark is about the internal 
>> variable 'isearch-forward', and whether its value should change when 
>> using "fast forward/backward" searches?  If so, would that make a 
>> difference in practice?  IIUC it is changed by 
>> isearch-beginning-of-buffer and isearch-end-of-buffer.
>
> isearch-beginning-of-buffer changes the direction to 'forward' because 
> it makes no sense to keep it 'backward' - there are no matches before 
> the first match.
>
> But I agree that when the user called scroll-up-command then it makes 
> more sense to change the direction to 'forward' to be able to navigate 
> the matches forward from the top of the next screen (as opposed to 
> keeping the backward direction to be able to quickly go back that is 
> less useful).
>

Yes, but even if it the direction did not change, I don't see what 
difference it would make in practice.  Because to repeat a search the user 
would have to type either C-s or C-r, thereby changing the direction 
anyway.  Am I missing something?

Re: Simple isearch concerns

[Gregory Heytings]

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


Thanks again for your detailed comments!

Here is the new patch.  As you will see, it is possible to put nil in the 
cdr of the symbol property, in which case the search direction does not 
change, but I would not recommend to do that: I tried it, and the 
resulting behavior is not what one would expect, very often the match 
doesn't change at all.  Therefore I did not mention that possibility in 
the manual.

Also, there's a condition-case around the funcall, to catch beginning of 
buffer / end of buffer errors.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=0001-New-user-option-to-scroll-isearch-matches.patch, Size: 6478 bytes --]

From c5855569efdcd5af20ea37609ece3f0eef9c4f3d Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Fri, 8 Apr 2021 22:30:07 +0000
Subject: [PATCH] New user option to scroll isearch matches

* lisp/isearch.el (isearch-allow-match-scroll): New user option.
(isearch-pre-command-hook): Handle the new option.

* etc/NEWS: Mention the new user option.

* doc/emacs/search.texi: Document the new user option.
---
 doc/emacs/search.texi | 22 ++++++++++++++++++++++
 etc/NEWS              | 10 ++++++++++
 lisp/isearch.el       | 41 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/doc/emacs/search.texi b/doc/emacs/search.texi
index f3c42bcea7..59e2841611 100644
--- a/doc/emacs/search.texi
+++ b/doc/emacs/search.texi
@@ -587,6 +587,14 @@ Not Exiting Isearch
 an incremental search.  This feature is disabled if
 @code{isearch-allow-scroll} is @code{nil} (which it is by default).
 
+@vindex isearch-allow-match-scroll
+  Likewise, if you change the variable @code{isearch-allow-match-scroll}
+to a non-@code{nil} value, this enables the use of the keyboard scrolling
+commands @kbd{M-<}, @kbd{M->}, @kbd{C-v} and @kbd{M-v}, to move
+respectively to the first occurrence of the current search string in
+the buffer, the last one, the first one after the current window,
+and the last one before the current window.
+
 @item Motion Commands
 @cindex motion commands, during incremental search
 When @code{isearch-yank-on-move} is customized to @code{shift},
@@ -598,6 +606,20 @@ Not Exiting Isearch
 search string without using the shift key for cursor motion commands,
 but it applies only for certain motion command that have the
 @code{isearch-move} property on their symbols.
+
+@cindex motion commands, during incremental search, change
+  When @code{isearch-allow-match-scroll} is non-@code{nil}, it
+is also possible to change the effect of motion commands during
+incremental search, by modifying the @code{isearch-match-scroll}
+property of their symbols.  For example, to make @kbd{C-p} and
+@kbd{C-n} move to the previous and next line and restart Isearch
+forward and backward respectively, you can put the following
+lines in your init file (@pxref{Init File}):
+
+@example
+(put 'next-line 'isearch-match-scroll '(next-line . forward))
+(put 'previous-line 'isearch-match-scroll '(previous-line . backward))
+@end example
 @end table
 
 @node Isearch Minibuffer
diff --git a/etc/NEWS b/etc/NEWS
index d3a8748ded..4d4b2f48e1 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -367,6 +367,16 @@ trying to be non-destructive.
 This command opens a new buffer called "*Memory Report*" and gives a
 summary of where Emacs is using memory currently.
 
++++
+** New user option 'isearch-allow-match-scroll'.
+When this option is set, the commands 'beginning-of-buffer',
+'end-of-buffer', 'scroll-up-command' and 'scroll-down-command' move
+respectively to the first occurrence of the current search string
+in the buffer, the last one, the first one after the current window,
+and the last one before the current window.  Additionally, users can
+change the meaning of other motion commands during Isearch by using
+their 'isearch-match-scroll' property.
+
 ** Outline
 
 +++
diff --git a/lisp/isearch.el b/lisp/isearch.el
index 4b4f44bdff..ca3baf5eee 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -2866,12 +2866,37 @@ isearch-allow-scroll
 However, you cannot scroll far enough that the current match is
 no longer visible (is off screen).  But if the value is `unlimited'
 that limitation is removed and you can scroll any distance off screen.
-If nil, scrolling commands exit Isearch mode."
+If nil, scrolling commands exit Isearch mode.
+See also the related option `isearch-allow-match-scroll'."
   :type '(choice (const :tag "Scrolling exits Isearch" nil)
                  (const :tag "Scrolling with current match on screen" t)
                  (const :tag "Scrolling with current match off screen" unlimited))
   :group 'isearch)
 
+(put 'beginning-of-buffer 'isearch-match-scroll
+     '((lambda () (goto-char (point-min))) . forward))
+(put 'end-of-buffer 'isearch-match-scroll
+     '((lambda () (goto-char (point-max))) . backward))
+(put 'scroll-up-command 'isearch-match-scroll
+     '((lambda () (goto-char (window-end))) . forward))
+(put 'scroll-down-command 'isearch-match-scroll
+     '((lambda () (goto-char (window-start)) (recenter nil t)) . backward))
+
+(put 'next-line 'isearch-match-scroll '(next-line . forward))
+(put 'previous-line 'isearch-match-scroll '(previous-line . backward))
+
+(defcustom isearch-allow-match-scroll nil
+  "Whether scrolling to another match is allowed during incremental search.
+If non-nil, the four scrolling commands `beginning-of-buffer',
+`end-of-buffer', `scroll-up-command' and `scroll-down-command' move
+respectively to the first first occurrence of the current search string in
+the buffer, the last one, the first one after the current window, and the
+last one before the current window.
+See also the related option `isearch-allow-scroll'."
+  :type '(choice (const :tag "Off" nil)
+                 (const :tag "On" t))
+  :group 'isearch)
+
 (defcustom isearch-allow-prefix t
   "Whether prefix arguments are allowed during incremental search.
 If non-nil, entering a prefix argument will not terminate the
@@ -2973,6 +2998,20 @@ isearch-pre-command-hook
      ;; Optionally edit the search string instead of exiting.
      ((eq search-exit-option 'edit)
       (setq this-command 'isearch-edit-string))
+     ;; Handle match scrolling functions.
+     ((and isearch-allow-match-scroll
+           (symbolp this-command)
+           (get this-command 'isearch-match-scroll))
+      (let* ((property (get this-command 'isearch-match-scroll))
+             (function (car property))
+             (direction (or (cdr property)
+                            (if isearch-forward 'forward 'backward))))
+        (setq isearch-just-started t)
+        (condition-case nil
+            (funcall function)
+          (error nil))
+        (isearch-repeat direction)
+        (setq this-command 'ignore)))
      ;; Handle a scrolling function or prefix argument.
      ((or (and isearch-allow-prefix
                (memq this-command '(universal-argument universal-argument-more
-- 
2.30.2

Re: Simple isearch concerns

[Eli Zaretskii]

> Date: Thu, 08 Apr 2021 22:40:42 +0000
> From: Gregory Heytings <gregory@heytings.org>
> Cc: Ergus <spacibba@aol.com>, emacs-devel@gnu.org
> 
> +@vindex isearch-allow-match-scroll

IMO, the name of this variable (and the corresponding property) is not
the best one.  "Scroll" is the wrong term here, and "match-scroll"
doesn't help understanding what it does.  How about
isearch-allow-motion-commands instead?

> +@cindex motion commands, during incremental search, change
> +  When @code{isearch-allow-match-scroll} is non-@code{nil}, it
> +is also possible to change the effect of motion commands during
> +incremental search, by modifying the @code{isearch-match-scroll}
> +property of their symbols.  For example, to make @kbd{C-p} and
> +@kbd{C-n} move to the previous and next line and restart Isearch
> +forward and backward respectively, you can put the following
> +lines in your init file (@pxref{Init File}):
> +
> +@example
> +(put 'next-line 'isearch-match-scroll '(next-line . forward))
> +(put 'previous-line 'isearch-match-scroll '(previous-line . backward))
> +@end example
>  @end table

This is too "Lispy" IMO to be in the user manual.  I suggest to move
the bulk of it into the ELisp manual, and leave only a reference to
that in the user manual.

> ++++
> +** New user option 'isearch-allow-match-scroll'.
> +When this option is set, the commands 'beginning-of-buffer',
> +'end-of-buffer', 'scroll-up-command' and 'scroll-down-command' move
> +respectively to the first occurrence of the current search string
> +in the buffer, the last one, the first one after the current window,
> +and the last one before the current window.

This sentence should make it clear that it talks about the named
commands that are invoked while in I-search.  Otherwise the sentence
is misleading.

> +(defcustom isearch-allow-match-scroll nil
> +  "Whether scrolling to another match is allowed during incremental search.
              ^^^^^^^^^
  Whether to allow movement between isearch matches by cursor motion commands.

> +If non-nil, the four scrolling commands `beginning-of-buffer',
> +`end-of-buffer', `scroll-up-command' and `scroll-down-command' move
> +respectively to the first first occurrence of the current search string in
> +the buffer, the last one, the first one after the current window, and the
> +last one before the current window.

Presumably if this option is nil, these commands exit I-search and are
executed?  This should be in the doc string.

> +See also the related option `isearch-allow-scroll'."
> +  :type '(choice (const :tag "Off" nil)
> +                 (const :tag "On" t))
> +  :group 'isearch)

The :version tag is missing here.

> +     ;; Handle match scrolling functions.

Again, "scrolling" is the wrong terminology for describing this.

> +        (condition-case nil
> +            (funcall function)
> +          (error nil))

Wouldn't it be better to at least display the error message (or some
message) in the case the function signals an error?

Thanks.

Re: Simple isearch concerns

[Gregory Heytings]

Thanks for your detailed comments!

>> +@vindex isearch-allow-match-scroll
>
> IMO, the name of this variable (and the corresponding property) is not 
> the best one.  "Scroll" is the wrong term here, and "match-scroll" 
> doesn't help understanding what it does.  How about 
> isearch-allow-motion-commands instead?
>

I tend to agree with you, but I'm not sure.  There is already an 
'isearch-allow-scroll' and its corresponding 'isearch-scroll' property, 
which changes the behavior of motion commands: it becomes possible to 
scroll (recenter, scroll-up, scroll-down, ...) but without changing the 
current match.  This patch does something related, but different: it 
becomes possible to scroll among the matches.

>> +@cindex motion commands, during incremental search, change
>> +  When @code{isearch-allow-match-scroll} is non-@code{nil}, it
>> +is also possible to change the effect of motion commands during
>> +incremental search, by modifying the @code{isearch-match-scroll}
>> +property of their symbols.  For example, to make @kbd{C-p} and
>> +@kbd{C-n} move to the previous and next line and restart Isearch
>> +forward and backward respectively, you can put the following
>> +lines in your init file (@pxref{Init File}):
>> +
>> +@example
>> +(put 'next-line 'isearch-match-scroll '(next-line . forward))
>> +(put 'previous-line 'isearch-match-scroll '(previous-line . backward))
>> +@end example
>>  @end table
>
> This is too "Lispy" IMO to be in the user manual.  I suggest to move the 
> bulk of it into the ELisp manual, and leave only a reference to that in 
> the user manual.
>

Okay.  I've put it there because just above the 'isearch-move' and 
'isearch-scroll' properties are discussed, but indeed no example is given, 
and these properties are only t/nil ones.

>> ++++
>> +** New user option 'isearch-allow-match-scroll'.
>> +When this option is set, the commands 'beginning-of-buffer',
>> +'end-of-buffer', 'scroll-up-command' and 'scroll-down-command' move
>> +respectively to the first occurrence of the current search string
>> +in the buffer, the last one, the first one after the current window,
>> +and the last one before the current window.
>
> This sentence should make it clear that it talks about the named 
> commands that are invoked while in I-search.  Otherwise the sentence is 
> misleading.
>

I'm not sure I understand what you mean.  How would you make it clearer?

>> +If non-nil, the four scrolling commands `beginning-of-buffer',
>> +`end-of-buffer', `scroll-up-command' and `scroll-down-command' move
>> +respectively to the first first occurrence of the current search string in
>> +the buffer, the last one, the first one after the current window, and the
>> +last one before the current window.
>
> Presumably if this option is nil, these commands exit I-search and are
> executed?  This should be in the doc string.
>

Indeed, if this option is nil, the meaning of these commands is unchanged, 
which means that they exit Isearch.  Should I add something like "If this 
option is nil, the meaning of these commands is unchanged"?  But that 
could be misleading, because that's not correct when isearch-allow-scroll 
is non-nil...

>> +        (condition-case nil
>> +            (funcall function)
>> +          (error nil))
>
> Wouldn't it be better to at least display the error message (or some 
> message) in the case the function signals an error?
>

In this specific case, no: what happens is that Isearch indicates that the 
search will wrap.

Re: Simple isearch concerns

[Juri Linkov]

>>> +@vindex isearch-allow-match-scroll
>>
>> IMO, the name of this variable (and the corresponding property) is not
>> the best one.  "Scroll" is the wrong term here, and "match-scroll"
>> doesn't help understanding what it does.  How about
>> isearch-allow-motion-commands instead?
>
> I tend to agree with you, but I'm not sure.  There is already an
> 'isearch-allow-scroll' and its corresponding 'isearch-scroll' property,
> which changes the behavior of motion commands: it becomes possible to
> scroll (recenter, scroll-up, scroll-down, ...) but without changing the
> current match.  This patch does something related, but different: it
> becomes possible to scroll among the matches.

Indeed, the name is not worse than 'isearch-allow-scroll'.  Maybe it's possible
to find a better name?  What about 'isearch-allow-repeat' with the meaning that
it repeats the search after moving point.

Re: Simple isearch concerns

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  spacibba@aol.com,  emacs-devel@gnu.org
> Date: Fri, 09 Apr 2021 11:37:17 +0300
> 
> >>> +@vindex isearch-allow-match-scroll
> >>
> >> IMO, the name of this variable (and the corresponding property) is not
> >> the best one.  "Scroll" is the wrong term here, and "match-scroll"
> >> doesn't help understanding what it does.  How about
> >> isearch-allow-motion-commands instead?
> >
> > I tend to agree with you, but I'm not sure.  There is already an
> > 'isearch-allow-scroll' and its corresponding 'isearch-scroll' property,
> > which changes the behavior of motion commands: it becomes possible to
> > scroll (recenter, scroll-up, scroll-down, ...) but without changing the
> > current match.  This patch does something related, but different: it
> > becomes possible to scroll among the matches.
> 
> Indeed, the name is not worse than 'isearch-allow-scroll'.  Maybe it's possible
> to find a better name?

I thought I suggested a better name, right there above, in the cited
part?

Re: Simple isearch concerns

[Juri Linkov]

>> >> How about isearch-allow-motion-commands instead?
>> >
>> > I tend to agree with you, but I'm not sure.  There is already an
>> > 'isearch-allow-scroll' and its corresponding 'isearch-scroll' property,
>> > which changes the behavior of motion commands: it becomes possible to
>> > scroll (recenter, scroll-up, scroll-down, ...) but without changing the
>> > current match.  This patch does something related, but different: it
>> > becomes possible to scroll among the matches.
>>
>> Indeed, the name is not worse than 'isearch-allow-scroll'.  Maybe it's possible
>> to find a better name?
>
> I thought I suggested a better name, right there above, in the cited
> part?

I agree that your suggested name is good, and a shorter name
isearch-allow-motion suggested by Gregory is even better.
And the property name isearch-motion matches the existing scheme of
isearch-allow-scroll and isearch-scroll.

Re: Simple isearch concerns

[Eli Zaretskii]

> Date: Fri, 09 Apr 2021 07:20:14 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: juri@linkov.net, spacibba@aol.com, emacs-devel@gnu.org
> 
> 
> >> +@vindex isearch-allow-match-scroll
> >
> > IMO, the name of this variable (and the corresponding property) is not 
> > the best one.  "Scroll" is the wrong term here, and "match-scroll" 
> > doesn't help understanding what it does.  How about 
> > isearch-allow-motion-commands instead?
> 
> I tend to agree with you, but I'm not sure.  There is already an 
> 'isearch-allow-scroll' and its corresponding 'isearch-scroll' property, 
> which changes the behavior of motion commands: it becomes possible to 
> scroll (recenter, scroll-up, scroll-down, ...) but without changing the 
> current match.  This patch does something related, but different: it 
> becomes possible to scroll among the matches.

The "scroll" part is what I think needs to be amended: the commands
you are affecting are not scroll commands, they are cursor motion
commands.

> >> ++++
> >> +** New user option 'isearch-allow-match-scroll'.
> >> +When this option is set, the commands 'beginning-of-buffer',
> >> +'end-of-buffer', 'scroll-up-command' and 'scroll-down-command' move
> >> +respectively to the first occurrence of the current search string
> >> +in the buffer, the last one, the first one after the current window,
> >> +and the last one before the current window.
> >
> > This sentence should make it clear that it talks about the named 
> > commands that are invoked while in I-search.  Otherwise the sentence is 
> > misleading.
> >
> 
> I'm not sure I understand what you mean.  How would you make it clearer?

I would say "the commands 'beginning-of-buffer', ..., when invoked
during I-search, move respectively to ...".

> >> +If non-nil, the four scrolling commands `beginning-of-buffer',
> >> +`end-of-buffer', `scroll-up-command' and `scroll-down-command' move
> >> +respectively to the first first occurrence of the current search string in
> >> +the buffer, the last one, the first one after the current window, and the
> >> +last one before the current window.
> >
> > Presumably if this option is nil, these commands exit I-search and are
> > executed?  This should be in the doc string.
> 
> Indeed, if this option is nil, the meaning of these commands is unchanged, 
> which means that they exit Isearch.  Should I add something like "If this 
> option is nil, the meaning of these commands is unchanged"?

No, just that they exit the search and are executed normally.

> >> +        (condition-case nil
> >> +            (funcall function)
> >> +          (error nil))
> >
> > Wouldn't it be better to at least display the error message (or some 
> > message) in the case the function signals an error?
> >
> 
> In this specific case, no: what happens is that Isearch indicates that the 
> search will wrap.

I don't understand: why are you wrapping the call with condition-case,
in that case?

Re: Simple isearch concerns

[Gregory Heytings]

>> I tend to agree with you, but I'm not sure.  There is already an 
>> 'isearch-allow-scroll' and its corresponding 'isearch-scroll' property, 
>> which changes the behavior of motion commands: it becomes possible to 
>> scroll (recenter, scroll-up, scroll-down, ...) but without changing the 
>> current match.  This patch does something related, but different: it 
>> becomes possible to scroll among the matches.
>
> The "scroll" part is what I think needs to be amended: the commands you 
> are affecting are not scroll commands, they are cursor motion commands.
>

Okay, so what you mean by "scroll" in this context is "change the visible 
portion of the buffer without moving the cursor".  I think I understand 
your point, even though that's not what scroll-up-command and 
scroll-down-command do.  Would "isearch-allow-motion" and the property 
"isearch-motion" be okay?

>
> I don't understand: why are you wrapping the call with condition-case, 
> in that case?
>

It's not necessary (but AFAIU it's harmless) for the four commands that 
are enabled by isearch-allow-motion, it is also not necessary for:

(put 'next-line 'isearch-match-scroll '(next-line . forward))

which will correctly wrap at EOB, but it necessary for:

(put 'previous-line 'isearch-match-scroll '(previous-line . backward))

For some reason, in this case if the error is not catched, Isearch does 
not wrap at BOB, but instead enters in a strange state where you can move 
the cursor around.  This might be a bug that should be corrected instead 
of circumventing it by catching the error.

Re: Simple isearch concerns

[Eli Zaretskii]

> Date: Fri, 09 Apr 2021 11:27:41 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: juri@linkov.net, spacibba@aol.com, emacs-devel@gnu.org
> 
> > The "scroll" part is what I think needs to be amended: the commands you 
> > are affecting are not scroll commands, they are cursor motion commands.
> >
> 
> Okay, so what you mean by "scroll" in this context is "change the visible 
> portion of the buffer without moving the cursor".  I think I understand 
> your point, even though that's not what scroll-up-command and 
> scroll-down-command do.  Would "isearch-allow-motion" and the property 
> "isearch-motion" be okay?

Yes, this is better.  Although perhaps the property and the variable
should have the same name.

> It's not necessary (but AFAIU it's harmless) for the four commands that 
> are enabled by isearch-allow-motion, it is also not necessary for:
> 
> (put 'next-line 'isearch-match-scroll '(next-line . forward))
> 
> which will correctly wrap at EOB, but it necessary for:
> 
> (put 'previous-line 'isearch-match-scroll '(previous-line . backward))
> 
> For some reason, in this case if the error is not catched, Isearch does 
> not wrap at BOB, but instead enters in a strange state where you can move 
> the cursor around.  This might be a bug that should be corrected instead 
> of circumventing it by catching the error.

So I think we should first investigate why it happens with the last
snippet.  And if the conclusion is that condition-case is indeed
needed, we should explain the need in a comment there.

Thanks.

Re: Simple isearch concerns

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Thu, 08 Apr 2021 23:05:18 +0300
> Cc: Ergus <spacibba@aol.com>, emacs-devel@gnu.org
> 
> isearch-beginning-of-buffer changes the direction to 'forward'
> because it makes no sense to keep it 'backward' - there are no matches
> before the first match.

Are you excluding the wrapped-isearch use case?

Re: Simple isearch concerns

[Juri Linkov]

>> isearch-beginning-of-buffer changes the direction to 'forward'
>> because it makes no sense to keep it 'backward' - there are no matches
>> before the first match.
>
> Are you excluding the wrapped-isearch use case?

I replied to Gregory about this use case that it's less useful
than to get ready to go forward with the next C-s.

Re: Simple isearch concerns

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: gregory@heytings.org,  spacibba@aol.com,  emacs-devel@gnu.org
> Date: Fri, 09 Apr 2021 11:39:31 +0300
> 
> >> isearch-beginning-of-buffer changes the direction to 'forward'
> >> because it makes no sense to keep it 'backward' - there are no matches
> >> before the first match.
> >
> > Are you excluding the wrapped-isearch use case?
> 
> I replied to Gregory about this use case that it's less useful
> than to get ready to go forward with the next C-s.

That might be your personal opinion, but it doesn't have to be
everyone else's.

Re: Simple isearch concerns

[Gregory Heytings]

>>>> isearch-beginning-of-buffer changes the direction to 'forward' 
>>>> because it makes no sense to keep it 'backward' - there are no 
>>>> matches before the first match.
>>>
>>> Are you excluding the wrapped-isearch use case?
>>
>> I replied to Gregory about this use case that it's less useful than to 
>> get ready to go forward with the next C-s.
>
> That might be your personal opinion, but it doesn't have to be everyone 
> else's.
>

I don't understand what the point is here.  If isearch-beginning-of-buffer 
is called, point moves to BOB, and isearch is restarted forward, which 
jumps to the first match.  That's what the docstring says: "Go to the 
first occurrence of the current search string."  That doesn't mean that 
wrapping isn't possible anymore, C-r C-r C-r (or just C-r C-r with 
isearch-wrap-pause 'no) works as usual.

If you do not change the search direction after moving to BOB, the result 
is not what you would expect.  For example, after C-r:

- with isearch-wrap-pause t, M-< tells you "failing I-search backward" 
even if there are in fact occurrences of the search string between point 
and BOB, and a second M-< will jump to the last occurrence in the buffer;

- with isearch-wrap-pause 'no, M-< jumps immediate to the last occurrence 
in the buffer.

Re: Simple isearch concerns

[Eli Zaretskii]

> Date: Fri, 09 Apr 2021 11:48:36 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: Juri Linkov <juri@linkov.net>, spacibba@aol.com, emacs-devel@gnu.org
> 
> >>>> isearch-beginning-of-buffer changes the direction to 'forward' 
> >>>> because it makes no sense to keep it 'backward' - there are no 
> >>>> matches before the first match.
> >>>
> >>> Are you excluding the wrapped-isearch use case?
> >>
> >> I replied to Gregory about this use case that it's less useful than to 
> >> get ready to go forward with the next C-s.
> >
> > That might be your personal opinion, but it doesn't have to be everyone 
> > else's.
> >
> 
> I don't understand what the point is here.

The issue is whether we need to change the direction as a side effect
of isearch-beginning-of-buffer.  From my POV, this is unexpected.
The direction should stay what it was before.

> If you do not change the search direction after moving to BOB, the result 
> is not what you would expect.  For example, after C-r:
> 
> - with isearch-wrap-pause t, M-< tells you "failing I-search backward" 
> even if there are in fact occurrences of the search string between point 
> and BOB, and a second M-< will jump to the last occurrence in the buffer;
> 
> - with isearch-wrap-pause 'no, M-< jumps immediate to the last occurrence 
> in the buffer.

Why do you think this is not what the user will expect?

Re: Simple isearch concerns

[Gregory Heytings]

>> I don't understand what the point is here.
>
> The issue is whether we need to change the direction as a side effect of 
> isearch-beginning-of-buffer.  From my POV, this is unexpected. The 
> direction should stay what it was before.
>

Yes, I understand the issue, but disagree that it is unexpected.  Just try 
it, replace:

(isearch-repeat 'forward arg)))

by:

(isearch-repeat (if isearch-forward 'forward 'backward) arg)))

in isearch-beginning-of-buffer, type C-r isearch M-s M-<, and you'll see 
that on the contrary not changing the direction is unexpected, that 
behavior can only be considered as a bug.

>> If you do not change the search direction after moving to BOB, the 
>> result is not what you would expect.  For example, after C-r:
>>
>> - with isearch-wrap-pause t, M-< tells you "failing I-search backward" 
>> even if there are in fact occurrences of the search string between 
>> point and BOB, and a second M-< will jump to the last occurrence in the 
>> buffer;
>>
>> - with isearch-wrap-pause 'no, M-< jumps immediate to the last 
>> occurrence in the buffer.
>
> Why do you think this is not what the user will expect?
>

Because this is not what M-< = beginning-of-buffer does, and this is also 
not what the docstring of isearch-beginning-of-buffer says: "Go to the 
first occurrence of the current search string. Move point to the beginning 
of the buffer and search forwards from the top."

Re: Simple isearch concerns

[Eli Zaretskii]

> Date: Fri, 09 Apr 2021 13:26:09 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: juri@linkov.net, spacibba@aol.com, emacs-devel@gnu.org
> 
> > The issue is whether we need to change the direction as a side effect of 
> > isearch-beginning-of-buffer.  From my POV, this is unexpected. The 
> > direction should stay what it was before.
> 
> Yes, I understand the issue, but disagree that it is unexpected.  Just try 
> it, replace:
> 
> (isearch-repeat 'forward arg)))
> 
> by:
> 
> (isearch-repeat (if isearch-forward 'forward 'backward) arg)))
> 
> in isearch-beginning-of-buffer, type C-r isearch M-s M-<, and you'll see 
> that on the contrary not changing the direction is unexpected, that 
> behavior can only be considered as a bug.

Why do you consider that behavior a bug?  If the user wants to change
the direction, he/she can do that any time.

> > Why do you think this is not what the user will expect?
> 
> Because this is not what M-< = beginning-of-buffer does, and this is also 
> not what the docstring of isearch-beginning-of-buffer says: "Go to the 
> first occurrence of the current search string. Move point to the beginning 
> of the buffer and search forwards from the top."

I'm saying that the side effect of changing the search direction is a
mistake.  Fixing the documentation to describe the behavior is a
no-brainer, the main issue is what kind of behavior do we want in this
case.

Re: Simple isearch concerns

[Gregory Heytings]

>>> The issue is whether we need to change the direction as a side effect 
>>> of isearch-beginning-of-buffer.  From my POV, this is unexpected. The 
>>> direction should stay what it was before.
>>
>> Yes, I understand the issue, but disagree that it is unexpected.  Just 
>> try it, replace:
>>
>> (isearch-repeat 'forward arg)))
>>
>> by:
>>
>> (isearch-repeat (if isearch-forward 'forward 'backward) arg)))
>>
>> in isearch-beginning-of-buffer, type C-r isearch M-s M-<, and you'll 
>> see that on the contrary not changing the direction is unexpected, that 
>> behavior can only be considered as a bug.
>
> Why do you consider that behavior a bug?  If the user wants to change 
> the direction, he/she can do that any time.
>

Did you actually try it?  With the code above, in the center of 
isearch.el, this is what is happening:

C-s isearch : you are searching forward
M-s M-> : you want to see the last occurrence (which corresponds to your search direction : forward), yet it says "Failing I-search"
M-s M-> : you are now on the first line of isearch.el
M-s M-> : "Failing wrapped I-search"

C-r isearch : you are searching backward
M-s M-< : you want to see the first occurrence (which corresponds to your search direction : backward), yet it says "Failing I-search backward"
M-s M-< : you are now on the last line of isearch.el
M-s M-< : "Failing wrapped I-search backward"

How on earth could such a behavior be considered a feature?

Re: Simple isearch concerns

[Eli Zaretskii]

> Date: Fri, 09 Apr 2021 14:36:53 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: spacibba@aol.com, emacs-devel@gnu.org, juri@linkov.net
> 
> C-s isearch : you are searching forward
> M-s M-> : you want to see the last occurrence (which corresponds to your search direction : forward), yet it says "Failing I-search"
> M-s M-> : you are now on the first line of isearch.el
> M-s M-> : "Failing wrapped I-search"

After the first "M-s M->", why do you keep pressing "M-x M->"? why not
C-s?

> C-r isearch : you are searching backward
> M-s M-< : you want to see the first occurrence (which corresponds to your search direction : backward), yet it says "Failing I-search backward"
> M-s M-< : you are now on the last line of isearch.el
> M-s M-< : "Failing wrapped I-search backward"
> 
> How on earth could such a behavior be considered a feature?

That's not the scenario I had in mind.

But if you want the above to be useful, why not ask whoever
implemented that the question about the rationale?  And since when are
we fixing a buggy behavior by modifying it in weird unexpected ways?

Re: Simple isearch concerns

[Gregory Heytings]

>
> After the first "M-s M->", why do you keep pressing "M-x M->"? why not 
> C-s?
>

Why would it not be possible to jump to the last occurrence in the buffer 
when searching forward?

>> How on earth could such a behavior be considered a feature?
>
> That's not the scenario I had in mind.
>
> But if you want the above to be useful, why not ask whoever implemented 
> that the question about the rationale?  And since when are we fixing a 
> buggy behavior by modifying it in weird unexpected ways?
>

I don't understand your questions.  This is not the behavior of 
isearch-{beginning,end}-of-buffer, this would be the behavior if the 
search direction did not change.  The actual behavior is what one would 
expect, M-s M-< and M-s M-> have the exact same effect after C-s and C-r, 
with point after the occurrence and before the last occurrence 
respectively.

Do I understand correctly that what you would have wanted is the 
following:

1. M-s M-< after C-s: point after the first occurrence (search direction forward)
2. M-s M-> after C-s: point after the last occurrence (search direction forward)
3. M-s M-< after C-r: point before the first occurrence (search direction backward)
4. M-s M-> after C-r: point before the last occurrence (search direction backward)

What we have now is 1 and 4, but not 2 and 3, what we have instead is:

2. M-s M-> after C-s: point before the last occurrence (search direction backward)
3. M-s M-< after C-r: point after the first occurrence (search direction backward)

Is this subtle nuance really important?  Isn't the most likely search 
direction one would want to use at BOB forward, and at EOB backward?  The 
other options are at BOB backward and at EOB forward, but this means 
wrapping, that is, doing what one could have done immediately with 1 and 4 
above...

Re: Simple isearch concerns

[Eli Zaretskii]

> Date: Fri, 09 Apr 2021 15:25:08 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: spacibba@aol.com, juri@linkov.net, emacs-devel@gnu.org
> 
> > After the first "M-s M->", why do you keep pressing "M-x M->"? why not 
> > C-s?
> 
> Why would it not be possible to jump to the last occurrence in the buffer 
> when searching forward?

You've lost me here: I cannot see how your question answers mine.

> Do I understand correctly that what you would have wanted is the 
> following:
> 
> 1. M-s M-< after C-s: point after the first occurrence (search direction forward)
> 2. M-s M-> after C-s: point after the last occurrence (search direction forward)
> 3. M-s M-< after C-r: point before the first occurrence (search direction backward)
> 4. M-s M-> after C-r: point before the last occurrence (search direction backward)

Yes.

> What we have now is 1 and 4, but not 2 and 3, what we have instead is:
> 
> 2. M-s M-> after C-s: point before the last occurrence (search direction backward)
> 3. M-s M-< after C-r: point after the first occurrence (search direction backward)
> 
> Is this subtle nuance really important?

IMO, yes.  I don't think M-s M-> and M-s M-< should change the search
direction.  Doing so second-guesses what the user intends to do, but
we have no business second-guessing that.

> Isn't the most likely search direction one would want to use at BOB
> forward, and at EOB backward?

No, not necessarily.  I frequently have to change direction because I
want to revisit previously found matches, and the direction I want to
use next depends on what I'm looking for.  It doesn't necessarily stay
the same for long.

RE: [External] : Re: Simple isearch concerns

[Drew Adams]

> > Isn't the most likely search direction one would want to use at BOB
> > forward, and at EOB backward?
> 
> No, not necessarily.  I frequently have to change direction because I
> want to revisit previously found matches, and the direction I want to
> use next depends on what I'm looking for.  It doesn't necessarily stay
> the same for long.

+1.

Re: Simple isearch concerns

[Gregory Heytings]

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


Thanks again for your comments and review.

I attach the updated patch, in which I added a second option to let users 
decide whether they want to change the search direction when using motion 
commands.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=0001-New-user-options-to-move-between-isearch-matches.patch, Size: 6681 bytes --]

From e2496d2ff3b740a24e52618b1e9599784ca84bb1 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Sat, 9 Apr 2021 23:08:22 +0000
Subject: [PATCH] New user options to move between isearch matches

* lisp/isearch.el (isearch-allow-motion,
isearch-motion-changes-direction): New user options.
(isearch-pre-command-hook): Handle the new options.

* etc/NEWS: Mention the new user options.

* doc/emacs/search.texi: Document the new user options.
---
 doc/emacs/search.texi | 12 ++++++++++
 etc/NEWS              | 13 +++++++++++
 lisp/isearch.el       | 52 ++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 76 insertions(+), 1 deletion(-)

diff --git a/doc/emacs/search.texi b/doc/emacs/search.texi
index 38430a2ab1..de644505f7 100644
--- a/doc/emacs/search.texi
+++ b/doc/emacs/search.texi
@@ -595,6 +595,18 @@ or the selected window and frame.  The command must not itself attempt
 an incremental search.  This feature is disabled if
 @code{isearch-allow-scroll} is @code{nil} (which it is by default).
 
+@vindex isearch-allow-motion
+@vindex isearch-motion-changes-direction
+  Likewise, if you change the variable @code{isearch-allow-motion}
+to a non-@code{nil} value, this enables the use of the keyboard motion
+commands @kbd{M-<}, @kbd{M->}, @kbd{C-v} and @kbd{M-v}, to move
+respectively to the first occurrence of the current search string in
+the buffer, the last one, the first one after the current window,
+and the last one before the current window.  The search direction
+does not change when these motion commands are used, unless you change
+the variable @code{isearch-motion-changes-direction} to a non-@code{nil}
+value.
+
 @item Motion Commands
 @cindex motion commands, during incremental search
 When @code{isearch-yank-on-move} is customized to @code{shift},
diff --git a/etc/NEWS b/etc/NEWS
index a0f05d8cf1..b8921f1405 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -373,6 +373,19 @@ When this option is set, direction changes in Isearch move to another
 search match, if there is one, instead of moving point to the other
 end of the current match.
 
++++
+** New user options 'isearch-allow-motion' and
+'isearch-motion-changes-direction'.
+When 'isearch-allow-motion' is set, the commands 'beginning-of-buffer',
+'end-of-buffer', 'scroll-up-command' and 'scroll-down-command', when
+invoked during I-search, move respectively to the first occurrence of
+the current search string in the buffer, the last one, the first one
+after the current window, and the last one before the current window.
+Additionally, users can change the meaning of other motion commands
+during I-search by using their 'isearch-motion' property.  When
+'isearch-motion-changes-direction' is set, the motion commands also
+change the direction of the search.
+
 ** Outline
 
 +++
diff --git a/lisp/isearch.el b/lisp/isearch.el
index 5efac4c78f..6df31af589 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -2882,12 +2882,45 @@ If non-nil, scrolling commands can be used in Isearch mode.
 However, you cannot scroll far enough that the current match is
 no longer visible (is off screen).  But if the value is `unlimited'
 that limitation is removed and you can scroll any distance off screen.
-If nil, scrolling commands exit Isearch mode."
+If nil, scrolling commands exit Isearch mode.
+See also the related option `isearch-allow-motion'."
   :type '(choice (const :tag "Scrolling exits Isearch" nil)
                  (const :tag "Scrolling with current match on screen" t)
                  (const :tag "Scrolling with current match off screen" unlimited))
   :group 'isearch)
 
+(put 'beginning-of-buffer 'isearch-motion
+     '((lambda () (goto-char (point-min))) . forward))
+(put 'end-of-buffer 'isearch-motion
+     '((lambda () (goto-char (point-max))) . backward))
+(put 'scroll-up-command 'isearch-motion
+     '((lambda () (goto-char (window-end))) . forward))
+(put 'scroll-down-command 'isearch-motion
+     '((lambda () (goto-char (window-start)) (recenter nil t)) . backward))
+
+(defcustom isearch-allow-motion nil
+  "Whether to allow movement between isearch matches by cursor motion commands.
+If non-nil, the four motion commands `beginning-of-buffer', `end-of-buffer',
+`scroll-up-command' and `scroll-down-command', when invoked during Isearch,
+move respectively to the first occurrence of the current search string in the
+buffer, the last one, the first one after the current window, and the last one
+before the current window.
+If nil, these motion commands normally exit Isearch and are executed.
+See also the related option `isearch-allow-scroll'."
+  :type '(choice (const :tag "Off" nil)
+                 (const :tag "On" t))
+  :group 'isearch
+  :version "28.1")
+
+(defcustom isearch-motion-changes-direction nil
+  "Whether motion commands during incremental search change search direction.
+If nil, the search direction (forward or backward) does not change when
+motion commands are used during incremental search, except when wrapping."
+  :type '(choice (const :tag "Off" nil)
+                 (const :tag "On" t))
+  :group 'isearch
+  :version "28.1")
+
 (defcustom isearch-allow-prefix t
   "Whether prefix arguments are allowed during incremental search.
 If non-nil, entering a prefix argument will not terminate the
@@ -2989,6 +3022,23 @@ See more for options in `search-exit-option'."
      ;; Optionally edit the search string instead of exiting.
      ((eq search-exit-option 'edit)
       (setq this-command 'isearch-edit-string))
+     ;; Handle motion command functions.
+     ((and isearch-allow-motion
+           (symbolp this-command)
+           (get this-command 'isearch-motion))
+      (let* ((property (get this-command 'isearch-motion))
+             (function (car property))
+             (current-direction (if isearch-forward 'forward 'backward))
+             (direction (or (cdr property)
+                            (if isearch-forward 'forward 'backward))))
+        (funcall function)
+        (setq isearch-just-started t)
+        (isearch-repeat direction)
+        (when (and isearch-success (not isearch-motion-changes-direction))
+          (unless (eq direction current-direction)
+            (let ((isearch-repeat-on-direction-change nil))
+              (isearch-repeat current-direction))))
+        (setq this-command 'ignore)))
      ;; Handle a scrolling function or prefix argument.
      ((or (and isearch-allow-prefix
                (memq this-command '(universal-argument universal-argument-more
-- 
2.30.2

Re: Simple isearch concerns

[Eli Zaretskii]

> Date: Fri, 09 Apr 2021 23:18:08 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: spacibba@aol.com, emacs-devel@gnu.org, juri@linkov.net
> 
> +@vindex isearch-allow-motion
> +@vindex isearch-motion-changes-direction
> +  Likewise, if you change the variable @code{isearch-allow-motion}
> +to a non-@code{nil} value, this enables the use of the keyboard motion
> +commands @kbd{M-<}, @kbd{M->}, @kbd{C-v} and @kbd{M-v}, to move
> +respectively to the first occurrence of the current search string in
> +the buffer, the last one, the first one after the current window,
> +and the last one before the current window.  The search direction
> +does not change when these motion commands are used, unless you change
> +the variable @code{isearch-motion-changes-direction} to a non-@code{nil}
> +value.

This text doesn't explain how the search direction changes when
isearch-motion-changes-direction is non-nil.  Also, the direction only
changes at BOB and EOB, right?  IOW, C-v and M-v don't change the
direction, right?  If so, the text leaves that unsaid.

> ++++
> +** New user options 'isearch-allow-motion' and
> +'isearch-motion-changes-direction'.

The heading should be a single line and a complete sentence, to
support reading in Outline mode.  If needed, please reword to make it
short enough to fit a 80-column line.

> +(defcustom isearch-allow-motion nil
> +  "Whether to allow movement between isearch matches by cursor motion commands.
> +If non-nil, the four motion commands `beginning-of-buffer', `end-of-buffer',
> +`scroll-up-command' and `scroll-down-command', when invoked during Isearch,
> +move respectively to the first occurrence of the current search string in the
> +buffer, the last one, the first one after the current window, and the last one
> +before the current window.
> +If nil, these motion commands normally exit Isearch and are executed.
> +See also the related option `isearch-allow-scroll'."

This odc string should also mention isearch-motion-changes-direction.

> +(defcustom isearch-motion-changes-direction nil
> +  "Whether motion commands during incremental search change search direction.
> +If nil, the search direction (forward or backward) does not change when
> +motion commands are used during incremental search, except when wrapping."

This should explain how the direction changes when the value is
non-nil.  It currently just says that the direction changes, but
doesn't say how.

Thanks.

Re: Simple isearch concerns

[Gregory Heytings]

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


Thanks for your comments!

>
> Also, the direction only changes at BOB and EOB, right?  IOW, C-v and 
> M-v don't change the direction, right?  If so, the text leaves that 
> unsaid.
>

No, with isearch-motion-changes-direction non-nil, M-< and C-v change the 
direction to forward, M-> and M-v to backward.  IOW, C-v is a faster 
repeated C-s, M-v is a faster repeated C-r, M-< is a faster RET M-< C-s 
C-s, and M-> is a faster RET M-> C-r C-r.  This is, I think, what a user 
would naturally expect, but I guess your preference is to leave the 
default of isearch-motion-changes-direction to nil (I would prefer to set 
it to t).

I attach the updated patch, in which I included your other suggestions.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=0001-New-user-options-to-move-between-isearch-matches.patch, Size: 6983 bytes --]

From 7afe352a3dc4b721ea1ee72e4e1b0fbede35ebc7 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Sat, 10 Apr 2021 10:27:41 +0000
Subject: [PATCH] New user options to move between isearch matches

* lisp/isearch.el (isearch-allow-motion,
isearch-motion-changes-direction): New user options.
(isearch-pre-command-hook): Handle the new options.

* etc/NEWS: Mention the new user options.

* doc/emacs/search.texi: Document the new user options.
---
 doc/emacs/search.texi | 13 ++++++++++
 etc/NEWS              | 12 ++++++++++
 lisp/isearch.el       | 56 ++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 80 insertions(+), 1 deletion(-)

diff --git a/doc/emacs/search.texi b/doc/emacs/search.texi
index 38430a2ab1..dd02dd23e3 100644
--- a/doc/emacs/search.texi
+++ b/doc/emacs/search.texi
@@ -595,6 +595,19 @@ or the selected window and frame.  The command must not itself attempt
 an incremental search.  This feature is disabled if
 @code{isearch-allow-scroll} is @code{nil} (which it is by default).
 
+@vindex isearch-allow-motion
+@vindex isearch-motion-changes-direction
+  Likewise, if you change the variable @code{isearch-allow-motion}
+to a non-@code{nil} value, this enables the use of the keyboard motion
+commands @kbd{M-<}, @kbd{M->}, @kbd{C-v} and @kbd{M-v}, to move
+respectively to the first occurrence of the current search string in
+the buffer, the last one, the first one after the current window,
+and the last one before the current window.  The search direction
+does not change when these motion commands are used, unless you change
+the variable @code{isearch-motion-changes-direction} to a non-@code{nil}
+value, in which case the search direction is forward after @kbd{M-<} and
+@kbd{C-v}, and backward after @kbd{M->} and @kbd{M-v}.
+
 @item Motion Commands
 @cindex motion commands, during incremental search
 When @code{isearch-yank-on-move} is customized to @code{shift},
diff --git a/etc/NEWS b/etc/NEWS
index a0f05d8cf1..52e9127d5f 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -373,6 +373,18 @@ When this option is set, direction changes in Isearch move to another
 search match, if there is one, instead of moving point to the other
 end of the current match.
 
++++
+** New user option 'isearch-allow-motion'.
+When 'isearch-allow-motion' is set, the commands 'beginning-of-buffer',
+'end-of-buffer', 'scroll-up-command' and 'scroll-down-command', when
+invoked during I-search, move respectively to the first occurrence of
+the current search string in the buffer, the last one, the first one
+after the current window, and the last one before the current window.
+Additionally, users can change the meaning of other motion commands
+during I-search by using their 'isearch-motion' property.  The
+option 'isearch-motion-changes-direction' controls whether the
+direction of the search changes after a motion command.
+
 ** Outline
 
 +++
diff --git a/lisp/isearch.el b/lisp/isearch.el
index 5efac4c78f..51df8c5195 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -2882,12 +2882,49 @@ If non-nil, scrolling commands can be used in Isearch mode.
 However, you cannot scroll far enough that the current match is
 no longer visible (is off screen).  But if the value is `unlimited'
 that limitation is removed and you can scroll any distance off screen.
-If nil, scrolling commands exit Isearch mode."
+If nil, scrolling commands exit Isearch mode.
+See also the related option `isearch-allow-motion'."
   :type '(choice (const :tag "Scrolling exits Isearch" nil)
                  (const :tag "Scrolling with current match on screen" t)
                  (const :tag "Scrolling with current match off screen" unlimited))
   :group 'isearch)
 
+(put 'beginning-of-buffer 'isearch-motion
+     '((lambda () (goto-char (point-min))) . forward))
+(put 'end-of-buffer 'isearch-motion
+     '((lambda () (goto-char (point-max))) . backward))
+(put 'scroll-up-command 'isearch-motion
+     '((lambda () (goto-char (window-end))) . forward))
+(put 'scroll-down-command 'isearch-motion
+     '((lambda () (goto-char (window-start)) (recenter nil t)) . backward))
+
+(defcustom isearch-allow-motion nil
+  "Whether to allow movement between isearch matches by cursor motion commands.
+If non-nil, the four motion commands `beginning-of-buffer', `end-of-buffer',
+`scroll-up-command' and `scroll-down-command', when invoked during Isearch,
+move respectively to the first occurrence of the current search string in the
+buffer, the last one, the first one after the current window, and the last one
+before the current window.
+If nil, these motion commands normally exit Isearch and are executed.
+See also the related options `isearch-motion-changes-direction' and
+`isearch-allow-scroll'."
+  :type '(choice (const :tag "Off" nil)
+                 (const :tag "On" t))
+  :group 'isearch
+  :version "28.1")
+
+(defcustom isearch-motion-changes-direction nil
+  "Whether motion commands during incremental search change search direction.
+If nil, the search direction (forward or backward) does not change when
+motion commands are used during incremental search, except when wrapping.
+If non-nil, the search direction is forward after `beginning-of-buffer' and
+`scroll-up-command', and backward after `end-of-buffer' and
+`scroll-down-command'."
+  :type '(choice (const :tag "Off" nil)
+                 (const :tag "On" t))
+  :group 'isearch
+  :version "28.1")
+
 (defcustom isearch-allow-prefix t
   "Whether prefix arguments are allowed during incremental search.
 If non-nil, entering a prefix argument will not terminate the
@@ -2989,6 +3026,23 @@ See more for options in `search-exit-option'."
      ;; Optionally edit the search string instead of exiting.
      ((eq search-exit-option 'edit)
       (setq this-command 'isearch-edit-string))
+     ;; Handle motion command functions.
+     ((and isearch-allow-motion
+           (symbolp this-command)
+           (get this-command 'isearch-motion))
+      (let* ((property (get this-command 'isearch-motion))
+             (function (car property))
+             (current-direction (if isearch-forward 'forward 'backward))
+             (direction (or (cdr property)
+                            (if isearch-forward 'forward 'backward))))
+        (funcall function)
+        (setq isearch-just-started t)
+        (isearch-repeat direction)
+        (when (and isearch-success (not isearch-motion-changes-direction))
+          (unless (eq direction current-direction)
+            (let ((isearch-repeat-on-direction-change nil))
+              (isearch-repeat current-direction))))
+        (setq this-command 'ignore)))
      ;; Handle a scrolling function or prefix argument.
      ((or (and isearch-allow-prefix
                (memq this-command '(universal-argument universal-argument-more
-- 
2.30.2

Re: Simple isearch concerns

[Eli Zaretskii]

> Date: Sat, 10 Apr 2021 10:36:38 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: spacibba@aol.com, juri@linkov.net, emacs-devel@gnu.org
> 
> I attach the updated patch, in which I included your other suggestions.

Thanks, this LGTM, but I wonder whether we should use \\[foo] instead
of a literal `foo' in the doc strings, since users are likely to use
the key sequences, not the command names.

Re: Simple isearch concerns

[Gregory Heytings]

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


>> I attach the updated patch, in which I included your other suggestions.
>
> Thanks, this LGTM, but I wonder whether we should use \\[foo] instead of 
> a literal `foo' in the doc strings, since users are likely to use the 
> key sequences, not the command names.
>

Thanks, see attached.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=0001-New-user-options-to-move-between-isearch-matches.patch, Size: 7003 bytes --]

From 35b243f991733a7ac5d296c7fbf673494d8b94f0 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Sat, 10 Apr 2021 10:54:59 +0000
Subject: [PATCH] New user options to move between isearch matches

* lisp/isearch.el (isearch-allow-motion,
isearch-motion-changes-direction): New user options.
(isearch-pre-command-hook): Handle the new options.

* etc/NEWS: Mention the new user options.

* doc/emacs/search.texi: Document the new user options.
---
 doc/emacs/search.texi | 13 ++++++++++
 etc/NEWS              | 12 ++++++++++
 lisp/isearch.el       | 56 ++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 80 insertions(+), 1 deletion(-)

diff --git a/doc/emacs/search.texi b/doc/emacs/search.texi
index 38430a2ab1..dd02dd23e3 100644
--- a/doc/emacs/search.texi
+++ b/doc/emacs/search.texi
@@ -595,6 +595,19 @@ or the selected window and frame.  The command must not itself attempt
 an incremental search.  This feature is disabled if
 @code{isearch-allow-scroll} is @code{nil} (which it is by default).
 
+@vindex isearch-allow-motion
+@vindex isearch-motion-changes-direction
+  Likewise, if you change the variable @code{isearch-allow-motion}
+to a non-@code{nil} value, this enables the use of the keyboard motion
+commands @kbd{M-<}, @kbd{M->}, @kbd{C-v} and @kbd{M-v}, to move
+respectively to the first occurrence of the current search string in
+the buffer, the last one, the first one after the current window,
+and the last one before the current window.  The search direction
+does not change when these motion commands are used, unless you change
+the variable @code{isearch-motion-changes-direction} to a non-@code{nil}
+value, in which case the search direction is forward after @kbd{M-<} and
+@kbd{C-v}, and backward after @kbd{M->} and @kbd{M-v}.
+
 @item Motion Commands
 @cindex motion commands, during incremental search
 When @code{isearch-yank-on-move} is customized to @code{shift},
diff --git a/etc/NEWS b/etc/NEWS
index a0f05d8cf1..52e9127d5f 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -373,6 +373,18 @@ When this option is set, direction changes in Isearch move to another
 search match, if there is one, instead of moving point to the other
 end of the current match.
 
++++
+** New user option 'isearch-allow-motion'.
+When 'isearch-allow-motion' is set, the commands 'beginning-of-buffer',
+'end-of-buffer', 'scroll-up-command' and 'scroll-down-command', when
+invoked during I-search, move respectively to the first occurrence of
+the current search string in the buffer, the last one, the first one
+after the current window, and the last one before the current window.
+Additionally, users can change the meaning of other motion commands
+during I-search by using their 'isearch-motion' property.  The
+option 'isearch-motion-changes-direction' controls whether the
+direction of the search changes after a motion command.
+
 ** Outline
 
 +++
diff --git a/lisp/isearch.el b/lisp/isearch.el
index 5efac4c78f..13370ea0bc 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -2882,12 +2882,49 @@ If non-nil, scrolling commands can be used in Isearch mode.
 However, you cannot scroll far enough that the current match is
 no longer visible (is off screen).  But if the value is `unlimited'
 that limitation is removed and you can scroll any distance off screen.
-If nil, scrolling commands exit Isearch mode."
+If nil, scrolling commands exit Isearch mode.
+See also the related option `isearch-allow-motion'."
   :type '(choice (const :tag "Scrolling exits Isearch" nil)
                  (const :tag "Scrolling with current match on screen" t)
                  (const :tag "Scrolling with current match off screen" unlimited))
   :group 'isearch)
 
+(put 'beginning-of-buffer 'isearch-motion
+     '((lambda () (goto-char (point-min))) . forward))
+(put 'end-of-buffer 'isearch-motion
+     '((lambda () (goto-char (point-max))) . backward))
+(put 'scroll-up-command 'isearch-motion
+     '((lambda () (goto-char (window-end))) . forward))
+(put 'scroll-down-command 'isearch-motion
+     '((lambda () (goto-char (window-start)) (recenter nil t)) . backward))
+
+(defcustom isearch-allow-motion nil
+  "Whether to allow movement between isearch matches by cursor motion commands.
+If non-nil, the four motion commands \\[beginning-of-buffer], \\[end-of-buffer], \
+\\[scroll-up-command] and \\[scroll-down-command], when invoked during
+Isearch, move respectively to the first occurrence of the current search string
+in the buffer, the last one, the first one after the current window, and the
+last one before the current window.
+If nil, these motion commands normally exit Isearch and are executed.
+See also the related options `isearch-motion-changes-direction' and
+`isearch-allow-scroll'."
+  :type '(choice (const :tag "Off" nil)
+                 (const :tag "On" t))
+  :group 'isearch
+  :version "28.1")
+
+(defcustom isearch-motion-changes-direction nil
+  "Whether motion commands during incremental search change search direction.
+If nil, the search direction (forward or backward) does not change when
+motion commands are used during incremental search, except when wrapping.
+If non-nil, the search direction is forward after \\[beginning-of-buffer] and \
+\\[scroll-up-command], and
+backward after \\[end-of-buffer] and \\[scroll-down-command]."
+  :type '(choice (const :tag "Off" nil)
+                 (const :tag "On" t))
+  :group 'isearch
+  :version "28.1")
+
 (defcustom isearch-allow-prefix t
   "Whether prefix arguments are allowed during incremental search.
 If non-nil, entering a prefix argument will not terminate the
@@ -2989,6 +3026,23 @@ See more for options in `search-exit-option'."
      ;; Optionally edit the search string instead of exiting.
      ((eq search-exit-option 'edit)
       (setq this-command 'isearch-edit-string))
+     ;; Handle motion command functions.
+     ((and isearch-allow-motion
+           (symbolp this-command)
+           (get this-command 'isearch-motion))
+      (let* ((property (get this-command 'isearch-motion))
+             (function (car property))
+             (current-direction (if isearch-forward 'forward 'backward))
+             (direction (or (cdr property)
+                            (if isearch-forward 'forward 'backward))))
+        (funcall function)
+        (setq isearch-just-started t)
+        (isearch-repeat direction)
+        (when (and isearch-success (not isearch-motion-changes-direction))
+          (unless (eq direction current-direction)
+            (let ((isearch-repeat-on-direction-change nil))
+              (isearch-repeat current-direction))))
+        (setq this-command 'ignore)))
      ;; Handle a scrolling function or prefix argument.
      ((or (and isearch-allow-prefix
                (memq this-command '(universal-argument universal-argument-more
-- 
2.30.2

Re: Simple isearch concerns

[Eli Zaretskii]

> Date: Sat, 10 Apr 2021 10:57:33 +0000
> From: Gregory Heytings <gregory@heytings.org>
> Cc: spacibba@aol.com, juri@linkov.net, emacs-devel@gnu.org
> 
> > Thanks, this LGTM, but I wonder whether we should use \\[foo] instead of 
> > a literal `foo' in the doc strings, since users are likely to use the 
> > key sequences, not the command names.
> >
> 
> Thanks, see attached.

No more comments from me, thanks.

Now branch isearch-vertical

[Ergus]

Hi Gregory:

I see that you added a new feature branch to finally implement
icomplete-vertical. Let's hope this time it will be added.

If you permit me 3 suggestions:

1) Could you add some customs for the separators? This may simplify the
"unless" calls you needed to add twice. But also will permit the user to
configure them.

IMO the best alternative is a sort of format; but at least setting the 
{} [] and () somehow will be very handy... maybe with a const pair.

2) I am wondering about the icomplete-code that determines the number of
candidates. IIRC the candidates are determined by the windows
width... so maybe that needs some check? Because with narrow
windows or long candidates the list may be needlessly truncated in
vertical?

3) When you corrected my branch that time ago I remember that your
recommendation was to respect the user option icomplete-separator. To
detect if it had a "\n". Now that most of the display issues are solved
and the setup is simpler.

Isn't it possible to do that with a simple (if) in
icomplete-minibuffer-setup instead of adding a hook?; setting some
variables and then avoid all the `unless` and `if` in the (if prospects)
part? So we don't need the extra mode and the setup for the user will be
simpler.

Best,
Ergus

On Sat, Apr 10, 2021 at 10:57:33AM +0000, Gregory Heytings wrote:
>
>>>I attach the updated patch, in which I included your other suggestions.
>>
>>Thanks, this LGTM, but I wonder whether we should use \\[foo] 
>>instead of a literal `foo' in the doc strings, since users are 
>>likely to use the key sequences, not the command names.
>>
>
>Thanks, see attached.

Re: Now branch isearch-vertical

[Gregory Heytings]

Hi Ergus,

>
> 1) Could you add some customs for the separators? This may simplify the 
> "unless" calls you needed to add twice. But also will permit the user to 
> configure them.
>
> IMO the best alternative is a sort of format; but at least setting the 
> {} [] and () somehow will be very handy... maybe with a const pair.
>

That's not necessary to implement icomplete/fido verticality, so it should 
better be done with another patch I think.  I'll submit a patch for that 
later.

(There is a single "unless" AFAICS.)

>
> 2) I am wondering about the icomplete-code that determines the number of 
> candidates. IIRC the candidates are determined by the windows width... 
> so maybe that needs some check? Because with narrow windows or long 
> candidates the list may be needlessly truncated in vertical?
>

The trick is that there is no calculation of the number of candidates.  A 
(long enough) list of candidates is added in an overlay, and Emacs 
automatically displays the first ones.  With a smaller window you'll see 
less candidates, with a larger one you'll see more of them.

>
> 3) When you corrected my branch that time ago I remember that your 
> recommendation was to respect the user option icomplete-separator. To 
> detect if it had a "\n". Now that most of the display issues are solved 
> and the setup is simpler.  Isn't it possible to do that with a simple 
> (if) in icomplete-minibuffer-setup instead of adding a hook?; setting 
> some variables and then avoid all the `unless` and `if` in the (if 
> prospects) part? So we don't need the extra mode and the setup for the 
> user will be simpler.
>

That would have been possible in principle, but the code would not have 
been much simpler, and I think changing the behavior for a specific value 
of a user variable isn't the best thing to do.  It's clearer to have a 
minor mode that users can toggle, and the setup in an init file is as 
simple as possible: "(icomplete-vertical-mode 1)".

Re: Simple isearch concerns

[Juri Linkov]

> +(put 'scroll-up-command 'isearch-motion
> +     '((lambda () (goto-char (window-end))) . forward))
> +(put 'scroll-down-command 'isearch-motion
> +     '((lambda () (goto-char (window-start)) (recenter nil t)) . backward))

These definitions have such a problem that after going to the next screen,
it's recentered, so the upper half of the screen displays old matches
already seen on the previous screen.

I get much better scrolling experience with these properties:

(put 'scroll-up-command 'isearch-motion
     '((lambda () (goto-char (window-end)) (recenter 1 t)) . forward))
(put 'scroll-down-command 'isearch-motion
     '((lambda () (goto-char (window-start)) (recenter -1 t)) . backward))

so only new unseen matches are highlighted.

> +        (when (and isearch-success (not isearch-motion-changes-direction))
> +          (unless (eq direction current-direction)
> +            (let ((isearch-repeat-on-direction-change nil))
> +              (isearch-repeat current-direction))))

This 'isearch-repeat' is broken when the new option
'isearch-repeat-on-direction-change' is customized to t.

Also in this case the old commands isearch-beginning-of-buffer
and isearch-end-of-buffer are broken too.  Maybe you need to wrap
their calls of 'isearch-repeat' with

    (let ((isearch-repeat-on-direction-change nil))
      (isearch-repeat 'forward arg))

Re: Simple isearch concerns

[Gregory Heytings]

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


>
> I get much better scrolling experience with these properties:
>
> (put 'scroll-up-command 'isearch-motion
>     '((lambda () (goto-char (window-end)) (recenter 1 t)) . forward))
> (put 'scroll-down-command 'isearch-motion
>     '((lambda () (goto-char (window-start)) (recenter -1 t)) . backward))
>
> so only new unseen matches are highlighted.
>

Indeed, I tried them, and I agree with you that these properties give a 
better experience.

>> +        (when (and isearch-success (not isearch-motion-changes-direction))
>> +          (unless (eq direction current-direction)
>> +            (let ((isearch-repeat-on-direction-change nil))
>> +              (isearch-repeat current-direction))))
>
> This 'isearch-repeat' is broken when the new option 
> 'isearch-repeat-on-direction-change' is customized to t.
>

Fixed.

>
> Also in this case the old commands isearch-beginning-of-buffer and 
> isearch-end-of-buffer are broken too.  Maybe you need to wrap their 
> calls of 'isearch-repeat' with
>
>    (let ((isearch-repeat-on-direction-change nil))
>      (isearch-repeat 'forward arg))
>

Indeed, it is necessary to let-bind isearch-repeat-on-direction-change to 
nil there, like elsewhere.  I attach a separate patch, because it's not 
related to the isearch-allow-motion patch.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=0001-New-user-options-to-move-between-isearch-matches.patch, Size: 7077 bytes --]

From 4bcc48e78649d903f7c5a91e32c80f7efa7105f0 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Sat, 10 Apr 2021 23:31:52 +0000
Subject: [PATCH] New user options to move between isearch matches

* lisp/isearch.el (isearch-allow-motion,
isearch-motion-changes-direction): New user options.
(isearch-pre-command-hook): Handle the new options.

* etc/NEWS: Mention the new user options.

* doc/emacs/search.texi: Document the new user options.
---
 doc/emacs/search.texi | 13 +++++++++
 etc/NEWS              | 12 +++++++++
 lisp/isearch.el       | 63 ++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 85 insertions(+), 3 deletions(-)

diff --git a/doc/emacs/search.texi b/doc/emacs/search.texi
index 38430a2ab1..dd02dd23e3 100644
--- a/doc/emacs/search.texi
+++ b/doc/emacs/search.texi
@@ -595,6 +595,19 @@ or the selected window and frame.  The command must not itself attempt
 an incremental search.  This feature is disabled if
 @code{isearch-allow-scroll} is @code{nil} (which it is by default).
 
+@vindex isearch-allow-motion
+@vindex isearch-motion-changes-direction
+  Likewise, if you change the variable @code{isearch-allow-motion}
+to a non-@code{nil} value, this enables the use of the keyboard motion
+commands @kbd{M-<}, @kbd{M->}, @kbd{C-v} and @kbd{M-v}, to move
+respectively to the first occurrence of the current search string in
+the buffer, the last one, the first one after the current window,
+and the last one before the current window.  The search direction
+does not change when these motion commands are used, unless you change
+the variable @code{isearch-motion-changes-direction} to a non-@code{nil}
+value, in which case the search direction is forward after @kbd{M-<} and
+@kbd{C-v}, and backward after @kbd{M->} and @kbd{M-v}.
+
 @item Motion Commands
 @cindex motion commands, during incremental search
 When @code{isearch-yank-on-move} is customized to @code{shift},
diff --git a/etc/NEWS b/etc/NEWS
index aaf38022c5..d4392b7e0d 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -384,6 +384,18 @@ When this option is set, direction changes in Isearch move to another
 search match, if there is one, instead of moving point to the other
 end of the current match.
 
++++
+** New user option 'isearch-allow-motion'.
+When 'isearch-allow-motion' is set, the commands 'beginning-of-buffer',
+'end-of-buffer', 'scroll-up-command' and 'scroll-down-command', when
+invoked during I-search, move respectively to the first occurrence of
+the current search string in the buffer, the last one, the first one
+after the current window, and the last one before the current window.
+Additionally, users can change the meaning of other motion commands
+during I-search by using their 'isearch-motion' property.  The
+option 'isearch-motion-changes-direction' controls whether the
+direction of the search changes after a motion command.
+
 ** Outline
 
 +++
diff --git a/lisp/isearch.el b/lisp/isearch.el
index 5efac4c78f..e03bfde950 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -2882,12 +2884,49 @@ If non-nil, scrolling commands can be used in Isearch mode.
 However, you cannot scroll far enough that the current match is
 no longer visible (is off screen).  But if the value is `unlimited'
 that limitation is removed and you can scroll any distance off screen.
-If nil, scrolling commands exit Isearch mode."
+If nil, scrolling commands exit Isearch mode.
+See also the related option `isearch-allow-motion'."
   :type '(choice (const :tag "Scrolling exits Isearch" nil)
                  (const :tag "Scrolling with current match on screen" t)
                  (const :tag "Scrolling with current match off screen" unlimited))
   :group 'isearch)
 
+(put 'beginning-of-buffer 'isearch-motion
+     '((lambda () (goto-char (point-min))) . forward))
+(put 'end-of-buffer 'isearch-motion
+     '((lambda () (goto-char (point-max))) . backward))
+(put 'scroll-up-command 'isearch-motion
+     '((lambda () (goto-char (window-end)) (recenter 1 t)) . forward))
+(put 'scroll-down-command 'isearch-motion
+     '((lambda () (goto-char (window-start)) (recenter -1 t)) . backward))
+
+(defcustom isearch-allow-motion nil
+  "Whether to allow movement between isearch matches by cursor motion commands.
+If non-nil, the four motion commands \\[beginning-of-buffer], \\[end-of-buffer], \
+\\[scroll-up-command] and \\[scroll-down-command], when invoked during
+Isearch, move respectively to the first occurrence of the current search string
+in the buffer, the last one, the first one after the current window, and the
+last one before the current window.
+If nil, these motion commands normally exit Isearch and are executed.
+See also the related options `isearch-motion-changes-direction' and
+`isearch-allow-scroll'."
+  :type '(choice (const :tag "Off" nil)
+                 (const :tag "On" t))
+  :group 'isearch
+  :version "28.1")
+
+(defcustom isearch-motion-changes-direction nil
+  "Whether motion commands during incremental search change search direction.
+If nil, the search direction (forward or backward) does not change when
+motion commands are used during incremental search, except when wrapping.
+If non-nil, the search direction is forward after \\[beginning-of-buffer] and \
+\\[scroll-up-command], and
+backward after \\[end-of-buffer] and \\[scroll-down-command]."
+  :type '(choice (const :tag "Off" nil)
+                 (const :tag "On" t))
+  :group 'isearch
+  :version "28.1")
+
 (defcustom isearch-allow-prefix t
   "Whether prefix arguments are allowed during incremental search.
 If non-nil, entering a prefix argument will not terminate the
@@ -2989,6 +3028,24 @@ See more for options in `search-exit-option'."
      ;; Optionally edit the search string instead of exiting.
      ((eq search-exit-option 'edit)
       (setq this-command 'isearch-edit-string))
+     ;; Handle motion command functions.
+     ((and isearch-allow-motion
+           (symbolp this-command)
+           (get this-command 'isearch-motion))
+      (let* ((property (get this-command 'isearch-motion))
+             (function (car property))
+             (current-direction (if isearch-forward 'forward 'backward))
+             (direction (or (cdr property)
+                            (if isearch-forward 'forward 'backward))))
+        (funcall function)
+        (setq isearch-just-started t)
+        (let ((isearch-repeat-on-direction-change nil))
+          (isearch-repeat direction))
+        (when (and isearch-success (not isearch-motion-changes-direction))
+          (unless (eq direction current-direction)
+            (let ((isearch-repeat-on-direction-change nil))
+              (isearch-repeat current-direction))))
+        (setq this-command 'ignore)))
      ;; Handle a scrolling function or prefix argument.
      ((or (and isearch-allow-prefix
                (memq this-command '(universal-argument universal-argument-more
-- 
2.30.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: Type: text/x-diff; name=0001-Fix-behavior-of-isearch-beginning-end-of-buffer.patch, Size: 1452 bytes --]

From 8ea397361cbe302ac49601652a68c4607713e66f Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Sat, 10 Apr 2021 23:37:16 +0000
Subject: [PATCH] Fix behavior of isearch-{beginning,end}-of-buffer

* lisp/isearch.el (isearch-beginning-of-buffer,
isearch-end-of-buffer): Fix their behavior when
isearch-repeat-on-direction-change is non-nil.
---
 lisp/isearch.el | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 5efac4c78f..27ee6f8414 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -1968,7 +1968,8 @@ type \\[isearch-repeat-forward] with a numeric argument."
     ;; don't forward char in isearch-repeat
     (setq isearch-just-started t)
     (goto-char (point-min))
-    (isearch-repeat 'forward arg)))
+    (let ((isearch-repeat-on-direction-change nil))
+      (isearch-repeat 'forward arg))))
 
 (defun isearch-end-of-buffer (&optional arg)
   "Go to the last occurrence of the current search string.
@@ -1982,7 +1983,8 @@ type \\[isearch-repeat-backward] with a numeric argument."
       (isearch-beginning-of-buffer (abs arg))
     (setq isearch-just-started t)
     (goto-char (point-max))
-    (isearch-repeat 'backward arg)))
+    (let ((isearch-repeat-on-direction-change nil))
+      (isearch-repeat 'backward arg))))
 
 \f
 ;;; Toggles for `isearch-regexp-function' and `search-default-mode'.
-- 
2.30.2

Re: Simple isearch concerns

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  spacibba@aol.com,  emacs-devel@gnu.org
> Date: Sun, 11 Apr 2021 01:12:28 +0300
> 
> > +(put 'scroll-up-command 'isearch-motion
> > +     '((lambda () (goto-char (window-end))) . forward))
> > +(put 'scroll-down-command 'isearch-motion
> > +     '((lambda () (goto-char (window-start)) (recenter nil t)) . backward))
> 
> These definitions have such a problem that after going to the next screen,
> it's recentered, so the upper half of the screen displays old matches
> already seen on the previous screen.
> 
> I get much better scrolling experience with these properties:
> 
> (put 'scroll-up-command 'isearch-motion
>      '((lambda () (goto-char (window-end)) (recenter 1 t)) . forward))
> (put 'scroll-down-command 'isearch-motion
>      '((lambda () (goto-char (window-start)) (recenter -1 t)) . backward))
> 
> so only new unseen matches are highlighted.

That's your personal preference wrt how much of the context should be
left on the screen; it is well known that there are other preferences
regarding this aspect.  Gregory's code provides the default Emacs
experience of recentering, so it is the right default.  If we want to
cater to other preferences, we need to make the 1st argument to
recenter depend on scroll-related options, or maybe introduce a new
user option.

Re: Simple isearch concerns

[Gregory Heytings]

>
> That's your personal preference wrt how much of the context should be 
> left on the screen; it is well known that there are other preferences 
> regarding this aspect.  Gregory's code provides the default Emacs 
> experience of recentering, so it is the right default.  If we want to 
> cater to other preferences, we need to make the 1st argument to recenter 
> depend on scroll-related options, or maybe introduce a new user option.
>

By popular demand, here is the patch, updated with a new option 
'isearch-motion-recenter', which defaults to t.

(Note that you should wait to push it until my paperwork has been 
processed by the FSF, I wasn't aware of that subtlety.)

Re: Simple isearch concerns

[Gregory Heytings]

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


>> That's your personal preference wrt how much of the context should be 
>> left on the screen; it is well known that there are other preferences 
>> regarding this aspect.  Gregory's code provides the default Emacs 
>> experience of recentering, so it is the right default.  If we want to 
>> cater to other preferences, we need to make the 1st argument to 
>> recenter depend on scroll-related options, or maybe introduce a new 
>> user option.
>
> By popular demand, here is the patch, updated with a new option 
> 'isearch-motion-recenter', which defaults to t.
>
> (Note that you should wait to push it until my paperwork has been 
> processed by the FSF, I wasn't aware of that subtlety.)
>

And of course I forgot to attach "the patch".  Here it is.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=0001-New-user-options-to-move-between-isearch-matches.patch, Size: 8232 bytes --]

From c4c77360f4476f794ea34ebbd355f43288d7843d Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Sun, 11 Apr 2021 08:43:18 +0000
Subject: [PATCH] New user options to move between isearch matches

* lisp/isearch.el (isearch-allow-motion,
isearch-motion-changes-direction, isearch-motion-recenter): New
user options.
(isearch-pre-command-hook): Handle the new options.

* etc/NEWS: Mention the new user options.

* doc/emacs/search.texi: Document the new user options.
---
 doc/emacs/search.texi | 17 ++++++++++
 etc/NEWS              | 14 ++++++++
 lisp/isearch.el       | 74 ++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 104 insertions(+), 1 deletion(-)

diff --git a/doc/emacs/search.texi b/doc/emacs/search.texi
index 38430a2ab1..b9ab335dea 100644
--- a/doc/emacs/search.texi
+++ b/doc/emacs/search.texi
@@ -595,6 +595,23 @@ or the selected window and frame.  The command must not itself attempt
 an incremental search.  This feature is disabled if
 @code{isearch-allow-scroll} is @code{nil} (which it is by default).
 
+@vindex isearch-allow-motion
+@vindex isearch-motion-changes-direction
+  Likewise, if you change the variable @code{isearch-allow-motion}
+to a non-@code{nil} value, this enables the use of the keyboard motion
+commands @kbd{M-<}, @kbd{M->}, @kbd{C-v} and @kbd{M-v}, to move
+respectively to the first occurrence of the current search string in
+the buffer, the last one, the first one after the current window,
+and the last one before the current window.  The search direction
+does not change when these motion commands are used, unless you change
+the variable @code{isearch-motion-changes-direction} to a non-@code{nil}
+value, in which case the search direction is forward after @kbd{M-<} and
+@kbd{C-v}, and backward after @kbd{M->} and @kbd{M-v}.  The current match
+is centered in the window when these motion commands are used, unless you
+change the variable @code{isearch-motion-recenter} to a @code{nil}
+value, in which case the current match will be near the top or bottom of
+the window.
+
 @item Motion Commands
 @cindex motion commands, during incremental search
 When @code{isearch-yank-on-move} is customized to @code{shift},
diff --git a/etc/NEWS b/etc/NEWS
index aaf38022c5..a7fe4167d5 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -384,6 +384,20 @@ When this option is set, direction changes in Isearch move to another
 search match, if there is one, instead of moving point to the other
 end of the current match.
 
++++
+** New user option 'isearch-allow-motion'.
+When 'isearch-allow-motion' is set, the commands 'beginning-of-buffer',
+'end-of-buffer', 'scroll-up-command' and 'scroll-down-command', when
+invoked during I-search, move respectively to the first occurrence of
+the current search string in the buffer, the last one, the first one
+after the current window, and the last one before the current window.
+Additionally, users can change the meaning of other motion commands
+during I-search by using their 'isearch-motion' property.  The
+option 'isearch-motion-changes-direction' controls whether the
+direction of the search changes after a motion command, and the option
+'isearch-motion-recenter' controls whether point is centered in the
+window after a motion command.
+
 ** Outline
 
 +++
diff --git a/lisp/isearch.el b/lisp/isearch.el
index 5efac4c78f..fdc32a181a 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -2882,12 +2882,64 @@ If non-nil, scrolling commands can be used in Isearch mode.
 However, you cannot scroll far enough that the current match is
 no longer visible (is off screen).  But if the value is `unlimited'
 that limitation is removed and you can scroll any distance off screen.
-If nil, scrolling commands exit Isearch mode."
+If nil, scrolling commands exit Isearch mode.
+See also the related option `isearch-allow-motion'."
   :type '(choice (const :tag "Scrolling exits Isearch" nil)
                  (const :tag "Scrolling with current match on screen" t)
                  (const :tag "Scrolling with current match off screen" unlimited))
   :group 'isearch)
 
+(put 'beginning-of-buffer 'isearch-motion
+     '((lambda () (goto-char (point-min))) . forward))
+(put 'end-of-buffer 'isearch-motion
+     '((lambda () (goto-char (point-max))) . backward))
+(put 'scroll-up-command 'isearch-motion
+     '((lambda () (goto-char (window-end))) . forward))
+(put 'scroll-down-command 'isearch-motion
+     '((lambda () (goto-char (window-start))
+         (when isearch-motion-recenter (recenter nil t)))
+       . backward))
+
+(defcustom isearch-allow-motion nil
+  "Whether to allow movement between isearch matches by cursor motion commands.
+If non-nil, the four motion commands \\[beginning-of-buffer], \\[end-of-buffer], \
+\\[scroll-up-command] and \\[scroll-down-command], when invoked during
+Isearch, move respectively to the first occurrence of the current search string
+in the buffer, the last one, the first one after the current window, and the
+last one before the current window.
+If nil, these motion commands normally exit Isearch and are executed.
+See also the related options `isearch-motion-changes-direction' and
+`isearch-allow-scroll'."
+  :type '(choice (const :tag "Off" nil)
+                 (const :tag "On" t))
+  :group 'isearch
+  :version "28.1")
+
+(defcustom isearch-motion-changes-direction nil
+  "Whether motion commands during incremental search change search direction.
+If nil, the search direction (forward or backward) does not change when
+motion commands are used during incremental search, except when wrapping.
+If non-nil, the search direction is forward after \\[beginning-of-buffer] and \
+\\[scroll-up-command], and
+backward after \\[end-of-buffer] and \\[scroll-down-command]."
+  :type '(choice (const :tag "Off" nil)
+                 (const :tag "On" t))
+  :group 'isearch
+  :version "28.1")
+
+(defcustom isearch-motion-recenter t
+  "Whether recentering happens after motion commands during incremental search.
+If non-nil, point will be at or near the center of the window after a motion
+command, and a half screen of matches that were visible before the motion command
+will still be visible.
+If nil, point will be near the top or bottom of the window after a motion
+command, and most matches that were visible before the motion command will not
+be visible anymore."
+  :type '(choice (const :tag "Off" nil)
+                 (const :tag "On" t))
+  :group 'isearch
+  :version "28.1")
+
 (defcustom isearch-allow-prefix t
   "Whether prefix arguments are allowed during incremental search.
 If non-nil, entering a prefix argument will not terminate the
@@ -2989,6 +3041,26 @@ See more for options in `search-exit-option'."
      ;; Optionally edit the search string instead of exiting.
      ((eq search-exit-option 'edit)
       (setq this-command 'isearch-edit-string))
+     ;; Handle motion command functions.
+     ((and isearch-allow-motion
+           (symbolp this-command)
+           (get this-command 'isearch-motion))
+      (let* ((property (get this-command 'isearch-motion))
+             (function (car property))
+             (current-direction (if isearch-forward 'forward 'backward))
+             (direction (or (cdr property)
+                            (if isearch-forward 'forward 'backward))))
+        (funcall function)
+        (setq isearch-just-started t)
+        (let ((isearch-repeat-on-direction-change nil))
+          (isearch-repeat direction))
+        (if (not isearch-motion-recenter)
+            (recenter (if (eq direction 'forward) 1 -1) t))
+        (when (and isearch-success (not isearch-motion-changes-direction))
+          (unless (eq direction current-direction)
+            (let ((isearch-repeat-on-direction-change nil))
+              (isearch-repeat current-direction))))
+        (setq this-command 'ignore)))
      ;; Handle a scrolling function or prefix argument.
      ((or (and isearch-allow-prefix
                (memq this-command '(universal-argument universal-argument-more
-- 
2.30.2

Re: Simple isearch concerns

[Juri Linkov]

> +(put 'beginning-of-buffer 'isearch-motion
> +     '((lambda () (goto-char (point-min))) . forward))
> +(put 'end-of-buffer 'isearch-motion
> +     '((lambda () (goto-char (point-max))) . backward))
> +(put 'scroll-up-command 'isearch-motion
> +     '((lambda () (goto-char (window-end))) . forward))
> +(put 'scroll-down-command 'isearch-motion
> +     '((lambda () (goto-char (window-start))
> +         (when isearch-motion-recenter (recenter nil t)))
> +       . backward))
> ...
> +        (if (not isearch-motion-recenter)
> +            (recenter (if (eq direction 'forward) 1 -1) t))

This 'recenter' should affect only 'scroll-up-command' and 'scroll-down-command'
like you used 'recenter' in the 'isearch-motion' property of 'scroll-down-command'
above.  Otherwise, this recentering has adverse effect in such definitions:

(put 'next-line 'isearch-motion '(forward-line . forward))
(put 'previous-line 'isearch-motion '((lambda () (forward-line 0)) . backward))

Re: Simple isearch concerns

[Juri Linkov]

>> (Note that you should wait to push it until my paperwork has been
>> processed by the FSF, I wasn't aware of that subtlety.)

While you are waiting for your paperwork to be processed,
I'd like to point out that when I tried to debug a problem
of the failed lazy-highlighting for non-nil lazy-highlight-buffer,
I noticed that the current implementation is not the best one:

> +(put 'beginning-of-buffer 'isearch-motion
> +     '((lambda () (goto-char (point-min))) . forward))
>
> +      (let* ((property (get this-command 'isearch-motion))
> +             (function (car property))
> +        (funcall function)
> +        (setq this-command 'ignore)))

The problem is that it executes a function as a command,
and resets the current command to 'ignore'.  This causes
more problems.

Instead of emulating the command execution, it would be more natural
to let the command to be completed as usual, then call 'isearch-repeat'
in 'isearch-post-command-hook' after the command moved point.

This is how other existing features work: isearch-allow-prefix,
isearch-allow-scroll, isearch-yank-on-move.  They prepare internal
variables in isearch-pre-command-hook, let the command to move point,
then do post-processing in isearch-post-command-hook to update
the search string, etc.

Then instead of such definition:

  (put 'beginning-of-buffer 'isearch-motion
       '((lambda () (goto-char (point-min))) . forward))

you can use a shorter definition

  (put 'beginning-of-buffer 'isearch-motion 'forward)

that means: let the command 'beginning-of-buffer' move point
to the beginning of the buffer, then call 'isearch-repeat'
with the argument 'forward' in isearch-pre-command-hook.

When there is no existing command with required functionality,
then the command body could be defined in the property, e.g.

  (put 'scroll-up-command 'isearch-motion
       '((lambda () (interactive) (goto-char (window-end))) . forward))

Then in isearch-pre-command-hook you can setq 'this-command' to
the command definition:

  (let* ((property (get this-command 'isearch-motion))
         (function (car property))
    (setq this-command function)))

then the definition will be executed as a command.

Re: Simple isearch concerns

[Gregory Heytings]

>
> While you are waiting for your paperwork to be processed, I'd like to 
> point out that when I tried to debug a problem of the failed 
> lazy-highlighting for non-nil lazy-highlight-buffer, I noticed that the 
> current implementation is not the best one:
>

Thanks for your feedback, I'll work on that.

Re: Simple isearch concerns

[Juri Linkov]

>> I get much better scrolling experience with these properties:
>> 
>> (put 'scroll-up-command 'isearch-motion
>>      '((lambda () (goto-char (window-end)) (recenter 1 t)) . forward))
>> (put 'scroll-down-command 'isearch-motion
>>      '((lambda () (goto-char (window-start)) (recenter -1 t)) . backward))
>> 
>> so only new unseen matches are highlighted.
>
> That's your personal preference wrt how much of the context should be
> left on the screen; it is well known that there are other preferences
> regarding this aspect.  Gregory's code provides the default Emacs
> experience of recentering, so it is the right default.  If we want to
> cater to other preferences, we need to make the 1st argument to
> recenter depend on scroll-related options, or maybe introduce a new
> user option.

In 'emacs -Q' typing 'C-v' (scroll-up-command) in a buffer shows the next
complete screen, only 2 lines from the previous screen are shown at the
top of the next screen.

I'm surprised that 'C-v' in your configuration scrolls only by half screen.

Re: Simple isearch concerns

[Ergus]

Hi Juri:

Sorry to bother. But I don't understand much of this email. Is this documented
somewhere? Could you add a simple use case example for a simple user
or dumb lisper like me?


On Wed, Apr 07, 2021 at 11:12:12PM +0300, Juri Linkov wrote:
>
>If you want a separate option, this is fine.  But then there is no need
>to duplicate the key remapping feature.  To make the new feature more powerful,
>you could attach to command symbol properties a function that
>defines where to move point before searching the next match.
>
>This is similar to how isearch-yank-internal funcalls its arg 'jumpform',
>and how its callers like isearch-yank-char uses
>
>  (lambda () (forward-char arg) (point))
>
>or isearch-yank-word uses
>
>  (lambda () (forward-word arg) (point))
>
>to define where to move point before using it as the buffer position.
>
>So command symbol properties could be defined using the same logic:
>
>  (put 'beginning-of-buffer 'isearch-match-scroll 'beginning-of-buffer)
>  (put 'end-of-buffer 'isearch-match-scroll 'end-of-buffer)
>  (put 'scroll-up-command 'isearch-match-scroll (lambda () (goto-char (window-end))))
>  (put 'scroll-down-command 'isearch-match-scroll (lambda () (goto-char (window-start))))
>
>where the symbol property is called like (funcall 'beginning-of-buffer)
>before repeating the search.
>
>Then there is no need to add such ad-hoc commands as isearch-scroll-up
>and isearch-scroll-down.
>
>And instead of
>
>  (setq this-command (get this-command 'isearch-match-scroll))
>
>isearch-pre-command-hook could contain the same code
>that is currently duplicated in several commands:
>
>  (setq isearch-just-started t)
>  (goto-char (window-end))  ;; only this line needs to be
>                            ;; replaced with (funcall jumpform)
>  (isearch-repeat 'forward)
>
>Whereas it would be easy to handle more commands like below:
>

>
>Actually, the 'isearch-match-scroll' above doesn't do what is needed.
>It just goes to the next match, whereas the need was to go to the next line
>before searching for a new match.
>
>With the logic above, this is easy to define like:
>
>  (put 'next-line 'isearch-match-scroll (lambda () (forward-line 1)))
>

Re: Simple isearch concerns

[Augusto Stoffel]

The non-quitting scroll commands in isearch are an extremely useful
addition.  But I looked at the implementation now and I'm finding it
problematic.

At some point it was decided that instead of just defining
'isearch-scroll-up' and 'isearch-scroll-down commands', some
pre-command-hook trickery would be used.  Presumably this is so that an
user option, 'isearch-allow-scroll', could be provided (that's what I
gather from the parent message of this email).

But this has several disadvantages: you can't assign different keys for
the quitting and non-quitting versions of the scroll commands; you can't
put the (nonexistent) isearch-scroll-up/down commands in a repeat map;
you can't activate the feature selectively, e.g. so that scrolling
doesn't quit isearch but beginning/end of buffer does; you can't advice
or redefine isearch-scroll-up/down, because they don't exist; it's
harder for external package to integrate (I had to copy a piece of the
isearch-pre-command-hook into isearch-mb.el).

The obvious and natural way to implement this feature would be to define
isearch-scroll-up/down commands, bind them in the M-s prefix, and let
whoever wants it by default to rebind the keys.

Perhaps it's not too late to fix this.  Sorry I didn't bring this up at
the time of the discussion.

Re: Simple isearch concerns

[Alan Mackenzie]

Hello, Augusto.

On Thu, Mar 03, 2022 at 17:36:35 +0100, Augusto Stoffel wrote:
> The non-quitting scroll commands in isearch are an extremely useful
> addition.  But I looked at the implementation now and I'm finding it
> problematic.

> At some point it was decided that instead of just defining
> 'isearch-scroll-up' and 'isearch-scroll-down commands', some
> pre-command-hook trickery would be used.  Presumably this is so that an
> user option, 'isearch-allow-scroll', could be provided (that's what I
> gather from the parent message of this email).

This is not quite how the history of this facility was.  (I wrote the
original version back in 2003.)  isearch-allow-scroll was at the centre
of the mechanism right from the beginning.  The idea was that users
could use the same keys for scrolling whilst inside an isearch as in
ordinary editing.  Commands like isearch-scroll-up never came into
consideration.  Originally, the pre-command- and post-command- hooks
weren't used.  They were implemented as a code clean-up some while later
(I think, by Juri Linkov).

> But this has several disadvantages: you can't assign different keys for
> the quitting and non-quitting versions of the scroll commands;

How is that a disadvantage?  To quit and scroll requires RET plus a
scrolling key sequence.  Were we to find new key sequences just for a
quitting version of lots of commands, they would be longer than for the
two distinct commands together.

> you can't put the (nonexistent) isearch-scroll-up/down commands in a
> repeat map; you can't activate the feature selectively, e.g. so that
> scrolling doesn't quit isearch but beginning/end of buffer does;

For this, you can just set the isearch-allow-scroll property to nil for
the commands you want to quit isearch.

> you can't advice or redefine isearch-scroll-up/down, because they
> don't exist;

You can advise the scrolling commands directly, testing for being inside
an isearch if needed.

> it's harder for external package to integrate (I had to copy a piece
> of the isearch-pre-command-hook into isearch-mb.el).

I'm not sure what you mean by "integrate", here: integrate with what, to
what purpose?

> The obvious and natural way to implement this feature would be to define
> isearch-scroll-up/down commands, bind them in the M-s prefix, and let
> whoever wants it by default to rebind the keys.

I think there are around 20 commands which have the
isearch-allow-scroll (or scroll-command) property set.  I have a few
more personal commands, bringing my total to around 30.

> Perhaps it's not too late to fix this.  Sorry I didn't bring this up at
> the time of the discussion.

What is to happen to my 30 scrolling commands?  Would I have to find 30
new key sequences to bind them to?  I wouldn't like that, and I don't
think most users of isearch scrolling would, either.

Could you perhaps be more explicit on what you're trying to do, and why
the current implementation of isearch scrolling makes this difficult?
Maybe there's a way forward which doesn't involve wholesale rewriting.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Simple isearch concerns

[Augusto Stoffel]

On Thu,  3 Mar 2022 at 17:50, Alan Mackenzie <acm@muc.de> wrote:

> Hello, Augusto.
>
> On Thu, Mar 03, 2022 at 17:36:35 +0100, Augusto Stoffel wrote:
>> The non-quitting scroll commands in isearch are an extremely useful
>> addition.  But I looked at the implementation now and I'm finding it
>> problematic.
>
>> At some point it was decided that instead of just defining
>> 'isearch-scroll-up' and 'isearch-scroll-down commands', some
>> pre-command-hook trickery would be used.  Presumably this is so that an
>> user option, 'isearch-allow-scroll', could be provided (that's what I
>> gather from the parent message of this email).
>
> This is not quite how the history of this facility was.  (I wrote the
> original version back in 2003.)  isearch-allow-scroll was at the centre
> of the mechanism right from the beginning.  The idea was that users
> could use the same keys for scrolling whilst inside an isearch as in
> ordinary editing.  Commands like isearch-scroll-up never came into
> consideration.  Originally, the pre-command- and post-command- hooks
> weren't used.  They were implemented as a code clean-up some while later
> (I think, by Juri Linkov).

Sorry, I meant to say the `isearch-allow-motion' option, which is new in
Emacs 28.

>> you can't put the (nonexistent) isearch-scroll-up/down commands in a
>> repeat map; you can't activate the feature selectively, e.g. so that
>> scrolling doesn't quit isearch but beginning/end of buffer does;
>
> For this, you can just set the isearch-allow-scroll property to nil for
> the commands you want to quit isearch.

Is `isearch-allow-scroll' as a property documented at all?

> I think there are around 20 commands which have the
> isearch-allow-scroll (or scroll-command) property set.  I have a few
> more personal commands, bringing my total to around 30.

Okay, I understand the reasons to have `isearch-allow-scroll, but only 4
commands have `isearch-motion', and two (beginning/end-of-buffer) are
redundant (they already had isearch variants).  So this new property
(and code path in the pre command hook) was essentially introduced for
the sake of 2 command (scroll up and down).

If the idea here was to leave space for even more motion commands (C-a,
C-n, etc.) to be made non quitting, then perhaps it makes sense.  But
this framework is so special that it's impossible for anyone who didn't
read isearch.el to suppose it exists.  Normally you would try to changes
things via customization variables, key bindings, and advices, in that
order...

Re: Simple isearch concerns

[Eli Zaretskii]

> From: Augusto Stoffel <arstoffel@gmail.com>
> Date: Thu, 03 Mar 2022 19:39:55 +0100
> Cc: gregory@heytings.org, emacs-devel@gnu.org, spacibba@aol.com,
>  juri@linkov.net
> 
> Is `isearch-allow-scroll' as a property documented at all?

Yes, see the node "Not Exiting Isearch" in the Emacs user manual.

Re: Simple isearch concerns

[Howard Melman]

Gregory Heytings <gregory@heytings.org> writes:

> Indeed, see the attached simplified patch.

> +@vindex isearch-allow-match-scroll
> +  Likewise, if you change the variable @code{isearch-allow-match-scroll}
> +to a non-@code{nil} value, this enables the use of the keyboard scrolling
> +commands @kbd{M-<}, @kbd{M->}, @kbd{C-v} and @kbd{M-v}, to move
> +respectively to the first first occurrence of the current search
> +string in the buffer, the last one, the first one after the current
> +window, and the last one before the current window.

I think there's a typo above: "the first first occurrence".

It's also repeated in the defcustom docstring.

Howard

Re: Simple isearch concerns

[Ergus]

>
>Note that neither C-RET nor S-RET are available in a terminal.
>
Then M-RET can be used?