bug#22118: 23.2; Hitting ^W in a search selects the wrong word.

bug#22118: 23.2; Hitting ^W in a search selects the wrong word.

[jms]

To reproduce this bug create a file that ends in the last 7 lines in
this message (it will work if you open this message in emacs).

Put the cursor at the first ''aap'' (line 2) hit ^S^W^W (''aap_noot'' at
line 2, now is highlighted). Hit ^S^S^S (Failing the I-search since the last
''aap_noot'' (at line 6) has been reached, call this 'last-occurrence' state).

Now hit ^W^W^W^W . Somewhere the state of 'last-occurrence' and add more
to the search string conflict, because it adds ''_aap'' for every ^W and
the edit window is not being updated.

Note that if you are not in 'last-occurrence' state, there is no
problem, even at the last line. This bug only manifests itself if ^S has
been hit until it shows "Failing the I-search" in the minibuffer.

Regards,

Jan-Mark

1 miesmiesmies
2 aap_noot_mies
3 miesmiesmies
4 aap_noot_does
5 miesmiesmies
6 aap_noot_aap
7 miesmiesmies

bug#22118: 23.2; Hitting ^W in a search selects the wrong word.

[Alan Mackenzie]

Hello, Jan-Mark.

In article <mailman.1705.1449600970.31583.bug-gnu-emacs@gnu.org> you wrote:

> To reproduce this bug create a file that ends in the last 7 lines in
> this message (it will work if you open this message in emacs).

> Put the cursor at the first ''aap'' (line 2) hit ^S^W^W (''aap_noot'' at
> line 2, now is highlighted). Hit ^S^S^S (Failing the I-search since the last
> ''aap_noot'' (at line 6) has been reached, call this 'last-occurrence' state).

> Now hit ^W^W^W^W . Somewhere the state of 'last-occurrence' and add more
> to the search string conflict, because it adds ''_aap'' for every ^W and
> the edit window is not being updated.

I can confirm this behaviour is still present in the latest development
sources.

> Note that if you are not in 'last-occurrence' state, there is no
> problem, even at the last line. This bug only manifests itself if ^S has
> been hit until it shows "Failing the I-search" in the minibuffer.

Why is this behaviour a bug?  I think that the effect of ^W in
"last-occurrence" state is probably undefined in the manual.  Adding
"_aap" to the search string for each ^W does give the user feedback that
the ^W has actually been received and processed.

What do you think should happen in these circumstances?  Does the current
behaviour actually give you problems?

> Regards,

> Jan-Mark

> 1 miesmiesmies
> 2 aap_noot_mies
> 3 miesmiesmies
> 4 aap_noot_does
> 5 miesmiesmies
> 6 aap_noot_aap
> 7 miesmiesmies

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#22118: 23.2; Hitting ^W in a search selects the wrong word.

[Alan Mackenzie]

Hello, Jan-Mark.

On Thu, Dec 10, 2015 at 11:01:39AM +0100, Jan-Mark wrote:
> Hi Alan,

> Thanks for your swift response and all the work you do on my favorite
> editor. People like you improve my life.

Thanks, that's appreciated.  Just one small request: when you reply,
would you please leave the Cc: to the bug archive.  That way, there's a
full record of the bug on the public list.

> >> Note that if you are not in 'last-occurrence' state, there is no
> >> problem, even at the last line. This bug only manifests itself if ^S has
> >> been hit until it shows "Failing the I-search" in the minibuffer.

> > Why is this behaviour a bug?  I think that the effect of ^W in
> > "last-occurrence" state is probably undefined in the manual.  Adding
> > "_aap" to the search string for each ^W does give the user feedback that
> > the ^W has actually been received and processed.

> Depending on how you define "bug". :-) Adding "_aap" is definitely what
> you would expect.

> There are three problems IMHO that make this a "bug":

> 1) The screen is inconsistent. The minibuffer shows a "aap_noot_aap" but
> the text buffer window's highlighting is not updated, only "aap_noot" is
> highlighted.

Don't forget, that by this stage the text in the minibuffer indicates
"not found".  So the main window highlights as much text as was found,
the minibuffer indicates the current search string, and any lazy
highlighting shows where the current search string would be found
earlier on.

All of this _is_ in the manual, on page "Errors in Incremental Search".

> 2) Hitting ^W again will add an other "_aap" (consistent with the end of
> the highlighting) which is unexpected to say the least. The minibuffer
> will contain "aap_noot_aap_aap" which is nowhere in the text.

This is true: the minibuffer shows what you are searching for.  This
indeed is nowhere in the text, but it's what you're searching for.

> 3) Not hitting ^W again, but ^S will wrap the search around looking for
> what is stated in the minibuffer ("aap_noot_aap") not what is
> highlighted in the text buffer window ("aap_noot"). The highlighting
> however is correct again ("aap_noot_aap").

The search is ALWAYS for what's in the minibuffer.  Once you hit ^S
again, the search becomes wrapped, and is no longer failing: it finds
"aap_noot_aap".

> > What do you think should happen in these circumstances?  Does the current
> > behaviour actually give you problems?

> Depending on how you define "problems". :-P However, there are two
> different ways emacs could deal with this that would definitely improve
> the ease of understanding and ease of use.

> a) The most consistent behavior, IMHO, would be to update the
> highlighting in the text buffer window. This way the the text buffer and
> mini buffer will be consistent. This way I don't have to check both to
> find out what is going on.

But when the search has failed, they should be different: in the main
window, you can sea the bit that matched, and in the minibuffer, in
bright red, you can see the bit that didn't match.  I think you're
asking for the distinction between successful and failing searches to be
removed.  This would surely be a Bad Thing.

> b) What would help me (personally) more would be if emacs would go back
> into search mode, highlight the whole search text in the text buffer
> window and remove the "Failing I-search:" from the mini buffer. The
> rational behind this is that changing the search string kind of "resets"
> the search status to the "default".

Internally, Isearch does indeed do this: it searches for the complete
string from the starting point each time you type a new character.  But
if it has already failed, it's going to stay failed unless something
like a C-s make the search wrap.

> I understand that if an I-search has failed extending the search string
> would keep you in the same search state (I.e., the last match.) However,
> by that reasoning hitting ^S should also not change the state. So I vote
> for behavior b) but I could live with behavior a).

> However I strongly feel, the mini buffer and the text buffer should be
> consistent.

They are the same as long as a search hasn't yet failed.  If it has, the
minibuffer shows what you're searching for, the text buffer what you
have found.

> If I type ^S until I find the last entry, hitting ^W appends
> "_aap" and updates the text buffer window. If I type ^S until I find the
> last entry, hit ^S again (going into Failing I-search) hitting ^W also
> appends "_aap" but does not update the text buffer window.

With the best will in the world, I honestly don't think there's a bug
here.  Could I ask you to have another look at that manual page ("Errors
in Incremental Search" in the Emacs manual) and carefully compare what
you read there with what you see on the screen.

By the way, Emacs 23.2 is now pretty old.  You might want to consider
upgrading to a newer version.  The current released version is 24.5.

> Regards,

> Jan-Mark

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#22118: 23.2; Hitting ^W in a search selects the wrong word.

[Jan-Mark]

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

> With the best will in the world, I honestly don't think there's a bug
> here.  Could I ask you to have another look at that manual page ("Errors
> in Incremental Search" in the Emacs manual) and carefully compare what
> you read there with what you see on the screen.
Ok, we disagree. Probably  you define "bug" as something that cannot be
made to fit the description.

The net effect is that at some point typing ^W will keep appending the
same word over and over. I would say, either update the text window too
or disallow using ^W after a failure. I prefer the former, preferably in
combination with a rest of the failure, just like ^S does.

> By the way, Emacs 23.2 is now pretty old.  You might want to consider
> upgrading to a newer version.  The current released version is 24.5.
Unfortunately it comes with the version of my OS X, and upgrading leads
to all kinds of trouble when I try to use regular (Apple) updates.

Thanks for your energy. I still hold this is unexpected and unwanted
behavior if not a bug, but I respect your different opinion and will
learn to live with it.

I guess, if I want it my way, I should do what you do and donate my time
for improving emacs. :-) Thanks again, btw. :-D

Regards,

--
Jan-Mark



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

bug#22118: 23.2; Hitting ^W in a search selects the wrong word.

[Juri Linkov]

> I would say, either update the text window too or disallow using ^W
> after a failure.

I agree that the current behavior is not ideal.  The problem is that
it's difficult to make it more intuitive to work with different
workflows such as when failing not at the end of the buffer
but due to a non-existent string, e.g. typing ‘zzz C-w C-w C-w’.
Please try this patch that takes into account such possible scenarios.

However, I don't agree this is a bug, I think it just provides a more
useful behavior, so perhaps it shouldn't be installed to emacs-25.

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 66fab0e..e9a99ea 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -1959,6 +1959,8 @@ (defun isearch-mouse-2 (click)
       (when (functionp binding)
 	(call-interactively binding)))))
 
+(defvar isearch-yank-prev-point nil)
+
 (defun isearch-yank-internal (jumpform)
   "Pull the text from point to the point reached by JUMPFORM.
 JUMPFORM is a lambda expression that takes no arguments and returns
@@ -1969,7 +1971,14 @@ (defun isearch-yank-internal (jumpform)
    (save-excursion
      (and (not isearch-forward) isearch-other-end
 	  (goto-char isearch-other-end))
-     (buffer-substring-no-properties (point) (funcall jumpform)))))
+     (and (not isearch-success) isearch-yank-prev-point
+	  (goto-char isearch-yank-prev-point))
+     (buffer-substring-no-properties
+      (point)
+      (prog1
+	  (setq isearch-yank-prev-point (funcall jumpform))
+	(when isearch-success
+	  (setq isearch-yank-prev-point nil)))))))
 
 (defun isearch-yank-char-in-minibuffer (&optional arg)
   "Pull next character from buffer into end of search string in minibuffer."

bug#22118: 23.2; Hitting ^W in a search selects the wrong word.

[Stefan Kangas]

Hi Juri,

Is the below patch still relevant?

Juri Linkov <juri@linkov.net> writes:

>> I would say, either update the text window too or disallow using ^W
>> after a failure.
>
> I agree that the current behavior is not ideal.  The problem is that
> it's difficult to make it more intuitive to work with different
> workflows such as when failing not at the end of the buffer
> but due to a non-existent string, e.g. typing ‘zzz C-w C-w C-w’.
> Please try this patch that takes into account such possible scenarios.
>
> However, I don't agree this is a bug, I think it just provides a more
> useful behavior, so perhaps it shouldn't be installed to emacs-25.
>
> diff --git a/lisp/isearch.el b/lisp/isearch.el
> index 66fab0e..e9a99ea 100644
> --- a/lisp/isearch.el
> +++ b/lisp/isearch.el
> @@ -1959,6 +1959,8 @@ (defun isearch-mouse-2 (click)
>        (when (functionp binding)
>  	(call-interactively binding)))))
>
> +(defvar isearch-yank-prev-point nil)
> +
>  (defun isearch-yank-internal (jumpform)
>    "Pull the text from point to the point reached by JUMPFORM.
>  JUMPFORM is a lambda expression that takes no arguments and returns
> @@ -1969,7 +1971,14 @@ (defun isearch-yank-internal (jumpform)
>     (save-excursion
>       (and (not isearch-forward) isearch-other-end
>  	  (goto-char isearch-other-end))
> -     (buffer-substring-no-properties (point) (funcall jumpform)))))
> +     (and (not isearch-success) isearch-yank-prev-point
> +	  (goto-char isearch-yank-prev-point))
> +     (buffer-substring-no-properties
> +      (point)
> +      (prog1
> +	  (setq isearch-yank-prev-point (funcall jumpform))
> +	(when isearch-success
> +	  (setq isearch-yank-prev-point nil)))))))
>
>  (defun isearch-yank-char-in-minibuffer (&optional arg)
>    "Pull next character from buffer into end of search string in minibuffer."

Best regards,
Stefan Kangas

bug#22118: 23.2; Hitting ^W in a search selects the wrong word.

[Juri Linkov]

> Is the below patch still relevant?

I don't know.  My patch tried to make the behavior in case of error
slightly more useful, but now I see nothing useful in it because
after a failure it makes no sense to type C-w C-w C-w ...

I don't recommend making code in isearch.el more complicated to handle
useless cases.  So I won't complain if you'll close this report.  :-)

>>> I would say, either update the text window too or disallow using ^W
>>> after a failure.
>>
>> I agree that the current behavior is not ideal.  The problem is that
>> it's difficult to make it more intuitive to work with different
>> workflows such as when failing not at the end of the buffer
>> but due to a non-existent string, e.g. typing ‘zzz C-w C-w C-w’.
>> Please try this patch that takes into account such possible scenarios.
>>
>> However, I don't agree this is a bug, I think it just provides a more
>> useful behavior, so perhaps it shouldn't be installed to emacs-25.

bug#22118: 23.2; Hitting ^W in a search selects the wrong word.

[Stefan Kangas]

Juri Linkov <juri@linkov.net> writes:

>> Is the below patch still relevant?
>
> I don't know.  My patch tried to make the behavior in case of error
> slightly more useful, but now I see nothing useful in it because
> after a failure it makes no sense to type C-w C-w C-w ...
>
> I don't recommend making code in isearch.el more complicated to handle
> useless cases.  So I won't complain if you'll close this report.  :-)

OK, thanks.  Any objections to closing this?

Best regards,
Stefan Kangas

bug#22118: 23.2; Hitting ^W in a search selects the wrong word.

[Jan-Mark]

> OK, thanks.  Any objections to closing this?
I don't remember even what this was about. But ^W should not add words at the cursor if the
search failed to match. Which is different from the search failing because it is at the last
match (about to warp to the first match again).

If the word 'aapX' is not in my text, 'aap noot' is, and I search for 'aapX' it brings me to
'aap noot' with a failure notice. If I than type ^W it should not start looking for 'aapX noot'.
Just take the ^W out of the game in cases like this. I mean if 'aapX' cannot be found (even
once) why should you allow people to look for 'aapX noot'?

I still feel it is a bug, but I agree that it will not hit many people often. I have this once
every 1000 hours (estimate). So it's not a big bug. But a bug nonetheless.

If there is bigger bugs to fry, close it, if it is an easy fix (should be?) I'd say fix it first.

Regards,

-- 
Jan-Mark

bug#22118: 23.2; Hitting ^W in a search selects the wrong word.

[Stefan Kangas]

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

Juri Linkov <juri@linkov.net> writes:

>> Is the below patch still relevant?
>
> I don't know.  My patch tried to make the behavior in case of error
> slightly more useful, but now I see nothing useful in it because
> after a failure it makes no sense to type C-w C-w C-w ...
>
> I don't recommend making code in isearch.el more complicated to handle
> useless cases.  So I won't complain if you'll close this report.  :-)

Jan-Mark <jms@codersco.com> writes:

>> OK, thanks.  Any objections to closing this?
> I don't remember even what this was about. But ^W should not add words at the cursor if the
> search failed to match. Which is different from the search failing because it is at the last
> match (about to warp to the first match again).
>
> If the word 'aapX' is not in my text, 'aap noot' is, and I search for 'aapX' it brings me to
> 'aap noot' with a failure notice. If I than type ^W it should not start looking for 'aapX noot'.
> Just take the ^W out of the game in cases like this. I mean if 'aapX' cannot be found (even
> once) why should you allow people to look for 'aapX noot'?
>
> I still feel it is a bug, but I agree that it will not hit many people often. I have this once
> every 1000 hours (estimate). So it's not a big bug. But a bug nonetheless.
>
> If there is bigger bugs to fry, close it, if it is an easy fix (should be?) I'd say fix it first.

Thanks.

Juri feels that we should close this bug as a wontfix because it is not
worth complicating the isearch code over this.

Testing this again, I'm leaning more towards fixing the bug.  I think
there is definitely a bug here (the recipe in OP is still reproducible
on master), but it is very minor.  We also already have a proposed fix.

I considered also the use case when you have 'isearch-wrapped' set to a
non-nil value, which would make yank after last occurrence more useful.
I do remember having seen stuff similar to this in the past.
(Unfortunately I no longer use isearch and can't remember the exact
details, or if it was this exact bug.)

I therefore propose to push the proposed changes to master.  The
complexity is localized to only one function, which is already in itself
not too complicated.  I've attached a patch with proper ChangeLog for
review.

Juri, if you are still not convinced, we can go ahead and close this as
wontfix.  I'm okay with either option.

Best regards,
Stefan Kangas

[-- Attachment #2: 0001-Fix-isearch-yank-on-last-occurrence-of-search-string.patch --]
[-- Type: text/x-diff, Size: 1578 bytes --]

From c5ebd3ee6c2c14727d926aef5d8e36682a00bb59 Mon Sep 17 00:00:00 2001
From: Juri Linkov <juri@linkov.net>
Date: Sat, 29 Aug 2020 18:33:05 +0200
Subject: [PATCH] Fix isearch yank on last occurrence of search string

* lisp/isearch.el (isearch-yank-internal):
(isearch-yank-prev-point): Fix yanking when we are at the last
occurrence of a search string.  (Bug#22118)
---
 lisp/isearch.el | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 81e83d7950..ac8b48e43a 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -2518,6 +2518,8 @@ isearch-xterm-paste
     (let ((pasted-text (nth 1 event)))
       (isearch-yank-string pasted-text))))
 
+(defvar isearch-yank-prev-point nil)
+
 (defun isearch-yank-internal (jumpform)
   "Pull the text from point to the point reached by JUMPFORM.
 JUMPFORM is a lambda expression that takes no arguments and returns
@@ -2528,7 +2530,14 @@ isearch-yank-internal
    (save-excursion
      (and (not isearch-forward) isearch-other-end
 	  (goto-char isearch-other-end))
-     (buffer-substring-no-properties (point) (funcall jumpform)))))
+     (and (not isearch-success) isearch-yank-prev-point
+	  (goto-char isearch-yank-prev-point))
+     (buffer-substring-no-properties
+      (point)
+      (prog1
+	  (setq isearch-yank-prev-point (funcall jumpform))
+	(when isearch-success
+	  (setq isearch-yank-prev-point nil)))))))
 
 (defun isearch-yank-char-in-minibuffer (&optional arg)
   "Pull next character from buffer into end of search string in minibuffer."
-- 
2.28.0

bug#22118: 23.2; Hitting ^W in a search selects the wrong word.

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> I therefore propose to push the proposed changes to master.  The
> complexity is localized to only one function, which is already in itself
> not too complicated.  I've attached a patch with proper ChangeLog for
> review.

Yes, I tried the patch, and the fixes this somewhat obscure problem, so
I went ahead and applied it to Emacs 28.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no