GNU bug report logs - #30187
M-e should restore isearch correctly in special modes

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 30187 in the body.
You can then email your comments to 30187 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: bug-gnu-emacs <at> gnu.org
Subject: M-e should restore isearch correctly in special modes
Date: Sat, 20 Jan 2018 23:10:16 +0200

[Message part 1 (text/plain, inline)]

There was an old problem that ‘isearch-edit-string’ incorrectly
restored a previous isearch mode in such commands as
‘comint-history-isearch-backward’ and ‘dired-isearch-filenames’
because they let-bind variables that affects the search parameters,
e.g. ‘(let ((dired-isearch-filenames t)))’ only at the starting
isearch, but when ‘M-e’ (isearch-edit-string) exits isearch,
and later starts again, these let-binding are not in effect
and thus restore the wrong search state.

All imaginable solutions were very bad.  But now I found a solution
that at least not bad.  It changes the global value of customizable
variables for the time while isearch is active (including the time
when isearch is suspended temporarily), and restores the original value
afterwards.

[isearch-suspended.patch (text/x-diff, inline)]

diff --git a/lisp/comint.el b/lisp/comint.el
index a79e34b..8dba317 100644
--- a/lisp/comint.el
+++ b/lisp/comint.el
@@ -1434,14 +1434,14 @@ comint-history-isearch
 (defun comint-history-isearch-backward ()
   "Search for a string backward in input history using Isearch."
   (interactive)
-  (let ((comint-history-isearch t))
-    (isearch-backward nil t)))
+  (setq comint-history-isearch t)
+  (isearch-backward nil t))
 
 (defun comint-history-isearch-backward-regexp ()
   "Search for a regular expression backward in input history using Isearch."
   (interactive)
-  (let ((comint-history-isearch t))
-    (isearch-backward-regexp nil t)))
+  (setq comint-history-isearch t)
+  (isearch-backward-regexp nil t))
 
 (defvar-local comint-history-isearch-message-overlay nil)
 
@@ -1472,7 +1472,9 @@ comint-history-isearch-end
   (setq isearch-message-function nil)
   (setq isearch-wrap-function nil)
   (setq isearch-push-state-function nil)
-  (remove-hook 'isearch-mode-end-hook 'comint-history-isearch-end t))
+  (remove-hook 'isearch-mode-end-hook 'comint-history-isearch-end t)
+  (unless isearch-suspended
+    (custom-reevaluate-setting 'comint-history-isearch)))
 
 (defun comint-goto-input (pos)
   "Put input history item of the absolute history position POS."
diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index 223b254..55b68a3 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -2766,7 +2766,9 @@ dired-isearch-filenames-end
   "Clean up the Dired file name search after terminating isearch."
   (define-key isearch-mode-map "\M-sff" nil)
   (dired-isearch-filenames-mode -1)
-  (remove-hook 'isearch-mode-end-hook 'dired-isearch-filenames-end t))
+  (remove-hook 'isearch-mode-end-hook 'dired-isearch-filenames-end t)
+  (unless isearch-suspended
+    (custom-reevaluate-setting 'dired-isearch-filenames)))
 
 (defun dired-isearch-filter-filenames (beg end)
   "Test whether some part of the current search match is inside a file name.
@@ -2779,15 +2781,15 @@ dired-isearch-filter-filenames
 (defun dired-isearch-filenames ()
   "Search for a string using Isearch only in file names in the Dired buffer."
   (interactive)
-  (let ((dired-isearch-filenames t))
-    (isearch-forward nil t)))
+  (setq dired-isearch-filenames t)
+  (isearch-forward nil t))
 
 ;;;###autoload
 (defun dired-isearch-filenames-regexp ()
   "Search for a regexp using Isearch only in file names in the Dired buffer."
   (interactive)
-  (let ((dired-isearch-filenames t))
-    (isearch-forward-regexp nil t)))
+  (setq dired-isearch-filenames t)
+  (isearch-forward-regexp nil t))
 
 
 ;; Functions for searching in tags style among marked files.
diff --git a/lisp/isearch.el b/lisp/isearch.el
index 3725779..b131437 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -1233,6 +1233,8 @@ isearch-new-regexp-function
 (define-obsolete-variable-alias 'isearch-new-word
   'isearch-new-regexp-function "25.1")
 
+(defvar isearch-suspended nil)
+
 (defmacro with-isearch-suspended (&rest body)
   "Exit Isearch mode, run BODY, and reinvoke the pending search.
 You can update the global isearch variables by setting new values to
@@ -1299,6 +1301,8 @@ with-isearch-suspended
 	       isearch-original-minibuffer-message-timeout)
 	      old-point old-other-end)
 
+          (setq isearch-suspended t)
+
 	  ;; Actually terminate isearching until editing is done.
 	  ;; This is so that the user can do anything without failure,
 	  ;; like switch buffers and start another isearch, and return.
@@ -1313,6 +1317,8 @@ with-isearch-suspended
 	  (unwind-protect
 	      (progn ,@body)
 
+            (setq isearch-suspended nil)
+
 	    ;; Always resume isearching by restarting it.
 	    (isearch-mode isearch-forward
 			  isearch-regexp
@@ -1374,6 +1380,7 @@ with-isearch-suspended
 		  (message "")))))
 
     (quit  ; handle abort-recursive-edit
+     (setq isearch-suspended nil)
      (isearch-abort)  ;; outside of let to restore outside global values
      )))
 

Message #8 received at 30187 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juri Linkov <juri <at> linkov.net>
Cc: 30187 <at> debbugs.gnu.org
Subject: Re: bug#30187: M-e should restore isearch correctly in special modes
Date: Sun, 21 Jan 2018 18:00:40 +0200

> From: Juri Linkov <juri <at> linkov.net>
> Date: Sat, 20 Jan 2018 23:10:16 +0200
> 
> All imaginable solutions were very bad.  But now I found a solution
> that at least not bad.  It changes the global value of customizable
> variables for the time while isearch is active (including the time
> when isearch is suspended temporarily), and restores the original value
> afterwards.

What if the user QUITs out of the search -- will the original value be
restored?

Thanks.

Message #11 received at 30187 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 30187 <at> debbugs.gnu.org
Subject: Re: bug#30187: M-e should restore isearch correctly in special modes
Date: Sun, 21 Jan 2018 23:23:03 +0200

>> All imaginable solutions were very bad.  But now I found a solution
>> that at least not bad.  It changes the global value of customizable
>> variables for the time while isearch is active (including the time
>> when isearch is suspended temporarily), and restores the original value
>> afterwards.
>
> What if the user QUITs out of the search -- will the original value be
> restored?

Yes, the original value is restored thanks to ‘condition-case’ with
‘quit’ in ‘with-isearch-suspended’ that guarantees that ‘isearch-abort’
is always called.

Message #16 received at 30187-done <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 30187-done <at> debbugs.gnu.org
Subject: Re: bug#30187: M-e should restore isearch correctly in special modes
Date: Tue, 23 Jan 2018 00:14:23 +0200

>>> All imaginable solutions were very bad.  But now I found a solution
>>> that at least not bad.  It changes the global value of customizable
>>> variables for the time while isearch is active (including the time
>>> when isearch is suspended temporarily), and restores the original value
>>> afterwards.
>>
>> What if the user QUITs out of the search -- will the original value be
>> restored?
>
> Yes, the original value is restored thanks to ‘condition-case’ with
> ‘quit’ in ‘with-isearch-suspended’ that guarantees that ‘isearch-abort’
> is always called.

Done.

Message #19 received at 30187 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: 30187 <at> debbugs.gnu.org
Subject: Re: bug#30187: M-e should restore isearch correctly in special modes
Date: Mon, 29 Jan 2018 23:57:17 +0200

> There was an old problem that ‘isearch-edit-string’ incorrectly
> restored a previous isearch mode in such commands as
> ‘comint-history-isearch-backward’ and ‘dired-isearch-filenames’
> because they let-bind variables that affects the search parameters,
> e.g. ‘(let ((dired-isearch-filenames t)))’ only at the starting
> isearch, but when ‘M-e’ (isearch-edit-string) exits isearch,
> and later starts again, these let-binding are not in effect
> and thus restore the wrong search state.

There is another problem with ‘comint-history-isearch-backward’:
when it is called at the end of an “*Async Shell Command*” buffer
it should not activate comint-history isearch, only a normal isearch
should be started.  I tried different approaches, but only workable
solution is to check whether the shell prompt is empty as it is
in all “*Async Shell Command*” buffers:

diff --git a/lisp/comint.el b/lisp/comint.el
index 8dba317..b7179e2 100644
--- a/lisp/comint.el
+++ b/lisp/comint.el
@@ -1448,10 +1448,17 @@ comint-history-isearch-message-overlay
 (defun comint-history-isearch-setup ()
   "Set up a comint for using Isearch to search the input history.
 Intended to be added to `isearch-mode-hook' in `comint-mode'."
-  (when (or (eq comint-history-isearch t)
-	    (and (eq comint-history-isearch 'dwim)
-		 ;; Point is at command line.
-		 (comint-after-pmark-p)))
+  (when (and (get-buffer-process (current-buffer))
+	     (or (eq comint-history-isearch t)
+		 (and (eq comint-history-isearch 'dwim)
+		      ;; Point is at command line.
+		      (comint-after-pmark-p)
+		      ;; Prompt is not empty like in Async Shell Command buffers
+		      (not (eq (save-excursion
+				 (goto-char (comint-line-beginning-position))
+				 (forward-line 0)
+				 (point))
+			       (comint-line-beginning-position))))))
     (setq isearch-message-prefix-add "history ")
     (setq-local isearch-search-fun-function
                 #'comint-history-isearch-search)

Message #22 received at 30187 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 30187 <at> debbugs.gnu.org
Subject: Re: master a710f8a: * lisp/comint.el (comint-history-isearch-setup):
 Check if process is live.
Date: Wed, 07 Feb 2018 23:20:02 +0200

> Changing comint-history-isearch-setup this way seems too pervasive. I
> have a use case where (get-buffer-process (current-buffer)) is always
> nil. Is there another way to work around the issue in *Async Shell
> Command*? Thanks.

Before this fix the search was broken in *Async Shell Command*
and in inactive shells.

For example, try to set comint-history-isearch to ‘dwim’
and type ‘C-r’ in a *Async Shell Command* buffer.  It fails with

  Lisp error: (wrong-type-argument processp nil)
  process-mark(nil)
  comint-after-pmark-p()
  comint-history-isearch-setup()
  ...

Or even when comint-history-isearch is nil by default, run shell ‘M-x shell’,
then exit it, and after “Process shell finished” type ‘M-r’ and any letter
to search for it:

  Lisp error: (wrong-type-argument processp nil)
  process-mark(nil)
  comint-delete-input()
  comint-goto-input(nil)
  comint-history-isearch-pop-state
  ...

In these cases the history can't be searched because there is no active shell.
I wonder what use case do you need in inactive shells without a prompt,
so it's impossible to search in the history.  How you used to search
through the shell history without failing in ‘comint-goto-input’ like
in the backtrace above?

Message #25 received at 30187 <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 30187 <at> debbugs.gnu.org
Subject: Re: bug#30187: master a710f8a: * lisp/comint.el
 (comint-history-isearch-setup): Check if process is live.
Date: Thu, 08 Feb 2018 11:13:46 +0800

On 2018-02-07 23:20 +0200, Juri Linkov wrote:
> Before this fix the search was broken in *Async Shell Command*
> and in inactive shells.

Thanks. I tried the following examples and saw their failures.

>
> For example, try to set comint-history-isearch to ‘dwim’
> and type ‘C-r’ in a *Async Shell Command* buffer.  It fails with
>
>   Lisp error: (wrong-type-argument processp nil)
>   process-mark(nil)
>   comint-after-pmark-p()
>   comint-history-isearch-setup()
>   ...

It seems to make a lot of sense to have comint-after-pmark-p return nil
instead. WDYT?

> Or even when comint-history-isearch is nil by default, run shell ‘M-x shell’,
> then exit it, and after “Process shell finished” type ‘M-r’ and any letter
> to search for it:
>
>   Lisp error: (wrong-type-argument processp nil)
>   process-mark(nil)
>   comint-delete-input()
>   comint-goto-input(nil)
>   comint-history-isearch-pop-state
>   ...
>
> In these cases the history can't be searched because there is no active shell.
> I wonder what use case do you need in inactive shells without a prompt,
> so it's impossible to search in the history.  How you used to search
> through the shell history without failing in ‘comint-goto-input’ like
> in the backtrace above?

My use case is using a function (or in Erlang's lingo a light-weight
process) to communicate with a remote Erlang shell. So there is no
process as comint/emacs understands.

Leo

Message #28 received at 30187 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 30187 <at> debbugs.gnu.org
Subject: Re: bug#30187: master a710f8a: * lisp/comint.el
 (comint-history-isearch-setup): Check if process is live.
Date: Thu, 08 Feb 2018 23:29:03 +0200

[Message part 1 (text/plain, inline)]

>> For example, try to set comint-history-isearch to ‘dwim’
>> and type ‘C-r’ in a *Async Shell Command* buffer.  It fails with
>>
>>   Lisp error: (wrong-type-argument processp nil)
>>   process-mark(nil)
>>   comint-after-pmark-p()
>>   comint-history-isearch-setup()
>>   ...
>
> It seems to make a lot of sense to have comint-after-pmark-p return nil
> instead. WDYT?

Good idea.  The patch at the end of this message also always checks
if the prompt is empty at the end of the shell buffer to exclude
Async Shell Command buffers and inactive shells.

>> Or even when comint-history-isearch is nil by default, run shell ‘M-x shell’,
>> then exit it, and after “Process shell finished” type ‘M-r’ and any letter
>> to search for it:
>>
>>   Lisp error: (wrong-type-argument processp nil)
>>   process-mark(nil)
>>   comint-delete-input()
>>   comint-goto-input(nil)
>>   comint-history-isearch-pop-state
>>   ...
>>
>> In these cases the history can't be searched because there is no active shell.
>> I wonder what use case do you need in inactive shells without a prompt,
>> so it's impossible to search in the history.  How you used to search
>> through the shell history without failing in ‘comint-goto-input’ like
>> in the backtrace above?
>
> My use case is using a function (or in Erlang's lingo a light-weight
> process) to communicate with a remote Erlang shell. So there is no
> process as comint/emacs understands.

Nice, good to know, as these days I completely switched to Elixir.

[comint-after-pmark-p.patch (text/x-diff, inline)]

diff --git a/lisp/comint.el b/lisp/comint.el
index b4fbfc8..22fbe06 100644
--- a/lisp/comint.el
+++ b/lisp/comint.el
@@ -1448,17 +1448,18 @@ comint-history-isearch-message-overlay
 (defun comint-history-isearch-setup ()
   "Set up a comint for using Isearch to search the input history.
 Intended to be added to `isearch-mode-hook' in `comint-mode'."
-  (when (and (get-buffer-process (current-buffer))
+  (when (and
+             ;; Prompt is not empty like in Async Shell Command buffers
+             ;; or in inactive shells
+             (not (eq (save-excursion
+		        (goto-char (comint-line-beginning-position))
+		        (forward-line 0)
+		        (point))
+		      (comint-line-beginning-position)))
 	     (or (eq comint-history-isearch t)
 		 (and (eq comint-history-isearch 'dwim)
 		      ;; Point is at command line.
-		      (comint-after-pmark-p)
-		      ;; Prompt is not empty like in Async Shell Command buffers
-		      (not (eq (save-excursion
-				 (goto-char (comint-line-beginning-position))
-				 (forward-line 0)
-				 (point))
-			       (comint-line-beginning-position))))))
+		      (comint-after-pmark-p))))
     (setq isearch-message-prefix-add "history ")
     (setq-local isearch-search-fun-function
                 #'comint-history-isearch-search)
@@ -2288,8 +2289,10 @@ comint-skip-prompt
 
 (defun comint-after-pmark-p ()
   "Return t if point is after the process output marker."
-  (let ((pmark (process-mark (get-buffer-process (current-buffer)))))
-    (<= (marker-position pmark) (point))))
+  (let ((process (get-buffer-process (current-buffer))))
+    (when process
+      (let ((pmark (process-mark process)))
+        (<= (marker-position pmark) (point))))))
 
 (defun comint-simple-send (proc string)
   "Default function for sending to PROC input STRING.

Message #31 received at 30187 <at> debbugs.gnu.org (full text, mbox):

From: Leo Liu <sdl.web <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 30187 <at> debbugs.gnu.org
Subject: Re: bug#30187: master a710f8a: * lisp/comint.el
 (comint-history-isearch-setup): Check if process is live.
Date: Fri, 09 Feb 2018 10:45:02 +0800

On 2018-02-08 23:29 +0200, Juri Linkov wrote:
> Good idea.  The patch at the end of this message also always checks
> if the prompt is empty at the end of the shell buffer to exclude
> Async Shell Command buffers and inactive shells.

Thanks for taking this on. The patch looks good.

>> My use case is using a function (or in Erlang's lingo a light-weight
>> process) to communicate with a remote Erlang shell. So there is no
>> process as comint/emacs understands.
>
> Nice, good to know, as these days I completely switched to Elixir.

Good to know too. The BEAM is underappreciated.

Leo

Message #34 received at 30187 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 30187 <at> debbugs.gnu.org
Subject: Re: bug#30187: master a710f8a: * lisp/comint.el
 (comint-history-isearch-setup): Check if process is live.
Date: Sun, 11 Feb 2018 23:44:16 +0200

>> Good idea.  The patch at the end of this message also always checks
>> if the prompt is empty at the end of the shell buffer to exclude
>> Async Shell Command buffers and inactive shells.
>
> Thanks for taking this on. The patch looks good.

This is now pushed to master.  Thanks for the feedback.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.