highlight failed part of isearch input

highlight failed part of isearch input

[Drew Adams]

I find that this minor tweak to `isearch-message' helps a good deal when
using Isearch. It highlights, within your Isearch input, the part that fails
to match.

(defface isearch-fail '((t (:foreground "Black" :background "Plum")))
  "Face for highlighting failed part in Isearch echo-area message."
  :group 'isearch)

(defun isearch-message (&optional c-q-hack ellipsis)
  ;; Generate and print the message string.
  (let ((cursor-in-echo-area ellipsis)
        (cmds isearch-cmds)
        succ-msg m)
    (while (not (isearch-success-state (car cmds))) (pop cmds))
    (setq succ-msg (and cmds (isearch-message-state (car cmds))))
    (setq m (concat
             (isearch-message-prefix c-q-hack ellipsis
isearch-nonincremental)
             succ-msg
             (and (not isearch-success)
                  (string-match succ-msg isearch-message)
                  (not (string= succ-msg isearch-message))
                  (propertize (substring isearch-message (match-end 0))
                              'face 'isearch-fail))))
    (when (string-match " +$" m)
      (propertize (substring m (match-beginning 0)) 'face
'trailing-whitespace))
    (setq m (concat m (isearch-message-suffix c-q-hack ellipsis)))
    (if c-q-hack m (let ((message-log-max nil)) (message "%s" m)))))

--

Here is the original `isearch-message', for reference:

(defun isearch-message (&optional c-q-hack ellipsis)
  ;; Generate and print the message string.
  (let ((cursor-in-echo-area ellipsis)
	(m (concat
	    (isearch-message-prefix c-q-hack ellipsis isearch-nonincremental)
	    (if (and (not isearch-success)
                     (string-match " +$" isearch-message))
                (concat
                 (substring isearch-message 0 (match-beginning 0))
                 (propertize (substring isearch-message (match-beginning 0))
                             'face 'trailing-whitespace))
              isearch-message)
	    (isearch-message-suffix c-q-hack ellipsis)
	    )))
    (if c-q-hack
	m
      (let ((message-log-max nil))
	(message "%s" m)))))

Re: highlight failed part of isearch input

[Juri Linkov]

> I find that this minor tweak to `isearch-message' helps a good deal when
> using Isearch. It highlights, within your Isearch input, the part that fails
> to match.

As you can see in the original `isearch-message', it already highlights
the part that fails to match, but only when this part contains trailing
spaces.  Your patch overwrites it with a new face.  I think these two
features could be merged.  Also I think it would be annoying to highlight
the whole search string when it fails (like when repeating the search with
the string from the search ring).  It could do this only during typing the
search string character by character.

-- 
Juri Linkov
http://www.jurta.org/emacs/

RE: highlight failed part of isearch input

[Drew Adams]

> > I find that this minor tweak to `isearch-message' helps a good deal when
> > using Isearch. It highlights, within your Isearch input, the
> > part that fails to match.
>
> As you can see in the original `isearch-message', it already highlights
> the part that fails to match, but only when this part contains trailing
> spaces.

Yes. That's probably where I got the idea; I don't remember.

> Your patch overwrites it with a new face.  I think these two
> features could be merged.

Sure. Unless someone thinks we should differentiate failure with trailing
space from other failure. (I see no reason to differentiate them,
personally.)

> Also I think it would be annoying to highlight
> the whole search string when it fails (like when repeating the search with
> the string from the search ring).  It could do this only during typing the
> search string character by character.

I don't agree about that. It lets you know right away what you've done. For
example, perhaps you changed buffers, so the previous search string is no
longer pertinent. The slight color change is enough to signal what's
happening, without your needing to really examine the minibuffer text and
process the "Failing" message mentally.

I do think it's important to use a face that is subdued, however, rather
than the red-foreground face that is the default for trailing space
highlighting. This should be something gently noticeable, not a blazing
warning of imminent danger.

If you haven't already, please give it a try, and see if you really find it
annoying for letting you know that the entire search string is a non-match.

Re: highlight failed part of isearch input

[Juri Linkov]

> I do think it's important to use a face that is subdued, however, rather
> than the red-foreground face that is the default for trailing space
> highlighting. This should be something gently noticeable, not a blazing
> warning of imminent danger.

Sorry, I forgot that I customized the trailing-whitespace face to pink
long ago, because red is too annoying color.  Therefore, I didn't see
a difference between two features (failed whitespace and other text).

> If you haven't already, please give it a try, and see if you really find it
> annoying for letting you know that the entire search string is a non-match.

There is one inconsistence: when incrementally adding characters to the
search string it highlights one part (only failed) of the search string,
but when repeating the search with the same string from the search ring,
it highlights the whole string.  This is a minor inconsistence, and can
be tolerated if unavoidable.

-- 
Juri Linkov
http://www.jurta.org/emacs/

RE: highlight failed part of isearch input

[Drew Adams]

> There is one inconsistence: when incrementally adding characters to the
> search string it highlights one part (only failed) of the search string,
> but when repeating the search with the same string from the search ring,
> it highlights the whole string.  This is a minor inconsistence, and can
> be tolerated if unavoidable.

Yes. I don't know how to fix that.

And I'm not sure it should be fixed. When you start again with C-s C-s,
recalling the last-used search string, it is normal that that whole string
is sought, and that whole string fails. If we were to highlight only the
failure part in the minibuffer, the cursor in the buffer should logically be
positioned at the last success, which is impossible (there might not even be
one in the current buffer).

I guess I'm saying that I think this is as good as it gets, but I'm open, if
someone has a better idea.

Re: highlight failed part of isearch input

[Stefan Monnier]

>> There is one inconsistence: when incrementally adding characters to the
>> search string it highlights one part (only failed) of the search string,
>> but when repeating the search with the same string from the search ring,
>> it highlights the whole string.  This is a minor inconsistence, and can
>> be tolerated if unavoidable.

> Yes. I don't know how to fix that.

We could simply try to find the longest prefix which does have a match.
Can't be that hard.


        Stefan

Re: highlight failed part of isearch input

[David Kastrup]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> There is one inconsistence: when incrementally adding characters to the
>>> search string it highlights one part (only failed) of the search string,
>>> but when repeating the search with the same string from the search ring,
>>> it highlights the whole string.  This is a minor inconsistence, and can
>>> be tolerated if unavoidable.
>
>> Yes. I don't know how to fix that.
>
> We could simply try to find the longest prefix which does have a match.
> Can't be that hard.

When doing a regexp search for
abc*de
what is the longest matching prefix for
abd?

What is it for
ab\(cdf\)?cgh matched to abcdc?  And what about character
alternatives?

"Can't be that hard" is always cause for fun...

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: highlight failed part of isearch input

[Stefan Monnier]

>>>> There is one inconsistence: when incrementally adding characters to the
>>>> search string it highlights one part (only failed) of the search string,
>>>> but when repeating the search with the same string from the search ring,
>>>> it highlights the whole string.  This is a minor inconsistence, and can
>>>> be tolerated if unavoidable.
>> 
>>> Yes. I don't know how to fix that.
>> 
>> We could simply try to find the longest prefix which does have a match.
>> Can't be that hard.

> When doing a regexp search for
> abc*de
> what is the longest matching prefix for
> abd?

I must be missing someting: what's hard about it?

> What is it for
> ab\(cdf\)?cgh matched to abcdc?  And what about character
> alternatives?

> "Can't be that hard" is always cause for fun...

I think you read too much in my suggestion.  I really meant "the longest
prefix of the user's input which does have a match", so you can simply start
from the user's input (which doesn't match) and iteratively remove the last
char until a search succeeds.  It may not always give you the very best
imaginable information, but least it gives you the same info as Drew's
proposed code in the case where the user just typed the text one char at
a time.
There may be performance issues (if the search text is long, the longest
prefix with a match is short (i.e. we need to iterate many times), and the
buffer is long (i.,e. each iteration's trial search takes a while)), but
other than that it should be pretty easy to code up.


        Stefan

Re: highlight failed part of isearch input

[Miles Bader]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
> There may be performance issues (if the search text is long, the longest
> prefix with a match is short (i.e. we need to iterate many times), and the
> buffer is long (i.,e. each iteration's trial search takes a while))

Binary search?

-miles
-- 
97% of everything is grunge

Re: highlight failed part of isearch input

[Stefan Monnier]

>> There may be performance issues (if the search text is long, the longest
>> prefix with a match is short (i.e. we need to iterate many times), and the
>> buffer is long (i.,e. each iteration's trial search takes a while))

> Binary search?

For plain strings it works, but for regexps it doesn't: the fact that
a prefix fails to match doesn't gurantee that some longer prefix will also
fail to match.


        Stefan

Re: highlight failed part of isearch input

[David Kastrup]

Miles Bader <miles.bader@necel.com> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> There may be performance issues (if the search text is long, the longest
>> prefix with a match is short (i.e. we need to iterate many times), and the
>> buffer is long (i.,e. each iteration's trial search takes a while))
>
> Binary search?

Won't work on regexp searches.

-- 
David Kastrup

RE: highlight failed part of isearch input

[Drew Adams]

> >> There is one inconsistence: when incrementally adding characters to the
> >> search string it highlights one part (only failed) of the
> >> search string, but when repeating the search with the same string
> >> from the search ring, it highlights the whole string.  This is a
> >> minor inconsistence, and can be tolerated if unavoidable.
>
> > Yes. I don't know how to fix that.
>
> We could simply try to find the longest prefix which does have a match.
> Can't be that hard.

The rest of my message said that I do not think such a fix is even a good
idea. I probably should not even have said that I don't know how to fix
that, since I don't think it should be "fixed".

Starting again with `C-s C-s' is, IMO, asking Emacs to find the whole search
string; it is not asking it to find the part that can be found. At least,
that's how I interpret such a user intention.

If that's so, it doesn't make sense to try to find as much as can be found.
It is not necessarily the case that the search is done in the same buffer as
the last search, or that that buffer has not changed considerably, or even
that the search is done in a buffer for which the previous search string
makes sense at all.

When you search for such a previously used string, Emacs does not, in any
case, try to find as much of it as it can find; it simply reports failure.
So, if you want the behavior you describe, it means more than a tweak to the
failure-highlighting I proposed; it implies a change of Isearch behavior.

When you search for such a string, Emacs does not let you simply use `DEL'
to back out the failure part, character by character; hitting `DEL' removes
the entire search string at once. Again, if you want the behavior you
describe, it means a change of Isearch behavior, generally.

I don't think that such a change in Isearch behavior is a good idea. I
prefer that it immediately fail, and that `DEL' immediately wipe out the
entire failed search string.

Consequently (with no change in Isearch behavior), there should be no
attempt to highlight only the failed part: the search string as a whole is
what is sought, and the search string as a whole represents a match failure.
It makes sense, given the current Isearch behavior, to highlight the whole
search string as a failure.

Re: highlight failed part of isearch input

[Robert J. Chassell]

    When you search for such a string, Emacs does not let you simply
    use `DEL' to back out the failure part, character by character ...

I do not understand.  

To back out of a failure, invoke isearch-del-char (`C-M-w').  That
deletes one character from the end of the search string and searches
again.

    When you search for such a string, Emacs does not let you simply
    use `DEL' to back out the failure part, character by character;
    hitting `DEL' removes the entire search string at once.

isearch-delete-char (`DEL') only removes the entire search string if
you use isearch-yank-word-or-char (`C-w') and only if you also make
that word be the only part of the search string.  If you invoke
isearch-yank-word-or-char (`C-w') twice, isearch-delete-char (`DEL')
removes only the last.

You can use isearch-yank-word-or-char (`C-w') to gather a complete
word and then remove characters from that string with isearch-del-char
(`C-M-w').

-- 
    Robert J. Chassell                          GnuPG Key ID: 004B4AC8
    bob@rattlesnake.com                         bob@gnu.org
    http://www.rattlesnake.com                  http://www.teak.cc

RE: highlight failed part of isearch input

[Drew Adams]

>     When you search for such a string, Emacs does not let you simply
>     use `DEL' to back out the failure part, character by character ...
>
> To back out of a failure, invoke isearch-del-char (`C-M-w').  That
> deletes one character from the end of the search string and searches
> again.

I see; thanks. I wasn't aware of that. It's new with Emacs 22, apparently,
and I hadn't come across it yet.

>     When you search for such a string, Emacs does not let you simply
>     use `DEL' to back out the failure part, character by character;
>     hitting `DEL' removes the entire search string at once.
>
> isearch-delete-char (`DEL') only removes the entire search string if
> you use isearch-yank-word-or-char (`C-w') and only if you also make
> that word be the only part of the search string.  If you invoke
> isearch-yank-word-or-char (`C-w') twice, isearch-delete-char (`DEL')
> removes only the last.

emacs -Q
In a buffer with text abcd, use `C-s abx RET'. Then use `C-s C-s DEL'. For
me, it removes the entire search string. You say that if I do not use `C-w'
during Isearch then `DEL' will not remove the entire search string, which is
not what I see. Am I misunderstanding you?

> You can use isearch-yank-word-or-char (`C-w') to gather a complete
> word and then remove characters from that string with isearch-del-char
> (`C-M-w').

Yes, `C-M-w' is new with Emacs 22. I was not aware of it.

Consequently, I withdraw what I wrote. Since you can now back out of a
repeat search incrementally (with `C-M-w'), I agree that it could be OK to
highlight only the failure part in the case of a repeat search also.

I notice something that strikes me as perhaps a bug, but I don't know which
behavior was intended by `C-M-w'. If you use `C-s abx C-a' in a buffer with
only `abcd', and then you use `C-s C-s C-M-w', the search string is changed
from `abx' to `ab', but the prompt/echo still says "Failing", which is not
really correct. In fact, it has not yet searched again with the updated
search string from `C-M-w' - you must hit `C-s' again to start the search
(which does not fail). Similarly, even without the repeated search: if you
use `C-s abx C-M-w', then the current search position is maintained, but the
minibuffer still says "Failing".

Is this a feature or a bug? `C-M-w' just edits the search string,
apparently; the edited string is not sought until you hit `C-s' again.
That's OK, I guess, but it means that subtracting search-string text with
`C-M-x' is not an inverse of adding it by typing additional characters.

In any case, shouldn't the prompt/echo indicate the state of the current
search position? That is, if backing out a character makes the current
search successful, shouldn't the indication change from "Failing" to
"Isearch" (even if `C-M-x' is limited to editing the search string)?

If this is the intended behavior, then there is all the more reason for
fixing the failure-part highlighting, so that at least it (even if not the
prompt) correctly reflects the current state.

In case of a repeated search that fails, followed by sufficient `C-M-w' to
reach success, shouldn't success also mean moving to the first match? That
is, should the fact that `C-M-w' only edits the search string be maintained
also for a repeated search?

WDOT?

Re: highlight failed part of isearch input

[Juri Linkov]

> Is this a feature or a bug? `C-M-w' just edits the search string,
> apparently; the edited string is not sought until you hit `C-s' again.
> That's OK, I guess, but it means that subtracting search-string text with
> `C-M-x' is not an inverse of adding it by typing additional characters.
>
> In any case, shouldn't the prompt/echo indicate the state of the current
> search position? That is, if backing out a character makes the current
> search successful, shouldn't the indication change from "Failing" to
> "Isearch" (even if `C-M-x' is limited to editing the search string)?

This looks like an unfinished feature.  Perhaps, it should display
"Pending I-search" instead of "Failing I-search" (i.e. setting the
variable `isearch-adjusted').

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: highlight failed part of isearch input

[Robert J. Chassell]

    emacs -Q 
    In a buffer with text abcd, use `C-s abx RET'. Then use `C-s C-s
    DEL'. For me, it removes the entire search string.

isearch-delete-char (`DEL') removes the last entry, whether that be a
string of letters or a single letter.  The easiest way I know to get a
string of letters is with isearch-yank-word-or-char (`C-w'), but
typing them in works, too.

When I invoke isearch twice without isearch-yank-word-or-char, but put
in several characters each time, a single isearch-delete-char (`DEL')
removes the last character.  Then, when I get to the first part, it
removes all of it.

    ... I withdraw what I wrote. ...  

    If you use `C-s abx C-a' in a buffer with only `abcd', and then
    you use `C-s C-s C-M-w', the search string is changed from `abx'
    to `ab', but the prompt/echo still says "Failing", which is not
    really correct.

I think it is a bug.  The isearch-forward command correctly says
`Failing I-search' the first time it fails.  However, use of
isearch-del-char (`C-M-w') causes the isearch-forward command to light
up the other strings it can find (my test buffer is this message and
mentions `ise' several times as part of `isearch') and I can reach
them.  That is the bug.

-- 
    Robert J. Chassell                          GnuPG Key ID: 004B4AC8
    bob@rattlesnake.com                         bob@gnu.org
    http://www.rattlesnake.com                  http://www.teak.cc

Re: highlight failed part of isearch input

[Juri Linkov]

>     I notice something that strikes me as perhaps a bug, but I don't know which
>     behavior was intended by `C-M-w'. If you use `C-s abx C-a' in a buffer with
>     only `abcd', and then you use `C-s C-s C-M-w', the search string is changed
>     from `abx' to `ab', but the prompt/echo still says "Failing", which is not
>     really correct. In fact, it has not yet searched again with the updated
>     search string from `C-M-w' - you must hit `C-s' again to start the search
>     (which does not fail).
>
> Given that it doesn't search again, the prompt may not be a bug.
> But the interface is definitely confusing.
>
> Juri Linkov is the author of this command.
> Juri, is there a reason it should work the way it does?

Semantically `isearch-del-char' should be equivalent to putting the search
string in the minibuffer for editing (`isearch-edit-string'), deleting the
last character in the minibuffer, exiting the minibuffer, and resuming the
search (i.e. three-key sequence `M-e DEL RET').

Using the Drew's test case (in a buffer containing text "abcd" type
`C-s abx M-e DEL RET') yields the same result - the minibuffer says
"Failing I-search".

It's even worse: in `isearch-edit-string' (unlike `isearch-yank-char')
adding the same characters that are under cursor to the search string
in the minibuffer, the resumed search can't find them (test case: in
a buffer containing text "abcd" type `C-s ab M-e cd C-s').  The result
is "Failing I-search".

I think we should try to find a proper place in low-level isearch
functions to fix such behavior, because for users it is confusing.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: highlight failed part of isearch input

[Richard Stallman]

    I think we should try to find a proper place in low-level isearch
    functions to fix such behavior, because for users it is confusing.

I agree.  Can you work on it?

Re: highlight failed part of isearch input

[Juri Linkov]

>     I think we should try to find a proper place in low-level isearch
>     functions to fix such behavior, because for users it is confusing.
>
> I agree.  Can you work on it?

The patch below fixes `isearch-del-char' to work correctly in all
reported cases.  It failed because `isearch-search-and-update' works
only for adding characters to the successful search (it doesn't call
`isearch-search' for unsuccessful non-regexp search).

A new solution is to set search starting point to the isearch-other-end
and start the search from this position and try to find the remaining
string again.

Index: lisp/isearch.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/isearch.el,v
retrieving revision 1.298
diff -c -r1.298 isearch.el
*** lisp/isearch.el	9 Jul 2007 14:45:01 -0000	1.298
--- lisp/isearch.el	14 Jul 2007 22:59:45 -0000
***************
*** 1260,1269 ****
        (ding)
      (setq isearch-string (substring isearch-string 0 (- (or arg 1)))
            isearch-message (mapconcat 'isearch-text-char-description
!                                      isearch-string "")
!           ;; Don't move cursor in reverse search.
!           isearch-yank-flag t))
!   (isearch-search-and-update))
  
  (defun isearch-yank-string (string)
    "Pull STRING into search string."
--- 1284,1296 ----
        (ding)
      (setq isearch-string (substring isearch-string 0 (- (or arg 1)))
            isearch-message (mapconcat 'isearch-text-char-description
!                                      isearch-string "")))
!   ;; Use the isearch-other-end as new starting point to be able
!   ;; to find the remaining part of the search string again.
!   (if isearch-other-end (goto-char isearch-other-end))
!   (isearch-search)
!   (isearch-push-state)
!   (isearch-update))
  
  (defun isearch-yank-string (string)
    "Pull STRING into search string."

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: highlight failed part of isearch input

[Richard Stallman]

    The patch below fixes `isearch-del-char' to work correctly in all
    reported cases.

If nobody finds a problem in this patch in the next few days,
please install it in the trunk and Emacs 22, and then ack.

Re: highlight failed part of isearch input

[Richard Stallman]

[I sent this message a week ago but did not get a response.
Could we get the discussion moving again?

Since another week has gone by, please install your patch and ack.]


    The patch below fixes `isearch-del-char' to work correctly in all
    reported cases.

If nobody finds a problem in this patch in the next few days,
please install it in the trunk and Emacs 22, and then ack.

Re: highlight failed part of isearch input

[Juri Linkov]

>     I think we should try to find a proper place in low-level isearch
>     functions to fix such behavior, because for users it is confusing.
>
> I agree.  Can you work on it?

The patch below fixes `isearch-edit-string' to work correctly in all
reported cases.  After editing the search string in the minibuffer it uses
the old value of `isearch-other-end' as a new search starting point, so it
can find the same string again.  It does this only in the case when the
user didn't change point in the buffer during editing the search string in
the minibuffer (by switching buffers and moving point).

Index: lisp/isearch.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/isearch.el,v
retrieving revision 1.298
diff -c -r1.298 isearch.el
*** lisp/isearch.el	9 Jul 2007 14:45:01 -0000	1.298
--- lisp/isearch.el	14 Jul 2007 22:59:45 -0000
***************
*** 992,998 ****
  	       isearch-original-minibuffer-message-timeout)
  	      (isearch-original-minibuffer-message-timeout
  	       isearch-original-minibuffer-message-timeout)
! 	      )
  
  	  ;; Actually terminate isearching until editing is done.
  	  ;; This is so that the user can do anything without failure,
--- 1004,1010 ----
  	       isearch-original-minibuffer-message-timeout)
  	      (isearch-original-minibuffer-message-timeout
  	       isearch-original-minibuffer-message-timeout)
! 	      old-point old-other-end)
  
  	  ;; Actually terminate isearching until editing is done.
  	  ;; This is so that the user can do anything without failure,
***************
*** 1001,1006 ****
--- 1013,1022 ----
  	      (isearch-done t t)
  	    (exit nil))			; was recursive editing
  
+ 	  ;; Save old point and isearch-other-end before reading from minibuffer
+ 	  ;; that can change their values.
+ 	  (setq old-point (point) old-other-end isearch-other-end)
+ 
  	  (isearch-message) ;; for read-char
  	  (unwind-protect
  	      (let* (;; Why does following read-char echo?
***************
*** 1036,1041 ****
--- 1052,1065 ----
  		      isearch-new-message
  		      (mapconcat 'isearch-text-char-description
  				 isearch-new-string "")))
+ 
+ 	    ;; Set the point at the start (end) of old match if forward (backward),
+ 	    ;; so after exiting minibuffer isearch resumes at the start (end)
+ 	    ;; of this match and can find it again.
+ 	    (if (and old-other-end (eq old-point (point))
+ 		     (eq isearch-forward isearch-new-forward))
+ 		(goto-char old-other-end))
+ 
  	    ;; Always resume isearching by restarting it.
  	    (isearch-mode isearch-forward
  			  isearch-regexp

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: highlight failed part of isearch input

[Richard Stallman]

    The patch below fixes `isearch-edit-string' to work correctly in all
    reported cases.

For this patch too, if nobody finds a problem in the patch in the next
few days, please install it in the trunk and Emacs 22, and then ack.

Re: highlight failed part of isearch input

[Richard Stallman]

    > We could simply try to find the longest prefix which does have a match.
    > Can't be that hard.

    The rest of my message said that I do not think such a fix is even a good
    idea. I probably should not even have said that I don't know how to fix
    that, since I don't think it should be "fixed".

    Starting again with `C-s C-s' is, IMO, asking Emacs to find the whole search
    string; it is not asking it to find the part that can be found. At least,
    that's how I interpret such a user intention.

I think that is correct behavior.

The fact that C-M-w can be used to delete chars from the string does
not invalidate it.  You can also use M-e to edit the search string in
any which way, but I don't think that matters here, and neither does
C-M-w.

Putting this together with the fact that it is hard to determine the
right "failing part" in this case, we get a thoroughly convincing
reason not to try.

Re: highlight failed part of isearch input

[Richard Stallman]

    There is one inconsistence: when incrementally adding characters to the
    search string it highlights one part (only failed) of the search string,
    but when repeating the search with the same string from the search ring,
    it highlights the whole string.  This is a minor inconsistence, and can
    be tolerated if unavoidable.

I think that is correct behavior.  When you repeat the search, and
that fails, what failed is the whole string.

RE: highlight failed part of isearch input

[Drew Adams]

> I think that is correct behavior.  When you repeat the search, and
> that fails, what failed is the whole string.

That's what I thought too, but see the reply from Bob.

Re: highlight failed part of isearch input

[Masatake YAMATO]

Hi,

> I find that this minor tweak to `isearch-message' helps a good deal when
> using Isearch. It highlights, within your Isearch input, the part that fails
> to match.

I like it. I'd like to use your patch with following patch.
`ding' is too noisy for me. Instead of `ding' your patch tells 
the failing of search calmly.

Masatake YAMATO

--- isearch.el	09 Apr 2007 19:11:44 +0900	1.297
+++ isearch.el	10 Jul 2007 23:01:32 +0900	
@@ -2069,7 +2069,8 @@
       nil
     ;; Ding if failed this time after succeeding last time.
     (and (isearch-success-state (car isearch-cmds))
-	 (ding))
+;	 (ding)
+)
     (if (functionp (isearch-pop-fun-state (car isearch-cmds)))
         (funcall (isearch-pop-fun-state (car isearch-cmds)) (car isearch-cmds)))
     (goto-char (isearch-point-state (car isearch-cmds)))))

ding susceptibility (was: highlight failed part of isearch input)

[Drew Adams]

> > I find that this minor tweak to `isearch-message' helps a good deal when
> > using Isearch. It highlights, within your Isearch input, the
> > part that fails to match.
>
> I like it. I'd like to use your patch with following patch.
> `ding' is too noisy for me. Instead of `ding' your patch tells
> the failing of search calmly.
>
> +;	 (ding)

Yes, but I can imagine that `ding' might be useful here to people with
difficulty seeing. Perhaps this could be configurable somehow, similar to
the way `ding' respects `visible-bell'.

I imagine that you don't simply want to customize `visible-bell' to non-nil,
yourself, because you want to hear the bell sometimes, right?

One possibility for this kind of thing might be to let `ding' accept a
second argument, which is a level of susceptibility. For example we could
have a wholenump user option `ding-threshold', whose value would be the ding
level above which `ding' actually rings the bell. The default value would be
0, meaning `ding' always rings the bell. Then, for example, this call in
isearch could be, say, (ding nil 2), meaning that the bell would ring only
if `ding-threshold' was 2 or greater.

`visible-bell' itself could be changed to respect an integer threshold
value, instead of adding a new option `ding-threshold'. In that case, there
would need to be a way to specify both (1) the flash frame vs bell choice
and (2) the threshold value (for both). Negative values could be used to do
this. However, in this case, the name `visible-bell' would no longer convey
the full meaning.

Just throwing this out for discussion. Sounds a bit ugly, I admit.

Re: highlight failed part of isearch input

[Richard Stallman]

    I like it. I'd like to use your patch with following patch.
    `ding' is too noisy for me. Instead of `ding' your patch tells 
    the failing of search calmly.

Not all terminals can show colors.  We could make the ding optional,
or conditional, but removing it entirely is not right.

Re: highlight failed part of isearch input

[Stefan Monnier]

> I find that this minor tweak to `isearch-message' helps a good deal when
> using Isearch.  It highlights, within your Isearch input, the part that
> fails to match.

Sounds like a good idea.  Please send such suggestions as context diffs
rather than "old code, new code".


        Stefan

Re: highlight failed part of isearch input

[Richard Stallman]

That is a nice feature.  Let's install it.

I am not convinced we should use the same face for trailing spaces
and other things.  And I am not convinced we want subdued colors for
this highlighting.

    > Also I think it would be annoying to highlight
    > the whole search string when it fails (like when repeating the search with
    > the string from the search ring).  It could do this only during typing the
    > search string character by character.

    I don't agree about that. It lets you know right away what you've done. For
    example, perhaps you changed buffers, so the previous search string is no
    longer pertinent. The slight color change is enough to signal what's
    happening,

I agree with you.

RE: highlight failed part of isearch input

[Drew Adams]

> From: Richard Stallman Sent: Tuesday, July 10, 2007 3:01 PM
> That is a nice feature.  Let's install it.
>
> I am not convinced we should use the same face for trailing spaces
> and other things.  And I am not convinced we want subdued colors for
> this highlighting.

It seems that only the tweaks that came out of the secondary discussion in
this thread were applied; the original proposal was not. Here is the diff.

---------8<-----------------------

*** isearch-CVS-2007-07-22.el	Sun Jul 22 16:29:20 2007
--- isearch-CVS-patched-2007-07-22.el	Sun Jul 22 16:34:00 2007
***************
*** 226,231 ****
--- 226,235 ----
    :group 'basic-faces)
  (defvar isearch 'isearch)

+ (defface isearch-fail '((t (:foreground "Black" :background "Plum")))
+   "Face for highlighting failed part in Isearch echo-area message."
+   :group 'isearch)
+
  (defcustom isearch-lazy-highlight t
    "*Controls the lazy-highlighting during incremental search.
  When non-nil, all text in the buffer matching the current search
***************
*** 1928,1948 ****
  (defun isearch-message (&optional c-q-hack ellipsis)
    ;; Generate and print the message string.
    (let ((cursor-in-echo-area ellipsis)
! 	(m (concat
  	    (isearch-message-prefix c-q-hack ellipsis isearch-nonincremental)
! 	    (if (and (not isearch-success)
!                      (string-match " +$" isearch-message))
!                 (concat
!                  (substring isearch-message 0 (match-beginning 0))
!                  (propertize (substring isearch-message (match-beginning
0))
!                              'face 'trailing-whitespace))
!               isearch-message)
! 	    (isearch-message-suffix c-q-hack ellipsis)
! 	    )))
!     (if c-q-hack
! 	m
!       (let ((message-log-max nil))
! 	(message "%s" m)))))

  (defun isearch-message-prefix (&optional c-q-hack ellipsis nonincremental)
    ;; If about to search, and previous search regexp was invalid,
--- 1932,1953 ----
  (defun isearch-message (&optional c-q-hack ellipsis)
      ;; Generate and print the message string.
      (let ((cursor-in-echo-area ellipsis)
!           (cmds isearch-cmds)
!           succ-msg m)
!       (while (not (isearch-success-state (car cmds))) (pop cmds))
!       (setq succ-msg (and cmds (isearch-message-state (car cmds))))
!       (setq m (concat
                 (isearch-message-prefix c-q-hack ellipsis
isearch-nonincremental)
!                succ-msg
!                (and (not isearch-success)
!                     (string-match succ-msg isearch-message)
!                     (not (string= succ-msg isearch-message))
!                     (propertize (substring isearch-message (match-end 0))
!                                 'face 'isearch-fail))))
!       (when (string-match " +$" m)
!         (propertize (substring m (match-beginning 0)) 'face
'trailing-whitespace))
!       (setq m (concat m (isearch-message-suffix c-q-hack ellipsis)))
!       (if c-q-hack m (let ((message-log-max nil)) (message "%s" m)))))

  (defun isearch-message-prefix (&optional c-q-hack ellipsis nonincremental)
    ;; If about to search, and previous search regexp was invalid,

Re: highlight failed part of isearch input

[Richard Stallman]

    It seems that only the tweaks that came out of the secondary discussion in
    this thread were applied; the original proposal was not. Here is the diff.

Would someone please apply that patch, and ack?

Re: highlight failed part of isearch input

[Juri Linkov]

>     It seems that only the tweaks that came out of the secondary discussion in
>     this thread were applied; the original proposal was not. Here is the diff.
>
> Would someone please apply that patch, and ack?

I like this proposal but there is a problem in the patch:

      (when (string-match " +$" m)
         (propertize (substring m (match-beginning 0)) 'face 'trailing-whitespace))

This is a no-op.  But maybe it is good to remove this code because the
failed trailing whitespace is highlighted in the new face isearch-failed.
We could change its background to more strong color, e.g. to something
like is used in Firefox failed search, so failed space will be more visible.

-- 
Juri Linkov
http://www.jurta.org/emacs/

RE: highlight failed part of isearch input

[Drew Adams]

> >     It seems that only the tweaks that came out of the
> >     secondary discussion in this thread were applied; the
> >     original proposal was not.  Here is the diff.
> >
> > Would someone please apply that patch, and ack?
>
> I like this proposal but there is a problem in the patch:
>
>       (when (string-match " +$" m)
>          (propertize (substring m (match-beginning 0)) 'face
>             'trailing-whitespace))
>
> This is a no-op.  But maybe it is good to remove this code because the
> failed trailing whitespace is highlighted in the new face isearch-failed.
> We could change its background to more strong color, e.g. to something
> like is used in Firefox failed search, so failed space will be
> more visible.

Good catch. We need to change the face of the pertinent part of `m' itself,
not just a copy. Thx. The face to use for that is `trailing-whitespace', no?

Re: highlight failed part of isearch input

[Juri Linkov]

>>       (when (string-match " +$" m)
>>          (propertize (substring m (match-beginning 0)) 'face
>>             'trailing-whitespace))
>>
>> This is a no-op.  But maybe it is good to remove this code because the
>> failed trailing whitespace is highlighted in the new face isearch-failed.
>> We could change its background to more strong color, e.g. to something
>> like is used in Firefox failed search, so failed space will be
>> more visible.
>
> Good catch. We need to change the face of the pertinent part of `m' itself,
> not just a copy. Thx. The face to use for that is `trailing-whitespace', no?

Why use `trailing-whitespace' when `isearch-fail' face makes failed
trailing whitespace visible as well?

-- 
Juri Linkov
http://www.jurta.org/emacs/

RE: highlight failed part of isearch input

[Drew Adams]

> >>       (when (string-match " +$" m)
> >>          (propertize (substring m (match-beginning 0)) 'face
> >>             'trailing-whitespace))
> >>
> >> This is a no-op.  But maybe it is good to remove this code because the
> >> failed trailing whitespace is highlighted in the new face
> >> isearch-failed. We could change its background to more strong color,
> >> e.g. to something like is used in Firefox failed search, so failed
> >> space will be more visible.
> >
> > Good catch. We need to change the face of the pertinent part of
> > `m' itself, not just a copy. Thx. The face to use for that is
> > `trailing-whitespace', no?
>
> Why use `trailing-whitespace' when `isearch-fail' face makes failed
> trailing whitespace visible as well?

I thought RMS decided he wanted two different faces. That's the only reason
I'm aware of.

Re: highlight failed part of isearch input

[Richard Stallman]

    > Why use `trailing-whitespace' when `isearch-fail' face makes failed
    > trailing whitespace visible as well?

    I thought RMS decided he wanted two different faces. That's the only reason
    I'm aware of.

On second thought, I don't have a strong opinion.

Re: highlight failed part of isearch input

[Richard Stallman]

    I like this proposal but there is a problem in the patch:

	  (when (string-match " +$" m)
	     (propertize (substring m (match-beginning 0)) 'face 'trailing-whitespace))

    This is a no-op.

Yes, if it is used for effect.

		      But maybe it is good to remove this code because the
    failed trailing whitespace is highlighted in the new face isearch-failed.

I don't entirely understand.  What change are you proposing?

RE: highlight failed part of isearch input

[Drew Adams]

>     I like this proposal but there is a problem in the patch:
>
> 	  (when (string-match " +$" m)
> 	     (propertize (substring m (match-beginning 0)) 'face
>                      'trailing-whitespace))
>
>     This is a no-op.
>
> Yes, if it is used for effect.

There was no effect. Maybe that's what you meant; I'm not sure.

That code was a mistake. The intention was to change the face in the string
`m', not in a copy. For instance, `put-text-property' could have been used
instead of `propertize'.

> 		      But maybe it is good to remove this code because the
>     failed trailing whitespace is highlighted in the new face
>     isearch-failed.
>
> I don't entirely understand.  What change are you proposing?

I think Juri meant that instead of using two different faces, one for
general failure and one for trailing whitespace, the former is enough
(trailing whitespace is subsumed by search failure).

RE: highlight failed part of isearch input

[Drew Adams]

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

> From: Richard Stallman Sent: Monday, July 23, 2007 11:06 AM
> 
>     It seems that only the tweaks that came out of the 
>     secondary discussion in this thread were applied;
>     the original proposal was not. Here is the diff.
> 
> Would someone please apply that patch, and ack?


The patch was apparently never applied. It highlights the part of your
isearch input that fails to match.

Attached is a new patch relative to today's CVS. It also incorporates the
comments following the email cited below.

And here is a change log entry:

2008-02-11 Drew Adams  <drew.adams@oracle.com>

	* isearch.el:
	(isearch-fail): New face.
	(isearch-message): Highlight failure part of input.


[-- Attachment #2: isearch-2008-02-11.patch --]
[-- Type: application/octet-stream, Size: 2908 bytes --]

diff -c -w isearch-CVS-2008-02-11.el isearch-patched-2008-02-11.el
*** isearch-CVS-2008-02-11.el	Mon Feb 11 15:09:22 2008
--- isearch-patched-2008-02-11.el	Mon Feb 11 15:14:18 2008
***************
*** 231,236 ****
--- 231,240 ----
    :group 'basic-faces)
  (defvar isearch 'isearch)
  
+ (defface isearch-fail '((t (:foreground "Black" :background "Plum")))
+   "Face for highlighting failed part in Isearch echo-area message."
+   :group 'isearch)
+ 
  (defcustom isearch-lazy-highlight t
    "*Controls the lazy-highlighting during incremental search.
  When non-nil, all text in the buffer matching the current search
***************
*** 1955,1975 ****
  (defun isearch-message (&optional c-q-hack ellipsis)
    ;; Generate and print the message string.
    (let ((cursor-in-echo-area ellipsis)
! 	(m (concat
  	    (isearch-message-prefix c-q-hack ellipsis isearch-nonincremental)
! 	    (if (and (not isearch-success)
!                      (string-match " +$" isearch-message))
!                 (concat
!                  (substring isearch-message 0 (match-beginning 0))
!                  (propertize (substring isearch-message (match-beginning 0))
!                              'face 'trailing-whitespace))
!               isearch-message)
! 	    (isearch-message-suffix c-q-hack ellipsis)
! 	    )))
!     (if c-q-hack
! 	m
!       (let ((message-log-max nil))
! 	(message "%s" m)))))
  
  (defun isearch-message-prefix (&optional c-q-hack ellipsis nonincremental)
    ;; If about to search, and previous search regexp was invalid,
--- 1959,1980 ----
  (defun isearch-message (&optional c-q-hack ellipsis)
      ;; Generate and print the message string.
      (let ((cursor-in-echo-area ellipsis)
!           (cmds isearch-cmds)
!           succ-msg m)
!       (while (not (isearch-success-state (car cmds))) (pop cmds))
!       (setq succ-msg (and cmds (isearch-message-state (car cmds))))
!       (setq m (concat
                 (isearch-message-prefix c-q-hack ellipsis isearch-nonincremental)
!                succ-msg
!                (and (not isearch-success)
!                     (string-match (regexp-quote succ-msg) isearch-message)
!                     (not (string= succ-msg isearch-message))
!                     (propertize (substring isearch-message (match-end 0))
!                                 'face 'isearch-fail))))
!       (when (and (not isearch-success) (string-match " +$" m))
!         (put-text-property (match-beginning 0) (length m) 'face 'trailing-whitespace m))
!       (setq m (concat m (isearch-message-suffix c-q-hack ellipsis)))
!       (if c-q-hack m (let ((message-log-max nil)) (message "%s" m)))))
  
  (defun isearch-message-prefix (&optional c-q-hack ellipsis nonincremental)
    ;; If about to search, and previous search regexp was invalid,

Diff finished.  Mon Feb 11 15:15:38 2008

Re: highlight failed part of isearch input

[Juri Linkov]

>>     It seems that only the tweaks that came out of the
>>     secondary discussion in this thread were applied;
>>     the original proposal was not. Here is the diff.
>>
>> Would someone please apply that patch, and ack?
>
> The patch was apparently never applied. It highlights the part of your
> isearch input that fails to match.

Did you fix all problems?  I remember there were some rough edges.

-- 
Juri Linkov
http://www.jurta.org/emacs/

RE: highlight failed part of isearch input

[Drew Adams]

> >>     It seems that only the tweaks that came out of the
> >>     secondary discussion in this thread were applied;
> >>     the original proposal was not. Here is the diff.
> >>
> >> Would someone please apply that patch, and ack?
> >
> > The patch was apparently never applied. It highlights the 
> > part of your isearch input that fails to match.
> 
> Did you fix all problems?  I remember there were some rough edges.

As I said, it also incorporates the comments following Richard's email.

It uses a separate face from trailing-whitespace, which was Richard's
preference, but he later changed his mind and said he doesn't care whether
one face or two are used. You preferred one face, IIRC. I don't care either
way - you are welcome to change it to use a single face.

Give it a try, if you are concerned that it might not do what you want.

Re: highlight failed part of isearch input

[Bastien]

"Drew Adams" <drew.adams@oracle.com> writes:

>> >>     It seems that only the tweaks that came out of the
>> >>     secondary discussion in this thread were applied;
>> >>     the original proposal was not. Here is the diff.
>> >>
>> >> Would someone please apply that patch, and ack?
>> >
>> > The patch was apparently never applied. It highlights the 
>> > part of your isearch input that fails to match.
>> 
>> Did you fix all problems?  I remember there were some rough edges.
>
> As I said, it also incorporates the comments following Richard's email.

I tested it and it looks fine.  

I applied the patch before reading Juri's email, so Juri please check
and let me know if you think it is okay.

-- 
Bastien

Re: highlight failed part of isearch input

[Juri Linkov]

>>> > The patch was apparently never applied. It highlights the
>>> > part of your isearch input that fails to match.
>>>
>>> Did you fix all problems?  I remember there were some rough edges.
>>
>> As I said, it also incorporates the comments following Richard's email.
>
> I tested it and it looks fine.
>
> I applied the patch before reading Juri's email, so Juri please check
> and let me know if you think it is okay.

There is one regression after installing this patch: `C-s M-p' now
doesn't put the previous search string from the search ring to the
isearch minibuffer.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: highlight failed part of isearch input

[Juri Linkov]

>>>> > The patch was apparently never applied. It highlights the
>>>> > part of your isearch input that fails to match.
>>>>
>>>> Did you fix all problems?  I remember there were some rough edges.
>>>
>>> As I said, it also incorporates the comments following Richard's email.
>>
>> I tested it and it looks fine.
>>
>> I applied the patch before reading Juri's email, so Juri please check
>> and let me know if you think it is okay.
>
> There is one regression after installing this patch: `C-s M-p' now
> doesn't put the previous search string from the search ring to the
> isearch minibuffer.

The patch below fixes this problem.  When the first message from the
isearch-cmds stack is not the same as isearch-message (this happens when
isearch-edit-string sets a different value) then it uses this value
for succ-msg.

Also this patch uses a better background color for the
`isearch-fail' face - the same color as used by Firefox
for the background of the failed search text.  UI designers
of Firefox made a good job and this color looks nice.

Index: lisp/isearch.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/isearch.el,v
retrieving revision 1.310
diff -c -r1.310 isearch.el
*** lisp/isearch.el	12 Feb 2008 00:50:44 -0000	1.310
--- lisp/isearch.el	23 Feb 2008 19:46:51 -0000
***************
*** 231,238 ****
    :group 'basic-faces)
  (defvar isearch 'isearch)
  
! (defface isearch-fail '((t (:foreground "Black" :background "Plum")))
    "Face for highlighting failed part in Isearch echo-area message."
    :group 'isearch)
  
  (defcustom isearch-lazy-highlight t
--- 231,245 ----
    :group 'basic-faces)
  (defvar isearch 'isearch)
  
! (defface isearch-fail
!   '((((class color) (min-colors 88))
!      (:background "IndianRed1"))
!     (((class color) (min-colors 16))
!      (:background "red"))
!     (((class color) (min-colors 8))
!      (:background "red")))
    "Face for highlighting failed part in Isearch echo-area message."
+   :version "23.1"
    :group 'isearch)
  
  (defcustom isearch-lazy-highlight t
***************
*** 1962,1968 ****
            (cmds isearch-cmds)
            succ-msg m)
        (while (not (isearch-success-state (car cmds))) (pop cmds))
!       (setq succ-msg (and cmds (isearch-message-state (car cmds))))
        (setq m (concat
  	    (isearch-message-prefix c-q-hack ellipsis isearch-nonincremental)
                 succ-msg
--- 1972,1981 ----
            (cmds isearch-cmds)
            succ-msg m)
        (while (not (isearch-success-state (car cmds))) (pop cmds))
!       (setq succ-msg
! 	    (if (equal (isearch-message-state (car isearch-cmds)) isearch-message)
! 		(and cmds (isearch-message-state (car cmds)))
! 	      isearch-message))
        (setq m (concat
  	    (isearch-message-prefix c-q-hack ellipsis isearch-nonincremental)
                 succ-msg

-- 
Juri Linkov
http://www.jurta.org/emacs/

RE: highlight failed part of isearch input

[Drew Adams]

> The patch below fixes this problem.  When the first message from the
> isearch-cmds stack is not the same as isearch-message (this 
> happens when isearch-edit-string sets a different value) then it
> uses this value for succ-msg.

Thanks; good catch.

> Also this patch uses a better background color for the
> `isearch-fail' face - the same color as used by Firefox
> for the background of the failed search text.  UI designers
> of Firefox made a good job and this color looks nice.

How about separating that face-change suggestion, which is independent of
the bug fix?

Personally, I disagree about the color, at least for a light background with
unlimited colors available - it is too vivid (bright, saturated, loud). For
that, Plum or some other pastel is a better default, IMO. It's important to
not only notice the failure but also easily read the text that is
highlighted.

For a dark background, the color should presumably be quite dark, not
bright.

Also, I think it should specify a foreground color by default, for the case
where someone uses a different foreground color for a standalone minibuffer.

How about something like this? The default color here for a dark background
is the complement of Plum (a light violet): a dark green. You certainly
don't want something like IndianRed1 or Plum on a dark background, I expect.

(defface isearch-fail
      '((((class color) (min-colors 88) (background dark))
         (:foreground "white" :background "#22225F5F2222"))
        (((class color) (min-colors 88) (background light))
         (:foreground "Black" :background "Plum"))
        (((class color) (min-colors 8)) (:background "red"))
        (((type tty) (class mono)) :inverse-video t)
        (t :background "gray"))
    "Face for highlighting failed part in Isearch echo-area message."
    :version "23.1" :group 'isearch)

I also added the mono case and the catch-all case. Copied them from the
definition for `region'.

I can't speak much to what should be the default for a dark background or
for when there are limited colors available. Perhaps people who use those
contexts could suggest an improvement.

BTW - I'm no expert on face specs, but isn't your duplication of the red
spec for (min-colors 8) and (min-colors 16) redundant? Doesn't (min-colors
8), as shown above, take care of both?


> ! (defface isearch-fail '((t (:foreground "Black" :background 
> "Plum")))
>     "Face for highlighting failed part in Isearch echo-area message."
>     :group 'isearch)
>   
>   (defcustom isearch-lazy-highlight t
> --- 231,245 ----
>     :group 'basic-faces)
>   (defvar isearch 'isearch)
>   
> ! (defface isearch-fail
> !   '((((class color) (min-colors 88))
> !      (:background "IndianRed1"))
> !     (((class color) (min-colors 16))
> !      (:background "red"))
> !     (((class color) (min-colors 8))
> !      (:background "red")))
>     "Face for highlighting failed part in Isearch echo-area message."
> +   :version "23.1"
>     :group 'isearch)

Re: highlight failed part of isearch input

[Juri Linkov]

> Personally, I disagree about the color, at least for a light background with
> unlimited colors available - it is too vivid (bright, saturated, loud). For
> that, Plum or some other pastel is a better default, IMO.

Plum is too washed-out and weak color to indicate the error.

> It's important to not only notice the failure but also easily read the
> text that is highlighted.

The failed search string on IndianRed1 is quite readable on a light
background as well as on a dark background.

> For a dark background, the color should presumably be quite dark, not
> bright.
>
> Also, I think it should specify a foreground color by default, for the case
> where someone uses a different foreground color for a standalone minibuffer.

I think it is better to use the default foreground color, and if it is
unreadable then the user can customize it to some other color that fits
to user's color theme.

> How about something like this? The default color here for a dark background
> is the complement of Plum (a light violet): a dark green. You certainly
> don't want something like IndianRed1 or Plum on a dark background, I expect.

I'm not against selecting a more suitable color on a dark background,
but a dark green definitely doesn't fit semantically and is unnoticeable.

> BTW - I'm no expert on face specs, but isn't your duplication of the red
> spec for (min-colors 8) and (min-colors 16) redundant? Doesn't (min-colors
> 8), as shown above, take care of both?

I don't know a reason for this duplication.  I took the definition from
the `isearch' face that has this duplication.

-- 
Juri Linkov
http://www.jurta.org/emacs/

RE: highlight failed part of isearch input

[Drew Adams]

> > Personally, I disagree about the color, at least for a 
> > light background with unlimited colors available - it
> > is too vivid (bright, saturated, loud). For that,
> > Plum or some other pastel is a better default, IMO.
> 
> Plum is too washed-out and weak color to indicate the error.

OK, we disagree. Maybe others, besides we two, have an opinion?

I think we want a washed-out color (Plum or another) to indicate this
"error". It should scarcely be noticeable, and it should certainly not be
distracting.

This is not about signaling some fatal error, it is about indicating a
typing mistake. I doubt most people want sirens to go off to alert them of
such a typo.

> > It's important to not only notice the failure but also 
> > easily read the text that is highlighted.
> 
> The failed search string on IndianRed1 is quite readable on a light
> background as well as on a dark background.

Again, let's try to get some more opinions on this. To me, this highlighting
should be only a subtle help - it shouldn't get in the way. And I don't find
black text on IndianRed1 to be very readable.

> > For a dark background, the color should presumably be quite 
> > dark, not bright.
> >
> > Also, I think it should specify a foreground color by 
> > default, for the case where someone uses a different
> > foreground color for a standalone minibuffer.
> 
> I think it is better to use the default foreground color,
> and if it is unreadable then the user can customize it
> to some other color that fits to user's color theme.

OK. 

But all the more reason, then, not to use the same highlight color for light
and dark background displays. And all the more reason for the highlight
color to not vary too much in value (brightness) from the default background
in each case. A pastel highlight is good for a light background, and a
darkish highlight is good for a dark background.

I don't care which hue we choose, but pastel for light background and
darkish for dark background is important, I think. We should stay away from
mid-range values.

> > How about something like this? The default color here for a 
> > dark background is the complement of Plum (a light violet):
> > a dark green. You certainly don't want something like
> > IndianRed1 or Plum on a dark background, I expect.
> 
> I'm not against selecting a more suitable color on a dark
> background, but a dark green definitely doesn't fit
> semantically and is unnoticeable.

Semantically? Do you mean fire-engine red as in "EMERGENCY!" vs green as in
"Go, it's agreen light?" ;-)

Looking for semantics in the color used here is stretching it. Better to
just look for something that works visually: dark enough, but not too dark,
etc. 

I don't care which hue (green, red, blue, magenta, cyan, yellow) we use, but
the difference in value (brightness) from the normal background should be
slight - noticeable, but slight.

> > BTW - I'm no expert on face specs, but isn't your 
> > duplication of the red spec for (min-colors 8) and
> > (min-colors 16) redundant? Doesn't (min-colors 8),
> > as shown above, take care of both?
> 
> I don't know a reason for this duplication.  I took the 
> definition from the `isearch' face that has this duplication.

Hm. Maybe someone who is wise in these things can light our lanterns.

Re: highlight failed part of isearch input

[Juri Linkov]

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

> I don't care which hue (green, red, blue, magenta, cyan, yellow) we use, but
> the difference in value (brightness) from the normal background should be
> slight - noticeable, but slight.

I agree that IndianRed1 is too loud color, but OTOH Plum is too mild and
lifeless.  However, taking into account all your arguments it is possible
to find a suitable color on the whole color space.

First, I think we should use red hue since this is used for error strings
usually.  As for lightness, on a light background it is better to shift it
to the maximum, because darker colors looks dirty on a light background.
So we are left with the single axis of saturation.  There we have three
named colors: "IndianRed1", "brown1" and "RosyBrown1".  First two are
too saturated, and "RosyBrown1" (#ffc1c1) is too close to the default
background color.  So I think we could use a color with the numeric notation
#ff8888.  This color looks good, and black text on it is readable.

Here is the comparison of #ff8888 (Current) with Plum (Old):

[-- Attachment #2: ff8888 --]
[-- Type: image/png, Size: 7623 bytes --]

[-- Attachment #3: Type: text/plain, Size: 1524 bytes --]


For a dark background, I think we should use the maximal saturation, and
pick a color on the lightness-darkness axis.  The complement for #ff8888
on this axis is "red4" which is dark enough.  I checked that it looks good
on a dark background:

Index: lisp/isearch.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/isearch.el,v
retrieving revision 1.310
diff -c -r1.310 isearch.el
*** lisp/isearch.el	12 Feb 2008 00:50:44 -0000	1.310
--- lisp/isearch.el	24 Feb 2008 17:30:47 -0000
***************
*** 231,238 ****
    :group 'basic-faces)
  (defvar isearch 'isearch)
  
! (defface isearch-fail '((t (:foreground "Black" :background "Plum")))
    "Face for highlighting failed part in Isearch echo-area message."
    :group 'isearch)
  
  (defcustom isearch-lazy-highlight t
--- 231,250 ----
    :group 'basic-faces)
  (defvar isearch 'isearch)
  
! (defface isearch-fail
!   '((((class color) (min-colors 88) (background light))
!      (:background "#ff8888"))
!     (((class color) (min-colors 88) (background dark))
!      (:background "red4"))
!     (((class color) (min-colors 16))
!      (:background "red"))
!     (((class color) (min-colors 8))
!      (:background "red"))
!     (((class color grayscale))
!      :foreground "grey")
!     (t (:inverse-video t)))
    "Face for highlighting failed part in Isearch echo-area message."
+   :version "23.1"
    :group 'isearch)
  
  (defcustom isearch-lazy-highlight t

-- 
Juri Linkov
http://www.jurta.org/emacs/

RE: highlight failed part of isearch input

[Drew Adams]

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

> > I don't care which hue (green, red, blue, magenta, cyan, 
> > yellow) we use, but the difference in value (brightness)
> > from the normal background should be
> > slight - noticeable, but slight.
>
> I agree that IndianRed1 is too loud color, but OTOH Plum is 
> too mild and lifeless.  However, taking into account all
> your arguments it is possible to find a suitable color on
> the whole color space.
> 
> First, I think we should use red hue since this is used for 
> error strings usually. As for lightness, on a light background
> it is better to shift it to the maximum, because darker colors
> looks dirty on a light background.
>
> As for lightness, on a light background it is better to shift it
> to the maximum, because darker colors looks dirty on a light 
> background.
>
> So we are left with the single axis of saturation.  There we 
> have three named colors: "IndianRed1", "brown1" and "RosyBrown1".
> First two are too saturated, and "RosyBrown1" (#ffc1c1) is too
> close to the default background color.
>
> So I think we could use a color with the numeric notation #ff8888.
> This color looks good, and black text on it is readable.
> Here is the comparison of #ff8888 (Current) with Plum (Old):

Juri, no one else seems to care, so please pick whatever color you like.

FWIW - The color you prefer, #FF8888, seems to be a more saturated version
of RosyBrown1. Attached are comparisons of RosyBrown1 (on top) with your
preference (on the bottom), and of your preference (on top) with Plum (on
the bottom).

You apparently want this highlighting to stand out more than I intended it
to. I think a lighter color, such as RosyBrown1, is better as the default on
a white background, but, again, pick whatever color you want. 


[-- Attachment #2: isearch-juri.png --]
[-- Type: image/png, Size: 16755 bytes --]

[-- Attachment #3: isearch-juri-plum.png --]
[-- Type: image/png, Size: 17022 bytes --]

Re: highlight failed part of isearch input

[Juri Linkov]

> You apparently want this highlighting to stand out more than I intended it
> to. I think a lighter color, such as RosyBrown1, is better as the default on
> a white background, but, again, pick whatever color you want.

OK, I agree that less saturated color RosyBrown1 would be better.  Done.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: highlight failed part of isearch input

[Bastien]

"Drew Adams" <drew.adams@oracle.com> writes:

>> So I think we could use a color with the numeric notation #ff8888.
>> This color looks good, and black text on it is readable.
>> Here is the comparison of #ff8888 (Current) with Plum (Old):
>
> Juri, no one else seems to care, so please pick whatever color you
> like.

FWIW I would inherit the `font-lock-warning-face' face, since this is
supposed to indicate an error of some kind.

-- 
Bastien

Re: highlight failed part of isearch input

[Dan Nicolaescu]

Juri Linkov <juri@jurta.org> writes:

  > > I don't care which hue (green, red, blue, magenta, cyan, yellow) we use, but
  > > the difference in value (brightness) from the normal background should be
  > > slight - noticeable, but slight.
  > 
  > I agree that IndianRed1 is too loud color, but OTOH Plum is too mild and
  > lifeless.  However, taking into account all your arguments it is possible
  > to find a suitable color on the whole color space.
  > 
  > First, I think we should use red hue since this is used for error strings
  > usually.  As for lightness, on a light background it is better to shift it
  > to the maximum, because darker colors looks dirty on a light background.
  > So we are left with the single axis of saturation.  There we have three
  > named colors: "IndianRed1", "brown1" and "RosyBrown1".  First two are
  > too saturated, and "RosyBrown1" (#ffc1c1) is too close to the default
  > background color.  So I think we could use a color with the numeric notation
  > #ff8888.  This color looks good, and black text on it is readable.

Please no #f!#%@, use a readable color name, there's plenty of them...

RE: highlight failed part of isearch input

[Drew Adams]

> Please no #f!#%@, use a readable color name, there's plenty of them...

Why?

The user sees the color in Customize (and in isearch). What's the problem
with using an unnamed color as the default?

FWIW, a name such as "peru" tells me less about a color than its RGB name:
#FFFFA7BA4E60. That at least says that there is a max of red, quite a bit of
green, and a little blue. Red + green = yellow/brown, so you have some idea
what to expect. With the color name "peru" you might expect a ceviche lime
green or a Pacific blue or anything else - no help at all.

Such hex codes are second nature to those who design and implement with Web
pages (which includes a lot of programmers), and even for novices they can
say as much or more about a color as an English name.

But any name or coding is not that helpful to get an idea of a color - you
have to see it.

I don't think we should pick colors for UI design (e.g. defaults) based on
their names, but based on their properties. Pick the right color, regardless
of what convention we use to name it.

Re: highlight failed part of isearch input

[Dan Nicolaescu]

"Drew Adams" <drew.adams@oracle.com> writes:

  > > Please no #f!#%@, use a readable color name, there's plenty of them...
  > 
  > Why?

Because it's beyond ugly. 

Re: highlight failed part of isearch input

[Jason Rumney]

Drew Adams wrote:

> I don't think we should pick colors for UI design (e.g. defaults) based on
> their names, but based on their properties. Pick the right color, regardless
> of what convention we use to name it.
>   

Colors are to the eye as musical notes are to the ear. The ones that 
look pleasing to the eye have names, and you'd be well advised to use 
them. Defining colors for a UI using hex RGB numbers is the same as 
composing music using frequencies.

RE: highlight failed part of isearch input

[Drew Adams]

> > I don't think we should pick colors for UI design (e.g. 
> > defaults) based on their names, but based on their properties.
> > Pick the right color, regardless of what convention we use
> > to name it.
> 
> Colors are to the eye as musical notes are to the ear. The ones that 
> look pleasing to the eye have names, and you'd be well advised to use 
> them. Defining colors for a UI using hex RGB numbers is the same as 
> composing music using frequencies.

Do as you like, but that, I'm afraid, is pure nonsense.

The part about "the ones that look pleasing to the eye", in particular.
Sounds like Joseph II's "too many notes" comment about the Marriage of
Figaro.

Again, do as you like. Pick "Hello Kitty Pink" if you like - quite pleasing,
and it should be recognizable from Xinjiang to Xixuanbanna, the long way
'round.

Re: highlight failed part of isearch input

[Juri Linkov]

>> I don't think we should pick colors for UI design (e.g. defaults) based on
>> their names, but based on their properties. Pick the right color, regardless
>> of what convention we use to name it.
>
> Colors are to the eye as musical notes are to the ear. The ones that look
> pleasing to the eye have names, and you'd be well advised to use
> them. Defining colors for a UI using hex RGB numbers is the same as
> composing music using frequencies.

I'd like to know the history of rgb.txt, and why unlike musical notes,
color names and values in this file are selected at irregular intervals
without a conceivable logic.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: highlight failed part of isearch input

[Juri Linkov]

>> There is one regression after installing this patch: `C-s M-p' now
>> doesn't put the previous search string from the search ring to the
>> isearch minibuffer.
>
> The patch below fixes this problem.  When the first message from the
> isearch-cmds stack is not the same as isearch-message (this happens when
> isearch-edit-string sets a different value) then it uses this value
> for succ-msg.

I discovered another problem that requires a different fix.

`C-s foo M-r' (in a buffer without this text) completely discards the
failed part and doesn't display it.

The best fix for this and related problems is to always keep the
original isearch-message intact, and just to add text properties to it.

The patch below also fixed another problem.  `C-M-s [a-z]' (typed
in the empty buffer) highlights as failed part only the closing bracket.
This patch uses `isearch-error' to highlight the whole `[a-z]' part:

Index: lisp/isearch.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/isearch.el,v
retrieving revision 1.310
diff -c -r1.310 isearch.el
*** lisp/isearch.el	12 Feb 2008 00:50:44 -0000	1.310
--- lisp/isearch.el	24 Feb 2008 17:28:43 -0000
***************
*** 1959,1980 ****
  (defun isearch-message (&optional c-q-hack ellipsis)
    ;; Generate and print the message string.
    (let ((cursor-in-echo-area ellipsis)
!           (cmds isearch-cmds)
!           succ-msg m)
!       (while (not (isearch-success-state (car cmds))) (pop cmds))
!       (setq succ-msg (and cmds (isearch-message-state (car cmds))))
!       (setq m (concat
! 	    (isearch-message-prefix c-q-hack ellipsis isearch-nonincremental)
!                succ-msg
!                (and (not isearch-success)
!                     (string-match (regexp-quote succ-msg) isearch-message)
!                     (not (string= succ-msg isearch-message))
!                     (propertize (substring isearch-message (match-end 0))
!                                 'face 'isearch-fail))))
!       (when (and (not isearch-success) (string-match " +$" m))
!         (put-text-property (match-beginning 0) (length m) 'face 'trailing-whitespace m))
!       (setq m (concat m (isearch-message-suffix c-q-hack ellipsis)))
!       (if c-q-hack m (let ((message-log-max nil)) (message "%s" m)))))
  
  (defun isearch-message-prefix (&optional c-q-hack ellipsis nonincremental)
    ;; If about to search, and previous search regexp was invalid,
--- 1979,2006 ----
  (defun isearch-message (&optional c-q-hack ellipsis)
    ;; Generate and print the message string.
    (let ((cursor-in-echo-area ellipsis)
! 	(m isearch-message)
! 	(cmds isearch-cmds)
! 	succ-msg)
!     (when (or (not isearch-success) isearch-error)
!       ;; Highlight failed part
!       (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)))
! 	    m (copy-sequence m))
!       (when (and (stringp succ-msg) (< (length succ-msg) (length m)))
! 	(add-text-properties (length succ-msg) (length m)
! 			     '(face isearch-fail) m))
!       ;; Highlight failed trailing whitespace
!       (when (string-match " +$" m)
! 	(add-text-properties (match-beginning 0) (match-end 0)
! 			     '(face trailing-whitespace) m)))
!     (setq m (concat
! 	     (isearch-message-prefix c-q-hack ellipsis isearch-nonincremental)
! 	     m
! 	     (isearch-message-suffix c-q-hack ellipsis)))
!     (if c-q-hack m (let ((message-log-max nil)) (message "%s" m)))))
  
  (defun isearch-message-prefix (&optional c-q-hack ellipsis nonincremental)
    ;; If about to search, and previous search regexp was invalid,

-- 
Juri Linkov
http://www.jurta.org/emacs/

RE: highlight failed part of isearch input

[Drew Adams]

Patch looks good. Thx.