isearch.el patch for `M-e' to put point at mismatch position

isearch.el patch for `M-e' to put point at mismatch position

[Drew Adams]

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

The attached patch makes `M-e' (`isearch-edit-string') put point at the first
mismatch position (or the search-string end if no mismatch).

When you hit `M-e' you are ready to edit at the mismatch position.  This makes
it easy to change or insert a char or two to make the string match.  (`C-g', on
the other hand just removes all of the mismatch.)

(This feature complements the use of highlighting to show users where the
mismatch starts.)

[-- Attachment #2: isearch-2011-05-16.patch --]
[-- Type: application/octet-stream, Size: 1968 bytes --]

diff -cw isearch.el isearch-patched-2011-05-16.el
*** isearch.el	Mon May 16 11:38:22 2011
--- isearch-patched-2011-05-16.el	Mon May 16 14:21:04 2011
***************
*** 1060,1065 ****
--- 1060,1080 ----
  
  (defvar minibuffer-history-symbol) ;; from external package gmhist.el
  
+ (defun isearch-fail-pos ()
+   "Position of first mismatch in search string, or its length if none."
+   (let ((cmds  isearch-cmds)
+         succ-msg)
+     (if (and isearch-success (not isearch-error))
+         (length isearch-message)
+       (while (or (not (isearch-success-state (car cmds)))
+                  (isearch-error-state (car cmds)))
+         (pop cmds))
+       (setq succ-msg  (and cmds (isearch-message-state (car cmds))))
+       (if (and (stringp succ-msg)  (< (length succ-msg) (length isearch-message))
+                (equal succ-msg (substring isearch-message 0 (length succ-msg))))
+           (length succ-msg)
+         0))))
+ 
  (defun isearch-edit-string ()
    "Edit the search string in the minibuffer.
  The following additional command keys are active while editing.
***************
*** 1139,1145 ****
  		(setq isearch-new-string
                        (read-from-minibuffer
                         (isearch-message-prefix nil nil isearch-nonincremental)
!                        isearch-string
                         minibuffer-local-isearch-map nil
                         (if isearch-regexp
  			   (cons 'regexp-search-ring
--- 1154,1160 ----
                   (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

Diff finished.  Mon May 16 14:23:06 2011

Re: isearch.el patch for `M-e' to put point at mismatch position

[Juri Linkov]

> The attached patch makes `M-e' (`isearch-edit-string') put point at the first
> mismatch position (or the search-string end if no mismatch).

Wouldn't this be too surprising for users?  Maybe there should be some
visual indication to explain why the cursor is in the middle of the
search string?  E.g. the same highlighting of the failed part with the
`isearch-fail' face.  Or more conveniently - activating the region on
the failed part, so the user can easily delete it with one keystroke.

RE: isearch.el patch for `M-e' to put point at mismatch position

[Drew Adams]

> > The attached patch makes `M-e' (`isearch-edit-string') put 
> > point at the first mismatch position (or the search-string
> > end if no mismatch).
> 
> Wouldn't this be too surprising for users?

No, I don't think so, or I wouldn't be using it (in isearch+.el), and I wouldn't
be proposing it here. ;-)

A user who hits `M-e' while looking at a search string with mismatch
highlighting will not be surprised to find the editing cursor positioned where
the highlighting began.

(This simply uses `read-from-minibuffer' with a cons INITIAL-CONTENTS argument.
Emacs doesn't take special measures when it does this.)

> Maybe there should be some
> visual indication to explain why the cursor is in the middle of the
> search string?  E.g. the same highlighting of the failed part with the
> `isearch-fail' face.

pro: For the reason you suppose.

con: It could give the impression that the highlighting is active (actively
updated), whereas it represents only a snapshot of the original state.  This
could be remedied by removing the highlighting as soon as any editing is done
(any change is made).  That would be OK.

But I really do not think it is needed.  The user just saw the highlighting,
before hitting `M-e'.

> Or more conveniently - activating the region on
> the failed part, so the user can easily delete it with one keystroke.

No, please.  C-k does the same thing, and it does not interfere, as activating
the region would.  And `C-g' after/before editing does the same thing.

There is no want of a quick way to remove the mismatched portion, once the
cursor is placed at the first mismatch position.  It is that positioning that
takes users time.  And it requires them to remember where the mismatch was.

FWIW, for a long time I did not move point to the mismatch position
automatically; I just bound `M-e', in the edit keymap, to let users move it
there on demand.  IOW, after `M-e' a second `M-e' moved point to the mismatch
position.

I used that approach for a long time, but I found this to be much better: (a) no
need to hit a key; (b) no need to remember which key does that; (c) I find I
want point positioned there 100% of the time.  For me it's a no-brainer.  But do
as you like.

Re: isearch.el patch for `M-e' to put point at mismatch position

[Stefan Monnier]

> The attached patch makes `M-e' (`isearch-edit-string') put point at the first
> mismatch position (or the search-string end if no mismatch).

That's a cute idea.  I like it.


        Stefan

RE: isearch.el patch for `M-e' to put point at mismatch position

[Drew Adams]

> > The attached patch makes `M-e' (`isearch-edit-string') put 
> > point at the first mismatch position (or the search-string
> > end if no mismatch).
> 
> That's a cute idea.  I like it.

Then how about adding it? ;-)

Re: isearch.el patch for `M-e' to put point at mismatch position

[Stefan Monnier]

> The attached patch makes `M-e' (`isearch-edit-string') put point at the first
> mismatch position (or the search-string end if no mismatch).

Installed.  In the future, please:
- include a changelog.
- stay within 80 columns.


        Stefan

Re: isearch.el patch for `M-e' to put point at mismatch position

[Juri Linkov]

> The attached patch makes `M-e' (`isearch-edit-string') put point at the first
> mismatch position (or the search-string end if no mismatch).
>
> When you hit `M-e' you are ready to edit at the mismatch position.  This makes
> it easy to change or insert a char or two to make the string match.  (`C-g', on
> the other hand just removes all of the mismatch.)
>
> (This feature complements the use of highlighting to show users where the
> mismatch starts.)
> 
> *** isearch.el	Mon May 16 11:38:22 2011
> --- isearch-patched-2011-05-16.el	Mon May 16 14:21:04 2011
> ***************
> *** 1060,1065 ****
> --- 1060,1080 ----
>   
>   (defvar minibuffer-history-symbol) ;; from external package gmhist.el
>   
> + (defun isearch-fail-pos ()
> +   "Position of first mismatch in search string, or its length if none."
> +   (let ((cmds  isearch-cmds)
> +         succ-msg)
> +     (if (and isearch-success (not isearch-error))
> +         (length isearch-message)
> +       (while (or (not (isearch-success-state (car cmds)))
> +                  (isearch-error-state (car cmds)))
> +         (pop cmds))
> +       (setq succ-msg  (and cmds (isearch-message-state (car cmds))))
> +       (if (and (stringp succ-msg)  (< (length succ-msg) (length isearch-message))
> +                (equal succ-msg (substring isearch-message 0 (length succ-msg))))
> +           (length succ-msg)
> +         0))))
> + 
>   (defun isearch-edit-string ()
>     "Edit the search string in the minibuffer.
>   The following additional command keys are active while editing.
> ***************
> *** 1139,1145 ****
>   		(setq isearch-new-string
>                         (read-from-minibuffer
>                          (isearch-message-prefix nil nil isearch-nonincremental)
> !                        isearch-string
>                          minibuffer-local-isearch-map nil
>                          (if isearch-regexp
>   			   (cons 'regexp-search-ring
> --- 1154,1160 ----
>                    (setq isearch-new-string
>                          (read-from-minibuffer
>                           (isearch-message-prefix nil nil isearch-nonincremental)
> !                         (cons isearch-string (1+ (isearch-fail-pos)))

I discovered now that you can't mix `isearch-message' and `isearch-string'.

Test case to reproduce the bug:

1. emacs -Q
2. insert a few TABs with e.g. `C-q TAB C-q TAB C-q TAB'
3. `M-<' (`beginning-of-buffer')
4. `C-s C-q TAB M-e'

fails with the error message "End of buffer".

The reason is that `isearch-message' is longer than `isearch-string',
because `isearch-text-char-description' replaces a single TAB
with two characters "^I" in `isearch-message'.

`isearch-fail-pos' calculates the position based on `isearch-message', but
uses it for `isearch-string' in `(cons isearch-string (1+ (isearch-fail-pos)))'
in `isearch-edit-string'.

Replacing `isearch-message' with `isearch-string' in `isearch-fail-pos'
will fix this bug.

BTW, there is exactly the same algorithm used in two functions
`isearch-fail-pos' and `isearch-message'.  Currently both functions
use the same variable `isearch-message', but after this fix the same algorithm
will use different variables `isearch-message' and `isearch-string'.
So sharing code between them will be more difficult.

Maybe it would be better to add a new argument to `isearch-fail-pos'
that will define whether it should return the failed position in
`isearch-message' or `isearch-string'?

RE: isearch.el patch for `M-e' to put point at mismatch position

[Drew Adams]

> > When you hit `M-e' you are ready to edit at the mismatch 
> > position.  This makes it easy to change or insert a char
> > or two to make the string match.  (`C-g', on
> > the other hand just removes all of the mismatch.)
> 
> I discovered now that you can't mix `isearch-message' and 
> `isearch-string'.
> Test case to reproduce the bug:
> 1. emacs -Q
> 2. insert a few TABs with e.g. `C-q TAB C-q TAB C-q TAB'
> 3. `M-<' (`beginning-of-buffer')
> 4. `C-s C-q TAB M-e'
> fails with the error message "End of buffer".

Good catch.

> The reason is that `isearch-message' is longer than `isearch-string',
> because `isearch-text-char-description' replaces a single TAB
> with two characters "^I" in `isearch-message'.
> 
> `isearch-fail-pos' calculates the position based on 
> `isearch-message', but uses it for `isearch-string' in
> `(cons isearch-string (1+ (isearch-fail-pos)))' in
> `isearch-edit-string'.
> 
> Replacing `isearch-message' with `isearch-string' in 
> `isearch-fail-pos' will fix this bug.

Sounds good.  Please go for it.

> BTW, there is exactly the same algorithm used in two functions
> `isearch-fail-pos' and `isearch-message'.

Well, not exactly, but close.

> Currently both functions use the same variable `isearch-message',
> but after this fix the same algorithm
> will use different variables `isearch-message' and `isearch-string'.
> So sharing code between them will be more difficult.

There's no real need to share code between them.

> Maybe it would be better to add a new argument to `isearch-fail-pos'
> that will define whether it should return the failed position in
> `isearch-message' or `isearch-string'?

That might be OK too, but I'd suggest just making your suggested change above:
replace `isearch-message' with `isearch-string' in `isearch-fail-pos'.

RE: isearch.el patch for `M-e' to put point at mismatch position

[Drew Adams]

> > Replacing `isearch-message' with `isearch-string' in 
> > `isearch-fail-pos' will fix this bug.
> 
> Sounds good.  Please go for it.

Actually, unless I'm missing something that doesn't work either.

emacs -Q
Insert a few tabs using C-q TAB.  Insert `w'.
M-<.
C-s C-q TAB k

Highlighting shows that the TAB char is matched but the `k' char is a mismatch.

Now hit M-e.  Instead of placing the cursor at the mismatch (just before the
`k'), it places it before the TAB, at the beginning of the search string.

(Without your fix, the cursor was placed after the `k', which is also
incorrect.)

Am I missing something?

Re: isearch.el patch for `M-e' to put point at mismatch position

[Juri Linkov]

> Actually, unless I'm missing something that doesn't work either.
>
> emacs -Q
> Insert a few tabs using C-q TAB.  Insert `w'.
> M-<.
> C-s C-q TAB k
>
> Highlighting shows that the TAB char is matched but the `k' char is a mismatch.
>
> Now hit M-e.  Instead of placing the cursor at the mismatch (just before the
> `k'), it places it before the TAB, at the beginning of the search string.
>
> (Without your fix, the cursor was placed after the `k', which is also
> incorrect.)
>
> Am I missing something?

Perhaps you forgot to replace `isearch-message-state' with `isearch-string-state'?

RE: isearch.el patch for `M-e' to put point at mismatch position

[Drew Adams]

> Perhaps you forgot to replace `isearch-message-state' with 
> `isearch-string-state'?

Yup.  Didn't forget, but I misunderstood your instructions.

Seems to work fine; thanks.

Re: isearch.el patch for `M-e' to put point at mismatch position

[Juri Linkov]

> Seems to work fine; thanks.

I fixed this with sharing code between `isearch-message' and `isearch-string'.
This was cleaner.