Re: [WIP PATCH] Controlling Isearch from the minibuffer

[Augusto Stoffel <arstoffel@gmail.com>]

[-- 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