bug#7534: 24.0.50; G-g within Isearch regexp mode

bug#7534: 24.0.50; G-g within Isearch regexp mode

[Dani Moncayo]

Recipe:
0) Start Emacs (-Q)
1) Place point at the beginning of the *scratch* buffer.
2) Search for the regexp "iss" (C-M-s iss) --> The last `s' is
unmatched as expected, OK.
3) Type C-g --> The unmatched part disappears. Everything OK so far.
4) Now repeat steps #2 and #3 with the regexp "is[". --> This time C-g
behaves differently. Instead of deleting the unmatched part, it ends
Isearch mode.


The fail seems to appear whenever `[' is the first character in the
unmatched part. For instance:
 * C-M-s iss[ C-g --> Works as expected (unmatched part deleted).
 * C-M-s is[blablabla --> Doesn't work as expected (Isearch cancelled).


-- 
Dani Moncayo

bug#7534: 24.0.50; G-g within Isearch regexp mode

[Juri Linkov]

> Recipe:
> 0) Start Emacs (-Q)
> 1) Place point at the beginning of the *scratch* buffer.
> 2) Search for the regexp "iss" (C-M-s iss) --> The last `s' is
> unmatched as expected, OK.
> 3) Type C-g --> The unmatched part disappears. Everything OK so far.
> 4) Now repeat steps #2 and #3 with the regexp "is[". --> This time C-g
> behaves differently. Instead of deleting the unmatched part, it ends
> Isearch mode.
>
> The fail seems to appear whenever `[' is the first character in the
> unmatched part. For instance:
>  * C-M-s iss[ C-g --> Works as expected (unmatched part deleted).
>  * C-M-s is[blablabla --> Doesn't work as expected (Isearch cancelled).

IMO, this is consistent behavior.  As the message says after typing `['
it's incomplete input, so C-g behaves exactly like if there is no input
for `[' (and an unfinished sequence of characters that follows it).

So the main principle here is that "incomplete input" means "no input"
for Isearch, and given this rule all your examples work as expected, i.e.

"C-M-s is[ C-g" works like "C-M-s is C-g"
"C-M-s iss[ C-g" works like "C-M-s iss C-g"
"C-M-s is[blablabla C-g" works like "C-M-s is C-g"

bug#7534: 24.0.50; G-g within Isearch regexp mode

[Dani Moncayo]

On Thu, Dec 2, 2010 at 23:31, Juri Linkov <juri@jurta.org> wrote:
[...]
>
> IMO, this is consistent behavior.  As the message says after typing `['
> it's incomplete input, so C-g behaves exactly like if there is no input
> for `[' (and an unfinished sequence of characters that follows it).
>
> So the main principle here is that "incomplete input" means "no input"
> for Isearch, and given this rule all your examples work as expected, i.e.
>
> "C-M-s is[ C-g" works like "C-M-s is C-g"
> "C-M-s iss[ C-g" works like "C-M-s iss C-g"
> "C-M-s is[blablabla C-g" works like "C-M-s is C-g"
>

Thanks for thinking on this, Juri.

Indeed, the current behaviour seems to be like you mentioned
("incomplete input" means "no input"), but IMO that doesn't fit well
user's needs. If the user is searching for "is[blablabla", and
suddenly changes his(her) mind, I think C-g should allow to get rid of
the unmatched part.

So, IMO, the main principle for C-g (within Isearch mode) should be:
* if there is unmatched and/or incomplete input --> Delete it.
* Otherwise --> Exit Isearch mode.

-- 
Dani Moncayo

bug#7534: 24.0.50; G-g within Isearch regexp mode

[Juri Linkov]

> So, IMO, the main principle for C-g (within Isearch mode) should be:
> * if there is unmatched and/or incomplete input --> Delete it.
> * Otherwise --> Exit Isearch mode.

Please try the following patch.  Does it provide the behavior
you are asking for?

=== modified file 'lisp/isearch.el'
--- lisp/isearch.el	2010-10-02 22:37:21 +0000
+++ lisp/isearch.el	2010-12-05 23:08:19 +0000
@@ -1253,7 +1253,7 @@ (defun isearch-abort ()
   (interactive)
 ;;  (ding)  signal instead below, if quitting
   (discard-input)
-  (if isearch-success
+  (if (and isearch-success (not isearch-error))
       ;; If search is successful, move back to starting point
       ;; and really do quit.
       (progn

bug#7534: 24.0.50; G-g within Isearch regexp mode

[Dani Moncayo]

On Mon, Dec 6, 2010 at 00:11, Juri Linkov <juri@jurta.org> wrote:
>> So, IMO, the main principle for C-g (within Isearch mode) should be:
>> * if there is unmatched and/or incomplete input --> Delete it.
>> * Otherwise --> Exit Isearch mode.
>
> Please try the following patch.  Does it provide the behavior
> you are asking for?
>

It does. Thank you.

PS: The patch bellow is like your's, but it also updates the comment
accordingly.


=== modified file 'lisp/isearch.el'
--- lisp/isearch.el	2010-10-16 14:11:06.000000000 +0200
+++ lisp/isearch.el	2010-12-06 11:27:33.000000000 +0100
@@ -1244,9 +1244,9 @@
   (interactive)
 ;;  (ding)  signal instead below, if quitting
   (discard-input)
-  (if isearch-success
-      ;; If search is successful, move back to starting point
-      ;; and really do quit.
+  (if (and isearch-success (not isearch-error))
+      ;; If search is successful and there is no incomplete regexp,
+      ;; move back to starting point and really do quit.
       (progn
         (setq isearch-success nil)
         (isearch-cancel))




-- 
Dani Moncayo

bug#7534: 24.0.50; G-g within Isearch regexp mode

[Juri Linkov]

>>> So, IMO, the main principle for C-g (within Isearch mode) should be:
>>> * if there is unmatched and/or incomplete input --> Delete it.
>>> * Otherwise --> Exit Isearch mode.
>>
>> Please try the following patch.  Does it provide the behavior
>> you are asking for?
>
> It does. Thank you.

I think the behavior of Isearch that Dani proposes is reasonable because
it seems this is how `isearch-abort' was supposed to work since its
comment says "If search has an incomplete regexp" but this branch currently
is never reached.  So I'd like to install it to the trunk.

bug#7534: 24.0.50; G-g within Isearch regexp mode

[Dani Moncayo]

On Sat, Dec 25, 2010 at 03:47, Juri Linkov <juri@jurta.org> wrote:
>
> I think the behavior of Isearch that Dani proposes is reasonable because
> it seems this is how `isearch-abort' was supposed to work since its
> comment says "If search has an incomplete regexp" but this branch currently
> is never reached.  So I'd like to install it to the trunk.
>

This is a fairly simple change. Please, could a maintainer take a look
to the proposed patch?

TIA.

-- 
Dani Moncayo

bug#7534: C-g in Isearch regexp mode

[Dani Moncayo]

>On Sat, Dec 25, 2010 at 03:47, Juri Linkov <juri <at> jurta.org> wrote:
>>
>> I think the behavior of Isearch that Dani proposes is reasonable because
>> it seems this is how `isearch-abort' was supposed to work since its
>> comment says "If search has an incomplete regexp" but this branch currently
>> is never reached.  So I'd like to install it to the trunk.
>>

> This is a fairly simple change. Please, could a maintainer take a look
> to the proposed patch?

Hello? Is anyone out there?   ;-)

-- 
Dani Moncayo

bug#7534: C-g in Isearch regexp mode

[Stefan Monnier]

>>> I think the behavior of Isearch that Dani proposes is reasonable because
>>> it seems this is how `isearch-abort' was supposed to work since its
>>> comment says "If search has an incomplete regexp" but this branch currently
>>> is never reached.  So I'd like to install it to the trunk.

I haven't had time to dig into it and understand the details, but
I'll trust Juri on this one.


        Stefan

bug#7534: C-g in Isearch regexp mode

[Juri Linkov]

>>>> I think the behavior of Isearch that Dani proposes is reasonable because
>>>> it seems this is how `isearch-abort' was supposed to work since its
>>>> comment says "If search has an incomplete regexp" but this branch currently
>>>> is never reached.  So I'd like to install it to the trunk.
>
> I haven't had time to dig into it and understand the details, but
> I'll trust Juri on this one.

Installed to the trunk.

bug#7534: what about committing the patch also to emacs-23?

[Dani Moncayo]

Hi,

Given that the patch for this bug was tiny and inoffensive, what do
you think about commiting it also in the emacs-23 branch (before the
23.3 release)?

TIA

--
Dani Moncayo

bug#7534: what about committing the patch also to emacs-23?

[Stefan Monnier]

> Given that the patch for this bug was tiny and inoffensive, what do
> you think about commiting it also in the emacs-23 branch (before the
> 23.3 release)?

We expect to be able to ship 23.3 *real soon now*, so we only want to
commit there fixes for regressions now.


        Stefan

bug#7534: what about committing the patch also to emacs-23?

[Dani Moncayo]

On Fri, Feb 4, 2011 at 15:40, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>> Given that the patch for this bug was tiny and inoffensive, what do
>> you think about commiting it also in the emacs-23 branch (before the
>> 23.3 release)?
>
> We expect to be able to ship 23.3 *real soon now*, so we only want to
> commit there fixes for regressions now.
>

mmm... so I've arrived a bit late. It is a pity, because IMO emacs
23.3 would be a little bit better with that bug fixed, but I
understand you.

Thanks anyway.


-- 
Dani Moncayo