bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Juri Linkov]

> 1. I'm still puzzled about this:
>
>   However, the current match will never scroll offscreen.
>   If `unlimited', the current match can scroll offscreen.
>
> Those two sentences don't make sense together.
> If the current match never scrolls offscreen then
> it can't be true that the current match can scroll
> offscreen.  Something different needs to be said
> here.

Do you think this is better?

  "If non-nil, scrolling commands can be used in Isearch mode.
  However, the current match can't scroll offscreen if the value is t.
  But if it's `unlimited', the current match can scroll offscreen.
  You may want to enable `lazy-highlight-buffer' in this case.
  If nil, scrolling commands will cancel Isearch mode."

If you don't agree, please suggest a better wording.

> 2. And the term "offscreen" doesn't seem like a
> good choice.  Don't we mean just off the window,
> i.e., out of view?  So this too bothers/confuses me:
>
>   "Allow scrolling within screen"
>
> I don't know what is meant by "screen" here.
> What are the limits of the "screen" within
> which I can scroll with this option value?

I guess a screen is part of the buffer visible in the window.
We use the word "screen" in other docstrings as well.

> 3. I'm also puzzled by this:
>
>   You may want to enable `lazy-highlight-buffer'
>   as well.
>
> Why say that?  I think it confuses people, by
> suggesting that for some reason you might need
> to do that, in order to see such highlighting
> if you scroll (e.g. "offscreen").
>
> That's not the case, is it?  (I hope not.)  I
> thought that the implementation of `unlimited'
> automatically lazy-highlights everything that
> you scroll to.  Is that not the case?

No, this is not the case.  This is very difficult
to implement, and not worth the effort because this feature
is already available by customizing lazy-highlight-buffer.

Moreover, even if implemented, it still makes no sense
because it can't highlight as quick as you scroll
thru the buffer.

The problem is that you haven't tried my patch
with lazy-highlight-buffer enabled.  If you tried
you wouldn't want any other implementation.

> 4. I think it would be better for the :tag
> "Disable scrolling" to say something like this:
> "Scrolling exits Isearch".

Thanks, fixed.

> 5. Doc string of `*-allow-shift-selection':
>
>   Whether motion is allowed to select text during
>   incremental search.
>
> That will possibly make users think that this is
> about activating the region (selecting text).
>
> The first doc-string line should kind of stand on
> its own, to give an overall idea.  I'd say
> something more like this:
>
>   Motion keys yank text to the search string.

Thanks, fixed.

> 6. `*-allow-shift-selection' behavior:
>
> Why is the value `move'?  Is that the best word?
>
> What happens with `move' if you use a shifted
> motion key?  If it acts the same as `t' then what
> you call "shifted" is really "-only_ when shifted".
> What you call just "motion" seems like just both
> shifted and unshifted.
>
> Why do we even have two such choices?  Why would
> someone who wants to yank chars by moving over
> them want to have to use Shift, ever?  Is it so
> that they can use unshifted to exit Isearch and
> move the cursor?  I guess so.  Maybe that could
> be made clearer (dunno).
>
> 7. OK, no, `move' is apparently more complicated
> than that (all the more reason why it shouldn't
> be called `move'.)  This text is a mouthful:
>
>   by motion commands that have the `isearch-move'
>   property on their symbols equal to `enabled',
>   or [for which] the shift-translated command is
>   not disabled by the value `disabled' of the same
>   property.
>
> That sounds a bit like a legal text. You can just
> repeat "property `isearch-move'" instead of saying
> "the same property" - that would be clearer.  But
> the sentence probably needs to be split.  And it
> may be a sign that the behavior is too complicated.
>
> Why do we have this complicated behavior?  Why
> not just have a `move' value that lets you yank
> text without having to use Shift (i.e. using Shift
> or not)?  What's the point of having to put such a
> property on some command symbols?
>
> And if there really is a use case for doing that
> to certain commands, so that _only they_ manifest
> a difference between `t' and `move' behavior,
> then why not have only the `move' behavior (no `t'
> behavior)?

Sorry, I don't understand what do you suggest here.

> 8.This option should not be called `*-shift-selection'
> if it is not necessarily about Shift selection.
> The option is apparently about yanking text you
> move the cursor over.

So I renamed it to `isearch-allow-yank-on-move',
and its options to `no-shift' and `shift'.

> 9. Again, I'm not crazy about this :tag, for the
> same reason as above:
>
>   Disable shift selection
>
> A nil option value doesn't disable anything.  It
> just means that cursor motion ends Isearch, so you
> just move over the buffer text.

Thanks for the suggestion, I changed it to
"Motion keys exits Isearch".

diff --git a/lisp/isearch.el b/lisp/isearch.el
index eb0b25f9b1..b0284a5972 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -72,21 +72,11 @@ search-exit-option
 If t, random control and meta characters terminate the search
 and are then executed normally.
 If `edit', edit the search string instead of exiting.
-If `move', extend the search string by motion commands
-that have the `isearch-move' property on their symbols
-equal to `enabled', or the shift-translated command is
-not disabled by the value `disabled' of the same property.
-If `shift-move', extend the search string by motion commands
-while holding down the shift key.
-Both `move' and `shift-move' extend the search string by yanking text
-that ends at the new position after moving point in the current buffer.
 If `append', the characters which you type that are not interpreted by
 the incremental search are simply appended to the search string.
 If nil, run the command without exiting Isearch."
   :type '(choice (const :tag "Terminate incremental search" t)
                  (const :tag "Edit the search string" edit)
-                 (const :tag "Extend the search string by motion commands" 
move)
-                 (const :tag "Extend the search string by shifted motion keys" 
shift-move)
                  (const :tag "Append control characters to the search string" 
append)
                  (const :tag "Don't terminate incremental search" nil))
   :version "27.1")
@@ -2746,9 +2736,13 @@ isearch-fallback
 (defcustom isearch-allow-scroll nil
   "Whether scrolling is allowed during incremental search.
 If non-nil, scrolling commands can be used in Isearch mode.
-However, the current match will never scroll offscreen.
-If nil, scrolling commands will first cancel Isearch mode."
-  :type 'boolean
+However, the current match can't scroll offscreen if the value is t.
+But if it's `unlimited', the current match can scroll offscreen.
+You may want to enable `lazy-highlight-buffer' in this case.
+If nil, scrolling commands will cancel Isearch mode."
+  :type '(choice (const :tag "Scrolling exits Isearch" nil)
+                 (const :tag "Allow scrolling within screen" t)
+                 (const :tag "Allow scrolling offscreen" unlimited))
   :group 'isearch)
 
 (defcustom isearch-allow-prefix t
@@ -2812,6 +2806,21 @@ isearch-back-into-window
 (defvar isearch-pre-scroll-point nil)
 (defvar isearch-pre-move-point nil)
 
+(defcustom isearch-allow-yank-on-move nil
+  "Motion keys yank text to the search string.
+If t, extend the search string by motion commands while holding down
+the shift key.  The search string is extended by yanking text that
+ends at the new position after moving point in the current buffer.
+If `move', extend the search string without the shift key pressed
+by motion commands that have the `isearch-move' property on their
+symbols equal to `enabled', or for which the shift-translated command
+is not disabled by the value `disabled' of property `isearch-move'."
+  :type '(choice (const :tag "Motion keys exits Isearch" nil)
+                 (const :tag "Motion keys extend the search string" no-shift)
+                 (const :tag "Shifted motion keys extend the search string" 
shift))
+  :group 'isearch
+  :version "27.1")
+
 (defun isearch-pre-command-hook ()
   "Decide whether to exit Isearch mode before executing the command.
 Don't exit Isearch if the key sequence that invoked this command
@@ -2845,7 +2854,7 @@ isearch-pre-command-hook
               (symbolp this-command)
               (or (eq (get this-command 'isearch-scroll) t)
                   (eq (get this-command 'scroll-command) t))))
-      (when isearch-allow-scroll
+      (when (and isearch-allow-scroll (not (eq isearch-allow-scroll 
'unlimited)))
        (setq isearch-pre-scroll-point (point))))
      ;; A mouse click on the isearch message starts editing the search string.
      ((and (eq (car-safe main-event) 'down-mouse-1)
@@ -2854,13 +2863,13 @@ isearch-pre-command-hook
       (read-event)
       (setq this-command 'isearch-edit-string))
      ;; Don't terminate the search for motion commands.
-     ((or (and (eq search-exit-option 'move)
+     ((or (and (eq isearch-allow-yank-on-move 'no-shift)
                (symbolp this-command)
                (or (eq (get this-command 'isearch-move) 'enabled)
                    (and (not (eq (get this-command 'isearch-move) 'disabled))
                         (stringp (nth 1 (interactive-form this-command)))
                         (string-match-p "^^" (nth 1 (interactive-form 
this-command))))))
-          (and (eq search-exit-option 'shift-move)
+          (and (eq isearch-allow-yank-on-move 'shift)
                this-command-keys-shift-translated))
       (setq this-command-keys-shift-translated nil)
       (setq isearch-pre-move-point (point)))
@@ -2883,7 +2892,7 @@ isearch-post-command-hook
        (goto-char isearch-pre-scroll-point)))
     (setq isearch-pre-scroll-point nil)
     (isearch-update))
-   ((memq search-exit-option '(move shift-move))
+   (isearch-allow-yank-on-move
     (when (and isearch-pre-move-point
                (not (eq isearch-pre-move-point (point))))
       (let ((string (buffer-substring-no-properties