C-f in isearch's non-incremental minibuffer edit

C-f in isearch's non-incremental minibuffer edit

[Stefan Monnier]

If you hit M-e in isearch (isearch-eidt-string), you're put in a normal
minibuffer where you can edit your search string with the usual command (but
not incrementally any more).  This comes in handy at times.

But I recently noticed that it behaves very strangely: if I move point
towards the end with something like C-f, when I reach the end of the
minibuffer instead of getting the usual beep I see chars from the main
buffer being added.

It seems like it's a new "feature", but it looks much more like a bug to me.
It was introduced as part of:

	* isearch.el (isearch-mode-map): Bind C-M-w to isearch-del-char,
	C-M-y to isearch-yank-char.  Bind M-% to isearch-query-replace,
	C-M-% to isearch-query-replace-regexp.
	(minibuffer-local-isearch-map): Add arrow key bindings.
	Bind C-f to isearch-yank-char-in-minibuffer.
	(isearch-forward): Doc fix.
	(isearch-edit-string): Doc fix.
	(isearch-query-replace, isearch-query-replace-regexp): New funs.
	(isearch-del-char): Add optional arg.  Set isearch-yank-flag to t.
	(isearch-yank-char): Add optional arg.
	(isearch-yank-char-in-minibuffer): New fun.

and I strongly suggest we revert this part of the change: the point of the
isearch-edit-string functionality is to get back a *normal* minibuffer.

While I'm at it, I also suggest we make isearch-edit-string use the usual
history functionality, so it behaves even more normally.

See patch below.  The important part is the one concerning
isearch-yank-char-in-minibuffer: I think it is important to remove those
surprising bindings.

If you object to this patch, please specify if you object to both changes
(the isearch-yank-char-in-minibuffer and the history part) or to only one of
the two.


        Stefan


--- orig/lisp/isearch.el
+++ mod/lisp/isearch.el
@@ -403,17 +403,9 @@
   (let ((map (make-sparse-keymap)))
     (set-keymap-parent map minibuffer-local-map)
     (define-key map "\r"    'isearch-nonincremental-exit-minibuffer)
-    (define-key map "\M-n"  'isearch-ring-advance-edit)
-    (define-key map [next]  'isearch-ring-advance-edit)
-    (define-key map [down]  'isearch-ring-advance-edit)
-    (define-key map "\M-p"  'isearch-ring-retreat-edit)
-    (define-key map [prior] 'isearch-ring-retreat-edit)
-    (define-key map [up]    'isearch-ring-retreat-edit)
     (define-key map "\M-\t" 'isearch-complete-edit)
     (define-key map "\C-s"  'isearch-forward-exit-minibuffer)
     (define-key map "\C-r"  'isearch-reverse-exit-minibuffer)
-    (define-key map "\C-f"  'isearch-yank-char-in-minibuffer)
-    (define-key map [right] 'isearch-yank-char-in-minibuffer)
     map)
   "Keymap for editing isearch strings in the minibuffer.")
 
@@ -911,8 +903,6 @@
 \\[isearch-nonincremental-exit-minibuffer] to do one nonincremental search.
 \\[isearch-forward-exit-minibuffer] to resume isearching forward.
 \\[isearch-reverse-exit-minibuffer] to resume isearching backward.
-\\[isearch-ring-advance-edit] to replace the search string with the next item in the search ring.
-\\[isearch-ring-retreat-edit] to replace the search string with the previous item in the search ring.
 \\[isearch-complete-edit] to complete the search string using the search ring.
 \\<isearch-mode-map>
 If first char entered is \\[isearch-yank-word-or-char], then do word search instead."
@@ -998,12 +988,12 @@
 		  (isearch-unread e))
 		(setq cursor-in-echo-area nil)
 		(setq isearch-new-string
-		      (let (junk-ring)
-			(read-from-minibuffer
-			 (isearch-message-prefix nil isearch-nonincremental)
-			 isearch-string
-			 minibuffer-local-isearch-map nil
-			 'junk-ring nil t))
+                      (read-from-minibuffer
+                       (isearch-message-prefix nil isearch-nonincremental)
+                       isearch-string
+                       minibuffer-local-isearch-map nil
+                       (if isearch-regexp 'regexp-search-ring 'search-ring)
+                       nil t)
 		      isearch-new-message
 		      (mapconcat 'isearch-text-char-description
 				 isearch-new-string "")))
@@ -1358,17 +1348,6 @@
 	  (goto-char isearch-other-end))
      (buffer-substring-no-properties (point) (funcall jumpform)))))
 
-(defun isearch-yank-char-in-minibuffer (&optional arg)
-  "Pull next character from buffer into end of search string in minibuffer."
-  (interactive "p")
-  (if (eobp)
-      (insert
-       (save-excursion
-         (set-buffer (cadr (buffer-list)))
-         (buffer-substring-no-properties
-          (point) (progn (forward-char arg) (point)))))
-    (forward-char arg)))
-
 (defun isearch-yank-char (&optional arg)
   "Pull next character from buffer into search string."
   (interactive "p")
@@ -1912,49 +1891,6 @@
   (interactive)
   (isearch-ring-adjust nil))
 
-(defun isearch-ring-advance-edit (n)
-  "Insert the next element of the search history into the minibuffer.
-With prefix arg N, insert the Nth element."
-  (interactive "p")
-  (let* ((yank-pointer-name (if isearch-regexp
-				'regexp-search-ring-yank-pointer
-			      'search-ring-yank-pointer))
-	 (yank-pointer (eval yank-pointer-name))
-	 (ring (if isearch-regexp regexp-search-ring search-ring))
-	 (length (length ring)))
-    (if (zerop length)
-	()
-      (set yank-pointer-name
-	   (setq yank-pointer
-		 (mod (- (or yank-pointer 0) n)
-		      length)))
-
-      (delete-field)
-      (insert (nth yank-pointer ring))
-      (goto-char (point-max)))))
-
-(defun isearch-ring-retreat-edit (n)
-  "Insert the previous element of the search history into the minibuffer.
-With prefix arg N, insert the Nth element."
-  (interactive "p")
-  (isearch-ring-advance-edit (- n)))
-
-;;(defun isearch-ring-adjust-edit (advance)
-;;  "Use the next or previous search string in the ring while in minibuffer."
-;;  (isearch-ring-adjust1 advance)
-;;  (erase-buffer)
-;;  (insert isearch-string))
-
-;;(defun isearch-ring-advance-edit ()
-;;  (interactive)
-;;  (isearch-ring-adjust-edit 'advance))
-
-;;(defun isearch-ring-retreat-edit ()
-;;  "Retreat to the previous search string in the ring while in the minibuffer."
-;;  (interactive)
-;;  (isearch-ring-adjust-edit nil))
-
-
 (defun isearch-complete1 ()
   ;; Helper for isearch-complete and isearch-complete-edit
   ;; Return t if completion OK, nil if no completion exists.

Re: C-f in isearch's non-incremental minibuffer edit

[Richard Stallman]

    But I recently noticed that it behaves very strangely: if I move point
    towards the end with something like C-f, when I reach the end of the
    minibuffer instead of getting the usual beep I see chars from the main
    buffer being added.

    It seems like it's a new "feature", but it looks much more like a bug to me.

I see your point; however, since C-f at the end of the buffer would
otherwise be an error, this feature seems to be harmlessly
upward-compatible.  Does it actually cause trouble or inconvenience,
or was it just surprise?

    While I'm at it, I also suggest we make isearch-edit-string use the usual
    history functionality, so it behaves even more normally.

Could you tell me, at the level of user-visible behavior, how its
currently differs from the usual history functionality?

Re: C-f in isearch's non-incremental minibuffer edit

[Stefan Monnier]

> I see your point; however, since C-f at the end of the buffer would
> otherwise be an error, this feature seems to be harmlessly
> upward-compatible.  Does it actually cause trouble or inconvenience,
> or was it just surprise?

I found it both surprising and inconvenient, since I then had to use undo
and be careful trying to figure out which change was "mine" and which change
was from C-f.
I can definitely live with it, but I don't think the extra feature is
important enough.

>     While I'm at it, I also suggest we make isearch-edit-string use the usual
>     history functionality, so it behaves even more normally.

> Could you tell me, at the level of user-visible behavior, how its
> currently differs from the usual history functionality?

No.  It basically works the same, although maybe it doesn't skip
things in quite the same order.  But it obeys user's choice to rebind M-p
and M-n in minibuffer-local-map (I have them bound to things similar to
next-complete-history-element).  And it's less code.


        Stefan

Re: C-f in isearch's non-incremental minibuffer edit

[Richard Stallman]

    > Could you tell me, at the level of user-visible behavior, how its
    > currently differs from the usual history functionality?

    No.  It basically works the same, although maybe it doesn't skip
    things in quite the same order.

Could you show me that patch again?  The code you're removing
must have had some motivation.  It might not be a good motivation,
but we can't judge it without seeing what it was.

Re: C-f in isearch's non-incremental minibuffer edit

[Stefan Monnier]

>>>>> "Richard" == Richard Stallman <rms@gnu.org> writes:

>> Could you tell me, at the level of user-visible behavior, how its
>> currently differs from the usual history functionality?

>     No.  It basically works the same, although maybe it doesn't skip
>     things in quite the same order.

> Could you show me that patch again?  The code you're removing
> must have had some motivation.  It might not be a good motivation,
> but we can't judge it without seeing what it was.

Here you go,


        Stefan


--- orig/lisp/isearch.el
+++ mod/lisp/isearch.el
@@ -405,12 +403,6 @@
   (let ((map (make-sparse-keymap)))
     (set-keymap-parent map minibuffer-local-map)
     (define-key map "\r"    'isearch-nonincremental-exit-minibuffer)
-    (define-key map "\M-n"  'isearch-ring-advance-edit)
-    (define-key map [next]  'isearch-ring-advance-edit)
-    (define-key map [down]  'isearch-ring-advance-edit)
-    (define-key map "\M-p"  'isearch-ring-retreat-edit)
-    (define-key map [prior] 'isearch-ring-retreat-edit)
-    (define-key map [up]    'isearch-ring-retreat-edit)
     (define-key map "\M-\t" 'isearch-complete-edit)
     (define-key map "\C-s"  'isearch-forward-exit-minibuffer)
     (define-key map "\C-r"  'isearch-reverse-exit-minibuffer)
@@ -937,8 +903,6 @@
 \\[isearch-nonincremental-exit-minibuffer] to do one nonincremental search.
 \\[isearch-forward-exit-minibuffer] to resume isearching forward.
 \\[isearch-reverse-exit-minibuffer] to resume isearching backward.
-\\[isearch-ring-advance-edit] to replace the search string with the next item in the search ring.
-\\[isearch-ring-retreat-edit] to replace the search string with the previous item in the search ring.
 \\[isearch-complete-edit] to complete the search string using the search ring.
 \\<isearch-mode-map>
 If first char entered is \\[isearch-yank-word-or-char], then do word search instead."
@@ -1024,12 +1024,12 @@
 		  (isearch-unread e))
 		(setq cursor-in-echo-area nil)
 		(setq isearch-new-string
-		      (let (junk-ring)
 			(read-from-minibuffer
                         (isearch-message-prefix nil isearch-nonincremental)
 			 isearch-string
 			 minibuffer-local-isearch-map nil
-			 'junk-ring nil t))
+                       (if isearch-regexp 'regexp-search-ring 'search-ring)
+                       nil t)
 		      isearch-new-message
 		      (mapconcat 'isearch-text-char-description
 				 isearch-new-string "")))
@@ -1860,49 +1890,6 @@
   (interactive)
   (isearch-ring-adjust nil))
 
-(defun isearch-ring-advance-edit (n)
-  "Insert the next element of the search history into the minibuffer.
-With prefix arg N, insert the Nth element."
-  (interactive "p")
-  (let* ((yank-pointer-name (if isearch-regexp
-				'regexp-search-ring-yank-pointer
-			      'search-ring-yank-pointer))
-	 (yank-pointer (eval yank-pointer-name))
-	 (ring (if isearch-regexp regexp-search-ring search-ring))
-	 (length (length ring)))
-    (if (zerop length)
-	()
-      (set yank-pointer-name
-	   (setq yank-pointer
-		 (mod (- (or yank-pointer 0) n)
-		      length)))
-
-      (delete-field)
-      (insert (nth yank-pointer ring))
-      (goto-char (point-max)))))
-
-(defun isearch-ring-retreat-edit (n)
-  "Insert the previous element of the search history into the minibuffer.
-With prefix arg N, insert the Nth element."
-  (interactive "p")
-  (isearch-ring-advance-edit (- n)))
-
-;;(defun isearch-ring-adjust-edit (advance)
-;;  "Use the next or previous search string in the ring while in minibuffer."
-;;  (isearch-ring-adjust1 advance)
-;;  (erase-buffer)
-;;  (insert isearch-string))
-
-;;(defun isearch-ring-advance-edit ()
-;;  (interactive)
-;;  (isearch-ring-adjust-edit 'advance))
-
-;;(defun isearch-ring-retreat-edit ()
-;;  "Retreat to the previous search string in the ring while in the minibuffer."
-;;  (interactive)
-;;  (isearch-ring-adjust-edit nil))
-
-
 (defun isearch-complete1 ()
   ;; Helper for isearch-complete and isearch-complete-edit
   ;; Return t if completion OK, nil if no completion exists.

Re: C-f in isearch's non-incremental minibuffer edit

[Richard Stallman]

    > Could you show me that patch again?  The code you're removing
    > must have had some motivation.  It might not be a good motivation,
    > but we can't judge it without seeing what it was.

    Here you go,

I think that code was written before there was a minibuffer history.
So please install your change.

Re: C-f in isearch's non-incremental minibuffer edit

[Stefan Monnier]

>> Could you show me that patch again?  The code you're removing
>> must have had some motivation.  It might not be a good motivation,
>> but we can't judge it without seeing what it was.

> I think that code was written before there was a minibuffer history.

Hard to believe, but it does seem to be the case indeed.

> So please install your change.

Done,


        Stefan