bug#21663: Subject: 25.0.50; isearch-edit-string dont resume multi isearches

bug#21663: Subject: 25.0.50; isearch-edit-string dont resume multi isearches

[Tino Calancha]

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


Start fresh session:
emacs -Q

C-x C-b
M-x multi-isearch-buffer RET *scratch* RET *Buffer List* RET RET s
(Using C-s several times will search "s" on the two buffers).

M-s e
(Now C-s should keep searching the same string "s" in the two buffers,
  but the string is searched just in the current buffer).

[-- Attachment #2: Type: text/plain, Size: 730 bytes --]

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 4fc9b38..5440be7 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -1086,7 +1086,10 @@ isearch-done
       ;; Update the ring data.
       (isearch-update-ring isearch-string isearch-regexp))
 
-  (let ((isearch-mode-end-hook-quit (and nopush (not edit))))
+  (let ((isearch-mode-end-hook-quit (and nopush (not edit)))
+        (isearch-mode-end-hook      (if (and nopush edit multi-isearch-buffer-list)
+                                        (delq 'multi-isearch-end isearch-mode-end-hook)
+                                      isearch-mode-end-hook)))
     (run-hooks 'isearch-mode-end-hook))
 
   ;; If there was movement, mark the starting position.

bug#21663: 25.0.50; isearch-edit-string dont resume multi-isearch-files

[Tino Calancha]

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


Previous patch would not work for `multi-isearch-files'
because it is not checking `multi-isearch-file-list', so that
`multi-isearch-end' would be called.

We may replace
(if (and nopush edit multi-isearch-buffer-list)
with
(if (and nopush edit (or multi-isearch-buffer-list multi-isearch-file-list))
but just
(if (and nopush edit)
wold work in any situation and its shorter.


[-- Attachment #2: Type: text/plain, Size: 705 bytes --]

gdiff --git a/lisp/isearch.el b/lisp/isearch.el
index 4fc9b38..6107bd5 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -1086,7 +1086,10 @@ isearch-done
       ;; Update the ring data.
       (isearch-update-ring isearch-string isearch-regexp))
 
-  (let ((isearch-mode-end-hook-quit (and nopush (not edit))))
+  (let ((isearch-mode-end-hook-quit (and nopush (not edit)))
+        (isearch-mode-end-hook      (if (and nopush edit)
+                                        (delq 'multi-isearch-end isearch-mode-end-hook)
+                                      isearch-mode-end-hook)))
     (run-hooks 'isearch-mode-end-hook))
 
   ;; If there was movement, mark the starting position.

bug#21663: 25.0.50; isearch-edit-string dont resume multi-isearch-files

[Juri Linkov]

>> Start fresh session:
>> emacs -Q
>>
>> C-x C-b
>> M-x multi-isearch-buffer RET *scratch* RET *Buffer List* RET RET s
>> (Using C-s several times will search "s" on the two buffers).
>>
>> M-s e
>> (Now C-s should keep searching the same string "s" in the two buffers,
>>  but the string is searched just in the current buffer).
>
> Previous patch would not work for `multi-isearch-files'
> because it is not checking `multi-isearch-file-list', so that
> `multi-isearch-end' would be called.
>
> We may replace
> (if (and nopush edit multi-isearch-buffer-list)
> with
> (if (and nopush edit (or multi-isearch-buffer-list multi-isearch-file-list))
> but just
> (if (and nopush edit)
> wold work in any situation and its shorter.

I wonder why ‘isearch-mode-end-hook-quit’ is not initialized as
‘(and nopush edit)’ in the first place?  If it was so, then
you could just check for it in ‘multi-isearch-end’.

But please note that not finishing multi-isearch correctly has such
side effects as e.g. after ‘M-s e’ switching from the minibuffer
to another buffer and starting another ordinary search in it will still
use multi-isearch, etc.  Instead of not finishing multi-isearch,
have you tried to re-initialize multi-isearch on resuming isearch
after exiting from the minibuffer?  (This could be achieved by
remembering its state variables).

bug#21663: 25.0.50; isearch-edit-string dont resume multi-isearch-files

[Tino Calancha]

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


> I wonder why ‘isearch-mode-end-hook-quit’ is not initialized as
> ‘(and nopush edit)’ in the first place?  If it was so, then
> you could just check for it in ‘multi-isearch-end’.

My understanding about that variable (i may be wrong) is that is should
be non nil just when the user quit the search with
C-g
we dont want this variable being non nil in the case of editing the string
with the idea to resume the same kind of search.

> have you tried to re-initialize multi-isearch on resuming isearch
> after exiting from the minibuffer?  (This could be achieved by
> remembering its state variables).

I like your proposal: this way is more consistent with the original code.
See attached new patch.

[-- Attachment #2: Type: text/plain, Size: 993 bytes --]

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 4fc9b38..74b1250 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -1242,6 +1242,14 @@ with-isearch-suspended
 	      (isearch-adjusted isearch-adjusted)
 	      (isearch-yank-flag isearch-yank-flag)
 	      (isearch-error isearch-error)
+              ;;  multi isearch variables
+	      (multi-isearch-file-list multi-isearch-file-list)
+	      (multi-isearch-buffer-list multi-isearch-buffer-list)
+	      (multi-isearch-next-buffer-current-function multi-isearch-next-buffer-current-function)
+	      (multi-isearch-current-buffer multi-isearch-current-buffer)
+	      (isearch-push-state-function isearch-push-state-function)
+	      (isearch-wrap-function isearch-wrap-function)
+	      (isearch-search-fun-function isearch-search-fun-function)
   ;;; Don't bind this.  We want isearch-search, below, to set it.
   ;;; And the old value won't matter after that.
   ;;;	    (isearch-other-end isearch-other-end)

bug#21663: 25.0.50; isearch-edit-string dont resume multi isearches

[Tino Calancha]

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


In previous patch (isearch_3.patch) the variables are not
restored because they were bind to the same symbol name, and
multi-isearch-end will set them to nil.

In addition, multi-isearch-end need to be added again to 
isearch-mode-end-hook, otherwise, even after exiting with C-g, C-s will 
start a multisearch instead of a normal isearch in the current buffer.

See attached patch: isearch_4.patch

[-- Attachment #2: Type: text/plain, Size: 2034 bytes --]

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 4fc9b38..617f3e7 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -1242,6 +1242,15 @@ with-isearch-suspended
 	      (isearch-adjusted isearch-adjusted)
 	      (isearch-yank-flag isearch-yank-flag)
 	      (isearch-error isearch-error)
+              ;; multi isearch variables
+              (is-multi-isearch (or multi-isearch-buffer-list multi-isearch-file-list))
+	      (multi-isearch-file-list-new multi-isearch-file-list)
+	      (multi-isearch-buffer-list-new multi-isearch-buffer-list)
+ 	      (multi-isearch-next-buffer-current-function-new multi-isearch-next-buffer-current-function)
+	      (multi-isearch-current-buffer-new multi-isearch-current-buffer)
+	      (isearch-push-state-function isearch-push-state-function)
+	      (isearch-wrap-function isearch-wrap-function)
+	      (isearch-search-fun-function isearch-search-fun-function)
   ;;; Don't bind this.  We want isearch-search, below, to set it.
   ;;; And the old value won't matter after that.
   ;;;	    (isearch-other-end isearch-other-end)
@@ -1298,12 +1307,17 @@ with-isearch-suspended
 			  nil
 			  isearch-word)
 
+            (when is-multi-isearch (add-hook 'isearch-mode-end-hook  'multi-isearch-end))
 	    ;; Copy new local values to isearch globals
 	    (setq isearch-string isearch-new-string
 		  isearch-message isearch-new-message
 		  isearch-forward isearch-new-forward
 		  isearch-word isearch-new-word
-		  isearch-case-fold-search isearch-new-case-fold))
+		  isearch-case-fold-search isearch-new-case-fold
+                  multi-isearch-file-list multi-isearch-file-list-new
+                  multi-isearch-buffer-list multi-isearch-buffer-list-new
+                  multi-isearch-current-buffer multi-isearch-current-buffer-new
+                  multi-isearch-next-buffer-current-function multi-isearch-next-buffer-current-function-new))
 
 	  ;; Empty isearch-string means use default.
 	  (when (= 0 (length isearch-string))

bug#21663: 25.0.50; isearch-edit-string dont resume multi isearches

[Juri Linkov]

> In previous patch (isearch_3.patch) the variables are not
> restored because they were bind to the same symbol name, and
> multi-isearch-end will set them to nil.

Thanks, it's a safer approach to restore them explicitly.

> In addition, multi-isearch-end need to be added again to
> isearch-mode-end-hook, otherwise, even after exiting with C-g, C-s will
> start a multisearch instead of a normal isearch in the current buffer.

To avoid the need to add multi-isearch-end to isearch-mode-end-hook,
you could try to restore saved variables before calling isearch-mode.
Then multi-isearch-setup called from isearch-mode should take care of
adding multi-isearch-end to isearch-mode-end-hook when necessary
variables are already restored, i.e. when you save the variable
multi-isearch-next-buffer-function instead of
multi-isearch-next-buffer-current-function.  But OTOH
multi-isearch-current-buffer still needs to be restored after
isearch-mode in the same place where you already added it in your patch.
This way restarting isearch will emulate binding local variables
multi-isearch-next-buffer-function and multi-isearch-buffer-list in
multi-isearch-buffers before starting multi-buffer search.

bug#21663: 25.0.50; isearch-edit-string dont resume multi isearches

[Tino Calancha]

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


Thank you Juri for your help on this.

I have tested the attached patch (isearch-5.patch)
with `multi-isearch-buffers'/`multi-isearch-files': it seems
OK now.

< To avoid the need to add multi-isearch-end to isearch-mode-end-hook,
< you could try to restore saved variables before calling isearch-mode.
< Then multi-isearch-setup called from isearch-mode should take care of
< adding multi-isearch-end to isearch-mode-end-hook when necessary
< variables are already restored, i.e. when you save the variable
< multi-isearch-next-buffer-function instead of
< multi-isearch-next-buffer-current-function.
I got the point. Actually I bind multi-isearch-next-buffer-function
to multi-isearch-next-buffer-current-function, because the former was nil
and multi-isearch-setup makes a non-nil test for this variable at the
beginning


< multi-isearch-current-buffer still needs to be restored after
< isearch-mode in the same place where you already added it in your patch.
OK.

[-- Attachment #2: Type: text/plain, Size: 1373 bytes --]

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 4fc9b38..4c7187b 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -1242,6 +1242,11 @@ with-isearch-suspended
 	      (isearch-adjusted isearch-adjusted)
 	      (isearch-yank-flag isearch-yank-flag)
 	      (isearch-error isearch-error)
+
+	      (multi-isearch-file-list-new multi-isearch-file-list)
+	      (multi-isearch-buffer-list-new multi-isearch-buffer-list)
+              (multi-isearch-next-buffer-function multi-isearch-next-buffer-current-function)
+	      (multi-isearch-current-buffer-new multi-isearch-current-buffer)
   ;;; Don't bind this.  We want isearch-search, below, to set it.
   ;;; And the old value won't matter after that.
   ;;;	    (isearch-other-end isearch-other-end)
@@ -1303,7 +1308,10 @@ with-isearch-suspended
 		  isearch-message isearch-new-message
 		  isearch-forward isearch-new-forward
 		  isearch-word isearch-new-word
-		  isearch-case-fold-search isearch-new-case-fold))
+		  isearch-case-fold-search isearch-new-case-fold
+                  multi-isearch-current-buffer multi-isearch-current-buffer-new
+                  multi-isearch-file-list multi-isearch-file-list-new
+                  multi-isearch-buffer-list multi-isearch-buffer-list-new))
 
 	  ;; Empty isearch-string means use default.
 	  (when (= 0 (length isearch-string))

bug#21663: 25.0.50; isearch-edit-string dont resume multi isearches

[Juri Linkov]

Thank you Tino, I have tested your patch and it works OK,
so please commit it.

bug#21663: 25.0.50; isearch-edit-string dont resume multi isearches

[Juri Linkov]

> Lars Ingebrigtsen wrote:
>
>> Thanks; I've now applied this to the trunk.  Seems to be working great,
>> and it's a feature I've missed for years.
>
> Was a copyright assignment ever completed?
> I don't see one on file.
>
> http://debbugs.gnu.org/21684#14
>
> Juri Linkov wrote:
>
>> Have you already signed FSF copyright assignment papers?
>>
>> Your patch for bug#21663 is short enough to be installed without the
>> copyright paperwork, but your changes for bug#21684 require it.

Thanks for the reminder.  This is now installed (required merging
with other changes in the same function).