[WIP PATCH] Controlling Isearch from the minibuffer

[WIP PATCH] Controlling Isearch from the minibuffer

[Augusto Stoffel]

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

I've attached a draft implementation of a minibuffer-controlled mode for
Isearch, as described for instance in
https://lists.gnu.org/archive/html/emacs-devel/2020-01/msg00447.html
To enable the feature, set `isearch-from-minibuffer' to t.

The basic trade-off is that this makes it easier to edit the search
string, and harder to quit the search.  It also drops some of the
eccentricities of regular Isearch.  For instance, DEL deletes and C-g
quits.

I'm sharing this preliminary version because two important questions can
already be answered:

- Does the approach taken here seem sufficiently robust?  Note in
  particular the `with-isearch-window' macro, which is now needed around
  several functions, as well as the somewhat hacky `run-with-idle-timer'
  call inside the `isearch-mode' function.

- Are the slightly backwards incompatible keybinding changes in
  `isearch-edit-string' acceptable?

If any of these answers is no, then I would provide a package for the
same feature.  But I think the feature is interesting enough to be built
in isearch.el.  Moreover, it would benefit from being official because
many third-party extensions to Isearch will need to take into account
the possibility that the search is being controlled remotely from a
minibuffer.

Some further remarks:

- The minibuffer-controlled mode is supposed to depend on the proposed
  `isearch-buffer-local' feature.  This will make the hack used to
  deactivate the `overriding-terminal-local-map' unnecessary.

- It seems necessary to let-bind `inhibit-redisplay' to nil in
  `with-isearch-window' in order to avoid flicker in the cursor.  This
  seems related to the recent thread "Temporarily select-window, without
  updating mode-line face and cursor fill?" in this list.  Any better
  solutions?

- I don't like the `with-isearch-window-quitting-edit' macro, but I
  don't see a different way of achieving the necessary effect.

- I don't use/know of all Isearch features, so let me know if you spot
  some incompatibility.

What do you think?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Control-Isearch-from-minibuffer-draft.patch --]
[-- Type: text/x-patch, Size: 20801 bytes --]

From d623ea3e00f07688a4b1cff8dd758c3bea45519c Mon Sep 17 00:00:00 2001
From: Augusto Stoffel <arstoffel@gmail.com>
Date: Sat, 8 May 2021 10:24:20 +0200
Subject: [PATCH] Control Isearch from minibuffer (draft)

---
 lisp/isearch.el | 272 ++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 215 insertions(+), 57 deletions(-)

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 9f3cfd70fb..c2a46f7690 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -821,15 +821,32 @@ isearch-tool-bar-map
             :image '(isearch-tool-bar-image "left-arrow")))
     map))
 
+;; WIP for debugging
+(makunbound 'minibuffer-local-isearch-map)
+
 (defvar minibuffer-local-isearch-map
   (let ((map (make-sparse-keymap)))
     (set-keymap-parent map minibuffer-local-map)
-    (define-key map "\r"    'exit-minibuffer)
+    (define-key map "\C-j" 'newline)
     (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)
+    (define-key map "\C-s" 'isearch-repeat-forward)
+    (define-key map [down] 'isearch-repeat-forward)
+    (define-key map "\C-r" 'isearch-repeat-backward)
+    (define-key map [up] 'isearch-repeat-backward)
+    (define-key map "\M-%" 'isearch-query-replace)
+    (define-key map [?\C-\M-%] 'isearch-query-replace-regexp)
+    (define-key map "\M-<" 'isearch-beginning-of-buffer)
+    (define-key map "\M->" 'isearch-end-of-buffer)
+    (define-key map "\M-s'" 'isearch-toggle-char-fold)
+    (define-key map "\M-s " 'isearch-toggle-lax-whitespace)
+    (define-key map "\M-s_" 'isearch-toggle-symbol)
+    (define-key map "\M-sc" 'isearch-toggle-case-fold)
+    (define-key map "\M-shr" 'isearch-highlight-regexp)
+    (define-key map "\M-shl" 'isearch-highlight-lines-matching-regexp)
+    (define-key map "\M-si" 'isearch-toggle-invisible)
+    (define-key map "\M-so" 'isearch-occur)
+    (define-key map "\M-sr" 'isearch-toggle-regexp)
+    (define-key map "\M-sw" 'isearch-toggle-word)
     map)
   "Keymap for editing Isearch strings in the minibuffer.")
 
@@ -980,6 +997,32 @@ search-map
 (define-key search-map    "." 'isearch-forward-symbol-at-point)
 (define-key search-map "\M-." 'isearch-forward-thing-at-point)
 
+;;; Minibuffer-based interface
+
+(defcustom isearch-from-minibuffer nil
+  "If non-nil, control Isearch from the minibuffer."
+  :type 'boolean)
+
+(defmacro with-isearch-window (&rest body)
+  "Execute BODY in the Isearch window."
+  `(if (and (minibufferp)
+            (not (eq isearch--current-buffer (current-buffer))))
+       (let ((inhibit-redisplay t))
+         (with-minibuffer-selected-window
+         ,@body))
+     ,@body))
+
+(defmacro with-isearch-window-quitting-edit (&rest body)
+  "Execute BODY in the Isearch window.
+
+Like `with-isearch-window', but quit editing the search string
+first if applicable.  In this case, control is returned to the
+caller of `isearch-edit-string'.  This must be used if BODY exits
+the search and uses the minibuffer."
+  `(if (and (minibufferp) (not (eq isearch--current-buffer (current-buffer))))
+       (throw 'isearch-edit--continue (lambda () ,@body))
+     ,@body))
+
 ;; Entry points to isearch-mode.
 
 (defun isearch-forward (&optional regexp-p no-recursive-edit)
@@ -1316,9 +1359,16 @@ isearch-mode
   ;; isearch-mode can be made modal (in the sense of not returning to
   ;; the calling function until searching is completed) by entering
   ;; a recursive-edit and exiting it when done isearching.
-  (if recursive-edit
+  (if (and isearch-from-minibuffer (not (minibufferp)))
+      (if recursive-edit
+          (isearch-edit-string t)
+        ;; WIP: This timer hack is used e.g. for
+        ;; `isearch-forward-symbol-at-point' as well as third party
+        ;; packages which prepare a special isearch state.
+        (run-with-idle-timer 0 nil #'isearch-edit-string t))
+    (when recursive-edit
       (let ((isearch-recursive-edit t))
-	(recursive-edit)))
+        (recursive-edit))))
   isearch-success)
 
 
@@ -1518,6 +1568,21 @@ isearch-update-from-string-properties
     (setq isearch-regexp-function
 	  (get-text-property 0 'isearch-regexp-function string))))
 
+(defun isearch-set-string (string &optional properties)
+  "Set the current search string.
+
+Return STRING.  If PROPERTIES is non-nil, also update the search
+mode from the text properties of STRING."
+  (when properties (isearch-update-from-string-properties string))
+  (when isearch-edit--minibuffer
+    (with-current-buffer isearch-edit--minibuffer
+      (let ((inhibit-modification-hooks t))
+        (delete-minibuffer-contents)
+        (insert string))
+      (end-of-buffer)))
+  (setq isearch-message (mapconcat 'isearch-text-char-description string "")
+        isearch-string string))
+
 \f
 ;; The search status structure and stack.
 
@@ -1783,41 +1848,111 @@ with-isearch-suspended
 
 (defvar minibuffer-history-symbol) ;; from external package gmhist.el
 
-(defun isearch-edit-string ()
+(defun isearch-edit--post-command-hook ()
+  "Hook to run from the minibuffer to update the Isearch state."
+  (set-text-properties (minibuffer-prompt-end) (point-max) nil)
+  (when-let ((fail-pos (isearch-fail-pos)))
+    (add-text-properties (+ (minibuffer-prompt-end) fail-pos)
+                         (point-max)
+                         '(face isearch-fail)))
+  (when isearch-error
+    (isearch--momentary-message isearch-error)))
+
+(defun isearch-edit--after-change (_ _ _)
+  "Hook to run from the minibuffer to update the Isearch state."
+  (let ((string (minibuffer-contents)))
+    (with-isearch-window
+     (setq isearch-string (substring-no-properties string))
+     (isearch-update-from-string-properties string)
+     ;; Backtrack to barrier and search, unless the `this-command'
+     ;; is special or the search regexp is invalid.
+     (if (or (and (symbolp this-command)
+                  (get this-command 'isearch-edit-string--no-search))
+             (and isearch-regexp
+                  (condition-case err
+                      (prog1 nil (string-match-p isearch-string ""))
+                    (invalid-regexp
+                     (prog1 t (isearch--momentary-message (cadr err)))))))
+         (isearch-update)
+       (goto-char isearch-barrier)
+       (setq isearch-adjusted t isearch-success t)
+       (isearch-search-and-update)))))
+
+(put 'next-history-element 'isearch-edit-string--no-search t)
+(put 'previous-history-element 'isearch-edit-string--no-search t)
+
+(defvar-local isearch-edit--prompt-overlay nil
+  "Overlay to display the Isearch status in `isearch-edit-string'.")
+
+(defvar-local isearch-edit--minibuffer nil
+  "Pointer to the minibuffer controlling the search.
+Local to the search buffer.  Non-nil only during an
+`isearch-edit-string' session.")
+
+(defun isearch-edit-string (&optional exit)
   "Edit the search string in the minibuffer.
+
+When EXIT is nil, exiting the minibuffer or repeating the search
+resumes Isearch with the edited string.  When EXIT is non-nil,
+exiting the minibuffer also ends the search.
+
 The following additional command keys are active while editing.
-\\<minibuffer-local-isearch-map>
-\\[exit-minibuffer] to resume incremental searching with the edited string.
-\\[isearch-forward-exit-minibuffer] to resume isearching forward.
-\\[isearch-reverse-exit-minibuffer] to resume isearching backward.
-\\[isearch-complete-edit] to complete the search string using the search ring."
+\\{minibuffer-local-isearch-map}"
   (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 "")))))
+  (condition-case nil
+      (funcall
+       (catch 'isearch-edit--continue
+         (let (;; WIP: This is a hack that can be removed when isearch
+               ;; local mode is available.
+               (overriding-terminal-local-map nil)
+               ;; We need to set `inhibit-redisplay' in `with-isearch-window' to
+               ;; avoid flicker.  As a side effect, window-start/end in
+               ;; `isearch-lazy-highlight-update' will have incorrect values,
+               ;; so we need to lazy-highlight the whole buffer.
+               (lazy-highlight-buffer (not (null isearch-lazy-highlight))))
+           (minibuffer-with-setup-hook
+               (lambda ()
+                 (add-hook 'after-change-functions 'isearch-edit--after-change nil t)
+                 (add-hook 'post-command-hook 'isearch-edit--post-command-hook nil t)
+                 (setq-local isearch-edit--prompt-overlay
+                             (make-overlay (point-min) (point-min) (current-buffer) t t))
+                 (let ((inhibit-modification-hooks t)
+                       (mb (current-buffer))
+                       (buf (window-buffer (minibuffer-selected-window))))
+                   (insert (buffer-local-value 'isearch-string buf))
+                   (with-current-buffer buf
+                     (setq-local isearch-edit--minibuffer mb)
+                     (isearch-message)))
+                 (when isearch-error (isearch--momentary-message isearch-error)))
+             (unwind-protect
+                 (read-from-minibuffer
+                  "I-search: "
+                  nil
+                  (if exit
+                      minibuffer-local-isearch-map
+                    (let ((map (make-sparse-keymap)))
+                      (set-keymap-parent map minibuffer-local-isearch-map)
+                      (define-key map
+                        [remap isearch-repeat-forward] 'isearch-forward-exit-minibuffer)
+                      (define-key map
+                        [remap isearch-repeat-backward] 'isearch-reverse-exit-minibuffer)
+                      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))))
+                  (thread-last isearch-forward-thing-at-point
+                               ;;  WIP: The above variable can be renamed
+                    (mapcar 'thing-at-point)
+                    (delq nil)
+                    (delete-dups)
+                    (mapcar (if isearch-regexp 'regexp-quote 'identity)))
+                  t)
+               (setq-local isearch-edit--minibuffer nil)))
+(if (and exit isearch-mode) 'isearch-done 'ignore))))
+    (quit (if (and exit isearch-mode) (isearch-cancel) (signal 'quit nil)))))
 
 (defun isearch-nonincremental-exit-minibuffer ()
   (interactive)
@@ -1879,13 +2014,8 @@ isearch-repeat
 	  ;; If search string is empty, use last one.
 	  (if (null (if isearch-regexp regexp-search-ring search-ring))
 	      (setq isearch-error "No previous search string")
-	    (setq isearch-string
-		  (car (if isearch-regexp regexp-search-ring search-ring))
-		  isearch-message
-		  (mapconcat 'isearch-text-char-description
-			     isearch-string "")
-		  isearch-case-fold-search isearch-last-case-fold-search)
-	    ;; After taking the last element, adjust ring to previous one.
+            (isearch-set-string (car (if isearch-regexp regexp-search-ring search-ring)) t)
+            ;; After taking the last element, adjust ring to previous one.
 	    (isearch-ring-adjust1 nil))
 	;; If already have what to search for, repeat it.
 	(unless (or isearch-success (null isearch-wrap-pause))
@@ -1955,6 +2085,7 @@ isearch-repeat-forward
 search string.  To find the absolute occurrence from the beginning
 of the buffer, type \\[isearch-beginning-of-buffer] with a numeric argument."
   (interactive "P")
+(with-isearch-window
   (if arg
       (let ((count (prefix-numeric-value arg)))
         (cond ((< count 0)
@@ -1968,6 +2099,7 @@ isearch-repeat-forward
                  (when (not isearch-forward) (setq count (1+ count))))
                (isearch-repeat 'forward count))))
     (isearch-repeat 'forward)))
+)
 
 (defun isearch-repeat-backward (&optional arg)
   "Repeat incremental search backwards.
@@ -1978,6 +2110,7 @@ isearch-repeat-backward
 search string.  To find the absolute occurrence from the end
 of the buffer, type \\[isearch-end-of-buffer] with a numeric argument."
   (interactive "P")
+(with-isearch-window
   (if arg
       (let ((count (prefix-numeric-value arg)))
         (cond ((< count 0)
@@ -1991,6 +2124,7 @@ isearch-repeat-backward
                  (when isearch-forward (setq count (1+ count))))
                (isearch-repeat 'backward count))))
     (isearch-repeat 'backward)))
+)
 
 (defun isearch-beginning-of-buffer (&optional arg)
   "Go to the first occurrence of the current search string.
@@ -2000,6 +2134,7 @@ isearch-beginning-of-buffer
 the beginning of the buffer.  To find the next relative occurrence forwards,
 type \\[isearch-repeat-forward] with a numeric argument."
   (interactive "p")
+(with-isearch-window
   (if (and arg (< arg 0))
       (isearch-end-of-buffer (abs arg))
     ;; For the case when the match is at bobp,
@@ -2007,6 +2142,7 @@ isearch-beginning-of-buffer
     (setq isearch-just-started t)
     (goto-char (point-min))
     (isearch-repeat 'forward arg)))
+)
 
 (defun isearch-end-of-buffer (&optional arg)
   "Go to the last occurrence of the current search string.
@@ -2016,11 +2152,13 @@ isearch-end-of-buffer
 the end of the buffer.  To find the next relative occurrence backwards,
 type \\[isearch-repeat-backward] with a numeric argument."
   (interactive "p")
+(with-isearch-window
   (if (and arg (< arg 0))
       (isearch-beginning-of-buffer (abs arg))
     (setq isearch-just-started t)
     (goto-char (point-max))
     (isearch-repeat 'backward arg)))
+)
 
 \f
 ;;; Toggles for `isearch-regexp-function' and `search-default-mode'.
@@ -2040,6 +2178,7 @@ isearch-define-mode-toggle
          ,(format "Toggle %s searching on or off.%s" mode
                   (if docstring (concat "\n" docstring) ""))
          (interactive)
+(with-isearch-window
          (unless isearch-mode (isearch-mode t))
          ,@(when function
              `((setq isearch-regexp-function
@@ -2049,6 +2188,7 @@ isearch-define-mode-toggle
          ,@body
          (setq isearch-success t isearch-adjusted t)
          (isearch-update))
+)
        (define-key isearch-mode-map ,key #',command-name)
        ,@(when (and function (symbolp function))
            `((put ',function 'isearch-message-prefix ,(format "%s " mode))
@@ -2075,6 +2215,9 @@ isearch-message-properties
 (defun isearch--momentary-message (string &optional seconds)
   "Print STRING at the end of the isearch prompt for 1 second.
 The optional argument SECONDS overrides the number of seconds."
+  (if isearch-edit--minibuffer
+      (message (propertize (concat " [" string "]")
+                           'face 'minibuffer-prompt))
   (let ((message-log-max nil))
     (message "%s%s%s"
              (isearch-message-prefix nil isearch-nonincremental)
@@ -2082,6 +2225,7 @@ isearch--momentary-message
              (apply #'propertize (format " [%s]" string)
                     isearch-message-properties)))
   (sit-for (or seconds 1)))
+)
 
 (isearch-define-mode-toggle lax-whitespace " " nil
   "In ordinary search, toggles the value of the variable
@@ -2311,6 +2455,7 @@ isearch-query-replace
 replacements from Isearch is `M-s w ... M-%'."
   (interactive
    (list current-prefix-arg))
+(with-isearch-window-quitting-edit
   (barf-if-buffer-read-only)
   (if regexp-flag (setq isearch-regexp t))
   (let ((case-fold-search isearch-case-fold-search)
@@ -2359,6 +2504,7 @@ isearch-query-replace
      (if (use-region-p) (region-end))
      backward))
   (and isearch-recursive-edit (exit-recursive-edit)))
+)
 
 (defun isearch-query-replace-regexp (&optional arg)
   "Start `query-replace-regexp' with string to replace from last search string.
@@ -2379,6 +2525,7 @@ isearch-occur
 for a literal string, REGEXP is constructed by quoting all the special
 characters in that string."
   (interactive
+(with-isearch-window
    (let* ((perform-collect (consp current-prefix-arg))
 	  (regexp (cond
 		   ((functionp isearch-regexp-function)
@@ -2404,6 +2551,8 @@ isearch-occur
 	     ;; Otherwise normal occur takes numerical prefix argument.
 	     (when current-prefix-arg
 	       (prefix-numeric-value current-prefix-arg))))))
+)
+(with-isearch-window
   (let ((case-fold-search isearch-case-fold-search)
 	;; Set `search-upper-case' to nil to not call
 	;; `isearch-no-upper-case-p' in `occur-1'.
@@ -2421,6 +2570,7 @@ isearch-occur
 	     regexp)
 	   nlines
 	   (if (use-region-p) (region-bounds)))))
+)
 
 (declare-function hi-lock-read-face-name "hi-lock" ())
 
@@ -2461,18 +2611,22 @@ isearch-highlight-regexp
 The arguments passed to `highlight-regexp' are the regexp from
 the last search and the face from `hi-lock-read-face-name'."
   (interactive)
+(with-isearch-window-quitting-edit
   (isearch--highlight-regexp-or-lines
    #'(lambda (regexp face lighter)
        (highlight-regexp regexp face nil lighter))))
+)
 
 (defun isearch-highlight-lines-matching-regexp ()
   "Exit Isearch mode and call `highlight-lines-matching-regexp'.
 The arguments passed to `highlight-lines-matching-regexp' are the
 regexp from the last search and the face from `hi-lock-read-face-name'."
   (interactive)
+(with-isearch-window-quitting-edit
   (isearch--highlight-regexp-or-lines
    #'(lambda (regexp face _lighter)
        (highlight-lines-matching-regexp regexp face))))
+)
 
 \f
 (defun isearch-delete-char ()
@@ -3227,7 +3381,7 @@ isearch-complete1
 		   (all-completions isearch-string ring))))
 	    t)
 	(and completion
-	     (setq isearch-string completion))))
+	  (isearch-set-string completion))))
      (t
       (message "No completion") ; waits a second if in minibuffer
       nil))))
@@ -3238,22 +3392,14 @@ isearch-complete
 If there is no completion possible, say so and continue searching."
   (interactive)
   (if (isearch-complete1)
-      (progn (setq isearch-message
-		   (mapconcat 'isearch-text-char-description
-			      isearch-string ""))
-	     (isearch-edit-string))
-    ;; else
+      (isearch-edit-string)
     (sit-for 1)
     (isearch-update)))
 
 (defun isearch-complete-edit ()
   "Same as `isearch-complete' except in the minibuffer."
   (interactive)
-  (setq isearch-string (field-string))
-  (if (isearch-complete1)
-      (progn
-	(delete-field)
-	(insert isearch-string))))
+  (with-isearch-window (isearch-complete1)))
 
 \f
 ;; Message string
@@ -3267,6 +3413,17 @@ isearch-message
   ;; circumstances are when follow-mode is active, the search string
   ;; spans two (or several) windows, and the message about to be
   ;; displayed will cause the echo area to expand.
+  (if isearch-from-minibuffer
+      (when-let ((mb isearch-edit--minibuffer)
+                 (ov (buffer-local-value 'isearch-edit--prompt-overlay mb)))
+        (overlay-put ov
+                     'before-string
+                     (concat
+                      (when isearch-lazy-count
+                        (format "%-6s" (isearch-lazy-count-format)))
+                      (capitalize
+                       (isearch--describe-regexp-mode
+                        isearch-regexp-function)))))
   (let ((cursor-in-echo-area ellipsis)
 	(m isearch-message)
 	(fail-pos (isearch-fail-pos t)))
@@ -3283,6 +3440,7 @@ isearch-message
 	     m
 	     (isearch-message-suffix c-q-hack)))
     (if c-q-hack m (let ((message-log-max nil)) (message "%s" m)))))
+)
 
 (defun isearch--describe-regexp-mode (regexp-function &optional space-before)
   "Make a string for describing REGEXP-FUNCTION.
-- 
2.30.2

Re: [WIP PATCH] Controlling Isearch from the minibuffer

[Alan Mackenzie]

Hello, Augusto.

On Sat, May 08, 2021 at 12:13:52 +0200, Augusto Stoffel wrote:
> I've attached a draft implementation of a minibuffer-controlled mode for
> Isearch, as described for instance in
> https://lists.gnu.org/archive/html/emacs-devel/2020-01/msg00447.html
> To enable the feature, set `isearch-from-minibuffer' to t.

> The basic trade-off is that this makes it easier to edit the search
> string, and harder to quit the search.

You mean, the patch makes incompatible changes to isearch, yes?

> It also drops some of the eccentricities of regular Isearch.  For
> instance, DEL deletes and C-g quits.

What to you are eccentricities, are features to other people.  I'm very
satisfied with the current workings of C-g, and would protest vehemently
were they to be changed.  I'm not sure what you mean by "DEL deletes" -
what does it delete, and in which circumstances?

> I'm sharing this preliminary version because two important questions can
> already be answered:

> - Does the approach taken here seem sufficiently robust?  Note in
>   particular the `with-isearch-window' macro, which is now needed around
>   several functions, as well as the somewhat hacky `run-with-idle-timer'
>   call inside the `isearch-mode' function.

> - Are the slightly backwards incompatible keybinding changes in
>   `isearch-edit-string' acceptable?

It depends what you mean by "slightly".  I suspect the answer to the
question is no.  Any changes here which aren't simply additions are
going to disturb somebody's workflow.

> If any of these answers is no, then I would provide a package for the
> same feature.  But I think the feature is interesting enough to be built
> in isearch.el.  Moreover, it would benefit from being official because
> many third-party extensions to Isearch will need to take into account
> the possibility that the search is being controlled remotely from a
> minibuffer.

Skimming through your patch (it is too large for me to take in every
detail at the moment), it seems it could make isearch.el even more
difficult to maintain than it currently is.  For example, the two
defmacros `with-...' are the sort that force somebody debugging (or
trying to read a patch) to look somewhere else to find out what they
mean.  This is irksome and tedious.  There are already macros like this
in isearch.el, and it would seem wise to minimise any further
proliferation.  Each of these macros expands the ,@body twice for every
invocation.  Would it not be possible to rearrange things, just to
expand them once?

In one hunk in the patch, I see a condition-case containing a funcall,
containing a catch, containing a minibuffer-with-setup-hook, which in
its turn contains a lambda function and an unwind-protect.  The lambda
function sets functions onto both after-change-functions and
post-command-hook.  Whew!  Each one of these constructs on its own
causes difficulty in debugging, but they have their justification.  Must
they really all appear together in this fashion?

The biggest question, which may have been answered somewhere in the
thread already, is why?  What is wrong with isearch at the moment that
the patch will fix, or enable to be fixed?  The emacs-devel post you
cite above (from 2020) doesn't seem to motivate this change.  Could you
possibly answer this question, or cite a post which answers it, please?

> Some further remarks:

> - The minibuffer-controlled mode is supposed to depend on the proposed
>   `isearch-buffer-local' feature.  This will make the hack used to
>   deactivate the `overriding-terminal-local-map' unnecessary.

> - It seems necessary to let-bind `inhibit-redisplay' to nil in
>   `with-isearch-window' in order to avoid flicker in the cursor.  This
>   seems related to the recent thread "Temporarily select-window, without
>   updating mode-line face and cursor fill?" in this list.  Any better
>   solutions?

> - I don't like the `with-isearch-window-quitting-edit' macro, but I
>   don't see a different way of achieving the necessary effect.

> - I don't use/know of all Isearch features, so let me know if you spot
>   some incompatibility.

> What do you think?

The patch is ~500 lines long.  This makes me worried.  I'm afraid I can
only react negatively and defensively at the moment.

[ patch snipped ].

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: [WIP PATCH] Controlling Isearch from the minibuffer

[Augusto Stoffel]

On Sun,  9 May 2021 at 13:36, Alan Mackenzie <acm@muc.de> wrote:

> Hello, Augusto.

Hi Alan,

>
> On Sat, May 08, 2021 at 12:13:52 +0200, Augusto Stoffel wrote:
>> I've attached a draft implementation of a minibuffer-controlled mode for
>> Isearch, as described for instance in
>> https://lists.gnu.org/archive/html/emacs-devel/2020-01/msg00447.html
>> To enable the feature, set `isearch-from-minibuffer' to t.
>
>> The basic trade-off is that this makes it easier to edit the search
>> string, and harder to quit the search.
>
> You mean, the patch makes incompatible changes to isearch, yes?

I wouldn't dare to suggest turning this on by default :-).

The *slight* incompatible changes are all in `isearch-edit-string'.
This seems like a quite unpolished command anyway, but of course I can
leave it untouched.  I suggest discussing the details in a future
subthread.

>
>> It also drops some of the eccentricities of regular Isearch.  For
>> instance, DEL deletes and C-g quits.
>
> What to you are eccentricities, are features to other people.  I'm very
> satisfied with the current workings of C-g, and would protest vehemently
> were they to be changed.  I'm not sure what you mean by "DEL deletes" -
> what does it delete, and in which circumstances?

I am sure most of the regulars in this thread share your view.
Otherwise, something like this proposal would have surfaced much
earlier.  But this doesn't mean this feature won't be appreciated by
other people.

In the minbuffer-controlled Isearch mode, DEL is simply
`delete-backward-char', not some kind of undo.  It deletes in all
circumstances.

>
>> I'm sharing this preliminary version because two important questions can
>> already be answered:
>
>> - Does the approach taken here seem sufficiently robust?  Note in
>>   particular the `with-isearch-window' macro, which is now needed around
>>   several functions, as well as the somewhat hacky `run-with-idle-timer'
>>   call inside the `isearch-mode' function.
>
>> - Are the slightly backwards incompatible keybinding changes in
>>   `isearch-edit-string' acceptable?
>
> It depends what you mean by "slightly".  I suspect the answer to the
> question is no.  Any changes here which aren't simply additions are
> going to disturb somebody's workflow.
>
>> If any of these answers is no, then I would provide a package for the
>> same feature.  But I think the feature is interesting enough to be built
>> in isearch.el.  Moreover, it would benefit from being official because
>> many third-party extensions to Isearch will need to take into account
>> the possibility that the search is being controlled remotely from a
>> minibuffer.
>
> Skimming through your patch (it is too large for me to take in every
> detail at the moment), it seems it could make isearch.el even more
> difficult to maintain than it currently is.  For example, the two
> defmacros `with-...' are the sort that force somebody debugging (or
> trying to read a patch) to look somewhere else to find out what they
> mean.  This is irksome and tedious.  There are already macros like this
> in isearch.el, and it would seem wise to minimise any further
> proliferation.  Each of these macros expands the ,@body twice for every
> invocation.  Would it not be possible to rearrange things, just to
> expand them once?

My difficulty in understanding Isearch is the very complicated internal
state.  This patch is just a UI thing and doesn't interact at lot with
the search state.

As to the duplication of ,@body, sure, once can always use a lambda to
avoid this.  However, I would rather break up a very long function that
requires being wrap in these macros (they will be very few, anyway) into
shorter and simpler pieces.

>
> In one hunk in the patch, I see a condition-case containing a funcall,
> containing a catch, containing a minibuffer-with-setup-hook, which in
> its turn contains a lambda function and an unwind-protect.  The lambda
> function sets functions onto both after-change-functions and
> post-command-hook.  Whew!  Each one of these constructs on its own
> causes difficulty in debugging, but they have their justification.  Must
> they really all appear together in this fashion?

The condition-case complicated, really.  The tag in question is thrown
at just one place (`with-isearch-window-quitting-edit') and caught at
just one place (`isearch-edit-string').  No spaghetti logic here at all.
It just passes a continuation, hence the funcall.

I admit the after-change function and post-command hook are hairy,
because they interact with the Isearch state.

>
> The biggest question, which may have been answered somewhere in the
> thread already, is why?  What is wrong with isearch at the moment that
> the patch will fix, or enable to be fixed?  The emacs-devel post you
> cite above (from 2020) doesn't seem to motivate this change.  Could you
> possibly answer this question, or cite a post which answers it, please?
>

I used Isearch for many years and never really understood/cared to
figure out why sometimes trying to delete a character jumped around the
buffer.  Or in which circumstances I had to press C-g once or twice to
quit (I still don't, but in the meanwhile I found `isearch-cancel').  I
would also inadvertently quit the search all the time when trying to
edit the string.  This is too complicated to my taste.

Only after an interlude using Swiper I realized that Isearch isn't even
using the minibuffer, like any other command that reads a string does.

This patch is supposed to provide a simple, intuitive mode of operation
for Isearch.

>> Some further remarks:
>
>> - The minibuffer-controlled mode is supposed to depend on the proposed
>>   `isearch-buffer-local' feature.  This will make the hack used to
>>   deactivate the `overriding-terminal-local-map' unnecessary.
>
>> - It seems necessary to let-bind `inhibit-redisplay' to nil in
>>   `with-isearch-window' in order to avoid flicker in the cursor.  This
>>   seems related to the recent thread "Temporarily select-window, without
>>   updating mode-line face and cursor fill?" in this list.  Any better
>>   solutions?
>
>> - I don't like the `with-isearch-window-quitting-edit' macro, but I
>>   don't see a different way of achieving the necessary effect.
>
>> - I don't use/know of all Isearch features, so let me know if you spot
>>   some incompatibility.
>
>> What do you think?
>
> The patch is ~500 lines long.  This makes me worried.  I'm afraid I can
> only react negatively and defensively at the moment.
>

That's because I didn't reindent anything yet :-)

Now seriously, the bulk of the change is to rewrite
`isearch-edit-string'.  Apart from that, it (1) wraps a bunch of
commands in a macro that does nothing when `isearch-from-minibuffer' is
nil, and (2) adds a condition at the end of `isearch-mode' to possibly
call `isearch-edit-string'.  That's about it.  Maybe with this short
summary the patch becomes easier to digest.

> [ patch snipped ].

Re: [WIP PATCH] Controlling Isearch from the minibuffer

[Juri Linkov]

> - Does the approach taken here seem sufficiently robust?  Note in
>   particular the `with-isearch-window' macro, which is now needed around
>   several functions,

You can avoid the with-isearch-window macro by checking
in isearch-pre-command-hook if the current buffer is the minibuffer,
then switch to the original buffer, and let the isearch command to
do what it normally does, then switch back to the minibuffer
in isearch-post-command-hook.

>   as well as the somewhat hacky `run-with-idle-timer'
>   call inside the `isearch-mode' function.

You can avoid the timer hack by adding a guard to
isearch-post-command-hook: when at the end of the isearch command,
point is not in the minibuffer, activate the minibuffer
(assuming that isearch-from-minibuffer is t).

> - Are the slightly backwards incompatible keybinding changes in
>   `isearch-edit-string' acceptable?

Only when isearch-from-minibuffer is t.

> - I don't like the `with-isearch-window-quitting-edit' macro, but I
>   don't see a different way of achieving the necessary effect.

The with-isearch-window-quitting-edit macro can be avoided the same way
as the with-isearch-window macro.

Re: [WIP PATCH] Controlling Isearch from the minibuffer

[Juri Linkov]

>> What do you think?
>
> The patch is ~500 lines long.  This makes me worried.  I'm afraid I can
> only react negatively and defensively at the moment.

This is what worried me too.  But hopefully the patch could be
reduced to just ~10 lines when using the same approach how you
implemented isearch-allow-scroll when isearch-pre-command-hook
allows to scroll, then isearch-post-command-hook checks the
post-conditions.  So I recommended Augusto the same solution.

Re: [WIP PATCH] Controlling Isearch from the minibuffer

[Alan Mackenzie]

Hello, Augusto.

On Sun, May 09, 2021 at 19:58:19 +0200, Augusto Stoffel wrote:
> On Sun,  9 May 2021 at 13:36, Alan Mackenzie <acm@muc.de> wrote:

> Hi Alan,

> > On Sat, May 08, 2021 at 12:13:52 +0200, Augusto Stoffel wrote:
> >> I've attached a draft implementation of a minibuffer-controlled mode for
> >> Isearch, as described for instance in
> >> https://lists.gnu.org/archive/html/emacs-devel/2020-01/msg00447.html
> >> To enable the feature, set `isearch-from-minibuffer' to t.

> >> The basic trade-off is that this makes it easier to edit the search
> >> string, and harder to quit the search.

> > You mean, the patch makes incompatible changes to isearch, yes?

> I wouldn't dare to suggest turning this on by default :-).

> The *slight* incompatible changes are all in `isearch-edit-string'.
> This seems like a quite unpolished command anyway, but of course I can
> leave it untouched.  I suggest discussing the details in a future
> subthread.

> >> It also drops some of the eccentricities of regular Isearch.  For
> >> instance, DEL deletes and C-g quits.

> > What to you are eccentricities, are features to other people.  I'm very
> > satisfied with the current workings of C-g, and would protest vehemently
> > were they to be changed.  I'm not sure what you mean by "DEL deletes" -
> > what does it delete, and in which circumstances?

> I am sure most of the regulars in this thread share your view.
> Otherwise, something like this proposal would have surfaced much
> earlier.

I think the commands came together the way they did in early Emacs
history through people trying things out and discarding what didn't work
very well.  What we're left with is what worked/works well.

> But this doesn't mean this feature won't be appreciated by other
> people.

Neither does it mean they will be.

> In the minbuffer-controlled Isearch mode, DEL is simply
> `delete-backward-char', not some kind of undo.  It deletes in all
> circumstances.

So, you're taking away one of the core bindings of isearch.  What are you
going to put in its place?  The ease of pressing DEL to undo the last
subcommand is one of the delights of isearch, from my point of view.

[ .... ]

> > Skimming through your patch (it is too large for me to take in every
> > detail at the moment), it seems it could make isearch.el even more
> > difficult to maintain than it currently is.  For example, the two
> > defmacros `with-...' are the sort that force somebody debugging (or
> > trying to read a patch) to look somewhere else to find out what they
> > mean.  This is irksome and tedious.  There are already macros like
> > this in isearch.el, and it would seem wise to minimise any further
> > proliferation.  Each of these macros expands the ,@body twice for
> > every invocation.  Would it not be possible to rearrange things, just
> > to expand them once?

> My difficulty in understanding Isearch is the very complicated internal
> state.  This patch is just a UI thing and doesn't interact at lot with
> the search state.

There's nothing "just" about UI things.  ;-)

> As to the duplication of ,@body, sure, once can always use a lambda to
> avoid this.

That wasn't really what I meant.  I was thinking more on the lines of
perhaps assigning the results of the ,@body to a variable, and doing
different things with that variable.  Or something like that.

> However, I would rather break up a very long function that requires
> being wrap in these macros (they will be very few, anyway) into shorter
> and simpler pieces.

As would I, provided the pieces are coherent wholes, and not random
fragments.

> > In one hunk in the patch, I see a condition-case containing a
> > funcall, containing a catch, containing a minibuffer-with-setup-hook,
> > which in its turn contains a lambda function and an unwind-protect.
> > The lambda function sets functions onto both after-change-functions
> > and post-command-hook.  Whew!  Each one of these constructs on its
> > own causes difficulty in debugging, but they have their
> > justification.  Must they really all appear together in this fashion?

> The condition-case [isn't] complicated, really.  The tag in question is
> thrown at just one place (`with-isearch-window-quitting-edit') and
> caught at just one place (`isearch-edit-string').  No spaghetti logic
> here at all.  It just passes a continuation, hence the funcall.

That's complicated difficult to follow logic.  Could it not be done more
simply with functions calling functions in the standard fashion?

> I admit the after-change function and post-command hook are hairy,
> because they interact with the Isearch state.

That looks to me like a source of future bugs.

> > The biggest question, which may have been answered somewhere in the
> > thread already, is why?  What is wrong with isearch at the moment
> > that the patch will fix, or enable to be fixed?  The emacs-devel post
> > you cite above (from 2020) doesn't seem to motivate this change.
> > Could you possibly answer this question, or cite a post which answers
> > it, please?

> I used Isearch for many years and never really understood/cared to
> figure out why sometimes trying to delete a character jumped around the
> buffer.

Maybe because DEL does not delete a character, but undoes a subcommand.

> Or in which circumstances I had to press C-g once or twice to quit (I
> still don't, but in the meanwhile I found `isearch-cancel').

I think one C-g works if the current search string has been found,
otherwise you need two.

> I would also inadvertently quit the search all the time when trying to
> edit the string.  This is too complicated to my taste.

Personally, I don't see it that way at all.  I find isearch delightfully
simple and intuitive.  But I can accept you see it differently.

> Only after an interlude using Swiper I realized that Isearch isn't even
> using the minibuffer, like any other command that reads a string does.

I didn't realise this till very recently either.  But isearch isn't
reading a string - at least not primarily - it's searching through a
buffer.  And as Drew pointed out, that buffer might well be the
minibuffer itself.

> This patch is supposed to provide a simple, intuitive mode of operation
> for Isearch.

Thanks, but that doesn't really answer my question.  You're unhappy about
some of the keybindings in isearch, which is fair enough.  But how does
switching to using the minibuffer have anything to do with that?

My feeling is that if you were to make the (optional) keybinding changes
whilst using the current echo-area manipulation, the patch would be
_much_ less that 500 lines long.  Why use the minibuffer at all?

[ .... ]

> >> What do you think?

> > The patch is ~500 lines long.  This makes me worried.  I'm afraid I
> > can only react negatively and defensively at the moment.

> That's because I didn't reindent anything yet :-)

;-)

> Now seriously, the bulk of the change is to rewrite
> `isearch-edit-string'.  Apart from that, it (1) wraps a bunch of
> commands in a macro that does nothing when `isearch-from-minibuffer' is
> nil, and (2) adds a condition at the end of `isearch-mode' to possibly
> call `isearch-edit-string'.  That's about it.  Maybe with this short
> summary the patch becomes easier to digest.

Probably.  But, again, what has using the minibuffer got to do with the
keybindings a user prefers?

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: [WIP PATCH] Controlling Isearch from the minibuffer

[Augusto Stoffel]

On Sun,  9 May 2021 at 22:05, Juri Linkov <juri@linkov.net> wrote:

>> - Does the approach taken here seem sufficiently robust?  Note in
>>   particular the `with-isearch-window' macro, which is now needed around
>>   several functions,
>
> You can avoid the with-isearch-window macro by checking
> in isearch-pre-command-hook if the current buffer is the minibuffer,
> then switch to the original buffer, and let the isearch command to
> do what it normally does, then switch back to the minibuffer
> in isearch-post-command-hook.

That's an interesting idea, thanks.  This indeed makes the changes more
localized, which seems desirable.  (On the other hand, it spreads the
logic currently contained in `with-isearch-window' across various
hooks.)

>
>>   as well as the somewhat hacky `run-with-idle-timer'
>>   call inside the `isearch-mode' function.
>
> You can avoid the timer hack by adding a guard to
> isearch-post-command-hook: when at the end of the isearch command,
> point is not in the minibuffer, activate the minibuffer
> (assuming that isearch-from-minibuffer is t).

That didn't work well, because when canceling a command called from the
post-command hook one gets an ugly error message.

>
>> - Are the slightly backwards incompatible keybinding changes in
>>   `isearch-edit-string' acceptable?
>
> Only when isearch-from-minibuffer is t.

I hope this is a guideline rather than an axiom, so let me describe what
the *slight* incompatible changes are:

1. The user is forced to see lazy highlight and lazy count while editing
   the search string via M-e, as long as these options are already
   enabled globally.
2. A M-s prefix is added to minibuffer-local-isearch-map, as well as a
   few extra commands (M->, M-<, etc.)
3. <right> and C-f are unbound.

>
>> - I don't like the `with-isearch-window-quitting-edit' macro, but I
>>   don't see a different way of achieving the necessary effect.
>
> The with-isearch-window-quitting-edit macro can be avoided the same way
> as the with-isearch-window macro.

I'm not sure this is possible.  Is there a way to get rid of the
minibuffer without returning control to the caller of
`read-from-minibuffer'?  I couldn't find a way to do this, hence the
throw/catch of a continuation function in the current version of the
patch.

Re: [WIP PATCH] Controlling Isearch from the minibuffer

[Juri Linkov]

>> You can avoid the timer hack by adding a guard to
>> isearch-post-command-hook: when at the end of the isearch command,
>> point is not in the minibuffer, activate the minibuffer
>> (assuming that isearch-from-minibuffer is t).
>
> That didn't work well, because when canceling a command called from the
> post-command hook one gets an ugly error message.

How do yo cancel such a command?

> I hope this is a guideline rather than an axiom, so let me describe what
> the *slight* incompatible changes are:
>
> 1. The user is forced to see lazy highlight and lazy count while editing
>    the search string via M-e, as long as these options are already
>    enabled globally.

Why not enable these incompatible changes only when isearch-from-minibuffer is t?

> 2. A M-s prefix is added to minibuffer-local-isearch-map, as well as a
>    few extra commands (M->, M-<, etc.)

The users might want to use M-< M-> to go to the beginning/end of the minibuffer.

> 3. <right> and C-f are unbound.

Removing <right> and C-f is a very good thing, indeed.

>> The with-isearch-window-quitting-edit macro can be avoided the same way
>> as the with-isearch-window macro.
>
> I'm not sure this is possible.  Is there a way to get rid of the
> minibuffer without returning control to the caller of
> `read-from-minibuffer'?  I couldn't find a way to do this, hence the
> throw/catch of a continuation function in the current version of the
> patch.

Why not return control to the caller with `exit-minibuffer'?
When still necessary, you could try `exit-recursive-edit'.

Re: [WIP PATCH] Controlling Isearch from the minibuffer

[Augusto Stoffel]

On Mon, 10 May 2021 at 19:51, Alan Mackenzie <acm@muc.de> wrote:

> Hello, Augusto.
>
> On Sun, May 09, 2021 at 19:58:19 +0200, Augusto Stoffel wrote:
>> On Sun,  9 May 2021 at 13:36, Alan Mackenzie <acm@muc.de> wrote:
>
>> Hi Alan,
>
>> > On Sat, May 08, 2021 at 12:13:52 +0200, Augusto Stoffel wrote:
>> >> I've attached a draft implementation of a minibuffer-controlled mode for
>> >> Isearch, as described for instance in
>> >> https://lists.gnu.org/archive/html/emacs-devel/2020-01/msg00447.html
>> >> To enable the feature, set `isearch-from-minibuffer' to t.
>
>> >> The basic trade-off is that this makes it easier to edit the search
>> >> string, and harder to quit the search.
>
>> > You mean, the patch makes incompatible changes to isearch, yes?
>
>> I wouldn't dare to suggest turning this on by default :-).
>
>> The *slight* incompatible changes are all in `isearch-edit-string'.
>> This seems like a quite unpolished command anyway, but of course I can
>> leave it untouched.  I suggest discussing the details in a future
>> subthread.
>
>> >> It also drops some of the eccentricities of regular Isearch.  For
>> >> instance, DEL deletes and C-g quits.
>
>> > What to you are eccentricities, are features to other people.  I'm very
>> > satisfied with the current workings of C-g, and would protest vehemently
>> > were they to be changed.  I'm not sure what you mean by "DEL deletes" -
>> > what does it delete, and in which circumstances?
>
>> I am sure most of the regulars in this thread share your view.
>> Otherwise, something like this proposal would have surfaced much
>> earlier.
>
> I think the commands came together the way they did in early Emacs
> history through people trying things out and discarding what didn't work
> very well.  What we're left with is what worked/works well.
>
>> But this doesn't mean this feature won't be appreciated by other
>> people.
>
> Neither does it mean they will be.

What is the evidence for that?  There are alternative search packages
out there which are quite popular, even though they lose several
interesting features from Isearch.

>
>> In the minbuffer-controlled Isearch mode, DEL is simply
>> `delete-backward-char', not some kind of undo.  It deletes in all
>> circumstances.
>
> So, you're taking away one of the core bindings of isearch.  What are you
> going to put in its place?  The ease of pressing DEL to undo the last
> subcommand is one of the delights of isearch, from my point of view.

I find it easier to press C-r to undo C-s, and vice versa.  But that's
completely irrelevant.  The question is whether it makes sense to
provide an alternative UI for Isearch where editing the search string is
not a special maneuver.  This includes DEL doing what it does everywhere
else, but is not limited to that.

>
> [ .... ]
>
>> > Skimming through your patch (it is too large for me to take in every
>> > detail at the moment), it seems it could make isearch.el even more
>> > difficult to maintain than it currently is.  For example, the two
>> > defmacros `with-...' are the sort that force somebody debugging (or
>> > trying to read a patch) to look somewhere else to find out what they
>> > mean.  This is irksome and tedious.  There are already macros like
>> > this in isearch.el, and it would seem wise to minimise any further
>> > proliferation.  Each of these macros expands the ,@body twice for
>> > every invocation.  Would it not be possible to rearrange things, just
>> > to expand them once?
>
>> My difficulty in understanding Isearch is the very complicated internal
>> state.  This patch is just a UI thing and doesn't interact at lot with
>> the search state.
>
> There's nothing "just" about UI things.  ;-)
>
>> As to the duplication of ,@body, sure, once can always use a lambda to
>> avoid this.
>
> That wasn't really what I meant.  I was thinking more on the lines of
> perhaps assigning the results of the ,@body to a variable, and doing
> different things with that variable.  Or something like that.
>
>> However, I would rather break up a very long function that requires
>> being wrap in these macros (they will be very few, anyway) into shorter
>> and simpler pieces.
>
> As would I, provided the pieces are coherent wholes, and not random
> fragments.
>
>> > In one hunk in the patch, I see a condition-case containing a
>> > funcall, containing a catch, containing a minibuffer-with-setup-hook,
>> > which in its turn contains a lambda function and an unwind-protect.
>> > The lambda function sets functions onto both after-change-functions
>> > and post-command-hook.  Whew!  Each one of these constructs on its
>> > own causes difficulty in debugging, but they have their
>> > justification.  Must they really all appear together in this fashion?
>
>> The condition-case [isn't] complicated, really.  The tag in question is
>> thrown at just one place (`with-isearch-window-quitting-edit') and
>> caught at just one place (`isearch-edit-string').  No spaghetti logic
>> here at all.  It just passes a continuation, hence the funcall.
>
> That's complicated difficult to follow logic.  Could it not be done more
> simply with functions calling functions in the standard fashion?

The minibuffer starts a recursive edit.  There may be ways to simplify
my patch, but functions which always return locally won't cut it.

>
>> I admit the after-change function and post-command hook are hairy,
>> because they interact with the Isearch state.
>
> That looks to me like a source of future bugs.

I think you should look at the code before claiming that.  The state
manipulation I'm adding is very mild compared to what's going on
elsewhere in isearch.el.

>
>> > The biggest question, which may have been answered somewhere in the
>> > thread already, is why?  What is wrong with isearch at the moment
>> > that the patch will fix, or enable to be fixed?  The emacs-devel post
>> > you cite above (from 2020) doesn't seem to motivate this change.
>> > Could you possibly answer this question, or cite a post which answers
>> > it, please?
>
>> I used Isearch for many years and never really understood/cared to
>> figure out why sometimes trying to delete a character jumped around the
>> buffer.
>
> Maybe because DEL does not delete a character, but undoes a subcommand.
>
>> Or in which circumstances I had to press C-g once or twice to quit (I
>> still don't, but in the meanwhile I found `isearch-cancel').
>
> I think one C-g works if the current search string has been found,
> otherwise you need two.
>
>> I would also inadvertently quit the search all the time when trying to
>> edit the string.  This is too complicated to my taste.
>
> Personally, I don't see it that way at all.  I find isearch delightfully
> simple and intuitive.  But I can accept you see it differently.
>
>> Only after an interlude using Swiper I realized that Isearch isn't even
>> using the minibuffer, like any other command that reads a string does.
>
> I didn't realise this till very recently either.  But isearch isn't
> reading a string - at least not primarily - it's searching through a
> buffer.  And as Drew pointed out, that buffer might well be the
> minibuffer itself.
>
>> This patch is supposed to provide a simple, intuitive mode of operation
>> for Isearch.
>
> Thanks, but that doesn't really answer my question.  You're unhappy about
> some of the keybindings in isearch, which is fair enough.  But how does
> switching to using the minibuffer have anything to do with that?
>
> My feeling is that if you were to make the (optional) keybinding
> changes

The patch is about making it easier to edit the search string.
Keybindings are easy to customize, and I'm not trying to share my config
here.

> whilst using the current echo-area manipulation, the patch would be
> _much_ less that 500 lines long.

The patch has 215 insertions, 57 deletions.  A good chunk of that is so
one gets lazy highlight and lazy count in the good old M-e.

>  Why use the minibuffer at all?

So that it's natural to edit the search string during a search.

>
> [ .... ]
>
>> >> What do you think?
>
>> > The patch is ~500 lines long.  This makes me worried.  I'm afraid I
>> > can only react negatively and defensively at the moment.
>
>> That's because I didn't reindent anything yet :-)
>
> ;-)
>
>> Now seriously, the bulk of the change is to rewrite
>> `isearch-edit-string'.  Apart from that, it (1) wraps a bunch of
>> commands in a macro that does nothing when `isearch-from-minibuffer' is
>> nil, and (2) adds a condition at the end of `isearch-mode' to possibly
>> call `isearch-edit-string'.  That's about it.  Maybe with this short
>> summary the patch becomes easier to digest.
>
> Probably.  But, again, what has using the minibuffer got to do with the
> keybindings a user prefers?

As stated in the original message, this feature is all about editing the
search string.  Having the same keybindings as everywhere is else might be
seen as a beneficial side effect by some people, but is incidental.

RE: [External] : Re: [WIP PATCH] Controlling Isearch from the minibuffer

[Drew Adams]

> The question is whether it makes sense to provide an
> alternative UI for Isearch where editing the search
> string is not a special maneuver.

Really?  Is that what this is all about?

Just what is the giant inconvenience you find with
Isearch's `M-e' for editing the search string?  Not
to mention that you can always edit it directly, by
deleting chars from the end.

But let's suppose you really feel that using `M-e'
edit the search pattern using the minibuffer is too
constraining/difficult/cumbersome (choose whatever
adjective you like).

Here's another alternative you might consider, which
doesn't throw out the adorable baby with the claimed
dirty bathwater.  It's offered by `isearch+.el' via
option `isearchp-initiate-edit-commands':

,----
| isearchp-initiate-edit-commands is a variable defined in `isearch+.el'.
| 
| Its value is 
| (backward-char left-char backward-sexp backward-word left-word)
| 
| Documentation:
| Commands whose key bindings initiate Isearch edit.
| When invoked by a key sequence, Isearch edits the search string,
| applying the command to it immediately.
| 
| Commands you might want to include here are typically commands that
| move point to the left, possibly deleting text along the way.
| 
| Set this to `nil' if you always want all such commands to exit Isearch
| and act on the buffer text.
| 
| You can customize this variable.
`----

By default, for example, `backward-sexp' (e.g. `C-M-b')
initiates editing the search pattern (yes, using the
minibuffer), moving point backward a sexp from the end.

That doesn't throw out the baby.  It's on-demand only.
It's like `M-e', but it also positions point within
the search pattern to be edited.

Remove all commands from the option list and you get
vanilla Emacs Isearch behavior: only `M-e' initiates
using the minibuffer to edit the search pattern.

And we already have `M-e' automatically putting point
at the position in the search pattern where matching
fails.  (Also something from Isearch+, BTW.)

> This includes DEL doing what it does everywhere else, 

"Everywhere else" means what?  DEL in Emacs has no
"everywhere else".  It does one thing in Info, another
in an editing mode.

Just as important: Why is it important for DEL to do
"what it does everywhere" - or ANYwhere - else?

> The minibuffer starts a recursive edit.  There may be
> ways to simplify my patch, but functions which always
> return locally won't cut it.

Another feature of Isearch+:

 `C-x o' (`isearchp-open-recursive-edit') opens a recursive
 editing session, where you can do anything you like (including
 search for something different).  Using `C-M-c' closes the
 recursive editing session and resumes the search (from the
 current position where you hit `C-M-c').

Doc string:

 Invoke the editor command loop recursively, during Isearch.
 Use `C-M-c' to end the recursive edit and resume searching from there.
 Or use `C-]' to exit the recursive edit and cancel the  previous search.

That one was suggested by Juri, BTW, who said,
"Executing any code in the macro's body will also
allow doing more amazing things like temporarily
going into a recursive edit."

https://lists.gnu.org/archive/html/emacs-devel/2012-12/msg00281.html

And `C-y C-M-c' adds text to the search pattern, from
point to wherever you go to in a recursive edit.
(Command `isearchp-yank-through-rec-edit-move'.)

`C-y C-M-k' acts similarly, but adds the text from point
to wherever a key moves.  (`isearchp-yank-through-key-move'.)

`C-y C-M-m' adds text from point through a match for
another search pattern, for which you're prompted.

`C-y C-M-z' adds text from point to the next
occurrence of a character, for which you're prompted.

Those are all ways to "edit" the search pattern, and
two of them make use of recursive editing.  Still,
the baby wasn't tossed - no need to.

> The patch is about making it easier to edit the search string.

Just what's the difficulty?  That info seems to be
missing.  We've seen a solution proposed, but what
problem does it want to solve?  (Please don't answer
by just saying that the search pattern is too hard
to edit.)

> >  Why use the minibuffer at all?
> 
> So that it's natural to edit the search string during a search.

What's unnatural about using `M-e' to use the
minibuffer to edit the search pattern during a
search?

Saying something is "natural" or more so, and that
it makes it easier to do something else, is vague.

What's the actual difficulty that you think making
Isearch always use the minibuffer eases?

If the problem is editing the search pattern, and you
want to be able to use the minibuffer for that, then
we already have/do that.  Why make Isearch use the
minibuffer all the time, for everything, if the goal
is to just to let it use the minibuffer to edit the
search pattern?

> > Probably.  But, again, what has using the minibuffer
> > got to do with the keybindings a user prefers?
> 
> As stated in the original message, this feature
> is all about editing the search string.

We already use the minibuffer to do that.

> Having the same keybindings as everywhere is else

See above, about DEL - define "everywhere else".
I'm guessing you mean outside Emacs, perhaps?  In
Emacs there is no such "everywhere".  Keys can do
different things in different contexts.

> might be seen as a beneficial side effect by some
> people, but is incidental.

It's hard to tell what you think is incidental and
what you think is essential to change.  But it seems
like the actual changes you propose are far-reaching.

For 40 years Isearch has won the world by NOT using
the minibuffer (except for one-off, on-demand pattern
editing).  Zillions of search UIs have adopted the
incremental searching that Isearch pioneered.  (The
"i" in "isearch" is for "incremental".)  Why, in 2021,
would we suddenly want cram all of Isearch into using
the minibuffer?

What's the real problem you're trying to solve?  Is
it really to be able to have keys like DEL do what
you're used to them doing outside Emacs?

To be clear, I'm generally in _favor_ of providing
"alternatives" for Emacs users.

But though this was pitched as just adding an
"alternative" behavior, the code changes seem deep
and wide-ranging, and IIUC, there was even talk of
the "alternative" arrangement being just temporary,
i.e., that Isearch would eventually always follow
your "alternative" implementation and behavior.

Why so?

Re: [External] : Re: [WIP PATCH] Controlling Isearch from the minibuffer

[Juri Linkov]

> Here's another alternative you might consider, which
> doesn't throw out the adorable baby with the claimed
> dirty bathwater.  It's offered by `isearch+.el' via
> option `isearchp-initiate-edit-commands':

This looks like the option 'edit' of 'search-exit-option'.

RE: [External] : Re: [WIP PATCH] Controlling Isearch from the minibuffer

[Drew Adams]

> > Here's another alternative you might consider, which
> > doesn't throw out the adorable baby with the claimed
> > dirty bathwater.  It's offered by `isearch+.el' via
> > option `isearchp-initiate-edit-commands':
> 
> This looks like the option 'edit' of 'search-exit-option'.

I see that you've changed that in Emacs 27, from its
former simple choice of ON/OFF.

But no, it's apparently not the same thing.  Your `edit'
value applies to all "Other control and meta characters",
though the doc string of that option isn't clear about
this, and that description is from the doc string of
`isearch-forward'.

A cursory look at 27.2 `isearch-pre-command-hook' code
indicates that the `edit' value of that option only
does what `M-e' does: it sets `this-command' to
`isearch-edit-string'.  IOW, it sounds like just a way
to do `M-e' by hitting "Other control and meta" keys.

The feature I described is very different.  It lets you
specify the behavior of _specific commands_, having
them (1) invoke editing the search pattern and (2)
perform their normal actions on the search pattern.
(A normal action being typically to move backward,
possibly deleting some text.)

So really, there's nothing in common with what an
`edit' value for `search-exit-option' does.

Re: [WIP PATCH] Controlling Isearch from the minibuffer

[Augusto Stoffel]

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

>>> You can avoid the timer hack by adding a guard to
>>> isearch-post-command-hook: when at the end of the isearch command,
>>> point is not in the minibuffer, activate the minibuffer
>>> (assuming that isearch-from-minibuffer is t).
>>
>> That didn't work well, because when canceling a command called from the
>> post-command hook one gets an ugly error message.
>
> How do yo cancel such a command?

C-g

>
>> I hope this is a guideline rather than an axiom, so let me describe what
>> the *slight* incompatible changes are:
>>
>> 1. The user is forced to see lazy highlight and lazy count while editing
>>    the search string via M-e, as long as these options are already
>>    enabled globally.
>
> Why not enable these incompatible changes only when
> isearch-from-minibuffer is t?

Without the improvements to the good old M-e, my patch would be totally
independent of the rest of isearch.el, so it could just as well be an
external package.  That's OK too, but why would anyone keep lazy
highlight/count on for normal Isearch, but wish to disable it after
pressing M-e?

To put it from another perspective: you said earlier that my patch could
be boiled down to 10 lines.  Well, adding lazy highlight/count to
`isearch-edit-string' is certainly more work than that.  But once this
is in place, then yes, the minibuffer-controlled mode is a small
addition.

>
>> 2. A M-s prefix is added to minibuffer-local-isearch-map, as well as a
>>    few extra commands (M->, M-<, etc.)
>
> The users might want to use M-< M-> to go to the beginning/end of the
> minibuffer.

This seems way less useful than going to the first/last match in the
search buffer, since in the minibuffer C-a and C-e are usually
sufficient.

By the way, what's the idea behind `minibuffer-beginning-of-buffer'?  It
moves past the prompt, which is a useless point to go.

>
>> 3. <right> and C-f are unbound.
>
> Removing <right> and C-f is a very good thing, indeed.
>
>>> The with-isearch-window-quitting-edit macro can be avoided the same way
>>> as the with-isearch-window macro.
>>
>> I'm not sure this is possible.  Is there a way to get rid of the
>> minibuffer without returning control to the caller of
>> `read-from-minibuffer'?  I couldn't find a way to do this, hence the
>> throw/catch of a continuation function in the current version of the
>> patch.
>
> Why not return control to the caller with `exit-minibuffer'?
> When still necessary, you could try `exit-recursive-edit'.

First of all, let me say that you suggestion to get rid of the
`with-isearch-window' macro works fine.  The remaining problem is with
commands that create a minibuffer, and therefore require that we quit
the `isearch-edit-string' minibuffer first.  One example would be
`isearch-query-replace'.

So here's the the situation in more detail:

- You are in an `isearch-edit-string' session
- Then you press M-%
- Now we are in the pre-command-hook. We check `this-command` and
  see that it will need the minibuffer.

From there, how can we get rid of the minibuffer and continue running
this-command?  Calling `exit-minibuffer' now would return control to
whoever called `isearch-edit-string', so `this-command' would never run.

Re: [External] : Re: [WIP PATCH] Controlling Isearch from the minibuffer

[Augusto Stoffel]

On Tue, 11 May 2021 at 15:34, Drew Adams <drew.adams@oracle.com> wrote:

>> The question is whether it makes sense to provide an
>> alternative UI for Isearch where editing the search
>> string is not a special maneuver.
>
> Really?  Is that what this is all about?

Yes.

>
> Just what is the giant inconvenience you find with
> Isearch's `M-e' for editing the search string?

To give one reason, M-e currently has no lazy highlight/count, so it's
not of much help to write a mildly complex regexp.

> [...]

> For 40 years Isearch has won the world by NOT using
> the minibuffer (except for one-off, on-demand pattern
> editing).  Zillions of search UIs have adopted the
> incremental searching that Isearch pioneered.  (The
> "i" in "isearch" is for "incremental".)

If it were called OK-search, where OK stands for overriding keymap, then
I would understand the talk of throwing out babies.

However, my proposal in no way diminishes the incremental aspect of
I-search.

> [...]
>
> But though this was pitched as just adding an
> "alternative" behavior, the code changes seem deep
> and wide-ranging,

You can think of my patch as (1) adding lazy highlight/count and an M-s
prefix to the good old M-e, which is indeed a bit of work, and (2)
adding the entirely optional minibuffer-controlled UI which only takes a
dozen of extra lines to implement.

If you have a technical argument as to why there shouldn't be lazy
highlight/count in the good old M-e, it won't be hard to persuade me.

> and IIUC, there was even talk of
> the "alternative" arrangement being just temporary,
> i.e., that Isearch would eventually always follow
> your "alternative" implementation and behavior.

Where?

>
> Why so?

Re: [External] : Re: [WIP PATCH] Controlling Isearch from the minibuffer

[Stefan Monnier]

> You can think of my patch as (1) adding lazy highlight/count and an M-s
> prefix to the good old M-e, which is indeed a bit of work,

[ Sorry I haven't followed this thread very closely, so just a comment
  from the sidelines: ]

I don't know what this `M-s` thingy refers to, but I for one would very
much welcome lazy highlighting during the `M-e` minibuffer editing.

> and (2) adding the entirely optional minibuffer-controlled UI which
> only takes a dozen of extra lines to implement.

I don't know what impacts it might have on the UI, but I've often wished
(from an implementation point of view) that Isearch used an actual plain
old minibuffer rather than mimicking one (which doesn't necessary mean
it'd be a good idea either, just that it would have some benefits and
that the downsides were not as immediately apparent ;-).


        Stefan

RE: [External] : Re: [WIP PATCH] Controlling Isearch from the minibuffer

[Drew Adams]

> You can think of my patch as (1) adding lazy highlight/count and an M-s
> prefix to the good old M-e, which is indeed a bit of work, and (2)
> adding the entirely optional minibuffer-controlled UI which only takes
> a dozen of extra lines to implement.
> 
> If you have a technical argument as to why there shouldn't be lazy
> highlight/count in the good old M-e, it won't be hard to persuade me.

I haven't said anything about #1, and I doubt that I'd
be concerned about it one way or another, unless the
changes made for it affect other things negatively.

My voice here is about #2, the "minibuffer-controlled UI".
A priori (and it's a big a priori), I'm opposed to having
Isearch use the minibuffer instead of its longstanding
implementation.

You will note the Subject line of this thread.  It seems
to speak only to #2 - controlling Isearch from the
minibuffer.

> > and IIUC, there was even talk of
> > the "alternative" arrangement being just temporary,
> > i.e., that Isearch would eventually always follow
> > your "alternative" implementation and behavior.
> 
> Where?

Dunno.  There may have been another thread that also
talked about your #2.  Or I may have (mistakenly?)
read between the lines.

Even if there will never be any proposal/attempt to
make Isearch use the minibuffer, I have the question
of why.

Why do we need/want an "alternative" implementation
of Isearch?  Different implementations will lead to
introduction of different features or different levels
of support for existing features.

What's the giant need for a minibuffer-based Isearch?

RE: [External] : Re: [WIP PATCH] Controlling Isearch from the minibuffer

[Drew Adams]

> I don't know what impacts it might have on the UI, but I've often
> wished (from an implementation point of view) that Isearch used
> an actual plain old minibuffer rather than mimicking one

Why?

[FWIW, I wouldn't say that Isearch mimics a minibuffer.
 But it's the case that given the place where you type
 search-pattern input Emacs users can mistake that for
 using a minibuffer.

 It's because Isearch exploits the echo-area, which users
 often mistake for the minibuffer anyway. even without,
 in effect, using it in an interaction that separately
 reads input.]

And what about from a user point of view?  What advantages?
You say you don't know what impacts it might have.  Fair
enough, I guess.

> (which doesn't necessary mean it'd be a good idea either,
> just that it would have some benefits

What benefits?

> and that the downsides were not as immediately apparent ;-).

What downsides, for implementation (e.g. compatibility
with 3rd-party code) - and for users?

The obvious way some complete rewrite/redesign should
come about, if at all, is for someone to write a separate
package that implements it independently: e.g. Mysearch,
not Isearch.  Plenty of people with try that, pound on it,
expound on its benefits and drawbacks, improve it, or
whatever.  It someone wants an "alternative" like that,
implement it separately as a real alternative, and see how
it goes.

As opposed to messing with the Isearch implementation,
which is not at all minibuffer-based.

Re: [WIP PATCH] Controlling Isearch from the minibuffer

[Juri Linkov]

>>>> You can avoid the timer hack by adding a guard to
>>>> isearch-post-command-hook: when at the end of the isearch command,
>>>> point is not in the minibuffer, activate the minibuffer
>>>> (assuming that isearch-from-minibuffer is t).
>>>
>>> That didn't work well, because when canceling a command called from the
>>> post-command hook one gets an ugly error message.
>>
>> How do yo cancel such a command?
>
> C-g

Do you mean the global C-g bound to keyboard-quit,
isearch-mode's C-g bound to isearch-abort, or
minibuffer's C-g bound to minibuffer-keyboard-quit?
And what was the error message?

> To put it from another perspective: you said earlier that my patch could
> be boiled down to 10 lines.  Well, adding lazy highlight/count to
> `isearch-edit-string' is certainly more work than that.  But once this
> is in place, then yes, the minibuffer-controlled mode is a small
> addition.

Adding lazy highlight/count should not be more work.  It's simple to do
with a few lines by setting minibuffer-local isearch-message-function,
the same way like minibuffer-history-isearch-setup sets it to
minibuffer-history-isearch-message.

>>> 2. A M-s prefix is added to minibuffer-local-isearch-map, as well as a
>>>    few extra commands (M->, M-<, etc.)
>>
>> The users might want to use M-< M-> to go to the beginning/end of the
>> minibuffer.
>
> This seems way less useful than going to the first/last match in the
> search buffer, since in the minibuffer C-a and C-e are usually
> sufficient.
>
> By the way, what's the idea behind `minibuffer-beginning-of-buffer'?  It
> moves past the prompt, which is a useless point to go.

It's useful when minibuffer-beginning-of-buffer-movement is customized
to t.  I don't know why currently its default is nil.

> First of all, let me say that you suggestion to get rid of the
> `with-isearch-window' macro works fine.  The remaining problem is with
> commands that create a minibuffer, and therefore require that we quit
> the `isearch-edit-string' minibuffer first.  One example would be
> `isearch-query-replace'.
>
> So here's the the situation in more detail:
>
> - You are in an `isearch-edit-string' session
> - Then you press M-%
> - Now we are in the pre-command-hook. We check `this-command` and
>   see that it will need the minibuffer.
>
> From there, how can we get rid of the minibuffer and continue running
> this-command?  Calling `exit-minibuffer' now would return control to
> whoever called `isearch-edit-string', so `this-command' would never run.

This is an interesting problem.  Would it be possible after calling
exit-minibuffer to allow the caller of isearch-edit-string (mosy likely
the caller should be isearch-mode when isearch-from-minibuffer is t)
to call the right function after exiting from the minibuffer,
e.g. when this-command-keys was M-% then call isearch-query-replace
after isearch-edit-string at the end of isearch-mode.

Re: [WIP PATCH] Controlling Isearch from the minibuffer

[Augusto Stoffel]

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

On Wed, 12 May 2021 at 20:13, Juri Linkov <juri@linkov.net> wrote:

Hi Juri,

>>>>> You can avoid the timer hack by adding a guard to
>>>>> isearch-post-command-hook: when at the end of the isearch command,
>>>>> point is not in the minibuffer, activate the minibuffer
>>>>> (assuming that isearch-from-minibuffer is t).
>>>>
>>>> That didn't work well, because when canceling a command called from the
>>>> post-command hook one gets an ugly error message.
>>>
>>> How do yo cancel such a command?
>>
>> C-g
>
> Do you mean the global C-g bound to keyboard-quit,
> isearch-mode's C-g bound to isearch-abort, or
> minibuffer's C-g bound to minibuffer-keyboard-quit?
> And what was the error message?

I mean keyboard quit.  You can see the error message by evaluating this
and then pressing C-g:

    (add-hook 'post-command-hook
              (lambda () (read-from-minibuffer "test")) t t)

Using a timer with zero delay instead works better.

>
>> To put it from another perspective: you said earlier that my patch could
>> be boiled down to 10 lines.  Well, adding lazy highlight/count to
>> `isearch-edit-string' is certainly more work than that.  But once this
>> is in place, then yes, the minibuffer-controlled mode is a small
>> addition.
>
> Adding lazy highlight/count should not be more work.  It's simple to do
> with a few lines by setting minibuffer-local isearch-message-function,
> the same way like minibuffer-history-isearch-setup sets it to
> minibuffer-history-isearch-message.
>

Setting `isearch-message-function' is of no help, because there are some
tests for `(null isearch-message-function)' as well as some explicit
calls to `(isearch-message)' in isearch.el.  As far as I can see, there
is no alternative to modifying the function `isearch-message' itself.

>>>> 2. A M-s prefix is added to minibuffer-local-isearch-map, as well as a
>>>>    few extra commands (M->, M-<, etc.)
>>>
>>> The users might want to use M-< M-> to go to the beginning/end of the
>>> minibuffer.
>>
>> This seems way less useful than going to the first/last match in the
>> search buffer, since in the minibuffer C-a and C-e are usually
>> sufficient.
>>
>> By the way, what's the idea behind `minibuffer-beginning-of-buffer'?  It
>> moves past the prompt, which is a useless point to go.
>
> It's useful when minibuffer-beginning-of-buffer-movement is customized
> to t.  I don't know why currently its default is nil.

I see.

>
>> First of all, let me say that you suggestion to get rid of the
>> `with-isearch-window' macro works fine.  The remaining problem is with
>> commands that create a minibuffer, and therefore require that we quit
>> the `isearch-edit-string' minibuffer first.  One example would be
>> `isearch-query-replace'.
>>
>> So here's the the situation in more detail:
>>
>> - You are in an `isearch-edit-string' session
>> - Then you press M-%
>> - Now we are in the pre-command-hook. We check `this-command` and
>>   see that it will need the minibuffer.
>>
>> From there, how can we get rid of the minibuffer and continue running
>> this-command?  Calling `exit-minibuffer' now would return control to
>> whoever called `isearch-edit-string', so `this-command' would never run.
>
> This is an interesting problem.  Would it be possible after calling
> exit-minibuffer to allow the caller of isearch-edit-string (mosy likely
> the caller should be isearch-mode when isearch-from-minibuffer is t)
> to call the right function after exiting from the minibuffer,
> e.g. when this-command-keys was M-% then call isearch-query-replace
> after isearch-edit-string at the end of isearch-mode.

I've attached a new patch (again, in rough draft form) where the
`with-isearch-window' and `with-isearch-window-quitting-edit' macros
from my previous patch are replaced by some pre/post command hook logic,
as you suggested.

In particular, the case of commands that end the search and create a
minibuffer is handled by this part of the minibuffer pre-command hook:

    (when (member this-command isearch-edit--quitting-commands)
      (run-with-timer 0 nil 'command-execute this-command)
      (exit-minibuffer))

While this version of the patch doesn't require wrapping several
existing commands in a new macro, I find it much more convoluted.
Moreover, one still has to manually keep a list of commands that need to
switch to the search buffer (cf. `isearch-edit--buffer-commands') and
commands that end the search (cf. `isearch-edit--quitting-commands'); I
see no way around that.  Therefore, I see no advantage here over the
proposed `with-isearch-window' macro.

I admit that the `with-isearch-window-quitting-edit' macro of my old
patch seems pretty ad-hoc.  However, it could be replaced by a
`with-isearch-done' macro which is of more general nature.  If you
search isearch.el for calls to `isearch-done', you'll see that some are
of the form

     (let (;; Set `isearch-recursive-edit' to nil to prevent calling
           ;; `exit-recursive-edit' in `isearch-done' that terminates
           ;; the execution of this command when it is non-nil.
           ;; We call `exit-recursive-edit' explicitly at the end below.
           (isearch-recursive-edit nil))
       (isearch-done nil t)

while a few others seem to just don't take into account that we may be
ending a recursive edit.  Third party packages, even the excellently
well written Consult, overlook this detail too:

https://github.com/minad/consult/blob/b873ceeefcb80ae0a00aa5e9ce7d70a71680aa4b/consult.el#L2161

So an `with-isearch-done' macro (which ends isearch, executes the body,
then possibly ends a recursive edit, and at the same time takes care to
unwind the minibuffer if we happen to be in `isearch-edit-string') would
seem a reasonable addition to isearch.el.

Without such a macro available, I think I would rather rely on advices
to implement the minibuffer-based Isearch UI (which, I gather, would
force it to be an external package).  I'm not quite sure yet.

Sorry for the long message :-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Control-Isearch-from-minibuffer-draft.patch --]
[-- Type: text/x-patch, Size: 14332 bytes --]

From 9de47983663640695f9ee6d9a018f515ec2da40f Mon Sep 17 00:00:00 2001
From: Augusto Stoffel <arstoffel@gmail.com>
Date: Sat, 8 May 2021 10:24:20 +0200
Subject: [PATCH] Control Isearch from minibuffer (draft)

---
 lisp/isearch.el | 247 +++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 202 insertions(+), 45 deletions(-)

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 9f3cfd70fb..b8228087ec 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -821,15 +821,30 @@ isearch-tool-bar-map
             :image '(isearch-tool-bar-image "left-arrow")))
     map))
 
+;; WIP for debugging
+(makunbound 'minibuffer-local-isearch-map)
+
 (defvar minibuffer-local-isearch-map
   (let ((map (make-sparse-keymap)))
     (set-keymap-parent map minibuffer-local-map)
-    (define-key map "\r"    'exit-minibuffer)
+    (define-key map "\C-j" 'newline)
     (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)
+    (define-key map "\C-s" 'isearch-repeat-forward)
+    (define-key map "\C-r" 'isearch-repeat-backward)
+    (define-key map "\M-%" 'isearch-query-replace)
+    (define-key map [?\C-\M-%] 'isearch-query-replace-regexp)
+    (define-key map "\M-<" 'isearch-beginning-of-buffer)
+    (define-key map "\M->" 'isearch-end-of-buffer)
+    (define-key map "\M-s'" 'isearch-toggle-char-fold)
+    (define-key map "\M-s " 'isearch-toggle-lax-whitespace)
+    (define-key map "\M-s_" 'isearch-toggle-symbol)
+    (define-key map "\M-sc" 'isearch-toggle-case-fold)
+    (define-key map "\M-shr" 'isearch-highlight-regexp)
+    (define-key map "\M-shl" 'isearch-highlight-lines-matching-regexp)
+    (define-key map "\M-si" 'isearch-toggle-invisible)
+    (define-key map "\M-so" 'isearch-occur)
+    (define-key map "\M-sr" 'isearch-toggle-regexp)
+    (define-key map "\M-sw" 'isearch-toggle-word)
     map)
   "Keymap for editing Isearch strings in the minibuffer.")
 
@@ -1518,6 +1533,21 @@ isearch-update-from-string-properties
     (setq isearch-regexp-function
 	  (get-text-property 0 'isearch-regexp-function string))))
 
+(defun isearch-set-string (string &optional properties)
+  "Set the current search string.
+
+Return STRING.  If PROPERTIES is non-nil, also update the search
+mode from the text properties of STRING."
+  (when properties (isearch-update-from-string-properties string))
+  (when isearch-edit--minibuffer
+    (with-current-buffer isearch-edit--minibuffer
+      (let ((inhibit-modification-hooks t))
+        (delete-minibuffer-contents)
+        (insert string))
+      (end-of-buffer)))
+  (setq isearch-message (mapconcat 'isearch-text-char-description string "")
+        isearch-string string))
+
 \f
 ;; The search status structure and stack.
 
@@ -1781,43 +1811,153 @@ with-isearch-suspended
      (isearch-abort)  ;; outside of let to restore outside global values
      )))
 
+;;; Edit search string in the minibuffer
+
+;; WIP delete this?
 (defvar minibuffer-history-symbol) ;; from external package gmhist.el
 
-(defun isearch-edit-string ()
+(defcustom isearch-from-minibuffer nil
+  "If non-nil, control Isearch from the minibuffer."
+  :type 'boolean)
+
+(defvar isearch-edit--quitting-commands
+  '(isearch-query-replace
+    isearch-query-replace-regexp
+    isearch-highlight-regexp
+    isearch-highlight-lines-matching-regexp))
+
+(defvar isearch-edit--buffer-commands
+  '(isearch-beginning-of-buffer
+    isearch-end-of-buffer
+    isearch-occur
+    isearch-repeat-backward
+    isearch-repeat-forward
+    isearch-toggle-case-fold
+    isearch-toggle-char-fold
+    isearch-toggle-invisible
+    isearch-toggle-lax-whitespace
+    isearch-toggle-regexp
+    isearch-toggle-symbol
+    isearch-toggle-word
+    isearch-query-replace
+    isearch-query-replace-regexp
+    isearch-highlight-regexp
+    isearch-highlight-lines-matching-regexp))
+
+(defvar isearch-edit--no-search-commands
+  '(next-history-element
+    previous-history-element))
+
+(defun isearch-edit--pre-command-hook ()
+  (when (member this-command isearch-edit--quitting-commands)
+    (run-with-timer 0 nil 'command-execute this-command)
+    (exit-minibuffer))
+  (when (member this-command isearch-edit--buffer-commands)
+    (setq inhibit-redisplay t)
+    (select-window (minibuffer-selected-window))
+    (when (and (local-variable-p 'pre-command-hook)
+               (member 'isearch-pre-command-hook pre-command-hook))
+      (isearch-pre-command-hook))))
+
+(defun isearch-edit--post-command-hook ()
+  "Hook to run from the minibuffer to update the Isearch state."
+  (setq inhibit-redisplay nil)
+  (set-text-properties (minibuffer-prompt-end) (point-max) nil)
+  (when-let ((fail-pos (isearch-fail-pos)))
+    (add-text-properties (+ (minibuffer-prompt-end) fail-pos)
+                         (point-max)
+                         '(face isearch-fail)))
+  (when isearch-error
+    (isearch--momentary-message isearch-error)))
+
+(defun isearch-edit--after-change (_ _ _)
+  "Hook to run from the minibuffer to update the Isearch state."
+  (let ((string (minibuffer-contents))
+        (inhibit-redisplay t))
+    (with-minibuffer-selected-window
+     (setq isearch-string (substring-no-properties string))
+     (isearch-update-from-string-properties string)
+     ;; Backtrack to barrier and search, unless the `this-command'
+     ;; is special or the search regexp is invalid.
+     (if (or (member this-command isearch-edit--no-search-commands)
+             (and isearch-regexp
+                  (condition-case err
+                      (prog1 nil (string-match-p isearch-string ""))
+                    (invalid-regexp
+                     (prog1 t (isearch--momentary-message (cadr err)))))))
+         (isearch-update)
+       (goto-char isearch-barrier)
+       (setq isearch-adjusted t isearch-success t)
+       (isearch-search-and-update)))))
+
+(defvar-local isearch-edit--prompt-overlay nil
+  "Overlay to display the Isearch status in `isearch-edit-string'.")
+
+(defvar-local isearch-edit--minibuffer nil
+  "Minibuffer currently editing the search string.
+Local to the search buffer, and non-nil only during an
+`isearch-edit-string' session.")
+
+(defun isearch-edit-string (&optional persist)
   "Edit the search string in the minibuffer.
+
+When PERSIST is nil, exiting the minibuffer or repeating the
+search resumes Isearch with the edited string.  When PERSIST is
+non-nil, exiting the minibuffer also ends the search.
+
 The following additional command keys are active while editing.
-\\<minibuffer-local-isearch-map>
-\\[exit-minibuffer] to resume incremental searching with the edited string.
-\\[isearch-forward-exit-minibuffer] to resume isearching forward.
-\\[isearch-reverse-exit-minibuffer] to resume isearching backward.
-\\[isearch-complete-edit] to complete the search string using the search ring."
+\\{minibuffer-local-isearch-map}"
   (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 "")))))
+  (condition-case nil
+      (minibuffer-with-setup-hook
+          (lambda ()
+            (setq-local tool-bar-map isearch-tool-bar-map)
+            (add-hook 'pre-command-hook 'isearch-edit--pre-command-hook nil t)
+            (add-hook 'after-change-functions 'isearch-edit--after-change nil t)
+            (add-hook 'post-command-hook 'isearch-edit--post-command-hook nil t)
+            (setq-local isearch-edit--prompt-overlay
+                        (make-overlay (point-min) (point-min) (current-buffer) t t))
+            (let ((inhibit-modification-hooks t)
+                  (mb (current-buffer))
+                  (buf (window-buffer (minibuffer-selected-window))))
+              (insert (buffer-local-value 'isearch-string buf))
+              (with-current-buffer buf
+                (setq-local isearch-edit--minibuffer mb)
+                (isearch-message)))
+            (when isearch-error (isearch--momentary-message isearch-error)))
+        (unwind-protect
+            (let (;; WIP: This is a hack that can be removed when isearch
+                  ;; local mode is available.
+                  (overriding-terminal-local-map nil)
+                  ;; We need to set `inhibit-redisplay' in `with-isearch-window' to
+                  ;; avoid flicker.  As a side effect, window-start/end in
+                  ;; `isearch-lazy-highlight-update' will have incorrect values,
+                  ;; so we need to lazy-highlight the whole buffer.
+                  (lazy-highlight-buffer (not (null isearch-lazy-highlight))))
+              (read-from-minibuffer
+               "I-search: "
+               nil
+               (if persist
+                   minibuffer-local-isearch-map
+                 (let ((map (make-sparse-keymap)))
+                   (set-keymap-parent map minibuffer-local-isearch-map)
+                   (define-key map
+                     [remap isearch-repeat-forward] 'isearch-forward-exit-minibuffer)
+                   (define-key map
+                     [remap isearch-repeat-backward] 'isearch-reverse-exit-minibuffer)
+                   map))
+               nil
+               (if isearch-regexp 'regexp-search-ring 'search-ring)
+               (thread-last isearch-forward-thing-at-point
+                 ;;  WIP: The above variable can be renamed
+                 (mapcar 'thing-at-point)
+                 (delq nil)
+                 (delete-dups)
+                 (mapcar (if isearch-regexp 'regexp-quote 'identity)))
+               t)
+              (setq-local isearch-edit--minibuffer nil)))
+        (when (and persist isearch-mode) (isearch-done)))
+    (quit (if (and persist isearch-mode) (isearch-cancel) (signal 'quit nil)))))
 
 (defun isearch-nonincremental-exit-minibuffer ()
   (interactive)
@@ -1879,13 +2019,8 @@ isearch-repeat
 	  ;; If search string is empty, use last one.
 	  (if (null (if isearch-regexp regexp-search-ring search-ring))
 	      (setq isearch-error "No previous search string")
-	    (setq isearch-string
-		  (car (if isearch-regexp regexp-search-ring search-ring))
-		  isearch-message
-		  (mapconcat 'isearch-text-char-description
-			     isearch-string "")
-		  isearch-case-fold-search isearch-last-case-fold-search)
-	    ;; After taking the last element, adjust ring to previous one.
+            (isearch-set-string (car (if isearch-regexp regexp-search-ring search-ring)) t)
+            ;; After taking the last element, adjust ring to previous one.
 	    (isearch-ring-adjust1 nil))
 	;; If already have what to search for, repeat it.
 	(unless (or isearch-success (null isearch-wrap-pause))
@@ -2075,6 +2210,9 @@ isearch-message-properties
 (defun isearch--momentary-message (string &optional seconds)
   "Print STRING at the end of the isearch prompt for 1 second.
 The optional argument SECONDS overrides the number of seconds."
+(if isearch-edit--minibuffer
+    (message (propertize (concat " [" string "]")
+                         'face 'minibuffer-prompt))
   (let ((message-log-max nil))
     (message "%s%s%s"
              (isearch-message-prefix nil isearch-nonincremental)
@@ -2082,6 +2220,7 @@ isearch--momentary-message
              (apply #'propertize (format " [%s]" string)
                     isearch-message-properties)))
   (sit-for (or seconds 1)))
+)
 
 (isearch-define-mode-toggle lax-whitespace " " nil
   "In ordinary search, toggles the value of the variable
@@ -3094,7 +3233,13 @@ isearch-post-command-hook
            (goto-char isearch-pre-move-point))
          (isearch-search-and-update)))
      (setq isearch-pre-move-point nil))
-  (force-mode-line-update))
+  (when (and isearch-from-minibuffer (not (minibufferp)))
+    (if isearch-edit--minibuffer
+        (progn
+          (select-window (minibuffer-window))
+          (isearch-edit--post-command-hook))
+      (run-with-idle-timer 0 nil 'isearch-edit-string t)))
+   (force-mode-line-update))
 
 (defun isearch-quote-char (&optional count)
   "Quote special characters for incremental search.
@@ -3267,6 +3412,17 @@ isearch-message
   ;; circumstances are when follow-mode is active, the search string
   ;; spans two (or several) windows, and the message about to be
   ;; displayed will cause the echo area to expand.
+  (if isearch-from-minibuffer
+      (when-let ((mb isearch-edit--minibuffer)
+                 (ov (buffer-local-value 'isearch-edit--prompt-overlay mb)))
+        (overlay-put ov
+                     'before-string
+                     (concat
+                      (when isearch-lazy-count
+                        (format "%-6s" (isearch-lazy-count-format)))
+                      (capitalize
+                       (isearch--describe-regexp-mode
+                        isearch-regexp-function)))))
   (let ((cursor-in-echo-area ellipsis)
 	(m isearch-message)
 	(fail-pos (isearch-fail-pos t)))
@@ -3283,6 +3439,7 @@ isearch-message
 	     m
 	     (isearch-message-suffix c-q-hack)))
     (if c-q-hack m (let ((message-log-max nil)) (message "%s" m)))))
+)
 
 (defun isearch--describe-regexp-mode (regexp-function &optional space-before)
   "Make a string for describing REGEXP-FUNCTION.
-- 
2.31.1


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


PS: What do you think of this idea: https://mail.gnu.org/archive/html/emacs-devel/2021-04/msg01359.html

Re: [External] : Re: [WIP PATCH] Controlling Isearch from the minibuffer

[Augusto Stoffel]

On Wed, 12 May 2021 at 08:44, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

>> You can think of my patch as (1) adding lazy highlight/count and an M-s
>> prefix to the good old M-e, which is indeed a bit of work,
>
> [ Sorry I haven't followed this thread very closely, so just a comment
>   from the sidelines: ]
>
> I don't know what this `M-s` thingy refers to, but I for one would very
> much welcome lazy highlighting during the `M-e` minibuffer editing.

The `M-s' thingy would be a prefix keymap including `isearch-occur'
`isearch-highlight-regexp' and what not.

>
>> and (2) adding the entirely optional minibuffer-controlled UI which
>> only takes a dozen of extra lines to implement.
>
> I don't know what impacts it might have on the UI, but I've often wished
> (from an implementation point of view) that Isearch used an actual plain
> old minibuffer rather than mimicking one (which doesn't necessary mean
> it'd be a good idea either, just that it would have some benefits and
> that the downsides were not as immediately apparent ;-).

So far, the biggest hurdle I can see concerns the handling of commands
that end the search and read from the minibuffer afterwards, such as
`isearch-query-replace'.  This is tricky because one has to somehow
unwind the `isearch-edit-string' minibuffer without relinquishing
control to its caller.

The first patch I sent in this thread used throw/catch of a continuation
function for this.  The second does the following in a pre-command hook,
which seems to work:

  (when (member this-command isearch-edit--quitting-commands)
    (run-with-timer 0 nil 'command-execute this-command)
    (exit-minibuffer))

I can't think of any simpler alternatives (and I still prefer the
throw/catch solution).

>
>
>         Stefan

Re: [External] : Re: [WIP PATCH] Controlling Isearch from the minibuffer

[Kévin Le Gouguec]

Drew Adams <drew.adams@oracle.com> writes:

>> I don't know what impacts it might have on the UI, but I've often
>> wished (from an implementation point of view) that Isearch used
>> an actual plain old minibuffer rather than mimicking one
>
> Why?

(Speaking for myself, with only a cursory reading of this thread)

While isearching, I regularly find myself reaching for regular
navigation/editing commands (C-a, C-b, M-b, M-DEL… even C-x o, on
occasion), and tripping out of isearch.  This thread made me discover
M-e, but I doubt I'll use it much, since it takes the "i" out of
"isearch".

From this user's candid[1] point of view, "controlling Isearch from the
minibuffer", as the subject says, sounds like an opportunity to have one
less ad-hoc UI.  Isearch already goes to some lengths to make it /look
like/ stuff is happening in a minibuffer[2], so I don't think it would
defy user expectations to actually use one.


[1] "Candid" as in, having no a priori opinion about the current,
    non-minibuffer implementation (aside from those minor grievances I
    just expressed); I can't tell what added value it brings, be it from
    a user's point of view or from a maintainer's.

[2] E.g. printing the user's keystrokes in the echo area where the
    minibuffer is by default, painting the prompt with
    isearch-message-properties ≡ minibuffer-prompt-properties…

RE: [External] : Re: [WIP PATCH] Controlling Isearch from the minibuffer

[Drew Adams]

> While isearching, I regularly find myself reaching for regular
> navigation/editing commands (C-a, C-b, M-b, M-DEL… even C-x o, on
> occasion), 

You "regularly" want to move within the search-pattern,
to do some editing of it besides at the end.  Is that it?

But in that case, what about `C-x o'?

> and tripping out of isearch.  

You don't want to end Isearch when you move to another
window?  Is it because you don't want to have to hit
`C-s' again, after `C-x o'?  Or do you regularly want
to both switch windows and edit the search string,
without hitting `C-s'?

> This thread made me discover M-e,

Really?  In that case, I suggest you first try using
that to do your editing, before asking that Isearch
be rewritten to be minibuffer-based.  It's really not
a big deal to use `M-e', IMO.

> but I doubt I'll use it much, since it takes the
> "i" out of "isearch".

I don't think so.  If you're using `M-b' and such to
get to the middle of the search pattern you're already
taking the "i" out of "isearch" to a degree.

`M-e M-b X C-s' is not a lot different from `M-b X',
and how often is this need "regularly" felt?

Anyway, I do sympathize with the need/desire to
sometimes want to directly go to the middle of the
pattern somewhere and insert or delete something
there.

Which is why I added an option to do that - but without
making Isearch depend on the minibuffer all the time.
Have Isearch use the minibuffer only for editing the
search pattern, as it does now, but obviate having to
hit `M-e' to do so, for certain movement etc. keys.

How would you feel about what I described as existing
in isearch+.el:

Customize `isearchp-initiate-edit-commands' to include
`beginning-of-line' (`C-a'), `backward-kill-word'
(`M-DEL'),...?

It already includes `C-b', `M-b', `C-M-b', `<left>',
C-<left>', and `M-<left>'.  Your wish is half granted,
even by default.

These are all prepared for possible inclusion, but are
commented out in the code - just to give folks an idea:

;; backward-delete-char                ; `DEL'
;; backward-delete-char-untabify       ; `DEL'
;; backward-kill-paragraph             ; `C-backspace'
;; backward-kill-sentence              ; `C-x DEL'
;; backward-kill-sexp                  ; `C-M-backspace'
;; backward-kill-word                  ; `M-DEL'
;; backward-list                       ; `C-M-p'
;; backward-page                       ; `C-x ['
;; backward-paragraph                  ; `C-up', `M-{'
;; backward-sentence                   ; `M-a'
;; backward-to-indentation             ; Not bound by default
;; backward-up-list                    ; `C-M-u', `C-M-up'
;; delete-backward-char
;; kill-backward-up-list               ; Not bound by default
;; beginning-of-buffer                 ; `M-<', `C-home'
;; beginning-of-defun                  ; `C-M-a', `C-M-home',
;; beginning-of-line                   ; `C-a', `home'
;; beginning-of-line-text              ; Not bound by default
;; beginning-of-visual-line            ; `C-a', `home'

(For most search patterns some of those likely wouldn't
be very useful - e.g., `backward-page'.  But if they're
not useful here then they're surely not useful in the
even more general case where all of Isearch would use
the minibuffer!)

With any such keys included, hitting the key does what
`M-e' does, but it also does what the command does.

E.g., `C-a' puts point at the beginning of the line in
the minibuffer (as `M-e C-a' would today).  Just insert
some text there, then hit `C-s' to continue with the
edited search pattern.

Like `M-e', this still uses the minibuffer only for
search-pattern editing.  And it lets users still use
other keys during Isearch that are specific to Isearch,
including using keys to exit Isearch.  (It's important
to some people to be able to exit using a bunch of keys
apparently.)  Isearch+ binds lots more keys during
Isearch - for things controlling search behavior (other
than just the search pattern).

Note that you can include any command.  But what
commands does it really make most sense to include?
Answer, I think: commands that move point _backward_
in the search pattern, possibly deleting some text
before or after movement (i.e. from the end or from
the new position).

Why?  Because the regular Isearch behavior of having
`DEL' delete chars from the end is _good_ - that's
what most pattern "editing" is about, in practice.

Being able to delete more than a char from the end
can sometimes be handy - e.g. delete a sexp.  Likewise,
moving backward various amounts, to insert or delete
some text from the middle or the beginning of the
pattern.

But _general_ minibuffer keys?  I don't think so.
(I really don't, as much as I practically live in the
minibuffer.)

Sure, even `C-x o' could be useful.  But in that case
do you want to continue searching with the same
pattern in the other window?  If so, even today that's
just `C-x o' then `C-s'.  Or do you want to, at the
same time, make some pattern edit?  The use case isn't
clear, to me.

Think about it.  Is this really pretty much only about
editing the search pattern?  If so, then I see no good
reason to reimplement Isearch based on the minibuffer.

What's needed for that is much less.  In that case, it
sounds like the only complaint is that some find the
need to hit `M-e' annoying.  Or it's something they're
not used to (perhaps, like you, they've just found out
about `M-e').  And to take care of that perceived
annoyance I propose what I mention above: implicit
`M-e' for certain keys.

Yes, to tell Emacs you're done editing the pattern in
a non-trivial way you need to hit `C-s'.  Worth it, IMO.

Re: [External] : Re: [WIP PATCH] Controlling Isearch from the minibuffer

[Kévin Le Gouguec]

Drew Adams <drew.adams@oracle.com> writes:

>> While isearching, I regularly find myself reaching for regular
>> navigation/editing commands (C-a, C-b, M-b, M-DEL… even C-x o, on
>> occasion), 
>
> You "regularly" want to move within the search-pattern,
> to do some editing of it besides at the end.  Is that it?
>
> But in that case, what about `C-x o'?

I'll admit that I struggle to see why I would have needed C-x o; I only
have the faintest impression that once in a blue moon I've wanted to use
it during a search.  Moving to a piece of text I was too lazy to retype
in order to kill it?  Do some light editing to the buffer before
resuming the search?  Surely nothing that I couldn't do by interrupting
the search, doing the thing, then C-s M-p (which… brings up a
minibuffer, AFAICT?  Which means all the usual navigation/editing
commands are available).

>> This thread made me discover M-e,
>
> Really?  In that case, I suggest you first try using
> that to do your editing, before asking that Isearch
> be rewritten to be minibuffer-based.  It's really not
> a big deal to use `M-e', IMO.

Sure; again, I didn't read the thread thoroughly, but my takeaway is
that the overarching tone of the proposal is indeed "it's not a big deal
to get used to isearch's idiosyncrasies, /but/ bolting an incremental
search on top of the minibuffer would [insert motivation]".

> How would you feel about what I described as existing
> in isearch+.el:
>
> Customize `isearchp-initiate-edit-commands' to include
> `beginning-of-line' (`C-a'), `backward-kill-word'
> (`M-DEL'),...?
>
> It already includes `C-b', `M-b', `C-M-b', `<left>',
> C-<left>', and `M-<left>'.  Your wish is half granted,
> even by default.

I described the proposal under discussion as "bolting an incremental
search on top of the minibuffer"; my reading of your description of
isearch+.el is "bolting regular editing/navigation keys on top of
isearch".

I don't think the distinction would matter much to me as a user, so I'd
probably be satisfied with isearch+.el (barring the manual curating of
isearchp-initiate-edit-commands).

As a theoretical greenfield implementer though, I'd probably start from
the existing minibuffer UI, since I'd get all the editing/navigation
commands "for free".

> Yes, to tell Emacs you're done editing the pattern in
> a non-trivial way you need to hit `C-s'.  Worth it, IMO.

I assume you have made this case elsewhere and I regret not taking the
time to dig it up, so apologies for making you repeat yourself: what
inherent qualities does the current, non-minibuffer-based,
implementation of isearch possess, that would be lost by using a
minibuffer?

Off the top of my head, I'd say "being able to exit a search and proceed
with my business using any key that is not bound in isearch-mode-map";
e.g. I regularly C-s a heading in org-mode and enjoy being able to C-c
C-x C-i (org-clock-in) without having to exit isearch first.

I suspect that would be implementable as a user option whose semantics
would be "if a key is neither self-inserting nor part of
isearch-mode-map, exit the minibuffer and run that key's binding".

I'm sure there are other reasons why the current implementation is
preferable though, hence my question.

Re: [WIP PATCH] Controlling Isearch from the minibuffer

[Juri Linkov]

> I mean keyboard quit.  You can see the error message by evaluating this
> and then pressing C-g:
>
>     (add-hook 'post-command-hook
>               (lambda () (read-from-minibuffer "test")) t t)

It's possible to handle C-g when needed:

  (add-hook 'post-command-hook
            (lambda () (condition-case nil
                           (read-from-minibuffer "test")
                         (quit)))
            t t)

> Using a timer with zero delay instead works better.

Using a timer is still a hack.  What could avoid a hack is
using the existing solution like recursive-edit is called
at the end of isearch-mode.

> Setting `isearch-message-function' is of no help, because there are some
> tests for `(null isearch-message-function)' as well as some explicit
> calls to `(isearch-message)' in isearch.el.  As far as I can see, there
> is no alternative to modifying the function `isearch-message' itself.

Tests for `(null isearch-message-function)' were added as a temporary
workaround until lazy count will be implemented in the minibuffer.
We need to remove these workarounds anyway.  So using isearch-message-function
should be the right thing to do.

> Moreover, one still has to manually keep a list of commands that need to
> switch to the search buffer (cf. `isearch-edit--buffer-commands') and
> commands that end the search (cf. `isearch-edit--quitting-commands'); I
> see no way around that.  Therefore, I see no advantage here over the
> proposed `with-isearch-window' macro.

There is no need to keep commands in a list.  The isearch-allow-scroll
feature uses symbol properties like

  (put 'recenter 'isearch-scroll t)
  (put 'recenter-top-bottom 'isearch-scroll t)
  (put 'reposition-window 'isearch-scroll t)

> I admit that the `with-isearch-window-quitting-edit' macro of my old
> patch seems pretty ad-hoc.  However, it could be replaced by a
> `with-isearch-done' macro which is of more general nature.  If you
> search isearch.el for calls to `isearch-done', you'll see that some are
> of the form
>
>      (let (;; Set `isearch-recursive-edit' to nil to prevent calling
>            ;; `exit-recursive-edit' in `isearch-done' that terminates
>            ;; the execution of this command when it is non-nil.
>            ;; We call `exit-recursive-edit' explicitly at the end below.
>            (isearch-recursive-edit nil))
>        (isearch-done nil t)
>
> while a few others seem to just don't take into account that we may be
> ending a recursive edit.

Indeed, this is an old unsolved problem, and setting isearch-recursive-edit
to prevent calling exit-recursive-edit is a workaround.  We need to fix this
anyway.  Then calling isearch-edit-string with read-from-minibuffer
at the end of isearch-mode should not be different from the current
calling of recursive-edit at the end of isearch-mode.
Both problems will use the same solution.

> So an `with-isearch-done' macro (which ends isearch, executes the body,
> then possibly ends a recursive edit, and at the same time takes care to
> unwind the minibuffer if we happen to be in `isearch-edit-string') would
> seem a reasonable addition to isearch.el.

Do you mean to pack the current isearch-recursive-edit/exit-recursive-edit
as a macro and use it in commands like isearch-query-replace, and also to
call exit-minibuffer to handle minibuffer exiting too.  This could be fine
if no better solution is found.

> Sorry for the long message :-)

It would be more appropriate to be sorry for the long patch ;-)
Usually a long patch means there are still unsolved workarounds
for old problems such as with isearch-message-function and
exit-recursive-edit above, etc.  After solving these problems
the patch should become much shorter.

Re: [ELPA?] Controlling Isearch from the minibuffer

[Augusto Stoffel]

On Thu, 13 May 2021 at 19:31, Juri Linkov <juri@linkov.net> wrote:

Hi Juri,

I'm pretty convinced that the approach of my first patch
(`with-isearch-window' macro, etc.) is the right way to implement this
and possibly other future features.  On the other hand, I understand
that it would be premature to just change isearch.el in this direction
without a deeper reflection and further testing.

So it seems that providing the feature in a package would be the best
way to proceed here.  Then we can observe which hurdles arise and make a
more informed change to isearch.el in the future.  I just worry a bit
this may end up being a very distant future :-).

What to you think?  I would then maintain the package on Github.  How
does the submission process to ELPA work?  Once again, here's the link:

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


>> I mean keyboard quit.  You can see the error message by evaluating this
>> and then pressing C-g:
>>
>>     (add-hook 'post-command-hook
>>               (lambda () (read-from-minibuffer "test")) t t)
>
> It's possible to handle C-g when needed:
>
>   (add-hook 'post-command-hook
>             (lambda () (condition-case nil
>                            (read-from-minibuffer "test")
>                          (quit)))
>             t t)
>
>> Using a timer with zero delay instead works better.
>
> Using a timer is still a hack.  What could avoid a hack is
> using the existing solution like recursive-edit is called
> at the end of isearch-mode.
>
>> Setting `isearch-message-function' is of no help, because there are some
>> tests for `(null isearch-message-function)' as well as some explicit
>> calls to `(isearch-message)' in isearch.el.  As far as I can see, there
>> is no alternative to modifying the function `isearch-message' itself.
>
> Tests for `(null isearch-message-function)' were added as a temporary
> workaround until lazy count will be implemented in the minibuffer.
> We need to remove these workarounds anyway.  So using isearch-message-function
> should be the right thing to do.

Okay, that' a low hanging fruit then.

>
>> Moreover, one still has to manually keep a list of commands that need to
>> switch to the search buffer (cf. `isearch-edit--buffer-commands') and
>> commands that end the search (cf. `isearch-edit--quitting-commands'); I
>> see no way around that.  Therefore, I see no advantage here over the
>> proposed `with-isearch-window' macro.
>
> There is no need to keep commands in a list.  The isearch-allow-scroll
> feature uses symbol properties like
>
>   (put 'recenter 'isearch-scroll t)
>   (put 'recenter-top-bottom 'isearch-scroll t)
>   (put 'reposition-window 'isearch-scroll t)
>

Fine, but this still means manually curating the list of relevant
commands.  I guess there's no way out of it.

>> I admit that the `with-isearch-window-quitting-edit' macro of my old
>> patch seems pretty ad-hoc.  However, it could be replaced by a
>> `with-isearch-done' macro which is of more general nature.  If you
>> search isearch.el for calls to `isearch-done', you'll see that some are
>> of the form
>>
>>      (let (;; Set `isearch-recursive-edit' to nil to prevent calling
>>            ;; `exit-recursive-edit' in `isearch-done' that terminates
>>            ;; the execution of this command when it is non-nil.
>>            ;; We call `exit-recursive-edit' explicitly at the end below.
>>            (isearch-recursive-edit nil))
>>        (isearch-done nil t)
>>
>> while a few others seem to just don't take into account that we may be
>> ending a recursive edit.
>
> Indeed, this is an old unsolved problem, and setting isearch-recursive-edit
> to prevent calling exit-recursive-edit is a workaround.  We need to fix this
> anyway.  Then calling isearch-edit-string with read-from-minibuffer
> at the end of isearch-mode should not be different from the current
> calling of recursive-edit at the end of isearch-mode.
> Both problems will use the same solution.
>
>> So an `with-isearch-done' macro (which ends isearch, executes the body,
>> then possibly ends a recursive edit, and at the same time takes care to
>> unwind the minibuffer if we happen to be in `isearch-edit-string') would
>> seem a reasonable addition to isearch.el.
>
> Do you mean to pack the current isearch-recursive-edit/exit-recursive-edit
> as a macro and use it in commands like isearch-query-replace, and also to
> call exit-minibuffer to handle minibuffer exiting too.  This could be fine
> if no better solution is found.
>

Yes.  Like I said, maybe this needs more time to mature, and seeing how
well the advice-based implementation of the isearch-mb package works may
inform this question.

>> Sorry for the long message :-)
>
> It would be more appropriate to be sorry for the long patch ;-)
> Usually a long patch means there are still unsolved workarounds
> for old problems such as with isearch-message-function and
> exit-recursive-edit above, etc.  After solving these problems
> the patch should become much shorter.

Okay, I'll try to be more granular next time.  I still want to work on
the lazy highlight for `isearch-edit-string' and `query-replace' and
friends, for instance.

Re: [ELPA?] Controlling Isearch from the minibuffer

[Jean Louis]

* Augusto Stoffel <arstoffel@gmail.com> [2021-05-13 23:13]:
> What to you think?  I would then maintain the package on Github.  How
> does the submission process to ELPA work?  Once again, here's the link:
> 
>     https://github.com/astoff/isearch-mb

After {M-x unload-feature RET isearch-mb RET} it causes problems, it
becomes almost impossible to use Emacs, but I loaded it again to tame
it. 

Maybe those compiler warnings have to be solved.

-- 
Jean

Take action in Free Software Foundation campaigns:
https://www.fsf.org/campaigns

Sign an open letter in support of Richard M. Stallman
https://stallmansupport.org/
https://rms-support-letter.github.io/

Re: [ELPA?] Controlling Isearch from the minibuffer

[Augusto Stoffel]

On Fri, 14 May 2021 at 04:17, Jean Louis <bugs@gnu.support> wrote:

Hi Jean,

> * Augusto Stoffel <arstoffel@gmail.com> [2021-05-13 23:13]:
>> What to you think?  I would then maintain the package on Github.  How
>> does the submission process to ELPA work?  Once again, here's the link:
>> 
>>     https://github.com/astoff/isearch-mb
>
> After {M-x unload-feature RET isearch-mb RET} it causes problems, it
> becomes almost impossible to use Emacs, but I loaded it again to tame
> it. 

Unloading a feature doesn't remove advices defined in that feature.
That's a bug in `unload-feature', no?

I've never seen a package taking the trouble to remove its advices on
unloading, so the problem you observed is very common, if more subtle.

>
> Maybe those compiler warnings have to be solved.

It's clean now.

Re: [ELPA?] Controlling Isearch from the minibuffer

[Augusto Stoffel]

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

On Thu, 13 May 2021 at 22:12, Augusto Stoffel <arstoffel@gmail.com> wrote:

>>> Setting `isearch-message-function' is of no help, because there are some
>>> tests for `(null isearch-message-function)' as well as some explicit
>>> calls to `(isearch-message)' in isearch.el.  As far as I can see, there
>>> is no alternative to modifying the function `isearch-message' itself.
>>
>> Tests for `(null isearch-message-function)' were added as a temporary
>> workaround until lazy count will be implemented in the minibuffer.
>> We need to remove these workarounds anyway.  So using isearch-message-function
>> should be the right thing to do.
>
> Okay, that' a low hanging fruit then.

I've attached a patch.  I can't see any deleterious effect for the
minibuffer and comint, which are the two places that set
`isearch-message-function' in Emacs.

This change is in preparation for adding lazy count in `query-replace',
`isearch-edit-string', etc.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Don-t-give-special-treatment-to-the-isearch-message-.patch --]
[-- Type: text/x-patch, Size: 6593 bytes --]

From b0867c7b7cb445dc2a8b84acc225d36b8f3ed073 Mon Sep 17 00:00:00 2001
From: Augusto Stoffel <arstoffel@gmail.com>
Date: Fri, 14 May 2021 11:58:35 +0200
Subject: [PATCH] Don't give special treatment to the isearch-message function

* lisp/isearch.el (isearch-message): defer to isearch-message-function
if non-nil.
(isearch-mode-end-hook-quit, isearch-update, with-isearch-suspended,
isearch-del-char, isearch-search-and-update, isearch-ring-adjust,
isearch-lazy-highlight-new-loop,
isearch-lazy-highlight-buffer-update):  Just call `isearch-message',
no need to check isearch-message-function anymore.
---
 lisp/isearch.el | 55 +++++++++++++++++++++++++------------------------
 1 file changed, 28 insertions(+), 27 deletions(-)

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 536c76ea5d..5db9ba9e4d 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -211,7 +211,8 @@ isearch-mode-end-hook-quit
 
 (defvar isearch-message-function nil
   "Function to call to display the search prompt.
-If nil, use function `isearch-message'.")
+If non-nil, the function `isearch-message' calls this function,
+with the same arguments, to do its job.")
 
 (defvar isearch-wrap-function nil
   "Function to call to wrap the search when search is failed.
@@ -1343,7 +1344,7 @@ isearch-update
 	   (null executing-kbd-macro))
       (progn
         (if (not (input-pending-p))
-          (funcall (or isearch-message-function #'isearch-message)))
+          (isearch-message))
         (if (and isearch-slow-terminal-mode
                  (not (or isearch-small-window
                           (pos-visible-in-window-group-p))))
@@ -1731,7 +1732,7 @@ with-isearch-suspended
             (isearch-update-from-string-properties isearch-string)
 
 	    ;; Restore the minibuffer message before moving point.
-            (funcall (or isearch-message-function #'isearch-message) nil t)
+            (isearch-message nil t)
 
 	    ;; Set point at the start (end) of old match if forward (backward),
 	    ;; so after exiting minibuffer isearch resumes at the start (end)
@@ -2504,7 +2505,7 @@ isearch-del-char
           isearch-message (mapconcat 'isearch-text-char-description
                                      isearch-string "")))
   ;; Do the following before moving point.
-  (funcall (or isearch-message-function #'isearch-message) nil t)
+  (isearch-message nil t)
   ;; Use the isearch-other-end as new starting point to be able
   ;; to find the remaining part of the search string again.
   ;; This is like what `isearch-search-and-update' does,
@@ -2765,7 +2766,7 @@ isearch-search-and-update
 		    (isearch-no-upper-case-p isearch-string isearch-regexp))))
       ;; Not regexp, not reverse, or no match at point.
       ;; Do the following before moving point.
-      (funcall (or isearch-message-function #'isearch-message) nil t)
+      (isearch-message nil t)
       (if (and isearch-other-end (not isearch-adjusted))
 	  (goto-char (if isearch-forward isearch-other-end
 		       (min isearch-opoint
@@ -3187,7 +3188,7 @@ isearch-ring-adjust
   (isearch-ring-adjust1 advance)
   (if search-ring-update
       (progn
-        (funcall (or isearch-message-function #'isearch-message) nil t)
+        (isearch-message nil t)
 	(isearch-search)
 	(isearch-push-state)
 	(isearch-update))
@@ -3267,22 +3268,24 @@ isearch-message
   ;; circumstances are when follow-mode is active, the search string
   ;; spans two (or several) windows, and the message about to be
   ;; displayed will cause the echo area to expand.
-  (let ((cursor-in-echo-area ellipsis)
-	(m isearch-message)
-	(fail-pos (isearch-fail-pos t)))
-    ;; Highlight failed part
-    (when fail-pos
-      (setq m (copy-sequence m))
-      (add-text-properties fail-pos (length m) '(face isearch-fail) m)
-      ;; Highlight failed trailing whitespace
-      (when (string-match " +$" m)
-	(add-text-properties (match-beginning 0) (match-end 0)
-			     '(face trailing-whitespace) m)))
-    (setq m (concat
-	     (isearch-message-prefix ellipsis isearch-nonincremental)
-	     m
-	     (isearch-message-suffix c-q-hack)))
-    (if c-q-hack m (let ((message-log-max nil)) (message "%s" m)))))
+  (if isearch-message-function
+      (funcall isearch-message-function c-q-hack ellipsis)
+    (let ((cursor-in-echo-area ellipsis)
+	  (m isearch-message)
+	  (fail-pos (isearch-fail-pos t)))
+      ;; Highlight failed part
+      (when fail-pos
+        (setq m (copy-sequence m))
+        (add-text-properties fail-pos (length m) '(face isearch-fail) m)
+        ;; Highlight failed trailing whitespace
+        (when (string-match " +$" m)
+	  (add-text-properties (match-beginning 0) (match-end 0)
+			       '(face trailing-whitespace) m)))
+      (setq m (concat
+	       (isearch-message-prefix ellipsis isearch-nonincremental)
+	       m
+	       (isearch-message-suffix c-q-hack)))
+      (if c-q-hack m (let ((message-log-max nil)) (message "%s" m))))))
 
 (defun isearch--describe-regexp-mode (regexp-function &optional space-before)
   "Make a string for describing REGEXP-FUNCTION.
@@ -3940,7 +3943,7 @@ isearch-lazy-highlight-new-loop
 			         isearch-lazy-highlight-window-end))))))
     ;; something important did indeed change
     (lazy-highlight-cleanup t (not (equal isearch-string ""))) ;stop old timer
-    (when (and isearch-lazy-count isearch-mode (null isearch-message-function))
+    (when isearch-lazy-count
       (when (or (equal isearch-string "")
                 ;; Check if this place was reached by a condition above
                 ;; other than changed window boundaries (that shouldn't
@@ -4010,9 +4013,7 @@ isearch-lazy-highlight-new-loop
                                    lazy-highlight-initial-delay)
                                  nil
                                  'isearch-lazy-highlight-start))))
-  ;; Update the current match number only in isearch-mode and
-  ;; unless isearch-mode is used specially with isearch-message-function
-  (when (and isearch-lazy-count isearch-mode (null isearch-message-function))
+  (when isearch-lazy-count
     ;; Update isearch-lazy-count-current only when it was already set
     ;; at the end of isearch-lazy-highlight-buffer-update
     (when isearch-lazy-count-current
@@ -4220,7 +4221,7 @@ isearch-lazy-highlight-buffer-update
 		    (setq looping nil
 			  nomore  t))))
 	    (if nomore
-		(when (and isearch-lazy-count isearch-mode (null isearch-message-function))
+		(when isearch-lazy-count
 		  (unless isearch-lazy-count-total
 		    (setq isearch-lazy-count-total 0))
 		  (setq isearch-lazy-count-current
-- 
2.31.1

Re: [ELPA?] Controlling Isearch from the minibuffer

[Juri Linkov]

> I'm pretty convinced that the approach of my first patch
> (`with-isearch-window' macro, etc.) is the right way to implement this
> and possibly other future features.  On the other hand, I understand
> that it would be premature to just change isearch.el in this direction
> without a deeper reflection and further testing.
>
> So it seems that providing the feature in a package would be the best
> way to proceed here.  Then we can observe which hurdles arise and make a
> more informed change to isearch.el in the future.  I just worry a bit
> this may end up being a very distant future :-).

As soon as we fix all long-standing problems, such as adding a new macro
for isearch-recursive-edit/exit-recursive-edit, and removing workarounds
from the lazy highlight, you will be able to implement the isearch-from-minibuffer
feature with just a few lines of code :-)

Re: [ELPA?] Controlling Isearch from the minibuffer

[Juri Linkov]

>>>> Setting `isearch-message-function' is of no help, because there are some
>>>> tests for `(null isearch-message-function)' as well as some explicit
>>>> calls to `(isearch-message)' in isearch.el.  As far as I can see, there
>>>> is no alternative to modifying the function `isearch-message' itself.
>>>
>>> Tests for `(null isearch-message-function)' were added as a temporary
>>> workaround until lazy count will be implemented in the minibuffer.
>>> We need to remove these workarounds anyway.  So using isearch-message-function
>>> should be the right thing to do.
>>
>> Okay, that' a low hanging fruit then.
>
> I've attached a patch.  I can't see any deleterious effect for the
> minibuffer and comint, which are the two places that set
> `isearch-message-function' in Emacs.

Do you see the correct lazy-count values in the minibuffer and comint
after removing `(null isearch-message-function)'?

> -          (funcall (or isearch-message-function #'isearch-message)))
> +          (isearch-message))

What do you think about adding `isearch-message' as the default value
of isearch-message-function?  E.g.

#+begin_src diff
-(defvar isearch-message-function nil
+(defvar isearch-message-function #'isearch-message
#+end_src

Re: [ELPA?] Controlling Isearch from the minibuffer

[Augusto Stoffel]

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

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

> Do you see the correct lazy-count values in the minibuffer and comint
> after removing `(null isearch-message-function)'?
>
>> -          (funcall (or isearch-message-function #'isearch-message)))
>> +          (isearch-message))

I would see the number of matches in the current line, which is only
logical.  But the relevant information here would be number of
candidates in the minibuffer/comint history that match the string, as
opposed to the total number of matches among those candidates.  So I
believe these modes should implement their own lazy count.  I just
disabled the count in the new patch below.

>
> What do you think about adding `isearch-message' as the default value
> of isearch-message-function?  E.g.
>
> #+begin_src diff
> -(defvar isearch-message-function nil
> +(defvar isearch-message-function #'isearch-message
> #+end_src

Yes, this is nicer, assuming no one relies on this variable usually
being nil up to now.

Now, I took a closer look at the history of changes around lazy
highlight and the interactions with other features seems very tricky.
I'm not super confident about the patch I'm attaching, but if you are
willing to review it and test a bit, I hope it helps.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Don-t-give-special-treatment-to-the-isearch-message-.patch --]
[-- Type: text/x-patch, Size: 10233 bytes --]

From 98226106351e2edba99ea647abf1fcd059e2e79d Mon Sep 17 00:00:00 2001
From: Augusto Stoffel <arstoffel@gmail.com>
Date: Fri, 14 May 2021 11:58:35 +0200
Subject: [PATCH] Don't give special treatment to the isearch-message function

* lisp/isearch.el (isearch-message-function): new default value.
(isearch-message): just defer to isearch-message-function, if non-nil.
(isearch-standard-message): new function, corresponds to the old
`isearch-message'.
(isearch-mode-end-hook-quit, isearch-update, with-isearch-suspended,
isearch-del-char, isearch-search-and-update, isearch-ring-adjust,
isearch-lazy-highlight-new-loop,
isearch-lazy-highlight-buffer-update):  just call `isearch-message',
no need to check isearch-message-function anymore.
* lisp/comint.el (comint-history-isearch-setup): disable lazy count,
since this only counts matches in the current line.
(comint-history-isearch-end): kill local variables instead of setting
them to the presumed previous value.
(comint-history-isearch-message): call isearch-standard-message
explicitly where needed.
* lisp/simple.el (minibuffer-history-isearch-setup): disable lazy count,
since this only counts matches in the current line.
(minibuffer-history-isearch-message): call `isearch-standard-message'
explicitly where needed.
---
 lisp/comint.el  | 18 ++++++++++--------
 lisp/isearch.el | 30 +++++++++++++++++-------------
 lisp/simple.el  |  5 +++--
 3 files changed, 30 insertions(+), 23 deletions(-)

diff --git a/lisp/comint.el b/lisp/comint.el
index ef34174305..cc062e3e5a 100644
--- a/lisp/comint.el
+++ b/lisp/comint.el
@@ -1487,7 +1487,7 @@ comint-history-isearch-setup
 	     (and (eq comint-history-isearch 'dwim)
 		  ;; Point is at command line.
 		  (comint-after-pmark-p))))
-    (setq isearch-message-prefix-add "history ")
+    (setq-local isearch-message-prefix-add "history ")
     (setq-local isearch-search-fun-function
                 #'comint-history-isearch-search)
     (setq-local isearch-message-function
@@ -1496,17 +1496,19 @@ comint-history-isearch-setup
                 #'comint-history-isearch-wrap)
     (setq-local isearch-push-state-function
                 #'comint-history-isearch-push-state)
+    (setq-local isearch-lazy-count nil)
     (add-hook 'isearch-mode-end-hook 'comint-history-isearch-end nil t)))
 
 (defun comint-history-isearch-end ()
   "Clean up the comint after terminating Isearch in comint."
   (if comint-history-isearch-message-overlay
       (delete-overlay comint-history-isearch-message-overlay))
-  (setq isearch-message-prefix-add nil)
-  (setq isearch-search-fun-function 'isearch-search-fun-default)
-  (setq isearch-message-function nil)
-  (setq isearch-wrap-function nil)
-  (setq isearch-push-state-function nil)
+  (kill-local-variable 'isearch-message-prefix-add)
+  (kill-local-variable 'isearch-search-fun-function)
+  (kill-local-variable 'isearch-message-function)
+  (kill-local-variable 'isearch-wrap-function)
+  (kill-local-variable 'isearch-push-state-function)
+  (kill-local-variable 'isearch-lazy-count)
   (remove-hook 'isearch-mode-end-hook 'comint-history-isearch-end t)
   (unless isearch-suspended
     (custom-reevaluate-setting 'comint-history-isearch)))
@@ -1588,11 +1590,11 @@ comint-history-isearch-message
 Otherwise, it displays the standard Isearch message returned from
 the function `isearch-message'."
   (if (not (and isearch-success (not isearch-error)))
-      ;; Use standard function `isearch-message' when not in comint prompt,
+      ;; Use `isearch-standard-message' when not in comint prompt,
       ;; or search fails, or has an error (like incomplete regexp).
       ;; This function displays isearch message in the echo area,
       ;; so it's possible to see what is wrong in the search string.
-      (isearch-message c-q-hack ellipsis)
+      (isearch-standard-message c-q-hack ellipsis)
     ;; Otherwise, put the overlay with the standard isearch prompt over
     ;; the initial comint prompt.
     (if (overlayp comint-history-isearch-message-overlay)
diff --git a/lisp/isearch.el b/lisp/isearch.el
index 536c76ea5d..f545bf5558 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -209,9 +209,10 @@ isearch-mode-end-hook
 (defvar isearch-mode-end-hook-quit nil
   "Non-nil while running `isearch-mode-end-hook' if the user quits the search.")
 
-(defvar isearch-message-function nil
-  "Function to call to display the search prompt.
-If nil, use function `isearch-message'.")
+(defvar isearch-message-function #'isearch-standard-message
+  "Function called by `isearch-message' to display the search prompt.
+This should be nil to supress printing messages, or a function
+taking the same arguments as `isearch-standard-message'.")
 
 (defvar isearch-wrap-function nil
   "Function to call to wrap the search when search is failed.
@@ -1343,7 +1344,7 @@ isearch-update
 	   (null executing-kbd-macro))
       (progn
         (if (not (input-pending-p))
-          (funcall (or isearch-message-function #'isearch-message)))
+          (isearch-message))
         (if (and isearch-slow-terminal-mode
                  (not (or isearch-small-window
                           (pos-visible-in-window-group-p))))
@@ -1731,7 +1732,7 @@ with-isearch-suspended
             (isearch-update-from-string-properties isearch-string)
 
 	    ;; Restore the minibuffer message before moving point.
-            (funcall (or isearch-message-function #'isearch-message) nil t)
+            (isearch-message nil t)
 
 	    ;; Set point at the start (end) of old match if forward (backward),
 	    ;; so after exiting minibuffer isearch resumes at the start (end)
@@ -2504,7 +2505,7 @@ isearch-del-char
           isearch-message (mapconcat 'isearch-text-char-description
                                      isearch-string "")))
   ;; Do the following before moving point.
-  (funcall (or isearch-message-function #'isearch-message) nil t)
+  (isearch-message nil t)
   ;; Use the isearch-other-end as new starting point to be able
   ;; to find the remaining part of the search string again.
   ;; This is like what `isearch-search-and-update' does,
@@ -2765,7 +2766,7 @@ isearch-search-and-update
 		    (isearch-no-upper-case-p isearch-string isearch-regexp))))
       ;; Not regexp, not reverse, or no match at point.
       ;; Do the following before moving point.
-      (funcall (or isearch-message-function #'isearch-message) nil t)
+      (isearch-message nil t)
       (if (and isearch-other-end (not isearch-adjusted))
 	  (goto-char (if isearch-forward isearch-other-end
 		       (min isearch-opoint
@@ -3187,7 +3188,7 @@ isearch-ring-adjust
   (isearch-ring-adjust1 advance)
   (if search-ring-update
       (progn
-        (funcall (or isearch-message-function #'isearch-message) nil t)
+        (isearch-message nil t)
 	(isearch-search)
 	(isearch-push-state)
 	(isearch-update))
@@ -3260,6 +3261,11 @@ isearch-complete-edit
 
 (defun isearch-message (&optional c-q-hack ellipsis)
   "Generate and print the message string."
+  (when isearch-message-function
+    (funcall isearch-message-function c-q-hack ellipsis)))
+
+(defun isearch-standard-message (&optional c-q-hack ellipsis)
+  "Generate and print the standard Isearch message string."
 
   ;; N.B.: This function should always be called with point at the
   ;; search point, because in certain (rare) circumstances, undesired
@@ -3940,7 +3946,7 @@ isearch-lazy-highlight-new-loop
 			         isearch-lazy-highlight-window-end))))))
     ;; something important did indeed change
     (lazy-highlight-cleanup t (not (equal isearch-string ""))) ;stop old timer
-    (when (and isearch-lazy-count isearch-mode (null isearch-message-function))
+    (when isearch-lazy-count
       (when (or (equal isearch-string "")
                 ;; Check if this place was reached by a condition above
                 ;; other than changed window boundaries (that shouldn't
@@ -4010,9 +4016,7 @@ isearch-lazy-highlight-new-loop
                                    lazy-highlight-initial-delay)
                                  nil
                                  'isearch-lazy-highlight-start))))
-  ;; Update the current match number only in isearch-mode and
-  ;; unless isearch-mode is used specially with isearch-message-function
-  (when (and isearch-lazy-count isearch-mode (null isearch-message-function))
+  (when isearch-lazy-count
     ;; Update isearch-lazy-count-current only when it was already set
     ;; at the end of isearch-lazy-highlight-buffer-update
     (when isearch-lazy-count-current
@@ -4220,7 +4224,7 @@ isearch-lazy-highlight-buffer-update
 		    (setq looping nil
 			  nomore  t))))
 	    (if nomore
-		(when (and isearch-lazy-count isearch-mode (null isearch-message-function))
+		(when isearch-lazy-count
 		  (unless isearch-lazy-count-total
 		    (setq isearch-lazy-count-total 0))
 		  (setq isearch-lazy-count-current
diff --git a/lisp/simple.el b/lisp/simple.el
index 0255f69e42..8deb07b31a 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2733,6 +2733,7 @@ minibuffer-history-isearch-setup
               #'minibuffer-history-isearch-wrap)
   (setq-local isearch-push-state-function
               #'minibuffer-history-isearch-push-state)
+  (setq-local isearch-lazy-count nil)
   (add-hook 'isearch-mode-end-hook 'minibuffer-history-isearch-end nil t))
 
 (defun minibuffer-history-isearch-end ()
@@ -2794,11 +2795,11 @@ minibuffer-history-isearch-message
 Otherwise, it displays the standard isearch message returned from
 the function `isearch-message'."
   (if (not (and (minibufferp) isearch-success (not isearch-error)))
-      ;; Use standard function `isearch-message' when not in the minibuffer,
+      ;; Use `isearch-standard-message' when not in the minibuffer,
       ;; or search fails, or has an error (like incomplete regexp).
       ;; This function overwrites minibuffer text with isearch message,
       ;; so it's possible to see what is wrong in the search string.
-      (isearch-message c-q-hack ellipsis)
+      (isearch-standard-message c-q-hack ellipsis)
     ;; Otherwise, put the overlay with the standard isearch prompt over
     ;; the initial minibuffer prompt.
     (if (overlayp minibuffer-history-isearch-message-overlay)
-- 
2.31.1

Re: [ELPA?] Controlling Isearch from the minibuffer

[Juri Linkov]

> I'm pretty convinced that the approach of my first patch
> (`with-isearch-window' macro, etc.) is the right way to implement this
> and possibly other future features.  On the other hand, I understand
> that it would be premature to just change isearch.el in this direction
> without a deeper reflection and further testing.

Wrapping the bodies of all isearch commands in a macro `with-isearch-window'
is much more intrusive change than handling commands in hooks
after adding just one such line for every command:

  (put 'isearch-repeat-forward 'isearch-from-minibuffer t)

Re: [ELPA?] Controlling Isearch from the minibuffer

[Juri Linkov]

>> Do you see the correct lazy-count values in the minibuffer and comint
>> after removing `(null isearch-message-function)'?
>>
>>> -          (funcall (or isearch-message-function #'isearch-message)))
>>> +          (isearch-message))
>
> I would see the number of matches in the current line, which is only
> logical.  But the relevant information here would be number of
> candidates in the minibuffer/comint history that match the string, as
> opposed to the total number of matches among those candidates.  So I
> believe these modes should implement their own lazy count.  I just
> disabled the count in the new patch below.

Thanks, binding isearch-lazy-count buffer-locally to nil
is the right way to disable lazy-count until it's implemented
in these modes.

> Now, I took a closer look at the history of changes around lazy
> highlight and the interactions with other features seems very tricky.
> I'm not super confident about the patch I'm attaching, but if you are
> willing to review it and test a bit, I hope it helps.

Everything looks right, but this needs more testing before pushing.

> +(defun isearch-standard-message (&optional c-q-hack ellipsis)
> +  "Generate and print the standard Isearch message string."

A more naming-standard complying name would be isearch-default-message,
or even better isearch-message-default like isearch-search-fun-default
(no need to send a new patch only with renamings).

Re: [ELPA?] Controlling Isearch from the minibuffer

[Augusto Stoffel]

On Sun, 16 May 2021 at 21:12, Juri Linkov <juri@linkov.net> wrote:

>> I'm pretty convinced that the approach of my first patch
>> (`with-isearch-window' macro, etc.) is the right way to implement this
>> and possibly other future features.  On the other hand, I understand
>> that it would be premature to just change isearch.el in this direction
>> without a deeper reflection and further testing.
>
> Wrapping the bodies of all isearch commands in a macro `with-isearch-window'
> is much more intrusive change than handling commands in hooks
> after adding just one such line for every command:
>
>   (put 'isearch-repeat-forward 'isearch-from-minibuffer t)

I agree that the hook approach is less intrusive, but I after
implementing it I felt it was too hacky, and probably more brittle.

In the meanwhile Daniel Mendler made several helpful suggestions for the
package version of the feature, now I feel there's not much technical
reason to make the feature built into isearch.el (I would love the extra
visibility/availability, but that's not an argument for inclusion).  In
particular, this use case is different from an improved
`isearch-edit-string' in several small ways, which would hinder a lot of
code reuse.

How do I formalize the ELPA submission?  (There's a Github link a few
messages back.)

Re: [ELPA?] Controlling Isearch from the minibuffer

[Augusto Stoffel]

General minibuffer question: When the minibuffer exits, the minibuffer
selected window sometimes gets recentered.  More specifically, the rule
seems to be that this happens whenever the minibuffer selected window
scrolls during the minibuffer session.

Is there any way to disable the automatic recentering (which doesn't
involve saving the window bounds beforehand and then undoing it)?

Re: [ELPA?] Controlling Isearch from the minibuffer

[Eli Zaretskii]

> From: Augusto Stoffel <arstoffel@gmail.com>
> Date: Fri, 21 May 2021 11:09:08 +0200
> Cc: Ergus <spacibba@aol.com>, emacs-devel@gnu.org
> 
> General minibuffer question: When the minibuffer exits, the minibuffer
> selected window sometimes gets recentered.  More specifically, the rule
> seems to be that this happens whenever the minibuffer selected window
> scrolls during the minibuffer session.

The mini-window is just a window, so it behaves like any other window
wrt scrolling and recentering.

> Is there any way to disable the automatic recentering (which doesn't
> involve saving the window bounds beforehand and then undoing it)?

Disable and do what instead?  Emacs scrolls a window when point goes
out of view.  What would you like it to do instead of recentering?
Since this is just a window, the same options that control scrolling
in other windows can do it here as well.

Re: [ELPA?] Controlling Isearch from the minibuffer

[Augusto Stoffel]

On Fri, 21 May 2021 at 13:25, Eli Zaretskii <eliz@gnu.org> wrote:

>> Is there any way to disable the automatic recentering (which doesn't
>> involve saving the window bounds beforehand and then undoing it)?
>
> Disable and do what instead?  Emacs scrolls a window when point goes
> out of view.  What would you like it to do instead of recentering?
> Since this is just a window, the same options that control scrolling
> in other windows can do it here as well.

If you evaluate the following in a sufficiently long buffer,

    (minibuffer-with-setup-hook
        (lambda () (scroll-other-window 100))
      (read-from-minibuffer "Pressing RET now will recenter"))

then the point will be at the top of the window.  From there, pressing
RET shifts the window so that the point is in the middle (the point
itself doesn't move).

I just want there to be no recentering, i.e., leave `window-start' where
the point is.

Re: [ELPA?] Controlling Isearch from the minibuffer

[Eli Zaretskii]

> From: Augusto Stoffel <arstoffel@gmail.com>
> Cc: juri@linkov.net,  spacibba@aol.com,  emacs-devel@gnu.org
> Date: Fri, 21 May 2021 13:56:26 +0200
> 
> > Disable and do what instead?  Emacs scrolls a window when point goes
> > out of view.  What would you like it to do instead of recentering?
> > Since this is just a window, the same options that control scrolling
> > in other windows can do it here as well.
> 
> If you evaluate the following in a sufficiently long buffer,
> 
>     (minibuffer-with-setup-hook
>         (lambda () (scroll-other-window 100))
>       (read-from-minibuffer "Pressing RET now will recenter"))
> 
> then the point will be at the top of the window.  From there, pressing
> RET shifts the window so that the point is in the middle (the point
> itself doesn't move).

read-from-minibuffer enters recursive-edit; when you exit that, Emacs
undoes any changes to windows you did during the recursive-edit, which
restores window-start point back to its original value.  Then Emacs
finds point outside the viewport, and recenters to show point.

The main point here is that you cannot (easily) affect windows while
in recursive-edit: Emacs is designed to undo any such changes.

Re: [ELPA?] Controlling Isearch from the minibuffer

[Augusto Stoffel]

On Fri, 21 May 2021 at 15:31, Eli Zaretskii <eliz@gnu.org> wrote:

> read-from-minibuffer enters recursive-edit; when you exit that, Emacs
> undoes any changes to windows you did during the recursive-edit, which
> restores window-start point back to its original value.  Then Emacs
> finds point outside the viewport, and recenters to show point.
>
> The main point here is that you cannot (easily) affect windows while
> in recursive-edit: Emacs is designed to undo any such changes.

I see, thanks for the clarification!

Re: [ELPA?] Controlling Isearch from the minibuffer

[Stefan Monnier]

> The main point here is that you cannot (easily) affect windows while
> in recursive-edit: Emacs is designed to undo any such changes.

Actually, AFAIK this is misleading.  I believe the cause of the problem
is the save&restore of the window configuration performed by
`read-from-minibuffer` (which is both a blessing and a curse, depending
on the situation).

There are some discussions of removing it (recently, for example, within
the longish threads around Alan's rework of the placement of
minibuffers), but it's not going to happen until someone takes on the
challenge to try and address the many use cases where restoring the
window-config is (very) useful.


        Stefan

Re: [ELPA?] Controlling Isearch from the minibuffer

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Augusto Stoffel <arstoffel@gmail.com>,  spacibba@aol.com,
>  emacs-devel@gnu.org,  juri@linkov.net
> Date: Fri, 21 May 2021 11:05:36 -0400
> 
> > The main point here is that you cannot (easily) affect windows while
> > in recursive-edit
> 
> Actually, AFAIK this is misleading.

Okay, in read-from-minibuffer.

Re: [ELPA?] Controlling Isearch from the minibuffer

[Juri Linkov]

>> Now, I took a closer look at the history of changes around lazy
>> highlight and the interactions with other features seems very tricky.
>> I'm not super confident about the patch I'm attaching, but if you are
>> willing to review it and test a bit, I hope it helps.
>
> Everything looks right, but this needs more testing before pushing.

Testing revealed some problems:

1. After enabling isearch-lazy-count, modes that use lazy-highligh,
   such as e.g. query-replace displays the isearch message
   overwriting query-replace prompt.  Maybe this is because of the
   removed check for 'isearch-mode' in isearch-lazy-highlight-* functions?

2. Using 'C-q' (isearch-quote-char) in the minibuffer or comint
   displays a truncated string, e.g. on 'M-! C-r C-q C-a'.
   Maybe the default message function should be called directly
   in isearch-quote-char?

3. Maybe the same default message function should be called directly
   also in some other functions like isearch-lazy-highlight-new-loop
   and isearch-lazy-highlight-buffer-update?

Re: [ELPA?] Controlling Isearch from the minibuffer

[Augusto Stoffel]

On Tue, 25 May 2021 at 23:50, Juri Linkov <juri@linkov.net> wrote:

> Testing revealed some problems:
>
> 1. After enabling isearch-lazy-count, modes that use lazy-highligh,
>    such as e.g. query-replace displays the isearch message
>    overwriting query-replace prompt.  Maybe this is because of the
>    removed check for 'isearch-mode' in isearch-lazy-highlight-* functions?
>
> 2. Using 'C-q' (isearch-quote-char) in the minibuffer or comint
>    displays a truncated string, e.g. on 'M-! C-r C-q C-a'.
>    Maybe the default message function should be called directly
>    in isearch-quote-char?
>
> 3. Maybe the same default message function should be called directly
>    also in some other functions like isearch-lazy-highlight-new-loop
>    and isearch-lazy-highlight-buffer-update?

Okay, it looks like we need to bind `isearch-message-function' in some
places, possibly to `ignore'.  I'll try this sometime soon.