bug#74287: [PATCH] Rework history Isearch for Eshell

bug#74287: [PATCH] Rework history Isearch for Eshell

[Pengji Zhang]

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

Hello,

This patch brings a comint-like interface for history Isearch to Eshell.
To try it, type 'M-r' in Eshell, and search through the input history
ring incrementally.

Compared to the existing implementation, this patch integrates with
Isearch properly, like what we do for the minibuffer and comint modes.

There are relevant discussions (thank you all for the feedback!) in the
mailing list:

    https://lists.gnu.org/archive/html/emacs-devel/2024-11/msg00069.html

For Jim's concern, I do not think this implementation closes the door
for a history completion interface, as shown by Sean and Juri.

Thanks!

Pengji


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Rework-history-Isearch-for-Eshell.patch --]
[-- Type: text/x-patch, Size: 20739 bytes --]

From d455ec1bf1b49871cf866457df77f6ab45f5623f Mon Sep 17 00:00:00 2001
From: Pengji Zhang <me@pengjiz.com>
Date: Sun, 10 Nov 2024 08:50:27 +0800
Subject: [PATCH] Rework history Isearch for Eshell

This is to make history Isearch for Eshell similar to that of
'comint-mode', by hooking into Isearch properly instead of
defining new commands to emulate Isearch.

* lisp/eshell/em-hist.el (eshell-history-isearch): New user
option.
(eshell-goto-history, eshell--isearch-setup)
(eshell-history-isearch-end, eshell-history-isearch-search)
(eshell-history-isearch-message, eshell-history-isearch-wrap)
(eshell-history-isearch-push-state): New functions.
(eshell-isearch-backward-regexp, eshell-isearch-forward-regexp):
New commands.
(eshell--history-isearch-messasge-overlay)
(eshell--stored-incomplete-input): New internal variables.
(eshell-hist-mode-map): Bind 'M-r' to
'eshell-isearch-backward-regexp' and free 'M-s' binding for
normal in-buffer search commands.
(eshell-isearch-backward, eshell-isearch-forward): Use the new
way to start searching.
(eshell-hist-initialize): Use the new Isearch setup function.
(eshell-previous-matching-input): Use 'eshell-goto-history'.
Also inhibit messages when searching.
(eshell-isearch-map, eshell-isearch-repeat-backward)
(eshell-isearch-abort, eshell-isearch-delete-char)
(eshell-isearch-return, eshell-isearch-cancel)
(eshell-isearch-repeat-forward, eshell-test-imatch)
(eshell-return-to-prompt, eshell-prepare-for-search): Remove.
These are for the old history Isearch implementation.

* doc/misc/eshell.texi (History): Document changes.
* etc/NEWS: Annouce changes.
---
 doc/misc/eshell.texi   |  15 +-
 etc/NEWS               |  23 +++
 lisp/eshell/em-hist.el | 322 ++++++++++++++++++++++++-----------------
 3 files changed, 223 insertions(+), 137 deletions(-)

diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi
index ee4d0ca09c8..701137ea1b4 100644
--- a/doc/misc/eshell.texi
+++ b/doc/misc/eshell.texi
@@ -2661,10 +2661,10 @@ History
 
 @table @kbd
 @kindex M-r
-@kindex M-s
 @item M-r
-@itemx M-s
-History I-search.
+History I-search.  @kbd{M-r} starts an incremental search in input
+history.  While searching, type @kbd{C-r} to move to the previous match,
+and @kbd{C-s} to move to the next match in the input history.
 
 @kindex M-p
 @kindex M-n
@@ -2675,6 +2675,15 @@ History
 previous or next line that begins with that string.
 @end table
 
+@vindex eshell-history-isearch
+If you would like to use the default Isearch key-bindings to search
+through input history, you may customize @code{eshell-history-isearch}
+to @code{t}.  That makes, for example, @kbd{C-r} and @kbd{C-M-r} in an
+Eshell buffer search in input history only.  In addition, if the value
+of @code{eshell-history-isearch} is @code{dwim}, those commands search
+in the history when the point is after the last prompt, and search in
+the buffer when the point is before or within the last prompt.
+
 @node Extension modules
 @chapter Extension modules
 Eshell provides a facility for defining extension modules so that they
diff --git a/etc/NEWS b/etc/NEWS
index d1c7303f976..c1df6f78d9f 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -288,6 +288,29 @@ This hook runs after an Eshell session has been fully initialized,
 immediately before running 'eshell-post-command-hook' for the first
 time.
 
++++
+*** Improved history Isearch.
+History Isearch in Eshell is reworked.  Two new commands
+'eshell-isearch-backward-regexp' and 'eshell-isearch-forward-regexp' are
+added for incrementally searching through the input history.
+'eshell-isearch-backward-regexp' is bound to 'M-r' by default, and 'M-s'
+is freed for normal search commands.  If you would like to restore the
+previous key-bindings for the non-incremental search commands, put in
+your configuration:
+
+    (keymap-set eshell-hist-mode-map "M-r"
+                #'eshell-previous-matching-input)
+    (keymap-set eshell-hist-mode-map "M-s"
+                #'eshell-next-matching-input)
+
++++
+*** New user option 'eshell-history-isearch'
+When 'eshell-history-isearch' is nil (the default), Isearch commands
+search in the buffer contents.  If you customize it to t, those commands
+only search in input history.  If you customize it to the symbol 'dwim',
+those commands search in input history only when the point is after the
+last prompt.
+
 ** SHR
 
 +++
diff --git a/lisp/eshell/em-hist.el b/lisp/eshell/em-hist.el
index fffd611c06f..de986b05f48 100644
--- a/lisp/eshell/em-hist.el
+++ b/lisp/eshell/em-hist.el
@@ -34,7 +34,6 @@
 ;; Also, most of `comint-mode's keybindings are accepted:
 ;;
 ;;   M-r     ; search backward for a previous command by regexp
-;;   M-s     ; search forward for a previous command by regexp
 ;;   M-p     ; access the last command entered, repeatable
 ;;   M-n     ; access the first command entered, repeatable
 ;;
@@ -132,6 +131,17 @@ eshell-input-filter
                 (function :tag "Other function"))
   :risky t)
 
+(defcustom eshell-history-isearch nil
+  "Non-nil to Isearch in input history only.
+If t, usual Isearch keys like \\[isearch-forward] in Eshell search in
+the input history only.  If `dwim', Isearch in the input history when
+point is at the command line, otherwise search in the current Eshell
+buffer."
+  :type '(choice (const :tag "Don't search in input history" nil)
+                 (const :tag "Search histroy when point is on command line" dwim)
+                 (const :tag "Always search in input history" t))
+  :version "31.1")
+
 (defun eshell-hist--update-keymap (symbol value)
   "Update `eshell-hist-mode-map' for `eshell-hist-match-partial'."
   ;; Don't try to set this before it is bound.  See below.
@@ -204,25 +214,17 @@ eshell-save-history-index
 (defvar eshell-hist--new-items nil
   "The number of new history items that have not been written to
 file.  This variable is local in each eshell buffer.")
-
-(defvar-keymap eshell-isearch-map
-  :doc "Keymap used in isearch in Eshell."
-  :parent isearch-mode-map
-  "C-m"         #'eshell-isearch-return
-  "C-r"         #'eshell-isearch-repeat-backward
-  "C-s"         #'eshell-isearch-repeat-forward
-  "C-g"         #'eshell-isearch-abort
-  "<backspace>" #'eshell-isearch-delete-char
-  "<delete>"    #'eshell-isearch-delete-char
-  "C-c C-c"     #'eshell-isearch-cancel)
+(defvar-local eshell--history-isearch-messasge-overlay nil
+  "Overlay for Isearch message when searching through input history.")
+(defvar-local eshell--stored-incomplete-input nil
+  "Stored input for history cycling.")
 
 (defvar-keymap eshell-hist-mode-map
   "<up>"     #'eshell-previous-matching-input-from-input
   "<down>"   #'eshell-next-matching-input-from-input
   "C-<up>"   #'eshell-previous-input
   "C-<down>" #'eshell-next-input
-  "M-r"      #'eshell-previous-matching-input
-  "M-s"      #'eshell-next-matching-input
+  "M-r"      #'eshell-isearch-backward-regexp
   "C-c M-r"  #'eshell-previous-matching-input-from-input
   "C-c M-s"  #'eshell-next-matching-input-from-input
   "C-c C-l"  #'eshell-list-history
@@ -261,20 +263,9 @@ eshell-hist-initialize
 	   (not eshell-non-interactive-p))
       (let ((rebind-alist eshell-rebind-keys-alist))
         (setq-local eshell-rebind-keys-alist
-	      (append rebind-alist eshell-hist-rebind-keys-alist))
-        (setq-local search-invisible t)
-        (setq-local search-exit-option t)
-	(add-hook 'isearch-mode-hook
-                  (lambda ()
-                    (if (>= (point) eshell-last-output-end)
-                        (setq overriding-terminal-local-map
-                              eshell-isearch-map)))
-                  nil t)
-	(add-hook 'isearch-mode-end-hook
-                  (lambda ()
-                    (setq overriding-terminal-local-map nil))
-                  nil t))
+	            (append rebind-alist eshell-hist-rebind-keys-alist)))
     (eshell-hist-mode))
+  (add-hook 'isearch-mode-hook #'eshell--isearch-setup nil t)
 
   (make-local-variable 'eshell-history-size)
   (or eshell-history-size
@@ -384,6 +375,23 @@ eshell-get-history
   "Get an input line from the history ring."
   (ring-ref (or ring eshell-history-ring) index))
 
+(defun eshell-goto-history (pos)
+  "Replace command line with the element at POS of history ring.
+Also update `eshell-history-index'.  As a special case, if POS is nil
+and `eshell--stored-incomplete-input' is a non-empty string, restore the
+saved input."
+  (when (null eshell-history-index)
+    (setq eshell--stored-incomplete-input
+          (buffer-substring-no-properties eshell-last-output-end
+                                          (point-max))))
+  (setq eshell-history-index pos)
+  ;; Can't use kill-region as it sets this-command
+  (delete-region eshell-last-output-end (point-max))
+  (if (and pos (not (ring-empty-p eshell-history-ring)))
+      (insert-and-inherit (eshell-get-history pos))
+    (when (> (length eshell--stored-incomplete-input) 0)
+      (insert-and-inherit eshell--stored-incomplete-input))))
+
 (defun eshell-add-input-to-history (input)
   "Add the string INPUT to the history ring.
 Input is entered into the input history ring, if the value of
@@ -897,12 +905,12 @@ eshell-previous-matching-input
     ;; Has a match been found?
     (if (null pos)
 	(error "Not found")
-      (setq eshell-history-index pos)
-      (unless (minibuffer-window-active-p (selected-window))
-	(message "History item: %d" (- (ring-length eshell-history-ring) pos)))
-      ;; Can't use kill-region as it sets this-command
-      (delete-region eshell-last-output-end (point))
-      (insert-and-inherit (eshell-get-history pos)))))
+      (eshell-goto-history pos)
+      (unless (or (minibuffer-window-active-p (selected-window))
+                  ;; No messages for Isearch because it will show the
+                  ;; same messages (and more).
+                  isearch-mode)
+	(message "History item: %d" (- (ring-length eshell-history-ring) pos))))))
 
 (defun eshell-next-matching-input (regexp arg)
   "Search forwards through input history for match for REGEXP.
@@ -937,114 +945,160 @@ eshell-next-matching-input-from-input
   (interactive "p")
   (eshell-previous-matching-input-from-input (- arg)))
 
-(defun eshell-test-imatch ()
-  "If isearch match good, put point at the beginning and return non-nil."
-  (if (get-text-property (point) 'history)
-      (progn (beginning-of-line) t)
-    (let ((before (point)))
-      (beginning-of-line)
-      (if (and (not (bolp))
-	       (<= (point) before))
-	  t
-	(if isearch-forward
-	    (progn
-	      (end-of-line)
-	      (forward-char))
-	  (beginning-of-line)
-	  (backward-char))))))
-
-(defun eshell-return-to-prompt ()
-  "Once a search string matches, insert it at the end and go there."
-  (setq isearch-other-end nil)
-  (let ((found (eshell-test-imatch)) before)
-    (while (and (not found)
-		(setq before
-		      (funcall (if isearch-forward
-				   're-search-forward
-				 're-search-backward)
-			       isearch-string nil t)))
-      (setq found (eshell-test-imatch)))
-    (if (not found)
-	(progn
-	  (goto-char eshell-last-output-end)
-	  (delete-region (point) (point-max)))
-      (setq before (point))
-      (let ((text (buffer-substring-no-properties
-		   (point) (line-end-position)))
-	    (orig (marker-position eshell-last-output-end)))
-	(goto-char eshell-last-output-end)
-	(delete-region (point) (point-max))
-	(when (and text (> (length text) 0))
-	  (insert text)
-	  (put-text-property (1- (point)) (point)
-			     'last-search-pos before)
-	  (set-marker eshell-last-output-end orig)
-	  (goto-char eshell-last-output-end))))))
-
-(defun eshell-prepare-for-search ()
-  "Make sure the old history file is at the beginning of the buffer."
-  (unless (get-text-property (point-min) 'history)
-    (save-excursion
-      (goto-char (point-min))
-      (let ((end (copy-marker (point) t)))
-	(insert-file-contents eshell-history-file-name)
-	(set-text-properties (point-min) end
-			     '(history t invisible t))))))
+(defun eshell--isearch-setup ()
+  "Set up Isearch to search the input history.
+Intended to be added to `isearch-mode-hook' in an Eshell buffer."
+  (when (and
+         ;; Eshell is busy running a foreground process
+         (not eshell-foreground-command)
+         (or (eq eshell-history-isearch t)
+             (and (eq eshell-history-isearch 'dwim)
+                  (>= (point) eshell-last-output-end))))
+    (setq isearch-message-prefix-add "history ")
+    (setq-local isearch-lazy-count nil)
+    (setq-local isearch-search-fun-function #'eshell-history-isearch-search
+                isearch-message-function #'eshell-history-isearch-message
+                isearch-wrap-function #'eshell-history-isearch-wrap
+                isearch-push-state-function #'eshell-history-isearch-push-state)
+    (add-hook 'isearch-mode-end-hook #'eshell-history-isearch-end nil t)))
+
+(defun eshell-history-isearch-end ()
+  "Clean up after terminating history Isearch."
+  (when (overlayp eshell--history-isearch-messasge-overlay)
+    (delete-overlay eshell--history-isearch-messasge-overlay))
+  (setq isearch-message-prefix-add nil)
+  (kill-local-variable 'isearch-lazy-count)
+  (setq-local isearch-search-fun-function #'isearch-search-fun-default
+              isearch-message-function nil
+              isearch-wrap-function nil
+              isearch-push-state-function nil)
+  (remove-hook 'isearch-mode-end-hook #'eshell-history-isearch-end t)
+  (setq isearch-opoint (point))
+  (unless isearch-suspended
+    (custom-reevaluate-setting 'eshell-history-isearch)))
+
+(defun eshell-history-isearch-search ()
+  "Return search function for Isearch in input history."
+  (lambda (string bound noerror)
+    (let ((search-fun (isearch-search-fun-default))
+          (found nil))
+      ;; Avoid highlighting matches in and before the last prompt
+      (when (and bound isearch-forward
+                 (< (point) eshell-last-output-end))
+        (goto-char eshell-last-output-end))
+      (or
+       ;; First search in the initial input
+       (funcall search-fun string
+                (if isearch-forward bound eshell-last-output-end)
+                noerror)
+       ;; Then search in the input history: put next/previous history
+       ;; element in the command line successively, then search the
+       ;; string in the command line.  Do this only when not
+       ;; lazy-highlighting (`bound' is nil).
+       (unless bound
+         (condition-case nil
+             (progn
+               (while (not found)
+                 (cond (isearch-forward
+                        ;; Signal an error explicitly to break
+                        (when (or (null eshell-history-index)
+                                  (eq eshell-history-index 0))
+                          (error "End of history; no next item"))
+                        (eshell-next-input 1)
+                        (goto-char eshell-last-output-end))
+                       (t
+                        ;; Signal an error explicitly to break
+                        (when (eq eshell-history-index
+                                  (1- (ring-length eshell-history-ring)))
+                          (error "Beginning of history; no preceding item"))
+                        (eshell-previous-input 1)
+                        (goto-char (point-max))))
+                 (setq isearch-barrier (point)
+                       isearch-opoint (point))
+                 ;; After putting an history element in the command
+                 ;; line, search the string in them.
+                 (setq found (funcall search-fun string
+                                      (unless isearch-forward
+                                        eshell-last-output-end)
+                                      noerror)))
+               (point))
+           ;; Return when no next/preceding element error signaled
+           (error nil)))))))
+
+(defun eshell-history-isearch-message (&optional c-q-hack ellipsis)
+  "Display the input history search prompt.
+If there are no search errors, this function displays an overlay with
+the Isearch prompt which replaces the original Eshell prompt.
+Otherwise, it displays the standard Isearch message returned from the
+function `isearch-message'."
+  (if (not (and isearch-success (not isearch-error)))
+      ;; Use standard message function (which displays a message in the
+      ;; echo area) when not in command line, or search fails or has
+      ;; errors (like incomplete regexp).
+      (isearch-message c-q-hack ellipsis)
+    ;; Otherwise, use an overlay over the Eshell prompt.
+    (if (overlayp eshell--history-isearch-messasge-overlay)
+        (move-overlay eshell--history-isearch-messasge-overlay
+                      (save-excursion
+                        (goto-char eshell-last-output-end)
+                        (forward-line 0)
+                        (point))
+                      eshell-last-output-end)
+      (setq eshell--history-isearch-messasge-overlay
+            (make-overlay (save-excursion
+                            (goto-char eshell-last-output-end)
+                            (forward-line 0)
+                            (point))
+                          eshell-last-output-end))
+      (overlay-put eshell--history-isearch-messasge-overlay 'evaporate t))
+    (overlay-put eshell--history-isearch-messasge-overlay
+                 'display (isearch-message-prefix ellipsis
+                                                  isearch-nonincremental))
+    (if (and eshell-history-index (not ellipsis))
+        (message "History item: %d" (- (ring-length eshell-history-ring)
+                                       eshell-history-index))
+      (message ""))))
+
+(defun eshell-history-isearch-wrap ()
+  "Wrap the input history search."
+  (if isearch-forward
+      (eshell-goto-history (1- (ring-length eshell-history-ring)))
+    (eshell-goto-history nil))
+  (goto-char (if isearch-forward eshell-last-output-end (point-max))))
+
+(defun eshell-history-isearch-push-state ()
+  "Save a function restoring the state of input history search.
+Save `eshell-history-index' to the additional state parameter in the
+search status stack."
+  (let ((index eshell-history-index))
+    (lambda (_cmd)
+      (eshell-goto-history index))))
 
 (defun eshell-isearch-backward (&optional invert)
-  "Do incremental regexp search backward through past commands."
-  (interactive)
-  (let ((inhibit-read-only t))
-    (eshell-prepare-for-search)
-    (goto-char (point-max))
-    (set-marker eshell-last-output-end (point))
-    (delete-region (point) (point-max)))
-  (isearch-mode invert t 'eshell-return-to-prompt))
-
-(defun eshell-isearch-repeat-backward (&optional invert)
-  "Do incremental regexp search backward through past commands."
-  (interactive)
-  (let ((old-pos (get-text-property (1- (point-max))
-				    'last-search-pos)))
-    (when old-pos
-      (goto-char old-pos)
-      (if invert
-	  (end-of-line)
-	(backward-char)))
-    (setq isearch-forward invert)
-    (isearch-search-and-update)))
+  "Do incremental search backward through past commands."
+  (interactive nil eshell-mode)
+  (setq eshell-history-isearch t)
+  (if invert
+      (isearch-forward nil t)
+    (isearch-backward nil t)))
 
 (defun eshell-isearch-forward ()
-  "Do incremental regexp search backward through past commands."
-  (interactive)
+  "Do incremental search forward through past commands."
+  (interactive nil eshell-mode)
   (eshell-isearch-backward t))
 
-(defun eshell-isearch-repeat-forward ()
+(defun eshell-isearch-backward-regexp (&optional invert)
   "Do incremental regexp search backward through past commands."
-  (interactive)
-  (eshell-isearch-repeat-backward t))
-
-(defun eshell-isearch-cancel ()
-  (interactive)
-  (goto-char eshell-last-output-end)
-  (delete-region (point) (point-max))
-  (call-interactively 'isearch-cancel))
-
-(defun eshell-isearch-abort ()
-  (interactive)
-  (goto-char eshell-last-output-end)
-  (delete-region (point) (point-max))
-  (call-interactively 'isearch-abort))
-
-(defun eshell-isearch-delete-char ()
-  (interactive)
-  (save-excursion
-  (isearch-delete-char)))
-
-(defun eshell-isearch-return ()
-  (interactive)
-  (isearch-done)
-  (eshell-send-input))
+  (interactive nil eshell-mode)
+  (setq eshell-history-isearch t)
+  (if invert
+      (isearch-forward-regexp nil t)
+    (isearch-backward-regexp nil t)))
+
+(defun eshell-isearch-forward-regexp ()
+  "Do incremental regexp search forward through past commands."
+  (interactive nil eshell-mode)
+  (eshell-isearch-backward-regexp t))
 
 (defun em-hist-unload-function ()
   (remove-hook 'kill-emacs-hook 'eshell-save-some-history))
-- 
2.47.0

bug#74287: [PATCH] Rework history Isearch for Eshell

[James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Pengji Zhang wrote:

> Hello,
>
> This patch brings a comint-like interface for history Isearch to Eshell.
> To try it, type 'M-r' in Eshell, and search through the input history
> ring incrementally.
>
> Compared to the existing implementation, this patch integrates with
> Isearch properly, like what we do for the minibuffer and comint modes.
>
> There are relevant discussions (thank you all for the feedback!) in the
> mailing list:
>
>     https://lists.gnu.org/archive/html/emacs-devel/2024-11/msg00069.html
>
> For Jim's concern, I do not think this implementation closes the door
> for a history completion interface, as shown by Sean and Juri.

Just wanna add a POC to the alternatives suggested. WDYT? Does not use
regexps for now, and C-. and C-, for cycling (rather than C-r and C-s):

(setq icomplete-in-buffer t)
(icomplete-mode 1)

(defun my/eshell-history-complete nil
  (interactive)
  (let ((icomplete-compute-delay 0)
	(completion-at-point-functions
	 '((lambda nil
	     `(,eshell-last-output-end ,(point) ,(append (cddr eshell-history-ring) nil) . nil)))))
    (completion-at-point)))

(define-key eshell-hist-mode-map (kbd "M-r") #'my/eshell-history-complete)

--

bug#74287: [PATCH] Rework history Isearch for Eshell

[Pengji Zhang]

James Thomas <jimjoe@gmx.net> writes:

> Just wanna add a POC to the alternatives suggested. WDYT? Does not use
> regexps for now, and C-. and C-, for cycling (rather than C-r and C-s):
>
> (setq icomplete-in-buffer t)
> (icomplete-mode 1)
>
> (defun my/eshell-history-complete nil
>   (interactive)
>   (let ((icomplete-compute-delay 0)
> 	(completion-at-point-functions
> 	 '((lambda nil
> 	     `(,eshell-last-output-end ,(point) ,(append (cddr eshell-history-ring) nil) . nil)))))
>     (completion-at-point)))
>
> (define-key eshell-hist-mode-map (kbd "M-r") #'my/eshell-history-complete)

Thanks! While I like the simplicity of your approach, I am not sure if
it is a good idea to depend on 'icomplete-mode'. People may have
different preferences for the in-buffer completion framework.

bug#74287: [PATCH] Rework history Isearch for Eshell

[Jim Porter]

On 11/9/2024 5:22 PM, Pengji Zhang wrote:
> Hello,
> 
> This patch brings a comint-like interface for history Isearch to Eshell.
> To try it, type 'M-r' in Eshell, and search through the input history
> ring incrementally.

 From inspection, this code all looks good to me. At least, it matches 
what Comint does; I don't know much about Isearch's implementation, so 
I'm just trusting that Comint is right here. Since this is a 
somewhat-large patch, I'm going to try and find some time this week to 
test it out so that I understand how it all works, and assuming I don't 
find any major problems, I'll merge it shortly after that.

Thanks for your contribution to Eshell (and Emacs as a whole)!

bug#74287: [PATCH] Rework history Isearch for Eshell

[James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Jim Porter wrote:

> On 11/9/2024 5:22 PM, Pengji Zhang wrote:
>> Hello,
>> This patch brings a comint-like interface for history Isearch to
>> Eshell.
>> To try it, type 'M-r' in Eshell, and search through the input history
>> ring incrementally.
>
> From inspection, this code all looks good to me. At least, it matches
> what Comint does; I don't know much about Isearch's implementation, so
> I'm just trusting that Comint is right here. Since this is a
> somewhat-large patch, I'm going to try and find some time this week to
> test it out so that I understand how it all works, and assuming I
> don't find any major problems, I'll merge it shortly after that.

My 2c: I now think it's better to hold out for something with way less
code, using capf.

--

bug#74287: [PATCH] Rework history Isearch for Eshell

[Jim Porter]

On 11/26/2024 1:41 AM, James Thomas via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> My 2c: I now think it's better to hold out for something with way less
> code, using capf.

Like mentioned in the emacs-devel list, that's probably how *I'd* want a 
command history search implementation to use, but I also think that an 
Isearch interface makes a lot of sense as an option, depending on user 
preferences. As far as I can tell, we'd have to write different code for 
each of these styles (i.e. you can't just make Isearch use capf). Though 
as mentioned I'm not very familiar with Isearch's internals.

So long as this doesn't prevent a capf-based implementation (and I'm 
guessing it wouldn't, since that would just be a new option), I think we 
could merge this patch. I also prefer to have Shell and Eshell work the 
same way unless there's some compelling reason why they should be different.

bug#74287: [PATCH] Rework history Isearch for Eshell

[James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Jim Porter wrote:

> On 11/26/2024 1:41 AM, James Thomas via Bug reports for GNU Emacs, the
> Swiss army knife of text editors wrote:
>> My 2c: I now think it's better to hold out for something with way less
>> code, using capf.
>
> Like mentioned in the emacs-devel list, that's probably how *I'd* want
> a command history search implementation to use, but I also think that
> an Isearch interface makes a lot of sense as an option, depending on
> user preferences. As far as I can tell, we'd have to write different
> code for each of these styles (i.e. you can't just make Isearch use
> capf). Though as mentioned I'm not very familiar with Isearch's
> internals.
>
> So long as this doesn't prevent a capf-based implementation (and I'm
> guessing it wouldn't, since that would just be a new option), I think
> we could merge this patch.

Okay. In that case, my preference would be for it to be a separate
module ala eshell-smart et. al., if possible; the motive being easing
the maintenance burden.

> I also prefer to have Shell and Eshell work the same way unless
> there's some compelling reason why they should be different.

Shell could change to be like eshell, for that, no? (like Wittgenstein's
bit with the ruler) ;-)

--

bug#74287: [PATCH] Rework history Isearch for Eshell

[Juri Linkov]

> Like mentioned in the emacs-devel list, that's probably how *I'd* want
> a command history search implementation to use, but I also think that an
> Isearch interface makes a lot of sense as an option, depending on user
> preferences. As far as I can tell, we'd have to write different code for
> each of these styles (i.e. you can't just make Isearch use capf). Though as
> mentioned I'm not very familiar with Isearch's internals.

I confirm that the patch uses the right Isearch's internals
to implement the search through Eshell history.

(There is only one typo in the variable name
'eshell--history-isearch-messasge-overlay'.)

> So long as this doesn't prevent a capf-based implementation (and I'm
> guessing it wouldn't, since that would just be a new option), I think we
> could merge this patch. I also prefer to have Shell and Eshell work the
> same way unless there's some compelling reason why they should be
> different.

Having Shell and Eshell work the same way is a nice goal.
There is a patch that implements completion for Shell in bug#74694.
Then Eshell could have something like this later as well.