all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* isearch.el patch for `M-e' to put point at mismatch position
@ 2011-05-16 21:36 Drew Adams
  2011-05-16 22:14 ` Juri Linkov
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Drew Adams @ 2011-05-16 21:36 UTC (permalink / raw)
  To: emacs-devel

[-- 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: isearch.el patch for `M-e' to put point at mismatch position
  2011-05-16 21:36 isearch.el patch for `M-e' to put point at mismatch position Drew Adams
@ 2011-05-16 22:14 ` Juri Linkov
  2011-05-16 22:37   ` Drew Adams
  2011-05-17  3:08 ` Stefan Monnier
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: Juri Linkov @ 2011-05-16 22:14 UTC (permalink / raw)
  To: Drew Adams; +Cc: emacs-devel

> 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.



^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: isearch.el patch for `M-e' to put point at mismatch position
  2011-05-16 22:14 ` Juri Linkov
@ 2011-05-16 22:37   ` Drew Adams
  0 siblings, 0 replies; 12+ messages in thread
From: Drew Adams @ 2011-05-16 22:37 UTC (permalink / raw)
  To: 'Juri Linkov'; +Cc: emacs-devel

> > 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.




^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: isearch.el patch for `M-e' to put point at mismatch position
  2011-05-16 21:36 isearch.el patch for `M-e' to put point at mismatch position Drew Adams
  2011-05-16 22:14 ` Juri Linkov
@ 2011-05-17  3:08 ` Stefan Monnier
  2011-05-27 20:54   ` Drew Adams
  2011-05-28  1:27 ` Stefan Monnier
  2011-09-10 11:54 ` Juri Linkov
  3 siblings, 1 reply; 12+ messages in thread
From: Stefan Monnier @ 2011-05-17  3:08 UTC (permalink / raw)
  To: Drew Adams; +Cc: emacs-devel

> 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



^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: isearch.el patch for `M-e' to put point at mismatch position
  2011-05-17  3:08 ` Stefan Monnier
@ 2011-05-27 20:54   ` Drew Adams
  0 siblings, 0 replies; 12+ messages in thread
From: Drew Adams @ 2011-05-27 20:54 UTC (permalink / raw)
  To: 'Stefan Monnier'; +Cc: emacs-devel

> > 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? ;-)




^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: isearch.el patch for `M-e' to put point at mismatch position
  2011-05-16 21:36 isearch.el patch for `M-e' to put point at mismatch position Drew Adams
  2011-05-16 22:14 ` Juri Linkov
  2011-05-17  3:08 ` Stefan Monnier
@ 2011-05-28  1:27 ` Stefan Monnier
  2011-09-10 11:54 ` Juri Linkov
  3 siblings, 0 replies; 12+ messages in thread
From: Stefan Monnier @ 2011-05-28  1:27 UTC (permalink / raw)
  To: Drew Adams; +Cc: emacs-devel

> 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



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: isearch.el patch for `M-e' to put point at mismatch position
  2011-05-16 21:36 isearch.el patch for `M-e' to put point at mismatch position Drew Adams
                   ` (2 preceding siblings ...)
  2011-05-28  1:27 ` Stefan Monnier
@ 2011-09-10 11:54 ` Juri Linkov
  2011-09-10 16:13   ` Drew Adams
  3 siblings, 1 reply; 12+ messages in thread
From: Juri Linkov @ 2011-09-10 11:54 UTC (permalink / raw)
  To: Drew Adams; +Cc: emacs-devel

> 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'?



^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: isearch.el patch for `M-e' to put point at mismatch position
  2011-09-10 11:54 ` Juri Linkov
@ 2011-09-10 16:13   ` Drew Adams
  2011-09-10 16:22     ` Drew Adams
  0 siblings, 1 reply; 12+ messages in thread
From: Drew Adams @ 2011-09-10 16:13 UTC (permalink / raw)
  To: 'Juri Linkov'; +Cc: emacs-devel

> > 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'.




^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: isearch.el patch for `M-e' to put point at mismatch position
  2011-09-10 16:13   ` Drew Adams
@ 2011-09-10 16:22     ` Drew Adams
  2011-09-12 11:28       ` Juri Linkov
  0 siblings, 1 reply; 12+ messages in thread
From: Drew Adams @ 2011-09-10 16:22 UTC (permalink / raw)
  To: 'Juri Linkov'; +Cc: emacs-devel

> > 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?




^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: isearch.el patch for `M-e' to put point at mismatch position
  2011-09-10 16:22     ` Drew Adams
@ 2011-09-12 11:28       ` Juri Linkov
  2011-09-12 14:58         ` Drew Adams
  0 siblings, 1 reply; 12+ messages in thread
From: Juri Linkov @ 2011-09-12 11:28 UTC (permalink / raw)
  To: Drew Adams; +Cc: emacs-devel

> 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'?



^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: isearch.el patch for `M-e' to put point at mismatch position
  2011-09-12 11:28       ` Juri Linkov
@ 2011-09-12 14:58         ` Drew Adams
  2011-09-14 16:08           ` Juri Linkov
  0 siblings, 1 reply; 12+ messages in thread
From: Drew Adams @ 2011-09-12 14:58 UTC (permalink / raw)
  To: 'Juri Linkov'; +Cc: emacs-devel

> 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.




^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: isearch.el patch for `M-e' to put point at mismatch position
  2011-09-12 14:58         ` Drew Adams
@ 2011-09-14 16:08           ` Juri Linkov
  0 siblings, 0 replies; 12+ messages in thread
From: Juri Linkov @ 2011-09-14 16:08 UTC (permalink / raw)
  To: Drew Adams; +Cc: emacs-devel

> Seems to work fine; thanks.

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



^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2011-09-14 16:08 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-05-16 21:36 isearch.el patch for `M-e' to put point at mismatch position Drew Adams
2011-05-16 22:14 ` Juri Linkov
2011-05-16 22:37   ` Drew Adams
2011-05-17  3:08 ` Stefan Monnier
2011-05-27 20:54   ` Drew Adams
2011-05-28  1:27 ` Stefan Monnier
2011-09-10 11:54 ` Juri Linkov
2011-09-10 16:13   ` Drew Adams
2011-09-10 16:22     ` Drew Adams
2011-09-12 11:28       ` Juri Linkov
2011-09-12 14:58         ` Drew Adams
2011-09-14 16:08           ` Juri Linkov

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.