isearch highlighting

isearch highlighting

[Juri Linkov]

I'd like to fix the following 3 problems in isearch highlighting:

1. When Emacs hangs while searching with isearch for a regexp with
nested repetitions, it can be interrupted with C-g.  Fine.  But when
it hangs while highlighting other occurrences of the search string with
isearch-lazy-highlight invoked by a timer, it can't be interrupted.
I think it is safe to put the body of `isearch-lazy-highlight-update'
in the `with-local-quit' block, because quit in this function can't
lead to a corrupted state.  It seems this change is unrelated to the
proposed solution of a new form of really hard C-g (which is useful to
interrupt the code which can put the process in a corrupted state).
Also there is no urgent need to detect infinite loops in regex.c,
because most regexp in Emacs packages are well tested and contains no
nested repetitions.  The only weak point is isearch (and query-replace)
where regexps are typed manually.  It should be possible to interrupt
isearch lazy highlighting with normal C-g.

2.  There is one problem in fontification of the minibuffer in isearch
mode.  When a sequence of a multi-character input method is unfinished
and the minibuffer is active, then the search string is highlighted by
isearch lazy highlighting in the minibuffer instead of the source
buffer.  To reproduce this bug, type:

C-x RET C-\ latin-1-postfix RET C-s a a

This could be fixed in `isearch-lazy-highlight-update' by selecting
the window where isearch was activated.

3. While typing a sequence of a multi-character input method in the
isearch minibuffer, the first part of the isearch string is
highlighted in `minibuffer-prompt' face, because it is used as the
part of the prompt.  Instead, it could be inserted into the minibuffer
as initial-contents.

Index: lisp/isearch.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/isearch.el,v
retrieving revision 1.244
diff -u -w -b -r1.244 isearch.el
--- lisp/isearch.el	6 Dec 2004 15:12:08 -0000	1.244
+++ lisp/isearch.el	9 Dec 2004 23:09:07 -0000
@@ -2385,6 +2386,11 @@
   (let ((max isearch-lazy-highlight-max-at-a-time)
         (looping t)
         nomore)
+    (with-local-quit
+      (save-selected-window
+	(if (and (window-live-p isearch-lazy-highlight-window)
+		 (not (eq (selected-window) isearch-lazy-highlight-window)))
+	    (select-window isearch-lazy-highlight-window))
     (save-excursion
       (save-match-data
         (goto-char (if isearch-forward
@@ -2437,7 +2443,7 @@
         (unless nomore
           (setq isearch-lazy-highlight-timer
                 (run-at-time isearch-lazy-highlight-interval nil
-                             'isearch-lazy-highlight-update)))))))
+				 'isearch-lazy-highlight-update)))))))))
 
 (defun isearch-resume (search regexp word forward message case-fold)
   "Resume an incremental search.

Index: lisp/international/isearch-x.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/international/isearch-x.el,v
retrieving revision 1.17
diff -u -r1.17 isearch-x.el
--- lisp/international/isearch-x.el	1 Sep 2003 15:45:28 -0000	1.17
+++ lisp/international/isearch-x.el	9 Dec 2004 23:16:02 -0000
@@ -97,7 +97,7 @@
 (defun isearch-process-search-multibyte-characters (last-char)
   (if (eq this-command 'isearch-printing-char)
       (let ((overriding-terminal-local-map nil)
-	    (prompt (concat (isearch-message-prefix) isearch-message))
+	    (prompt (concat (isearch-message-prefix)))
 	    (minibuffer-local-map isearch-minibuffer-local-map)
 	    str)
 	(if isearch-input-method-function
@@ -107,11 +107,12 @@
 		    (cons 'with-input-method
 			  (cons last-char unread-command-events))
 		    ;; Inherit current-input-method in a minibuffer.
-		    str (read-string prompt nil nil nil t))
+		    str (read-string prompt isearch-message nil nil t))
 	      (if (not str)
 		  ;; All inputs were deleted while the input method
 		  ;; was working.
 		  (setq str "")
+		(setq str (substring str (length isearch-message)))
 		(if (and (= (length str) 1)
 			 (= (aref str 0) last-char)
 			 (>= last-char 128))

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

Re: isearch highlighting

[Stefan Monnier]

> I think it is safe to put the body of `isearch-lazy-highlight-update'
> in the `with-local-quit' block, because quit in this function can't
> lead to a corrupted state.

Agreed.  I'd even put it in `while-no-input'.  You might want to add the
patch below, tho, just so the code can't be interrupted after placing the
new overlay but before it's recorded on the list.  There might be other
similar issues, I haven't checked.

> It seems this change is unrelated to the
> proposed solution of a new form of really hard C-g (which is useful to
> interrupt the code which can put the process in a corrupted state).

Of course.  The idea of a "really hard C-g" is to work around problems,
but it should not be an excuse to not fix those problems.

> because most regexp in Emacs packages are well tested and contains no
> nested repetitions.

Is that an attempt at humor?


        Stefan


--- orig/lisp/isearch.el
+++ mod/lisp/isearch.el
@@ -2480,10 +2480,10 @@
 
                     ;; non-zero-length match
                     (let ((ov (make-overlay mb me)))
+                      (push ov isearch-lazy-highlight-overlays)
                       (overlay-put ov 'face isearch-lazy-highlight-face)
                       (overlay-put ov 'priority 0) ;lower than main overlay
-                      (overlay-put ov 'window (selected-window))
-                      (push ov isearch-lazy-highlight-overlays)))
+                      (overlay-put ov 'window (selected-window))))
                   (if isearch-forward
                       (setq isearch-lazy-highlight-end (point))
                     (setq isearch-lazy-highlight-start (point)))))

Re: isearch highlighting

[Juri Linkov]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> I think it is safe to put the body of `isearch-lazy-highlight-update'
>> in the `with-local-quit' block, because quit in this function can't
>> lead to a corrupted state.
>
> Agreed.  I'd even put it in `while-no-input'.

Yes, `while-no-input' would be appropriate here, especially on
slow machines.  But I see no such thing in Emacs CVS.  I only see it
in emacs-devel archives where it was heavily discussed two years ago.

> You might want to add the patch below, tho, just so the code can't
> be interrupted after placing the new overlay but before it's
> recorded on the list.

OK.

>> because most regexps in Emacs packages are well tested and contains no
>> nested repetitions.
>
> Is that an attempt at humor?

I looked through rose-colored glasses :-)

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

RE: isearch highlighting

[klaus.berndl]

Stefan Monnier wrote:
>> I think it is safe to put the body of `isearch-lazy-highlight-update'
>> in the `with-local-quit' block, because quit in this function can't
>> lead to a corrupted state.
> 
> Agreed.  I'd even put it in `while-no-input'.  You might want to add

OT: you mention `while-no-input' whic remembers me to a discussion about this.
is this macro now available or have i misunderstood here something??

Klaus

> the patch below, tho, just so the code can't be interrupted after
> placing the new overlay but before it's recorded on the list.  There
> might be other similar issues, I haven't checked.
> 
>> It seems this change is unrelated to the
>> proposed solution of a new form of really hard C-g (which is useful
>> to interrupt the code which can put the process in a corrupted
>> state). 
> 
> Of course.  The idea of a "really hard C-g" is to work around
> problems, 
> but it should not be an excuse to not fix those problems.
> 
>> because most regexp in Emacs packages are well tested and contains no
>> nested repetitions.
> 
> Is that an attempt at humor?
> 
> 
>         Stefan
> 
> 
> --- orig/lisp/isearch.el
> +++ mod/lisp/isearch.el
> @@ -2480,10 +2480,10 @@
> 
>                      ;; non-zero-length match
>                      (let ((ov (make-overlay mb me)))
> +                      (push ov isearch-lazy-highlight-overlays)
>                        (overlay-put ov 'face
>                        isearch-lazy-highlight-face) (overlay-put ov
> 'priority 0) ;lower than main overlay 
> -                      (overlay-put ov 'window (selected-window))
> -                      (push ov isearch-lazy-highlight-overlays)))
> +                      (overlay-put ov 'window (selected-window))))
>                    (if isearch-forward
>                        (setq isearch-lazy-highlight-end (point))
>                      (setq isearch-lazy-highlight-start (point)))))
> 
> 
> _______________________________________________
> Emacs-devel mailing list
> Emacs-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: isearch highlighting

[Richard Stallman]

    1. When Emacs hangs while searching with isearch for a regexp with
    nested repetitions, it can be interrupted with C-g.  Fine.  But when
    it hangs while highlighting other occurrences of the search string with
    isearch-lazy-highlight invoked by a timer, it can't be interrupted.
    I think it is safe to put the body of `isearch-lazy-highlight-update'
    in the `with-local-quit' block, because quit in this function can't
    lead to a corrupted state.

I just installed the code for while-no-input.  I think you should
use that.

    C-x RET C-\ latin-1-postfix RET C-s a a

    This could be fixed in `isearch-lazy-highlight-update' by selecting
    the window where isearch was activated.

That seems reasonable, but if the idea is to call the
isearch-lazy-highlight code from query-replace as well, please verify
that this fix is correct for that case too.  I have no specific reason
to think it is wrong, I'm just asking you to double-check.

    3. While typing a sequence of a multi-character input method in the
    isearch minibuffer, the first part of the isearch string is
    highlighted in `minibuffer-prompt' face, because it is used as the
    part of the prompt.  Instead, it could be inserted into the minibuffer
    as initial-contents.

I suppose you've tested that patch; but I wonder, does this lead
to a danger of being able to edit that initial contents?

Re: isearch highlighting

[Juri Linkov]

Richard Stallman <rms@gnu.org> writes:
> I just installed the code for while-no-input.  I think you should
> use that.

I will see how well it works with isearch lazy highlighting timer.

>     This could be fixed in `isearch-lazy-highlight-update' by selecting
>     the window where isearch was activated.
>
> That seems reasonable, but if the idea is to call the
> isearch-lazy-highlight code from query-replace as well, please verify
> that this fix is correct for that case too.  I have no specific reason
> to think it is wrong, I'm just asking you to double-check.

When I proposed this fix, one of the reasons was to make it work
with query-replace too, where the minibuffer is often activated.
So yes, it works with query-replace.

>     3. While typing a sequence of a multi-character input method in the
>     isearch minibuffer, the first part of the isearch string is
>     highlighted in `minibuffer-prompt' face, because it is used as the
>     part of the prompt.  Instead, it could be inserted into the minibuffer
>     as initial-contents.
>
> I suppose you've tested that patch; but I wonder, does this lead
> to a danger of being able to edit that initial contents?

No danger because input method functions don't allow to delete
more previous characters than typed after invocation of the
current input method.

There is also another `read-string' in `isearch-process-search-multibyte-characters'.
but I haven't added `isearch-message' as an initial contents for it,
because I think that no more than one character can be entered
with keyboard-coding-system, and this one character is pushed to
unread-command-events, so `read-string' exits immediately after
starting, and users will not see an incomplete search string
in the minibuffer.

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

Re: isearch highlighting

[Richard Stallman]

Ok, please install your highlighting fixes.

Re: isearch highlighting

[Juri Linkov]

Richard Stallman <rms@gnu.org> writes:
> I just installed the code for while-no-input.  I think you should
> use that.

I tried while-no-input, but it doesn't work as intended.

Inside a long loop matching a regexp with nested repetition operators
only C-g quits the body, not any key.

The simplest example demonstrating this:

(while-no-input (string-match "<\\( *\\)+>" "<                            "))

But in other cases it works well, e.g.

(while-no-input (while t))

quits when any key is typed.

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

Re: isearch highlighting

[Richard Stallman]

    Inside a long loop matching a regexp with nested repetition operators
    only C-g quits the body, not any key.

    The simplest example demonstrating this:

    (while-no-input (string-match "<\\( *\\)+>" "<                            "))

The reason is that while-no-input doesn't work with
immediate_quit.

Does this patch make it work well?

*** keyboard.c	16 Dec 2004 07:02:15 -0500	1.804
--- keyboard.c	20 Dec 2004 04:30:52 -0500	
***************
*** 3717,3723 ****
        && event->kind != FOCUS_IN_EVENT
        && event->kind != HELP_EVENT
        && event->kind != DEICONIFY_EVENT)
!     Vquit_flag = Vthrow_on_input;
  }
  
  
--- 3717,3727 ----
        && event->kind != FOCUS_IN_EVENT
        && event->kind != HELP_EVENT
        && event->kind != DEICONIFY_EVENT)
!     {
!       Vquit_flag = Vthrow_on_input;
!       if (immediate_quit && NILP (Vinhibit_quit))
! 	QUIT;
!     }
  }

Re: isearch highlighting

[Juri Linkov]

Richard Stallman <rms@gnu.org> writes:
>     Inside a long loop matching a regexp with nested repetition operators
>     only C-g quits the body, not any key.
>
>     The simplest example demonstrating this:
>
>     (while-no-input (string-match "<\\( *\\)+>" "<                            "))
>
> The reason is that while-no-input doesn't work with
> immediate_quit.
>
> Does this patch make it work well?

No.  The first key ends the execution of body.  This part is fixed,
right.  But during reading the next key (when control returns to the
top command loop) it goes into an infinite loop in
wait_reading_process_output not interruptible even with C-g.

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

Re: isearch highlighting

[Juri Linkov]

>> The reason is that while-no-input doesn't work with
>> immediate_quit.
>>
>> Does this patch make it work well?
>
> No.  The first key ends the execution of body.  This part is fixed,
> right.  But during reading the next key (when control returns to the
> top command loop) it goes into an infinite loop in
> wait_reading_process_output not interruptible even with C-g.

`while-no-input' is more reliable now, but once I managed to put
it to an infinite loop in wait_reading_process_output.  I can't repeat
this anymore.  Whereas I can look at it more, I want to note that
using `while-no-input' in `isearch-lazy-highlight-update' instead of
`with-local-quit' will require major changes in `isearch-lazy-highlight-update'.

`with-local-quit' was added for very rare cases where search functions
go into an infinite loop and so interrupting them with C-g and leaving
the rest of matches unhighlighted is much better than killing Emacs.
Trying to continue highlighting of other matches in this case will
likely put Emacs into another infinite loop.

But `while-no-input', where any normal key (including most frequent
C-s repeating the search) can quit the `isearch-lazy-highlight-update',
requires significant changes: after quitting it should restart the
timer and restore everything (global variables and overlays) to the
initial state.  This is the only way I see to avoid incomplete states.

Is it worth doing this now considering that the default value of
lazy-highlight-max-at-a-time (20) already provides quite good
responsiveness of isearch to key presses.

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

Re: isearch highlighting

[Richard Stallman]

    Is it worth doing this now considering that the default value of
    lazy-highlight-max-at-a-time (20) already provides quite good
    responsiveness of isearch to key presses.

I am not sure, but someone complained about the current code before.
What was the complaint?

Re: isearch highlighting

[Stefan Monnier]

> Is it worth doing this now considering that the default value of
> lazy-highlight-max-at-a-time (20) already provides quite good
> responsiveness of isearch to key presses.

If it's responsive enough, then it won't be interrupted by while-no-input.
IIRC there are pathological cases where it's not responsive enough, in which
case while-no-input may help.


        Stefan

Re: isearch highlighting

[Richard Stallman]

    > Is it worth doing this now considering that the default value of
    > lazy-highlight-max-at-a-time (20) already provides quite good
    > responsiveness of isearch to key presses.

    If it's responsive enough, then it won't be interrupted by while-no-input.

That is not entirely true.  If it spends even a tiny amount of time
being busy, then it can sometimes be interrupted by while-no-input.
So we would need to do the work to make it use while-no-input
correctly, even if those cases are rather rare.

What does it need to do?  Any input character that triggers
while-no-input will certainly be intepreted by the search command
loop.  Most of the time, this will change the search string.

Re: isearch highlighting

[Juri Linkov]

Richard Stallman <rms@gnu.org> writes:
>     If it's responsive enough, then it won't be interrupted by while-no-input.
>
> That is not entirely true.  If it spends even a tiny amount of time
> being busy, then it can sometimes be interrupted by while-no-input.
> So we would need to do the work to make it use while-no-input
> correctly, even if those cases are rather rare.

My tests show that those cases are not rare, especially with rapid
repeated presses of C-s.

> What does it need to do?  Any input character that triggers
> while-no-input will certainly be intepreted by the search command
> loop.  Most of the time, this will change the search string.

With the change of the search string it has nothing to do other than
removing all already highlighted overlays and starting the new
highlighting loop.  With an input character that doesn't modify
the search string (e.g. C-s) the case is more complicated: it
should remove a part of created highlighting overlays (that were
added in the current invocation of `isearch-lazy-highlight-update')
and to restore the previous values of isearch-lazy-highlight-start,
isearch-lazy-highlight-end, isearch-lazy-highlight-window-start,
isearch-lazy-highlight-window-end, isearch-lazy-highlight-wrapped,
and to restart the timer.

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