bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Dani Moncayo]

Hi,

I sometimes want to search for some text that I know is already in the
search ring, but I'm unsure if it is at the tip.

In these cases, I intuitively do "C-s M-p", expecting to get precisely
the last search string used, i.e., the tip of the search ring.  But I
fact M-p brings me the second entry.

IMO, "C-s M-p" should bring the _first_ (more recent) entry.


In GNU Emacs 24.0.50.1 (i386-mingw-nt5.1.2600)
 of 2011-07-25 on 3249CTO
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (4.5) --no-opt'

-- 
Dani Moncayo

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Dani Moncayo]

> IMO, "C-s M-p" should bring the _first_ (more recent) entry.

IOW, "C-s M-p <RET>" should be equivalent to "C-s C-s".


-- 
Dani Moncayo

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Deniz Dogan]

On 2011-07-28 10:18, Dani Moncayo wrote:
>> IMO, "C-s M-p" should bring the _first_ (more recent) entry.
>
> IOW, "C-s M-p<RET>" should be equivalent to "C-s C-s".
>

How do you suggest users go back in the ring?

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Dani Moncayo]

On Thu, Jul 28, 2011 at 10:20, Deniz Dogan <deniz@dogan.se> wrote:
>
> How do you suggest users go back in the ring?
>

??

To get older entries from the search ring, repeat "M-p" (as always).
Every additional "M-p" will bring an additional entry from the search
ring (as always).

What I say is that _the first_ time you do M-p, you should get the tip
of the ring, not the second entry (as happens now).


-- 
Dani Moncayo

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Deniz Dogan]

On 2011-07-28 10:43, Dani Moncayo wrote:
> On Thu, Jul 28, 2011 at 10:20, Deniz Dogan<deniz@dogan.se>  wrote:
>>
>> How do you suggest users go back in the ring?
>>
>
> ??
>
> To get older entries from the search ring, repeat "M-p" (as always).
> Every additional "M-p" will bring an additional entry from the search
> ring (as always).
>
> What I say is that _the first_ time you do M-p, you should get the tip
> of the ring, not the second entry (as happens now).
>

C-s C-s immediately searches for the latest entry in the ring.  If C-s 
M-p did that too, things would be...weird.  At least to me.  I wouldn't 
really mind that much if M-p brings back the latest entry but _doesn't_ 
perform the actual search, but it just feels kind of unnecessary since 
if you wanted to search for that you could just C-s C-s to begin with.

Deniz

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Dani Moncayo]

On Thu, Jul 28, 2011 at 10:50, Deniz Dogan <deniz@dogan.se> wrote:
>
> C-s C-s immediately searches for the latest entry in the ring.  If C-s M-p
> did that too, things would be...weird.  At least to me.  I wouldn't really
> mind that much if M-p brings back the latest entry but _doesn't_ perform the
> actual search,

You are forgetting to add <RET> after "C-s M-p".  I see no weirdness
here at all.  It feel totally natural to me:
* C-s start Isearch.
* M-p brings the *last* ring entry (not the second one, as does now).
* <RET> accepts the selected entry.


> but it just feels kind of unnecessary since if you wanted to
> search for that you could just C-s C-s to begin with.

"C-s C-s" is the quick way of repeating the last search, but, as I
explained in the OP, it doesn't allow you to check the search string
before starting the search.


Again: The fact that M-p brings the second entry from the ring
(instead of the first one) seems annoying and counter-intuitive to me.
 It doesn't allow me to check the tip of the search ring before using
it.

-- 
Dani Moncayo

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Deniz Dogan]

On 2011-07-28 11:08, Dani Moncayo wrote:
> On Thu, Jul 28, 2011 at 10:50, Deniz Dogan<deniz@dogan.se>  wrote:
>>
>> C-s C-s immediately searches for the latest entry in the ring.  If C-s M-p
>> did that too, things would be...weird.  At least to me.  I wouldn't really
>> mind that much if M-p brings back the latest entry but _doesn't_ perform the
>> actual search,
>
> You are forgetting to add<RET>  after "C-s M-p".  I see no weirdness
> here at all.  It feel totally natural to me:
> * C-s start Isearch.
> * M-p brings the *last* ring entry (not the second one, as does now).
> *<RET>  accepts the selected entry.
>

You never said to hit RET, that's why I was confused.

>> but it just feels kind of unnecessary since if you wanted to
>> search for that you could just C-s C-s to begin with.
>
> "C-s C-s" is the quick way of repeating the last search, but, as I
> explained in the OP, it doesn't allow you to check the search string
> before starting the search.
>
>
> Again: The fact that M-p brings the second entry from the ring
> (instead of the first one) seems annoying and counter-intuitive to me.
>   It doesn't allow me to check the tip of the search ring before using
> it.
>

That's true.  I'm with you on this one, but since this behavior is so 
old, I wouldn't count on it changing any time soon.

Deniz

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Dani Moncayo]

>>
>> You are forgetting to add<RET>  after "C-s M-p".  I see no weirdness
>> here at all.  It feel totally natural to me:
>> * C-s start Isearch.
>> * M-p brings the *last* ring entry (not the second one, as does now).
>> *<RET>  accepts the selected entry.
>>
>
> You never said to hit RET, that's why I was confused.

Sorry I said it.  Please check the thread:
   >> IOW, "C-s M-p <RET>" should be equivalent to "C-s C-s".


> That's true.  I'm with you on this one

Thanks!

>, but since this behavior is so old, I
> wouldn't count on it changing any time soon.

I hope that it wasn't an argument to prevent improving Emacs.

-- 
Dani Moncayo

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Andreas Schwab]

Deniz Dogan <deniz@dogan.se> writes:

> C-s C-s immediately searches for the latest entry in the ring.  If C-s 
> M-p did that too, things would be...weird.  At least to me.  I
> wouldn't really mind that much if M-p brings back the latest entry but
> _doesn't_ perform the actual search, but it just feels kind of
> unnecessary since if you wanted to search for that you could just C-s
> C-s to begin with.

You may want to edit it first.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Juri Linkov]

> IMO, "C-s M-p" should bring the _first_ (more recent) entry.

In http://lists.gnu.org/archive/html/emacs-devel/2004-07/msg00059.html
I submitted a patch to fix this.  But later I found a reason
why it's not a good change.  And now I forgot why ;(

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Juri Linkov]

>> IMO, "C-s M-p" should bring the _first_ (more recent) entry.
>
> In http://lists.gnu.org/archive/html/emacs-devel/2004-07/msg00059.html
> I submitted a patch to fix this.  But later I found a reason
> why it's not a good change.  And now I forgot why ;(

I found the explanation why this is a bad change ;)  Because to repeat the
last search, users type `C-s C-s', and to repeat the second last search,
users type `C-s M-p C-s' but with this change there are two redundant
keysequences to repeat the last search, `C-s C-s' and `C-s M-p C-s', and to
repeat the second last search, the keysequence is too long `C-s M-p M-p C-s'.

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Dani Moncayo]

On Thu, Jul 28, 2011 at 11:50, Juri Linkov <juri@jurta.org> wrote:
> I found the explanation why this is a bad change ;)  Because to repeat the
> last search, users type `C-s C-s', and to repeat the second last search,
> users type `C-s M-p C-s' but with this change there are two redundant
> keysequences to repeat the last search, `C-s C-s' and `C-s M-p C-s', and to
> repeat the second last search, the keysequence is too long `C-s M-p M-p C-s'.

Please, reconsider this.

I understand what you say, but IMO, clearly the advantages of the
current behavior (save one extra M-p) in no way makes up for the
drawbacks (counter intuitive behavior; no way to check the tip of the
search ring before using it).

TIA.


-- 
Dani Moncayo

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Andreas Schwab]

Dani Moncayo <dmoncayo@gmail.com> writes:

> I understand what you say, but IMO, clearly the advantages of the
> current behavior (save one extra M-p) in no way makes up for the
> drawbacks (counter intuitive behavior; no way to check the tip of the
> search ring before using it).

You can still type C-s M-p M-n to get the first entry for inspection.
It's a trade-off which is more important.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Dani Moncayo]

On Thu, Jul 28, 2011 at 13:13, Andreas Schwab <schwab@linux-m68k.org> wrote:
> Dani Moncayo <dmoncayo@gmail.com> writes:
>
>> I understand what you say, but IMO, clearly the advantages of the
>> current behavior (save one extra M-p) in no way makes up for the
>> drawbacks (counter intuitive behavior; no way to check the tip of the
>> search ring before using it).
>
> You can still type C-s M-p M-n to get the first entry for inspection.
> It's a trade-off which is more important.
>

Indeed.  (I realized it just after posting).

So what do people think it would be more convenient?

Isn't the requested behavior of M-p more intuitive?
Isn't it more consistent with the behavior of M-p in other contexts?
Isn't "M-p M-p" almost as quick to type than a single "M-p"?
...


-- 
Dani Moncayo

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Juri Linkov]

>>> I understand what you say, but IMO, clearly the advantages of the
>>> current behavior (save one extra M-p) in no way makes up for the
>>> drawbacks (counter intuitive behavior; no way to check the tip of the
>>> search ring before using it).

I agree that it's counter intuitive behavior.

>> You can still type C-s M-p M-n to get the first entry for inspection.
>> It's a trade-off which is more important.

To do the same, I use `C-s M-e M-p'.

> So what do people think it would be more convenient?

I don't mind the change.  If people agree with it,
I'll send a new patch that implements this change.

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Juri Linkov]

Severity: wishlist
Tags: patch

> So what do people think it would be more convenient?
>
> Isn't it more consistent with the behavior of M-p in other contexts?

Most importantly it will be more consistent with `C-s M-e M-p [M-p ...]'
The amount of typed M-p will be the same for the same previous search string.

So when people will agree, here is the patch:

Using parent branch file:///home/work/emacs/bzr/emacs/http-trunk/
=== modified file 'lisp/isearch.el'
--- lisp/isearch.el	2011-07-15 13:33:07 +0000
+++ lisp/isearch.el	2011-07-30 08:49:36 +0000
@@ -2071,7 +2086,7 @@ (defun isearch-ring-adjust1 (advance)
 	()
       (set yank-pointer-name
 	   (setq yank-pointer
-		 (mod (+ (or yank-pointer 0)
+		 (mod (+ (or yank-pointer (if advance 0 -1))
 			 (if advance -1 1))
 		      length)))
       (setq isearch-string (nth yank-pointer ring)

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Dani Moncayo]

> So when people will agree, here is the patch:

Stefan, Chong, Please, could you take a look at Juri's patch?  (It is
a fairly simple issue, despite the number of post in this thread).

TIA

-- 
Dani Moncayo

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Stefan Monnier]

>> So what do people think it would be more convenient?
>> Isn't it more consistent with the behavior of M-p in other contexts?

> Most importantly it will be more consistent with `C-s M-e M-p [M-p ...]'
> The amount of typed M-p will be the same for the same previous search string.
> So when people will agree, here is the patch:

I don't think that the inconsistency of the current behavior is
a serious problem.  There are already plenty of other inconsistencies
between the isearch behavior is typical minibuffer prompts, so it's not
like resolving this would make it all consistent.


        Stefan

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Dani Moncayo]

On Sat, Aug 13, 2011 at 17:24, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>>> So what do people think it would be more convenient?
>>> Isn't it more consistent with the behavior of M-p in other contexts?
>
>> Most importantly it will be more consistent with `C-s M-e M-p [M-p ...]'
>> The amount of typed M-p will be the same for the same previous search string.
>> So when people will agree, here is the patch:
>
> I don't think that the inconsistency of the current behavior is
> a serious problem.  There are already plenty of other inconsistencies
> between the isearch behavior is typical minibuffer prompts, so it's not
> like resolving this would make it all consistent.


Maybe there are other inconsistencies, so what?  Now we are talking
about this one.  Should not fixing it be a step forward?

Honestly, I'm pretty surprised about the resistance to do such a
trivial and logical improvement.

-- 
Dani Moncayo

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Stefan Monnier]

>>>> So what do people think it would be more convenient?
>>>> Isn't it more consistent with the behavior of M-p in other contexts?
>>> Most importantly it will be more consistent with `C-s M-e M-p [M-p
>>> ...]'  The amount of typed M-p will be the same for the same
>>> previous search string.  So when people will agree, here is the
>>> patch:
>> I don't think that the inconsistency of the current behavior is
>> a serious problem.  There are already plenty of other inconsistencies
>> between the isearch behavior is typical minibuffer prompts, so it's not
>> like resolving this would make it all consistent.
> Maybe there are other inconsistencies, so what?  Now we are talking
> about this one.  Should not fixing it be a step forward?
> Honestly, I'm pretty surprised about the resistance to do such a
> trivial and logical improvement.

As mentioned in the thread it's not an improvement for everyone.


        Stefan

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Dani Moncayo]

> As mentioned in the thread it's not an improvement for everyone.

???

Could you quote a sentence in the thread that supports such claim?

At the end, both Deniz and Jury agreed with me that it would be better
to make M-p bring the top of the search ring, and Andread Just pointed
out an alternative way to achieve that with the current Emacs.  So I
don't see any opposition for the change (except yours).

For God's sake... what do people expect M-p to do but get the _previous_ thing?

-- 
Dani Moncayo.  Feeling tired...

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Dani Moncayo]

Ping!

The current counter-intuitive behavior (which IMO should not have been
qualified as a feature, but as a bug) has been bugging me for some
time, as I'm sure will bug anyone that tries to navigate through the
search ring.

Juri, I guess you also want to get this fixed in the trunk.  Maybe if
you chime in, Stefan will see the light ;).  I still can't understand
his reluctance to fix this, and I seem to lack the credit/ability to
convince him.

TIA.

-- 
Dani Moncayo

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Juri Linkov]

> The current counter-intuitive behavior (which IMO should not have been
> qualified as a feature, but as a bug) has been bugging me for some
> time, as I'm sure will bug anyone that tries to navigate through the
> search ring.
>
> Juri, I guess you also want to get this fixed in the trunk.  Maybe if
> you chime in, Stefan will see the light ;).  I still can't understand
> his reluctance to fix this, and I seem to lack the credit/ability to
> convince him.

You exaggerate the importance of the problem.  When I noticed this small
inconsistency I still continued to use isearch without any inconvenience.
So the only reason to change it is for the consistency with `C-s M-e M-p'
that has its own problems.  The main problem is that when you retrieve
the previous search string with `C-s C-s', then typing `M-p' will not have
a visible effect, because `M-p' will retrieve the previous search string
and the same string is already used as the current search string.

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Dani Moncayo]

> You exaggerate the importance of the problem.  When I noticed this small
> inconsistency

The inconsistency may be small, but you agree that there exists.

> I still continued to use isearch without any inconvenience.

I think that a (small) inconsistency should provoke a (small) inconvenience.

> So the only reason to change it is for the consistency with `C-s M-e M-p'
> that has its own problems.  The main problem is that when you retrieve
> the previous search string with `C-s C-s', then typing `M-p' will not have
> a visible effect, because `M-p' will retrieve the previous search string
> and the same string is already used as the current search string.

If that is the resulting behavior, it is indeed a problem, but a small
one IMO.  Since `C-s C-s' retrieves the first entry from the search
ring, `M-p' should retrieve the _previous_ one, i.e., the second.

-- 
Dani Moncayo

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Juri Linkov]

> If that is the resulting behavior, it is indeed a problem, but a small
> one IMO.  Since `C-s C-s' retrieves the first entry from the search
> ring, `M-p' should retrieve the _previous_ one, i.e., the second.

This complicates the behavior of `M-p'.
Do you propose the following logic?

1. When the user starts Isearch with `C-s', then
   `M-p' should retrieve the _first_ entry from the search ring.

2. But when the user starts Isearch with `C-s C-s', then
   `M-p' should retrieve the _second_ entry from the search ring.

But what to do when the user starts Isearch with `C-s C-s', but
changes his mind and deletes it from the current search string,
i.e. types `C-s C-s DEL'.  What should `M-p' bring in this case?
The first or the second entry?

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Dani Moncayo]

> This complicates the behavior of `M-p'.

I don't know if the underlying implementation becomes a bit more
complex, but the resulting behavior is clearly better for the user
(IMO).

> Do you propose the following logic?
>
> 1. When the user starts Isearch with `C-s', then
>   `M-p' should retrieve the _first_ entry from the search ring.

Yes. This is obviously what the average user should expect.

> 2. But when the user starts Isearch with `C-s C-s', then
>   `M-p' should retrieve the _second_ entry from the search ring.

Yes too.  In this case, when the user types `M-p', s?he should want to
get the second entry, because the first one is already in effect.
IOW, `M-p' should always retrieve the _previous_ entry from the ring,
which in this case is the second one.

> But what to do when the user starts Isearch with `C-s C-s', but
> changes his mind and deletes it from the current search string,
> i.e. types `C-s C-s DEL'.  What should `M-p' bring in this case?
> The first or the second entry?

In this case, the user has discarded the first entry and then has
typed `M-p'.  Thus, TRT for `M-p' in this case is IMO to retrieve the
second entry.

The same question is applicable to the sequence `C-s M-p
<delete_the_entry_in_the_minibuffer> M-p'.  I've just tested this and
see that the current behavior is that, i.e., the last `M-p' retrieves
the second entry (which IMO is TRT).

-- 
Dani Moncayo

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Stefan Monnier]

A patch that makes it work like you describe (i.e. C-s M-p gets first,
and C-s C-s M-p gets the second) sounds OK, assuming the patch
is not too ugly.


        Stefan

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Juri Linkov]

> A patch that makes it work like you describe (i.e. C-s M-p gets first,
> and C-s C-s M-p gets the second) sounds OK, assuming the patch
> is not too ugly.

There are two places in isearch.el that reuse the last element of the
ring when the search string is empty.  One is `isearch-repeat' (used by
`C-s C-s') and another is `isearch-edit-string'.  The latter is questionable.
An old comment used to say:

    ;; This used to set the last search string,
    ;; but I think it is not right to do that here.
    ;; Only the string actually used should be saved.

It doesn't correspond to the actual code, so I deleted it.
I have no opinion how useless is to reuse the last element of the
ring after exiting from `isearch-edit-string' with empty input.
But with the existing functionality this place needs to adjust the ring.

So three diff hunks below fix the following cases:

1. in isearch-edit-string: C-s M-e RET M-p
2. in isearch-repeat: C-s C-s M-p
3. in isearch-ring-adjust1: C-s M-p

=== modified file 'lisp/isearch.el'
--- lisp/isearch.el	2011-07-15 13:33:07 +0000
+++ lisp/isearch.el	2011-08-23 09:51:16 +0000
@@ -1191,19 +1200,17 @@ (defun isearch-edit-string ()
 		  isearch-word isearch-new-word))
 
 	  ;; Empty isearch-string means use default.
-	  (if (= 0 (length isearch-string))
-	      (setq isearch-string (or (car (if isearch-regexp
-						regexp-search-ring
-					      search-ring))
-				       "")
-
-		    isearch-message
-		    (mapconcat 'isearch-text-char-description
-			       isearch-string ""))
-	    ;; This used to set the last search string,
-	    ;; but I think it is not right to do that here.
-	    ;; Only the string actually used should be saved.
-	    ))
+	  (when (= 0 (length isearch-string))
+	    (setq isearch-string (or (car (if isearch-regexp
+					      regexp-search-ring
+					    search-ring))
+				     "")
+
+		  isearch-message
+		  (mapconcat 'isearch-text-char-description
+			     isearch-string ""))
+	    ;; After taking the last element, adjust ring to previous one.
+	    (isearch-ring-adjust1 nil)))
 
 	;; This used to push the state as of before this C-s, but it adds
 	;; an inconsistent state where part of variables are from the
@@ -1290,7 +1297,9 @@ (defun isearch-repeat (direction)
 		  isearch-message
 		  (mapconcat 'isearch-text-char-description
 			     isearch-string "")
-		  isearch-case-fold-search isearch-last-case-fold-search))
+		  isearch-case-fold-search isearch-last-case-fold-search)
+	    ;; After taking the last element, adjust ring to previous one.
+	    (isearch-ring-adjust1 nil))
 	;; If already have what to search for, repeat it.
 	(or isearch-success
 	    (progn
@@ -2071,7 +2080,7 @@ (defun isearch-ring-adjust1 (advance)
 	()
       (set yank-pointer-name
 	   (setq yank-pointer
-		 (mod (+ (or yank-pointer 0)
+		 (mod (+ (or yank-pointer (if advance 0 -1))
 			 (if advance -1 1))
 		      length)))
       (setq isearch-string (nth yank-pointer ring)

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Stefan Monnier]

> So three diff hunks below fix the following cases:

> 1. in isearch-edit-string: C-s M-e RET M-p
> 2. in isearch-repeat: C-s C-s M-p
> 3. in isearch-ring-adjust1: C-s M-p

That looks OK.


        Stefan

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Juri Linkov]

>> So three diff hunks below fix the following cases:
>
>> 1. in isearch-edit-string: C-s M-e RET M-p
>> 2. in isearch-repeat: C-s C-s M-p
>> 3. in isearch-ring-adjust1: C-s M-p
>
> That looks OK.

Installed.

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Dani Moncayo]

Hi Juri,

I've tested your patch and it works fine for the three cases you said:

> 1. in isearch-edit-string: C-s M-e RET M-p
> 2. in isearch-repeat: C-s C-s M-p
> 3. in isearch-ring-adjust1: C-s M-p

But I've found another use-case that seems to need a fix too:
0. C-s a <RET> C-s b <RET> C-s c <RET>
1. C-s C-s     --> "c" is selected, OK.
2. M-p <RET>   --> "b" is selected, OK.
3. M-p         --> Now "c" (the previous used entry) should have been
selected, but I see that "b" (the current entry) remains selected
instead.

Do you see the same behavior?

-- 
Dani Moncayo

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Juri Linkov]

> But I've found another use-case that seems to need a fix too:
> 0. C-s a <RET> C-s b <RET> C-s c <RET>
> 1. C-s C-s     --> "c" is selected, OK.
> 2. M-p <RET>   --> "b" is selected, OK.
> 3. M-p         --> Now "c" (the previous used entry) should have been
> selected, but I see that "b" (the current entry) remains selected
> instead.

This is a separate problem caused when exiting from the minibuffer in
`isearch-edit-string' by `read-from-minibuffer' (i.e. step 2
`M-p <RET>' above) adds a new element "b" to the history (search ring).

I agree that `isearch-edit-string' should not update the search ring.
It should be updated only by `isearch-update-ring' in `isearch-done'.
So e.g. `C-s C-s M-e RET C-g' or `C-s M-p RET C-g' should keep the
search ring unchanged.  Note also that `isearch-done' with the arg
NOPUSH=t already tries to take care of not updating the search ring,
but unsuccessfully.  It requires the following patch for the complete fix:

Using parent branch file:///home/work/emacs/bzr/emacs/http-trunk/
=== modified file 'lisp/isearch.el'
--- lisp/isearch.el	2011-07-15 13:33:07 +0000
+++ lisp/isearch.el	2011-08-24 15:43:01 +0000
@@ -1152,9 +1161,14 @@ (defun isearch-edit-string ()
 
 	  (unwind-protect
 	      (let* ((message-log-max nil)
+		     ;; Protect the global search ring from updating
+		     ;; by read-from-minibuffer.  It should be updated only
+		     ;; by isearch-update-ring in isearch-done.
+		     (search-ring search-ring)
+		     (regexp-search-ring regexp-search-ring)
 		     ;; Binding minibuffer-history-symbol to nil is a work-around
 		     ;; for some incompatibility with gmhist.
 		     (minibuffer-history-symbol))
 		(setq isearch-new-string
                       (read-from-minibuffer
                        (isearch-message-prefix nil nil isearch-nonincremental)
		        (cons isearch-string (1+ (isearch-fail-pos)))
                        minibuffer-local-isearch-map nil
                        (if isearch-regexp
 			   (cons 'regexp-search-ring

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Dani Moncayo]

> It requires the following patch for the complete fix:

Great Juri, your patch fixes the last problem ;).

However, on further testing, I've found still another misbehavior:
0. C-s a <RET> C-s b <RET> C-s c <RET>
1. C-s C-s     --> "c" is selected, OK.
2. M-e <RET>   --> "c" remains selected (the search ring is unchanged:
"a;b;c"), OK.
3. M-p         --> Now "b" (the previous used entry) should have been
selected, but I see that "c" (the current entry) remains selected
instead.


-- 
Dani Moncayo

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Juri Linkov]

> However, on further testing, I've found still another misbehavior:
> 0. C-s a <RET> C-s b <RET> C-s c <RET>
> 1. C-s C-s     --> "c" is selected, OK.
> 2. M-e <RET>   --> "c" remains selected (the search ring is unchanged:
> "a;b;c"), OK.
> 3. M-p         --> Now "b" (the previous used entry) should have been
> selected, but I see that "c" (the current entry) remains selected
> instead.

So you are asking that after `isearch-edit-string' indexes of the
search ring should remain unchanged.  This is accomplished by this patch:

=== modified file 'lisp/isearch.el'
--- lisp/isearch.el	2011-07-15 13:33:07 +0000
+++ lisp/isearch.el	2011-08-24 18:27:33 +0000
@@ -1131,6 +1140,9 @@ (defun isearch-edit-string ()
 	      (isearch-recursive-edit isearch-recursive-edit)
 	      ;; Save current configuration so we can restore it here.
 	      (isearch-window-configuration (current-window-configuration))
+	      ;; Save index of the search ring.
+	      (search-ring-yank-pointer search-ring-yank-pointer)
+	      (regexp-search-ring-yank-pointer regexp-search-ring-yank-pointer)
 
 	      ;; Temporarily restore `minibuffer-message-timeout'.
 	      (minibuffer-message-timeout
@@ -1152,9 +1164,14 @@ (defun isearch-edit-string ()
 
 	  (unwind-protect
 	      (let* ((message-log-max nil)
+		     ;; Protect the global search ring from updating
+		     ;; by read-from-minibuffer.  It should be updated only
+		     ;; by isearch-update-ring in isearch-done.
+		     (search-ring search-ring)
+		     (regexp-search-ring regexp-search-ring)
 		     ;; Binding minibuffer-history-symbol to nil is a work-around
 		     ;; for some incompatibility with gmhist.
 		     (minibuffer-history-symbol))
 		(setq isearch-new-string
                       (read-from-minibuffer
                        (isearch-message-prefix nil nil isearch-nonincremental)
		       (cons isearch-string (1+ (isearch-fail-pos)))
                        minibuffer-local-isearch-map nil
                        (if isearch-regexp
 			   (cons 'regexp-search-ring

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Dani Moncayo]

Hi Juri,

I think we are very close to the final/complete solution ;).

I've just applied all your patches, an tested again all use-cases.
All work very well now, except this one:

0. C-s a <RET> C-s b <RET> C-s c <RET>
1. C-s M-e <RET>    --> "c" is selected, OK [*].
2. M-p              --> Now "b" (the previous entry) should have been
selected, but I see that "c" (the current entry) remains selected
instead.



[*] FWIW: This is indeed the current behavior (use the last entry if
the minibuffer is empty), but I don't find it very suitable.  If the
user wanted to use the last entry, s?he would have typed M-p.
Therefore, IMO TRT here would be to return to Isearch with an empty
string.

-- 
Dani Moncayo

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Dani Moncayo]

Another failing use-case:

0. C-s a <RET> C-s b <RET> C-s c <RET> C-s d <RET>
1. C-s C-s        --> "d" is selected (ok).
2. M-p M-p <RET>  --> "b" is selected (ok).
3. M-p            --> "a" should have been selected, but I see that
"b" remains selected instead.

-- 
Dani Moncayo

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Juri Linkov]

> 0. C-s a <RET> C-s b <RET> C-s c <RET>
> 1. C-s M-e <RET>    --> "c" is selected, OK [*].
> 2. M-p              --> Now "b" (the previous entry) should have been
> selected, but I see that "c" (the current entry) remains selected
> instead.

This can be easily fixed by moving `isearch-ring-adjust1' out of
let-binding of `search-ring-yank-pointer'.  But I think we should start
from the top of the search ring after `M-e RET' because we can't reliably
update the index.  See more below.

> [*] FWIW: This is indeed the current behavior (use the last entry if
> the minibuffer is empty), but I don't find it very suitable.  If the
> user wanted to use the last entry, s?he would have typed M-p.
> Therefore, IMO TRT here would be to return to Isearch with an empty
> string.

I suppose the last entry is used because otherwise an empty search
string makes no sense.  I have nothing against this functionality
because I never exit from the M-e minibuffer with empty input.

> Another failing use-case:
> 0. C-s a <RET> C-s b <RET> C-s c <RET> C-s d <RET>
> 1. C-s C-s        --> "d" is selected (ok).
> 2. M-p M-p <RET>  --> "b" is selected (ok).
> 3. M-p            --> "a" should have been selected, but I see that
> "b" remains selected instead.

We can't reliably count the number of typed M-p in `read-from-minibuffer'
in `isearch-edit-string' to adjust the index accordingly and to add the
same number of typed M-p to the index.  There is no reliable way to do this:
we could try to find the input string in the history (search ring) and
add its index to `search-ring-yank-pointer', but what to do when the user
types the same string that exists in the history - then the index
in the search ring will be incorrect.

Since `read-from-minibuffer' can't return the exact number of typed `M-p'
then I think that M-p after `M-e M-p M-p <RET>' should start from the
top of of the search ring, so `C-s C-s M-p M-p RET M-p' should select "d".
This will provide a predictable result.

OTOH, when `search-ring-update' is non-nil, `isearch-edit-string' is not used,
so everything works correctly.

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Dani Moncayo]

>> [*] FWIW: This is indeed the current behavior (use the last entry if
>> the minibuffer is empty), but I don't find it very suitable.  If the
>> user wanted to use the last entry, s?he would have typed M-p.
>> Therefore, IMO TRT here would be to return to Isearch with an empty
>> string.
>
> I suppose the last entry is used because otherwise an empty search
> string makes no sense.

It makes sense, I think: by "to set an empty search string" I mean to
return to the same state that had at the very beginning the Isearch
session, i.e., as if the user had deleted the current search string by
typing <DEL> repeatedly.

> I have nothing against this functionality
> because I never exit from the M-e minibuffer with empty input.

Neither me.  So this is not a very important issue (at least to us),
and we can leave it for now.  But I believe that setting an empty
search string would be TRT in that case.

>> Another failing use-case:
>> 0. C-s a <RET> C-s b <RET> C-s c <RET> C-s d <RET>
>> 1. C-s C-s        --> "d" is selected (ok).
>> 2. M-p M-p <RET>  --> "b" is selected (ok).
>> 3. M-p            --> "a" should have been selected, but I see that
>> "b" remains selected instead.
>
> We can't reliably count the number of typed M-p in `read-from-minibuffer'
> in `isearch-edit-string' to adjust the index accordingly and to add the
> same number of typed M-p to the index.  There is no reliable way to do this:

As you know, I still lack the knowledge of Elisp to help you in
implementation details, so I can say very little about this.  But I
think I should be possible to implement, since the intended behavior
is well defined and relatively simple:
* At the beggining of the Isearch session, the index would start at
the top of the ring (as does now).
* During the Isearch session, each typed M-p/M-n should use the index
to retrieve the suitable entry, and then should update the index,
regardless of whether we are in the minibuffer or outside it.

IMO, this is the behaviour most users would expect.  But if you find
it difficult to implement, you can adopt (by now) the alternative
behavior you've pointed out:

> Since `read-from-minibuffer' can't return the exact number of typed `M-p'
> then I think that M-p after `M-e M-p M-p <RET>' should start from the
> top of of the search ring, so `C-s C-s M-p M-p RET M-p' should select "d".
> This will provide a predictable result.

Since this is a different issue than the original one of this bug
report, I could file a separate bug asking for the wanted behavior.
WDYT?

-- 
Dani Moncayo

bug#9185: 24.0.50; "C-s M-p" does not bring the tip of the search ring

[Juri Linkov]

>> But I've found another use-case that seems to need a fix too:
>> 0. C-s a <RET> C-s b <RET> C-s c <RET>
>> 1. C-s C-s     --> "c" is selected, OK.
>> 2. M-p <RET>   --> "b" is selected, OK.
>> 3. M-p         --> Now "c" (the previous used entry) should have been
>> selected, but I see that "b" (the current entry) remains selected
>> instead.
>
> This is a separate problem caused when exiting from the minibuffer in
> `isearch-edit-string' by `read-from-minibuffer' (i.e. step 2
> `M-p <RET>' above) adds a new element "b" to the history (search ring).
>
> I agree that `isearch-edit-string' should not update the search ring.
> It should be updated only by `isearch-update-ring' in `isearch-done'.
> So e.g. `C-s C-s M-e RET C-g' or `C-s M-p RET C-g' should keep the
> search ring unchanged.  Note also that `isearch-done' with the arg
> NOPUSH=t already tries to take care of not updating the search ring,
> but unsuccessfully.  It requires the following patch for the complete fix:

I noticed now that `isearch-edit-string' should not add new elements
to the search ring, but should allow old elements to be deleted
from the search ring with a command like `delete-history-element'
(published in http://thread.gmane.org/gmane.emacs.devel/24353/focus=25269)
So I installed another fix that let-binds `history-add-new-input' to nil.