RE: PATCH: isearch-yank-until-match

[Drew Adams <drew.adams@oracle.com>]

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

Sorry this message is long.  I've tried to make it
clear.

> C-s C-w C-r C-w messes up the search string.  But I
> believe this could be fixed.

Thx.  The attached patch takes care of this, I think.

> > Which part(s) do you think should be only optional?
> 
> Changing the behavior of yanking in backward search.
> C-r C-w is still useful to yank forward like it does now.
> I don't remember when needed to yank backward.
> Do you use backward yanking often?

This message tries to make things clearer.  See the
attached patch for fixes and related new features.

[As for whether I "use backward yanking often", the
answer is no, because it isn't available! ;-)  But when
searching backward it makes sense to be able to yank
buffer text to the left of the search point, prepending
(not appending) it to the search string.]

---

I see 3 kinds of "yanking" to the search string (there
may be more):

 1. Yank an arbitrary string.

    Examples:
    * text from the kill-ring
    * secondary-selection text
    * text from a register

 2. Yank consecutive text at the search point from the
    buffer.

    Examples:
    * successive chars
    * successive words
    * successive lines
    * successive chars up to a particular char

 3. Yank buffer text from the search point up to
    an arbitrary buffer position.

    Examples:
    * text up to the destination of a movement
      command/key
    * text up to the position resulting from a
      recursive edit

In general, these call for, or invite, three different
behaviors:

 1. An arbitrary string should always be pulled onto
    the end of the search string, regardless of the
    current search direction.

 2. Consecutive text at point should be pulled onto the
    front of the search string, where "front" is in the
    search direction.  This means prepend the text when
    searching backward and append it when searching
    forward.

    (If you want to grab both text that precedes the
    search hit and text that succeeds it then you need
    to change search directions between the two grabs.)

 3. When buffer text from the search point to an
    arbitrary position is involved we should let that
    position adjust the search string, to either expand
    or reduce it.

    That is, we should not limit such adjustment to
    extension; we should let the arbitrary position
    determine whether to extend or retract, as follows:

    * If the position is outside the search hit then
      extend the search string to add the buffer text
      from the hit to the position.

    * If the position is inside the search hit then
      reduce the search string to remove the buffer
      text from the hit to the position.

    Search direction remains the same, in any case.

    (That means that, unlike case #2, you don't need to
    change search direction to grab text both left and
    right of the search hit.)

For case #2, you've asked that respecting search
direction be made optional.  OK, the attached patch
does that (option `isearch-directional-yank').

I don't think there's a good reason not to respect
search direction by default, however, so the option
value respects direction by default.  (Just one
opinion.)

 [FWIW, I think that unless search-direction is
  respected (prepend/append), yanking text that is
  consecutive in the forward direction when searching
  backward is just, well, broken/wrong.

  That it has always been broken (so no one uses it) is
  not a good reason to keep the old, wrong behavior by
  default.  The real right thing is to not bother with
  the option and just always respect search direction
  for this case #2.  But I've respected your request.]

The attached patch does those things.  Following case
#2, it makes these commands respect search direction,
provided option `isearch-directional-yank' is non-nil:

 `isearch-yank-word-or-char'
 `isearch-yank-char'
 `isearch-yank-word'
 `isearch-yank-line',
 `isearch-yank-until-char'
 `isearch-yank-until-match'

And following case #3, it defines these commands that
adjust the search string according to an arbitrary
buffer position (inside or outside, cursor movement
left or right from the search point):

  `isearch-yank-to-key-destination'
  `isearch-yank-through-move'

The new commands are these:

* `isearch-yank-until-char': A version of Karl's
  command that respects `isearch-directional-yank'.

* `isearch-yank-until-match' (mentioned previously):
  Yank text up through a match for another pattern.

* `isearch-yank-to-key-destination': A version of the
  imagined command we discussed that lets you use a
  movement key (e.g. `M-f', `C-b') to adjust the search
  string.

  But it works whether the key moves point left or
  right, and whether the destination is inside or
  outside the search hit.

  In addition, you can use a prefix arg with the
  command.  The prefix arg is transferred to the
  movement key.

* `isearch-yank-through-move': Enters a recursive edit,
  where you can move the cursor any way you like, any
  number of times, and then exit the recursive edit.
  The command adjusts the search string according to
  the resulting cursor position.

The last two allow other action, besides just cursor
movement.  I think this is generally a plus, but I can
imagine that someone might disagree.

(In that case, `isearch-yank-to-key-destination' could
bind `buffer-read-only', to prevent buffer changes.
But there's no way to control what a user might do in a
recursive edit, AFAIK.  E.g., there doesn't seem to be
any hook that's used when entering or exiting a
recursive edit.)

A macro, `define-isearch-yank-movement-command', is
used to define the last two.  Their only differences
are:

* `isearch-yank-to-key-destination' reads a key,
  inhibits field-text motion, disables bindings of the
  Isearch keymap, and invokes the key's command.

* `isearch-yank-through-move' calls `recursive-edit'.

As for key bindings, here are my thoughts:

* `C-M-.' for `isearch-yank-until-char'.

* `C-M-m' (aka `M-RET') for `isearch-yank-until-match'.

* `C-M-c' for `isearch-yank-through-move' (_not_ for
  `isearch-yank-until-char').

  `C-M-c' is bound globally to `exit-recursive-edit'.
  It's how you tell `isearch-yank-through-move' that
  you're done moving the cursor.  It helps to use the
  same key to start and end the command.

  And because of that global binding, no (other)
  Isearch command should be bound to `C-M-c'.  (I
  hadn't considered this before, when I said I was OK
  with `C-M-c' for `isearch-yank-until-char'.)

The only binding I feel strongly about is `C-M-c'.  I
don't have a suggestion yet about a binding for
`isearch-yank-to-key-destination'.
___

P.S. The patch also renames the parameter to
     `isearch-yank-internal' from JUMPFORM to JUMPFUN,
     because it's a function, not a form.  And it fixes
     the doc string accordingly: "function", not
     necessarily "lambda expression".

[-- Attachment #2: isearch-2019-08-19c.patch --]
[-- Type: application/octet-stream, Size: 16150 bytes --]

diff -u isearch-2019-08-19a.el isearch-2019-08-19c-PATCH.el
--- isearch-2019-08-19a.el	2019-08-18 17:10:13.609445800 -0700
+++ isearch-2019-08-19c-PATCH.el	2019-08-20 14:52:42.410184900 -0700
@@ -173,6 +173,10 @@
 command history."
   :type 'boolean)
 
+(defcustom isearch-directional-yank t
+  "Non-nil if yanking consecutive text at point respects search direction."
+  :type 'boolean)
+
 (defvar isearch-mode-hook nil
   "Function(s) to call after starting up an incremental search.")
 
@@ -706,6 +710,9 @@
     (define-key map "\M-\C-y" 'isearch-yank-char)
     (define-key map    "\C-y" 'isearch-yank-kill)
     (define-key map "\M-s\C-e" 'isearch-yank-line)
+    (define-key map (kbd "C-M-.") 'isearch-yank-until-char)
+    (define-key map (kbd "C-M-m") 'isearch-yank-until-match)
+    (define-key map (kbd "C-M-c") 'isearch-yank-through-move)
 
     (define-key map "\M-s\M-<" 'isearch-beginning-of-buffer)
     (define-key map "\M-s\M->" 'isearch-end-of-buffer)
@@ -1731,6 +1738,64 @@
      (isearch-abort)  ;; outside of let to restore outside global values
      )))
 
+(defmacro define-isearch-yank-movement-command (command arguments doc-string
+                                                        interactive
+                                                        bindings action)
+  "Define Isearch COMMAND to adjust search string based on cursor position.
+The command should move the cursor, which is at one end of the current
+search hit, to a new location.
+
+The existing search string is expanded or reduced to include the
+buffer text from the search hit through the new cursor position.
+
+ARGUMENTS is a list of arguments to the command.
+DOC-STRING is the command's doc string.
+INTERACTIVE is `interative' form.
+BINDINGS is a list of `let*' bindings added around the command code.
+  Local variable `isearch-new-position' is also bound, before the
+  BINDINGS you provide - see ACTION, below.
+BINDINGS is macroexpanded, so it can also be a macro call that expands
+to a list of bindings.
+
+ACTION is your code that moves the cursor.  It should set variable
+`isearch-new-position' to the new cursor position."
+  (let ((fwd     (make-symbol "fwd"))
+        (beg     (make-symbol "beg"))
+        (end     (make-symbol "end"))
+        (min-be  (make-symbol "min-be"))
+        (max-be  (make-symbol "max-be")))
+    `(defun ,command ,arguments ,doc-string
+            ,interactive
+            (let ((,fwd  isearch-forward)
+                  (,beg  isearch-other-end)
+                  (,end  (point))
+                  isearch-new-position)
+              (let* ,bindings
+                ,action
+                (let ((,min-be  (min ,beg ,end))
+                      (,max-be  (max ,beg ,end)))
+                  (setq isearch-string     (if (< isearch-new-position ,beg)
+                                               (buffer-substring
+                                                (min isearch-new-position ,max-be)
+                                                (max isearch-new-position ,max-be))
+                                             (buffer-substring
+                                              (min isearch-new-position ,min-be)
+                                              (max isearch-new-position ,min-be)))
+                        isearch-message    (mapconcat 'isearch-text-char-description
+                                                      isearch-string "")
+                        isearch-barrier    (if (or (and ,fwd
+                                                        (< isearch-new-position ,beg))
+                                                   (and (not ,fwd)
+                                                        (not (< isearch-new-position ,beg))))
+                                               ,end
+                                             isearch-new-position)
+                        isearch-other-end  (if (< isearch-new-position ,beg)
+                                               (if ,fwd isearch-new-position ,beg)
+                                             (if (not ,fwd) isearch-new-position ,beg)))
+                  (goto-char isearch-barrier)
+                  (isearch-highlight isearch-other-end isearch-barrier)
+                  (when isearch-lazy-highlight (isearch-lazy-highlight-new-loop))))))))
+
 (defvar minibuffer-history-symbol) ;; from external package gmhist.el
 
 (defun isearch-edit-string ()
@@ -2447,8 +2512,9 @@
   (isearch-push-state)
   (isearch-update))
 
-(defun isearch-yank-string (string)
-  "Pull STRING into search string."
+(defun isearch-yank-string (string &optional respect-direction)
+  "Pull STRING into search string.
+Non-nil RESPECT-DIRECTION means prepend STRING if searching backward."
   ;; Downcase the string if not supposed to case-fold yanked strings.
   (if (and isearch-case-fold-search
 	   (eq 'not-yanks search-upper-case))
@@ -2456,8 +2522,9 @@
   (if isearch-regexp (setq string (regexp-quote string)))
   ;; Don't move cursor in reverse search.
   (setq isearch-yank-flag t)
-  (isearch-process-search-string
-   string (mapconcat 'isearch-text-char-description string "")))
+  (isearch-process-search-string string
+				 (mapconcat 'isearch-text-char-description string "")
+				 respect-direction))
 
 (defun isearch-yank-kill ()
   "Pull string from kill ring into search string."
@@ -2508,17 +2575,18 @@
   (interactive)
   (isearch-yank-string (xterm--pasted-text)))
 
-(defun isearch-yank-internal (jumpform)
-  "Pull the text from point to the point reached by JUMPFORM.
-JUMPFORM is a lambda expression that takes no arguments and returns
-a buffer position, possibly having moved point to that position.
-For example, it might move point forward by a word and return point,
-or it might return the position of the end of the line."
+(defun isearch-yank-internal (jumpfun &optional respect-direction)
+  "Pull the text from point to the point reached by JUMPFUN.
+JUMPFUN is a function that takes no arguments and returns a buffer
+position, possibly having moved point to that position.
+
+For example, JUMPFUN might move forward by a word and return point, or
+it might return the position of the end of the line.
+
+Non-nil RESPECT-DIRECTION means prepend text if searching backward."
   (isearch-yank-string
-   (save-excursion
-     (and (not isearch-forward) isearch-other-end
-	  (goto-char isearch-other-end))
-     (buffer-substring-no-properties (point) (funcall jumpform)))))
+   (save-excursion (buffer-substring-no-properties (point) (funcall jumpfun)))
+   respect-direction))
 
 (defun isearch-yank-char-in-minibuffer (&optional arg)
   "Pull next character from buffer into end of search string in minibuffer."
@@ -2531,44 +2599,189 @@
     (forward-char arg)))
 
 (defun isearch-yank-char (&optional arg)
-  "Pull next character from buffer into search string.
-If optional ARG is non-nil, pull in the next ARG characters."
+  "Pull character from buffer into search string.
+If `isearch-directional-yank' is non-nil then use next char for
+forward search, previous char for backward search.
+
+With a numeric prefix ARG, pull in ARG characters."
   (interactive "p")
-  (isearch-yank-internal (lambda () (forward-char arg) (point))))
+  (isearch-yank-internal
+   (lambda ()
+     (funcall (if (or isearch-forward  (not isearch-directional-yank))
+                  #'forward-char
+                #'backward-char)
+              arg)
+     (point))
+   isearch-directional-yank))
 
-(defun isearch--yank-char-or-syntax (syntax-list fn)
+(defun isearch--yank-char-or-syntax (syntax-list fn &optional respect-direction)
   (isearch-yank-internal
    (lambda ()
      (if (or (memq (char-syntax (or (char-after) 0)) syntax-list)
              (memq (char-syntax (or (char-after (1+ (point))) 0))
                    syntax-list))
 	 (funcall fn 1)
-       (forward-char 1))
-     (point))))
+       (if (or isearch-forward  (not respect-direction))
+           (forward-char 1)
+         (backward-char 1)))
+     (point))
+   respect-direction))
 
 (defun isearch-yank-word-or-char ()
+  "Pull character or word from buffer into search string.
+If `isearch-directional-yank' is non-nil then yank next one for
+forward search, previous one for backward search."
+  (interactive)
+  (if (or isearch-forward  (not isearch-directional-yank))
+      (isearch--yank-char-or-syntax '(?w) 'forward-word isearch-directional-yank)
+    (isearch--yank-char-or-syntax '(?w) 'backward-word 'RESPECT-DIRECTION)))
+
+(defun isearch-yank-word-or-char-forward ()
   "Pull next character or word from buffer into search string."
   (interactive)
-  (isearch--yank-char-or-syntax '(?w) 'forward-word))
+  (isearch--yank-char-or-syntax '(?w) 'forward-word isearch-directional-yank))
 
-(defun isearch-yank-symbol-or-char ()
-  "Pull next character or symbol from buffer into search string."
+(defun isearch-yank-word-or-char-backward ()
+  "Pull previous character or word from buffer into search string."
   (interactive)
-  (isearch--yank-char-or-syntax '(?w ?_) 'forward-symbol))
+  (isearch--yank-char-or-syntax '(?w) 'backward-word 'RESPECT-DIRECTION))
+
+(defun isearch-yank-symbol-or-char ()
+  "Pull character or symbol from buffer into search string.
+If `isearch-directional-yank' is non-nil then pull next one for
+forward search, previous one for backward search."
+  (interactive)
+  (if (or isearch-forward  (not isearch-directional-yank))
+      (isearch--yank-char-or-syntax '(?w ?_) 'forward-symbol isearch-directional-yank)
+    (isearch--yank-char-or-syntax '(?w ?_) 'backward-symbol 'RESPECT-DIRECTION)))
 
 (defun isearch-yank-word (&optional arg)
-  "Pull next word from buffer into search string.
-If optional ARG is non-nil, pull in the next ARG words."
+  "Pull word from buffer into search string.
+If `isearch-directional-yank' is non-nil then pull next word for
+forward search, previous word for backward search.
+
+With a numeric prefix ARG, pull in ARG words."
   (interactive "p")
-  (isearch-yank-internal (lambda () (forward-word arg) (point))))
+  (isearch-yank-internal
+   (lambda ()
+     (funcall (if (or isearch-forward  (not isearch-directional-yank))
+                  #'forward-word
+                #'backward-word)
+              arg)
+     (point))
+   isearch-directional-yank))
 
 (defun isearch-yank-line (&optional arg)
   "Pull rest of line from buffer into search string.
-If optional ARG is non-nil, yank the next ARG lines."
+If `isearch-directional-yank' is non-nil then pull in rest of line in
+search direction.
+
+With a numeric prefix ARG, pull in ARG lines."
+  (interactive "p")
+  (if (or isearch-forward  (not isearch-directional-yank))
+      (isearch-yank-line-forward arg)
+    (isearch-yank-line-backward arg)))
+
+(defun isearch-yank-line-forward (&optional arg)
+  "Pull rest of line, going forward, from buffer into search string.
+With a numeric prefix ARG, pull in the next ARG lines."
+  (interactive "p")
+  (isearch-yank-internal
+   (lambda ()
+     (let ((inhibit-field-text-motion  t))
+       (line-end-position (if (eolp) (1+ arg) arg))))))
+
+(defun isearch-yank-line-backward (&optional arg)
+  "Pull rest of line, going backward, from buffer into search string.
+With a numeric prefix ARG, pull in the previous ARG lines."
   (interactive "p")
   (isearch-yank-internal
-   (lambda () (let ((inhibit-field-text-motion t))
-		(line-end-position (if (eolp) (1+ arg) arg))))))
+   (lambda ()
+     (let ((inhibit-field-text-motion  t)
+	   (arg2                       (- 2 arg)))
+       (line-beginning-position (if (bolp) (1- arg2) arg2))))
+   'RESPECT-DIRECTION))
+
+(defun isearch-yank-until-char (char)
+  "Pull buffer text, up to next instance of CHAR, into search string.
+You are prompted for CHAR."
+  (interactive "cYank until character: ")
+  (isearch-yank-internal
+   (lambda ()
+     (let ((inhibit-field-text-motion  t))
+       (funcall (if (or isearch-forward  (not isearch-directional-yank))
+                    #'search-forward
+                  #'search-backward)
+		(char-to-string char))
+       (if isearch-forward (backward-char) (forward-char))
+       (point)))
+   isearch-directional-yank))
+
+(defun isearch-yank-until-match (arg)
+  "Pull text, through match for another pattern, into search string.
+You are prompted for the pattern.
+With a prefix arg, match the pattern as a regexp."
+  (interactive "P")
+  (let ((fwd  (or isearch-forward  (not isearch-directional-yank)))
+        pattern)
+    (with-isearch-suspended
+     (setq pattern  (if arg (read-regexp "Match regexp: ") (read-string "Match: "))))
+    (isearch-yank-internal
+     (lambda ()
+       (let ((inhibit-field-text-motion  t))
+         (funcall (if arg
+                      (if fwd #'search-forward-regexp #'search-backward-regexp)
+                    (if fwd #'search-forward #'search-backward))
+                  pattern)
+         (point)))
+     isearch-directional-yank)))
+
+(define-isearch-yank-movement-command isearch-yank-to-key-destination (key)
+  "Adjust search to use text from search hit through a key destination.
+You are prompted for a key sequence that moves the cursor.  The key
+can do anything else as well, but only the new cursor position is used
+by the command.
+
+If it makes sense for the key, you can use a prefix arg with `\\<isearch-mode-map>\
+\\[isearch-yank-to-key-destination]'
+to apply the prefix arg to the key.  For example, `C-u 5 \
+\\[isearch-yank-to-key-destination] M-f'
+moves the cursor forward 5 words and adjusts the search string
+accordingly.
+
+If the new position is outside the existing search hit then the text
+from the search hit to the new position is added to the search string.
+If the position is inside the hit, then the text from the edge of the
+hit through the new position is removed from the search string."
+  (interactive "kKey sequence (to move cursor): ")
+  ((inhibit-field-text-motion  t)
+   (isearch-mode-map           nil)
+   (command                    (or (local-key-binding key t)
+                                   (global-key-binding key t))))
+  (save-excursion
+    (call-interactively command)
+    (setq isearch-new-position  (point))))
+
+(define-isearch-yank-movement-command isearch-yank-through-move ()
+  "Adjust search to use text from search hit through a new cursor position.
+You enter a recursive edit to move the cursor any way you like.
+Use \\[exit-recursive-edit] to resume search with the adjusted search string.
+
+In the recursive edit you can do anything, but the effect used by the
+command is only cursor movement to a new position.
+
+If the new position is outside the existing search hi,t then the text
+from the search hit to the new position is added to the search string.
+If the position is inside the hit, then the text from the edge of the
+hit through the new position is removed from the search string."
+  (interactive)
+  ()
+  (with-isearch-suspended
+   (save-excursion
+     (message (substitute-command-keys
+               "RECURSIVE edit. `\\[exit-recursive-edit]' to resume Isearch"))
+     (recursive-edit)
+     (setq isearch-new-position  (point)))))
 
 (defun isearch-char-by-name (&optional count)
   "Read a character by its Unicode name and add it to the search string.
@@ -3014,9 +3227,16 @@
 		    (mapconcat 'isearch-text-char-description string ""))))
     (isearch-process-search-string string message)))
 
-(defun isearch-process-search-string (string message)
-  (setq isearch-string (concat isearch-string string)
-	isearch-message (concat isearch-message message))
+(defun isearch-process-search-string (string message &optional respect-direction)
+  "Add STRING to `isearch-string' and MESSAGE to `isearch-message'.
+Append STRING if searching forward.  Prepend if searching backward."
+  (let ((fwd  (or isearch-forward  (not respect-direction))))
+    (setq isearch-string   (if fwd
+			       (concat isearch-string string)
+			     (concat string isearch-string))
+	  isearch-message  (if fwd
+			       (concat isearch-message message)
+			     (concat message isearch-message))))
   (isearch-search-and-update))
 
 \f