bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page

bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page

[Alan Mackenzie]

Emacs 24.2.92.

emacs -Q
C-u C-h i
Enter path/to/elisp.info <CR>
g Syntax Table Internals

1. Place point at the start of the first paragraph ("Syntax tables are
implemented ...").  Attempt C-s M-s C-e (isearch-yank-line).  This
produces the error message:

Failing I-search: syntax tables are implemented as char-tables (*note char-tables::), but [end of node]

This is a bug.

2. Place point at the start of the second paragraph ("Each entry in a
....").  Attempt C-s M-s C-e (isearch-yeank-line).  This works, but
wrongly highlights the gap preceding the paragraph with lazy-highlight
face.


-- 
Alan Mackenzie (Nuremberg, Germany).

bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page

[Andreas Schwab]

Alan Mackenzie <acm@muc.de> writes:

> 2. Place point at the start of the second paragraph ("Each entry in a
> ....").  Attempt C-s M-s C-e (isearch-yeank-line).  This works, but
> wrongly highlights the gap preceding the paragraph with lazy-highlight
> face.

That's due to isearch-lax-whitespace (try M-s SPC).

Andreas.

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

bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page

[Juri Linkov]

> C-u C-h i
> Enter path/to/elisp.info <CR>
> g Syntax Table Internals
>
> 1. Place point at the start of the first paragraph ("Syntax tables are
> implemented ...").  Attempt C-s M-s C-e (isearch-yank-line).  This
> produces the error message:
>
> Failing I-search: syntax tables are implemented as char-tables (*note char-tables::), but [end of node]

Info hides the text "*note Char-Tables::" and uses the text property `display'
to put another text "(see Char-Tables)" instead.  Some users complained
when isearch-yank-line yanked invisible text "*note", so now it ignores
invisible text.  But it seems ignoring invisible text is worse than yanking.
So perhaps `Info-isearch-filter' shouldn't skip invisible text.

bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page

[Alan Mackenzie]

Hello, Juri.

On Fri, Jan 11, 2013 at 02:43:16AM +0200, Juri Linkov wrote:
> > C-u C-h i
> > Enter path/to/elisp.info <CR>
> > g Syntax Table Internals

> > 1. Place point at the start of the first paragraph ("Syntax tables are
> > implemented ...").  Attempt C-s M-s C-e (isearch-yank-line).  This
> > produces the error message:

> > Failing I-search: syntax tables are implemented as char-tables (*note char-tables::), but [end of node]

> Info hides the text "*note Char-Tables::" and uses the text property `display'
> to put another text "(see Char-Tables)" instead.  Some users complained
> when isearch-yank-line yanked invisible text "*note", so now it ignores
> invisible text.  But it seems ignoring invisible text is worse than yanking.
> So perhaps `Info-isearch-filter' shouldn't skip invisible text.

I think it should either skip it properly, or not at all.  There is an
inconsistency here between parts of isearch.  C-s C-w also produces
these error messages, and the "[end of node]" is just wierd, from a
user's point of view.

I think this should be fixed, somehow, for Emacs 24.3.  I would agree
with your suggestion that isearch should simply yank the invisible text.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page

[Juri Linkov]

> I think it should either skip it properly, or not at all.  There is an
> inconsistency here between parts of isearch.

This can be fixed by the patch below that takes into account
the default value `open' of `search-invisible' and treats it the same
as its value `t', i.e. matches hidden text (but it still can't "open" it
in a meaningful way).

> C-s C-w also produces these error messages, and the "[end of node]"
> is just wierd, from a user's point of view.

I have no idea for a better error message.

> I think this should be fixed, somehow, for Emacs 24.3.  I would agree
> with your suggestion that isearch should simply yank the invisible text.

This is not a regression, so this patch is intended for trunk:

=== modified file 'lisp/info.el'
--- lisp/info.el	2013-02-12 07:57:04 +0000
+++ lisp/info.el	2013-02-13 17:52:53 +0000
@@ -2162,7 +2162,7 @@ (defun Info-isearch-filter (beg-found fo
     (let ((backward (< found beg-found)))
       (not
        (or
-	(and (not (eq search-invisible t))
+	(and (null search-invisible)
 	     (if backward
 		 (or (text-property-not-all found beg-found 'invisible nil)
 		     (text-property-not-all found beg-found 'display nil))

bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page

[Alan Mackenzie]

Hi, Juri.

On Wed, Feb 13, 2013 at 07:53:28PM +0200, Juri Linkov wrote:
> > I think it should either skip it properly, or not at all.  There is an
> > inconsistency here between parts of isearch.

> This can be fixed by the patch below that takes into account
> the default value `open' of `search-invisible' and treats it the same
> as its value `t', i.e. matches hidden text (but it still can't "open" it
> in a meaningful way).

OK.  The patch seems to work.

> > I think this should be fixed, somehow, for Emacs 24.3.  I would agree
> > with your suggestion that isearch should simply yank the invisible text.

> This is not a regression, so this patch is intended for trunk:

Indeed, I tried it on some older Emacsen.  It does go back some way.

> === modified file 'lisp/info.el'
> --- lisp/info.el	2013-02-12 07:57:04 +0000
> +++ lisp/info.el	2013-02-13 17:52:53 +0000
> @@ -2162,7 +2162,7 @@ (defun Info-isearch-filter (beg-found fo
>      (let ((backward (< found beg-found)))
>        (not
>         (or
> -	(and (not (eq search-invisible t))
> +	(and (null search-invisible)
>  	     (if backward
>  		 (or (text-property-not-all found beg-found 'invisible nil)
>  		     (text-property-not-all found beg-found 'display nil))

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page

[Juri Linkov]

>> > I think it should either skip it properly, or not at all.
>> > There is an inconsistency here between parts of isearch.
>>
>> This can be fixed by the patch below that takes into account
>> the default value `open' of `search-invisible' and treats it the same
>> as its value `t', i.e. matches hidden text (but it still can't "open" it
>> in a meaningful way).
>
> OK.  The patch seems to work.

Thanks for confirmation.  This is now installed in trunk and the bug closed.

bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch]

[Alan Mackenzie]

On Thu, Jan 10, 2013 at 01:25:30PM +0000, Alan Mackenzie wrote:
> Emacs 24.2.92.

> emacs -Q
> C-u C-h i
> Enter path/to/elisp.info <CR>
> g Syntax Table Internals

[ .... ]

> 2. Place point at the start of the second paragraph ("Each entry in a
> ....").  Attempt C-s M-s C-e (isearch-yeank-line).  This works, but
> wrongly highlights the gap preceding the paragraph with lazy-highlight
> face.

This wrong highlighting is a regression, introduced when a space in a
normal isearch was made to match any sequence of WS characters.

The bug's mechanism is that the current match in the buffer is
redundantly given lazy highlighting, although it is already highlighted
with isearch-face.  Sometimes the region being lazy-highlit is bigger
than the current match, and the lazy highlighting spills into the
surrounding area.  This is what is happening here.

The solution I propose is NOT to lazy-highlight when the LH region
overlaps with the current match.

Here is a patch for this change:



=== modified file 'lisp/isearch.el'
*** lisp/isearch.el	2013-02-01 23:38:41 +0000
--- lisp/isearch.el	2013-02-14 19:49:37 +0000
***************
*** 2862,2867 ****
--- 2862,2868 ----
  (defvar isearch-lazy-highlight-end-limit nil)
  (defvar isearch-lazy-highlight-start nil)
  (defvar isearch-lazy-highlight-end nil)
+ (defvar isearch-lazy-highlight-point nil)
  (defvar isearch-lazy-highlight-timer nil)
  (defvar isearch-lazy-highlight-last-string nil)
  (defvar isearch-lazy-highlight-window nil)
***************
*** 2938,2943 ****
--- 2939,2945 ----
  	  isearch-lazy-highlight-window-end   (window-end)
  	  isearch-lazy-highlight-start        (point)
  	  isearch-lazy-highlight-end          (point)
+ 	  isearch-lazy-highlight-point        (point)
  	  isearch-lazy-highlight-wrapped      nil
  	  isearch-lazy-highlight-last-string  isearch-string
  	  isearch-lazy-highlight-case-fold-search isearch-case-fold-search
***************
*** 3014,3033 ****
  		(if found
  		    (let ((mb (match-beginning 0))
  			  (me (match-end 0)))
! 		      (if (= mb me)	;zero-length match
! 			  (if isearch-lazy-highlight-forward
! 			      (if (= mb (if isearch-lazy-highlight-wrapped
! 					    isearch-lazy-highlight-start
! 					  (window-end)))
! 				  (setq found nil)
! 				(forward-char 1))
  			    (if (= mb (if isearch-lazy-highlight-wrapped
! 					  isearch-lazy-highlight-end
! 					(window-start)))
  				(setq found nil)
! 			      (forward-char -1)))
! 
! 			;; non-zero-length match
  			(let ((ov (make-overlay mb me)))
  			  (push ov isearch-lazy-highlight-overlays)
  			  ;; 1000 is higher than ediff's 100+,
--- 3016,3041 ----
  		(if found
  		    (let ((mb (match-beginning 0))
  			  (me (match-end 0)))
! 		      (cond
! 		       ((= mb me) 	;zero-length match
! 			(if isearch-lazy-highlight-forward
  			    (if (= mb (if isearch-lazy-highlight-wrapped
! 					  isearch-lazy-highlight-start
! 					(window-end)))
  				(setq found nil)
! 			      (forward-char 1))
! 			  (if (= mb (if isearch-lazy-highlight-wrapped
! 					isearch-lazy-highlight-end
! 				      (window-start)))
! 			      (setq found nil)
! 			    (forward-char -1))))
! 
! 		       ((if isearch-lazy-highlight-forward
! 			    (or (<= me isearch-other-end)
! 				(>= mb isearch-lazy-highlight-point))
! 			  (or (<= me isearch-lazy-highlight-point)
! 			      (>= mb isearch-other-end)))
! 			;; non-zero-length match not overlapping found string
  			(let ((ov (make-overlay mb me)))
  			  (push ov isearch-lazy-highlight-overlays)
  			  ;; 1000 is higher than ediff's 100+,
***************
*** 3035,3040 ****
--- 3043,3052 ----
  			  (overlay-put ov 'priority 1000)
  			  (overlay-put ov 'face lazy-highlight-face)
  			  (overlay-put ov 'window (selected-window))))
+ 		       
+ 		       ; (t nil)   ; the match overlaps current found string.
+ 		       )
+ 
  		      (if isearch-lazy-highlight-forward
  			  (setq isearch-lazy-highlight-end (point))
  			(setq isearch-lazy-highlight-start (point)))))



> -- 
> Alan Mackenzie (Nuremberg, Germany).

bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch]

[Glenn Morris]

(No need to cc emacs-devel.)

This seems like a minor cosmetic issue to me, so personally I would just
fix it in trunk. But if it bothers you and you are sure the fix is safe,
put it in emacs-24.

bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch]

[Juri Linkov]

> This wrong highlighting is a regression, introduced when a space in a
> normal isearch was made to match any sequence of WS characters.

Actually this is not a regression.  You can see the same in Emacs 23.1
and earlier versions.

emacs -Q
C-u C-h i
Enter path/to/elisp.info <CR>
g Syntax Table Internals
Place point at the start of the second paragraph ("Each entry in a
....").  Attempt C-M-s C-y (isearch-yank-line).

The difference is that in Emacs 23.1 it was observable in regexp isearch
and now in non-regexp normal isearch.

> The solution I propose is NOT to lazy-highlight when the LH region
> overlaps with the current match.
>
> Here is a patch for this change:

Thanks, I'll test your patch now.  Sometimes I've seen lazy-highlighting
under point when the current match was not highlighted.  I'll try
to recall such test cases (probably occurs when switching to word mode).

bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch]

[Juri Linkov]

>> Here is a patch for this change:
>
> Thanks, I'll test your patch now.

I noticed one problem: going to the next match with `C-s'
(isearch-repeat-forward) doesn't lazy-highlight the previous match
that was unhighlighted.  The previous match needs to be re-highlighted
and the next match unhighlighted.  But this requires performing the complete
round of lazy-highlighting all of lazy-matches on every `C-s' key press
that will cause significant slow down in lazy-highlighting because it will
remove the current optimization where `C-s' doesn't cause re-highlighting
of lazy-matches most of the time (when there is no scrolling or toggling
of search parameters).

bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch]

[Alan Mackenzie]

Hi, Juri.

On Fri, Feb 15, 2013 at 12:10:45PM +0200, Juri Linkov wrote:
> >> Here is a patch for this change:

> > Thanks, I'll test your patch now.

> I noticed one problem: going to the next match with `C-s'
> (isearch-repeat-forward) doesn't lazy-highlight the previous match
> that was unhighlighted.  The previous match needs to be re-highlighted
> and the next match unhighlighted.

OK.  Sorry I didn't spot this.

> But this requires performing the complete round of lazy-highlighting
> all of lazy-matches on every `C-s' key press that will cause
> significant slow down in lazy-highlighting because it will remove the
> current optimization where `C-s' doesn't cause re-highlighting of
> lazy-matches most of the time (when there is no scrolling or toggling
> of search parameters).

Not necessarily.  Another approach, which I'm going to try, is to create
the "lazy" overlay for the current match, but not to give it the face
property.  On C-s, that overlay can get its face, and the overlay for
the current match can lose its "lazy face".  That way, each match will
have exactly one isearch overlay face active.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch]

[Alan Mackenzie]

Hi, Juri.

On Fri, Feb 15, 2013 at 12:10:45PM +0200, Juri Linkov wrote:
> >> Here is a patch for this change:

> > Thanks, I'll test your patch now.

> I noticed one problem: going to the next match with `C-s'
> (isearch-repeat-forward) doesn't lazy-highlight the previous match
> that was unhighlighted.  The previous match needs to be re-highlighted
> and the next match unhighlighted.  But this requires performing the complete
> round of lazy-highlighting all of lazy-matches on every `C-s' key press
> that will cause significant slow down in lazy-highlighting because it will
> remove the current optimization where `C-s' doesn't cause re-highlighting
> of lazy-matches most of the time (when there is no scrolling or toggling
> of search parameters).

OK, here's a better patch.  As already suggested, every match now has its
lazy-highlight overlay, just that the one overlapping the current match
no longer has the 'face property set.  I don't think there can be more
than one overlapping match.  This approach preserves the optimisation
with `C-s'.



=== modified file 'lisp/isearch.el'
*** lisp/isearch.el	2013-02-01 23:38:41 +0000
--- lisp/isearch.el	2013-02-15 13:09:26 +0000
***************
*** 2862,2867 ****
--- 2862,2869 ----
  (defvar isearch-lazy-highlight-end-limit nil)
  (defvar isearch-lazy-highlight-start nil)
  (defvar isearch-lazy-highlight-end nil)
+ (defvar isearch-lazy-highlight-point nil)
+ (defvar isearch-lazy-highlight-shadowed nil)
  (defvar isearch-lazy-highlight-timer nil)
  (defvar isearch-lazy-highlight-last-string nil)
  (defvar isearch-lazy-highlight-window nil)
***************
*** 2881,2891 ****
  is nil.  This function is called when exiting an incremental search if
  `lazy-highlight-cleanup' is non-nil."
    (interactive '(t))
!   (if (or force lazy-highlight-cleanup)
!       (while isearch-lazy-highlight-overlays
!         (delete-overlay (car isearch-lazy-highlight-overlays))
!         (setq isearch-lazy-highlight-overlays
!               (cdr isearch-lazy-highlight-overlays))))
    (when isearch-lazy-highlight-timer
      (cancel-timer isearch-lazy-highlight-timer)
      (setq isearch-lazy-highlight-timer nil)))
--- 2883,2894 ----
  is nil.  This function is called when exiting an incremental search if
  `lazy-highlight-cleanup' is non-nil."
    (interactive '(t))
!   (when (or force lazy-highlight-cleanup)
!     (while isearch-lazy-highlight-overlays
!       (delete-overlay (car isearch-lazy-highlight-overlays))
!       (setq isearch-lazy-highlight-overlays
! 	    (cdr isearch-lazy-highlight-overlays)))
!     (setq isearch-lazy-highlight-shadowed nil))
    (when isearch-lazy-highlight-timer
      (cancel-timer isearch-lazy-highlight-timer)
      (setq isearch-lazy-highlight-timer nil)))
***************
*** 2894,2955 ****
                                  'lazy-highlight-cleanup
                                  "22.1")
  
  (defun isearch-lazy-highlight-new-loop (&optional beg end)
    "Cleanup any previous `lazy-highlight' loop and begin a new one.
  BEG and END specify the bounds within which highlighting should occur.
  This is called when `isearch-update' is invoked (which can cause the
  search string to change or the window to scroll).  It is also used
  by other Emacs features."
!   (when (and (null executing-kbd-macro)
!              (sit-for 0)         ;make sure (window-start) is credible
!              (or (not (equal isearch-string
!                              isearch-lazy-highlight-last-string))
!                  (not (eq (selected-window)
!                           isearch-lazy-highlight-window))
! 		 (not (eq isearch-lazy-highlight-case-fold-search
! 			  isearch-case-fold-search))
! 		 (not (eq isearch-lazy-highlight-regexp
! 			  isearch-regexp))
! 		 (not (eq isearch-lazy-highlight-word
! 			  isearch-word))
! 		 (not (eq isearch-lazy-highlight-lax-whitespace
! 			  isearch-lax-whitespace))
! 		 (not (eq isearch-lazy-highlight-regexp-lax-whitespace
! 			  isearch-regexp-lax-whitespace))
!                  (not (= (window-start)
!                          isearch-lazy-highlight-window-start))
!                  (not (= (window-end)   ; Window may have been split/joined.
! 			 isearch-lazy-highlight-window-end))
! 		 (not (eq isearch-forward
! 			  isearch-lazy-highlight-forward))
! 		 ;; In case we are recovering from an error.
! 		 (not (equal isearch-error
! 			     isearch-lazy-highlight-error))))
!     ;; something important did indeed change
!     (lazy-highlight-cleanup t) ;kill old loop & remove overlays
!     (setq isearch-lazy-highlight-error isearch-error)
!     ;; It used to check for `(not isearch-error)' here, but actually
!     ;; lazy-highlighting might find matches to highlight even when
!     ;; `isearch-error' is non-nil.  (Bug#9918)
!     (setq isearch-lazy-highlight-start-limit beg
! 	  isearch-lazy-highlight-end-limit end)
!     (setq isearch-lazy-highlight-window       (selected-window)
! 	  isearch-lazy-highlight-window-start (window-start)
! 	  isearch-lazy-highlight-window-end   (window-end)
! 	  isearch-lazy-highlight-start        (point)
! 	  isearch-lazy-highlight-end          (point)
! 	  isearch-lazy-highlight-wrapped      nil
! 	  isearch-lazy-highlight-last-string  isearch-string
! 	  isearch-lazy-highlight-case-fold-search isearch-case-fold-search
! 	  isearch-lazy-highlight-regexp       isearch-regexp
! 	  isearch-lazy-highlight-lax-whitespace   isearch-lax-whitespace
! 	  isearch-lazy-highlight-regexp-lax-whitespace isearch-regexp-lax-whitespace
! 	  isearch-lazy-highlight-word         isearch-word
! 	  isearch-lazy-highlight-forward      isearch-forward)
!       (unless (equal isearch-string "")
! 	(setq isearch-lazy-highlight-timer
! 	      (run-with-idle-timer lazy-highlight-initial-delay nil
! 				   'isearch-lazy-highlight-update)))))
  
  (defun isearch-lazy-highlight-search ()
    "Search ahead for the next or previous match, for lazy highlighting.
--- 2897,2979 ----
                                  'lazy-highlight-cleanup
                                  "22.1")
  
+ (defun isearch-lazy-highlight-move-shadow ()
+   "Move the lazy highlight \"shadow\" to the current match position."
+   (if isearch-lazy-highlight-shadowed
+       (overlay-put isearch-lazy-highlight-shadowed 'face lazy-highlight-face))
+   (let ((ovs (if isearch-forward
+ 		 (overlays-in isearch-other-end (point))
+ 	       (overlays-in (point) isearch-other-end)))
+ 	ov)
+     (while ovs
+       (setq ov (car ovs)
+ 	    ovs (cdr ovs))
+       (when (memq ov isearch-lazy-highlight-overlays)
+ 	(overlay-put ov 'face nil)
+ 	(setq isearch-lazy-highlight-shadowed ov)))))
+ 
  (defun isearch-lazy-highlight-new-loop (&optional beg end)
    "Cleanup any previous `lazy-highlight' loop and begin a new one.
  BEG and END specify the bounds within which highlighting should occur.
  This is called when `isearch-update' is invoked (which can cause the
  search string to change or the window to scroll).  It is also used
  by other Emacs features."
!   (if (and (null executing-kbd-macro)
! 	   (sit-for 0)		 ;make sure (window-start) is credible
! 	   (or (not (equal isearch-string
! 			   isearch-lazy-highlight-last-string))
! 	       (not (eq (selected-window)
! 			isearch-lazy-highlight-window))
! 	       (not (eq isearch-lazy-highlight-case-fold-search
! 			isearch-case-fold-search))
! 	       (not (eq isearch-lazy-highlight-regexp
! 			isearch-regexp))
! 	       (not (eq isearch-lazy-highlight-word
! 			isearch-word))
! 	       (not (eq isearch-lazy-highlight-lax-whitespace
! 			isearch-lax-whitespace))
! 	       (not (eq isearch-lazy-highlight-regexp-lax-whitespace
! 			isearch-regexp-lax-whitespace))
! 	       (not (= (window-start)
! 		       isearch-lazy-highlight-window-start))
! 	       (not (= (window-end) ; Window may have been split/joined.
! 		       isearch-lazy-highlight-window-end))
! 	       (not (eq isearch-forward
! 			isearch-lazy-highlight-forward))
! 	       ;; In case we are recovering from an error.
! 	       (not (equal isearch-error
! 			   isearch-lazy-highlight-error))))
!       ;; something important did indeed change
!       (progn
! 	(lazy-highlight-cleanup t)    ;kill old loop & remove overlays
! 	(setq isearch-lazy-highlight-error isearch-error)
! 	;; It used to check for `(not isearch-error)' here, but actually
! 	;; lazy-highlighting might find matches to highlight even when
! 	;; `isearch-error' is non-nil.  (Bug#9918)
! 	(setq isearch-lazy-highlight-start-limit beg
! 	      isearch-lazy-highlight-end-limit end)
! 	(setq isearch-lazy-highlight-window       (selected-window)
! 	      isearch-lazy-highlight-window-start (window-start)
! 	      isearch-lazy-highlight-window-end   (window-end)
! 	      isearch-lazy-highlight-start        (point)
! 	      isearch-lazy-highlight-end          (point)
! 	      isearch-lazy-highlight-point        (point)
! 	      isearch-lazy-highlight-shadowed     nil
! 	      isearch-lazy-highlight-wrapped      nil
! 	      isearch-lazy-highlight-last-string  isearch-string
! 	      isearch-lazy-highlight-case-fold-search isearch-case-fold-search
! 	      isearch-lazy-highlight-regexp       isearch-regexp
! 	      isearch-lazy-highlight-lax-whitespace   isearch-lax-whitespace
! 	      isearch-lazy-highlight-regexp-lax-whitespace isearch-regexp-lax-whitespace
! 	      isearch-lazy-highlight-word         isearch-word
! 	      isearch-lazy-highlight-forward      isearch-forward)
! 	(unless (equal isearch-string "")
! 	  (setq isearch-lazy-highlight-timer
! 		(run-with-idle-timer lazy-highlight-initial-delay nil
! 				     'isearch-lazy-highlight-update))))
! 
!     ;; Swap the previously "shadowed" lazy highlight overlay for the new one.
!     (isearch-lazy-highlight-move-shadow)))
  
  (defun isearch-lazy-highlight-search ()
    "Search ahead for the next or previous match, for lazy highlighting.
***************
*** 3033,3039 ****
  			  ;; 1000 is higher than ediff's 100+,
  			  ;; but lower than isearch main overlay's 1001
  			  (overlay-put ov 'priority 1000)
! 			  (overlay-put ov 'face lazy-highlight-face)
  			  (overlay-put ov 'window (selected-window))))
  		      (if isearch-lazy-highlight-forward
  			  (setq isearch-lazy-highlight-end (point))
--- 3057,3069 ----
  			  ;; 1000 is higher than ediff's 100+,
  			  ;; but lower than isearch main overlay's 1001
  			  (overlay-put ov 'priority 1000)
! 			  (if (if isearch-lazy-highlight-forward
! 				  (or (<= me isearch-other-end)
! 				      (>= mb isearch-lazy-highlight-point))
! 				(or (<= me isearch-lazy-highlight-point)
! 				    (>= mb isearch-other-end)))
! 			      (overlay-put ov 'face lazy-highlight-face)
! 			    (setq isearch-lazy-highlight-shadowed ov))
  			  (overlay-put ov 'window (selected-window))))
  		      (if isearch-lazy-highlight-forward
  			  (setq isearch-lazy-highlight-end (point))


-- 
Alan Mackenzie (Nuremberg, Germany).

bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch]

[Juri Linkov]

> OK, here's a better patch.  As already suggested, every match now has its
> lazy-highlight overlay, just that the one overlapping the current match
> no longer has the 'face property set.  I don't think there can be more
> than one overlapping match.  This approach preserves the optimisation
> with `C-s'.

Yes, this is a more reliable approach, and it works correctly now in isearch.
It fails only in `replace-highlight', i.e. after running `query-replace'
and answering `n' to skip to the next match, it doesn't re-highlight
the previous skipped match.

But in principle I don't oppose your approach provided it's working
correctly in all cases.

While testing I noticed an interesting effect.  Test case:

emacs -Q
Eval (info "(elisp) Syntax Table Internals")
Place point at the start of the second paragraph
("   Each entry in a ....").
Type `C-s C-w C-w' that puts " each" to the search string.

Type another C-s (isearch-repeat-forward) that will go to the second
match of " each" (try to use a smaller font to be able to see both
matches of the string " each" on the same screen).

As soon as the cursor leaves the first match, its highlighting
(with a different color for a lazy match) grows to a wider region
previously hidden by your patch.

This indicates that surprises will remain even after using your approach.
Another case surprising to users is reversing the search direction -
e.g. when the cursor is on the second match, type `C-r' and see how
the lazy-highlighted first match shrinks from multi-line to single-line.
It seems nothing can be done to fix this case.

bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch]

[Juri Linkov]

> It fails only in `replace-highlight', i.e. after running `query-replace'
> and answering `n' to skip to the next match, it doesn't re-highlight
> the previous skipped match.

Also I noticed that typing `C-s M-s SPC C-s' quickly causes the error:

Debugger entered--Lisp error: (wrong-type-argument integer-or-marker-p nil)
  overlays-in(nil 244)
  (if isearch-forward (overlays-in isearch-other-end (point)) (overlays-in (point) isearch-other-end))
  (let ((ovs (if isearch-forward (overlays-in isearch-other-end (point)) (overlays-in (point) isearch-other-end))) ov) (while ovs (setq ov (car ovs) ovs (cdr ovs)) (if (memq ov isearch-lazy-highlight-overlays) (progn (overlay-put ov (quote face) nil) (setq isearch-lazy-highlight-shadowed ov)))))
  isearch-lazy-highlight-move-shadow()
  (if (and (null executing-kbd-macro) (sit-for 0) (or (not (equal isearch-string isearch-lazy-highlight-last-string)) (not (eq (selected-window) isearch-lazy-highlight-window)) (not (eq isearch-lazy-highlight-case-fold-search isearch-case-fold-search)) (not (eq isearch-lazy-highlight-regexp isearch-regexp)) (not (eq isearch-lazy-highlight-word isearch-word)) (not (eq isearch-lazy-highlight-lax-whitespace isearch-lax-whitespace)) (not (eq isearch-lazy-highlight-regexp-lax-whitespace isearch-regexp-lax-whitespace)) (not (= (window-start) isearch-lazy-highlight-window-start)) (not (= (window-end) isearch-lazy-highlight-window-end)) (not (eq isearch-forward isearch-lazy-highlight-forward)) (not (equal isearch-error isearch-lazy-highlight-error)))) (progn (lazy-highlight-cleanup t) (setq isearch-lazy-highlight-error isearch-error) (setq isearch-lazy-highlight-start-limit beg isearch-lazy-highlight-end-limit end) (setq isearch-lazy-highlight-window (selected-window) isearch-lazy-highlight-window-start (window-start) isearch-lazy-highlight-window-end (window-end) isearch-lazy-highlight-start (point) isearch-lazy-highlight-end (point) isearch-lazy-highlight-point (point) isearch-lazy-highlight-shadowed nil isearch-lazy-highlight-wrapped nil isearch-lazy-highlight-last-string isearch-string isearch-lazy-highlight-case-fold-search isearch-case-fold-search isearch-lazy-highlight-regexp isearch-regexp isearch-lazy-highlight-lax-whitespace isearch-lax-whitespace isearch-lazy-highlight-regexp-lax-whitespace isearch-regexp-lax-whitespace isearch-lazy-highlight-word isearch-word isearch-lazy-highlight-forward isearch-forward) (if (equal isearch-string "") nil (setq isearch-lazy-highlight-timer (run-with-idle-timer lazy-highlight-initial-delay nil (quote isearch-lazy-highlight-update))))) (isearch-lazy-highlight-move-shadow))
  isearch-lazy-highlight-new-loop()
  isearch-update()
  isearch-toggle-lax-whitespace()
  call-interactively(isearch-toggle-lax-whitespace nil nil)


Oh, and another problem: after customizing `lazy-highlight-cleanup' to `nil',
exiting isearch doesn't leave the current match lazy-highlighted.

The problem is that other features are expecting that _all_ matches
are lazy-highlighted, so I'm starting to doubt whether it's worth the
trouble trying to improve such cosmetic issue?

bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch]

[Alan Mackenzie]

Hi, Juri.

I'm answering both your last two emails together, here.

On Sat, Feb 16, 2013 at 11:50:39PM +0200, Juri Linkov wrote:
> > OK, here's a better patch.  As already suggested, every match now has its
> > lazy-highlight overlay, just that the one overlapping the current match
> > no longer has the 'face property set.  I don't think there can be more
> > than one overlapping match.  This approach preserves the optimisation
> > with `C-s'.

> Yes, this is a more reliable approach, and it works correctly now in isearch.
> It fails only in `replace-highlight', i.e. after running `query-replace'
> and answering `n' to skip to the next match, it doesn't re-highlight
> the previous skipped match.

Fixed, see patch below.

> But in principle I don't oppose your approach provided it's working
> correctly in all cases.

.../textmodes/ispell.el also uses the lazy highlighting, so it will need
amending too.

> While testing I noticed an interesting effect.  Test case:

> emacs -Q
> Eval (info "(elisp) Syntax Table Internals")
> Place point at the start of the second paragraph
> ("   Each entry in a ....").
> Type `C-s C-w C-w' that puts " each" to the search string.

> Type another C-s (isearch-repeat-forward) that will go to the second
> match of " each" (try to use a smaller font to be able to see both
> matches of the string " each" on the same screen).

> As soon as the cursor leaves the first match, its highlighting
> (with a different color for a lazy match) grows to a wider region
> previously hidden by your patch.

Yes.  This is logical - the (extended) lazy highlighting indicates what
would get fully highlighted on a future (wrapped) repeated search.  I'm
not saying it's a feature, but it's certainly logical.  ;-)

> This indicates that surprises will remain even after using your approach.
> Another case surprising to users is reversing the search direction -
> e.g. when the cursor is on the second match, type `C-r' and see how
> the lazy-highlighted first match shrinks from multi-line to single-line.
> It seems nothing can be done to fix this case.

This is present even without my patch.  This shrunk lazy highlight
indicates precisely what would be matched on a repeated backward search.
This is sort of logical too.  It would be too much work to make the
backward search match all the whitespace greedily.

> Also I noticed that typing `C-s M-s SPC C-s' quickly causes the error:

> Debugger entered--Lisp error: (wrong-type-argument integer-or-marker-p
> nil)
> overlays-in(nil 244)

[ .... ]

This is now fixed, too.  It seems that `run-with-idle-timer' doesn't
retain the current `let'-bindings when it runs, even though the
documentation doesn't mention this.  This is the reason for all these
isearch-lazy-highlight-.... static variables shadowing dynamic ones.
I've learnt something today.  :-)

> Oh, and another problem: after customizing `lazy-highlight-cleanup' to
> `nil', exiting isearch doesn't leave the current match
> lazy-highlighted.

Yuck, what a variable!  I've fixed this, too.

> The problem is that other features are expecting that _all_ matches
> are lazy-highlighted, so I'm starting to doubt whether it's worth the
> trouble trying to improve such cosmetic issue?

It made me seriously unhappy, to the point where I would have disabled
whatever was causing it.  It looks very scrappy, unlike the rest of
Emacs.  I predict, if we don't fix it, there will be quite a few angry
bug reports, a bit like when the "fringe" was introduced in 21.1 with no
way of disabling it, but probably not as bad.

And this _is_ a regression, maybe not in the code, but certainly from the
point of view of a user.  It is true that this effect can be disabled by
`isearch-toggle-lax-whitespace' but there is no way a normal user can
find this out, apart from stumbling on it by luck.  We can't really
document this either.

I'm surprised how difficult the fix is.  Maybe lazy-highlighting should
be refactored out of isearch.el and given a better interface.

Here's the latest patch, which I think now works, apart from ispell.el.



=== modified file 'lisp/isearch.el'
*** lisp/isearch.el	2013-02-01 23:38:41 +0000
--- lisp/isearch.el	2013-02-19 13:05:44 +0000
***************
*** 2862,2867 ****
--- 2862,2870 ----
  (defvar isearch-lazy-highlight-end-limit nil)
  (defvar isearch-lazy-highlight-start nil)
  (defvar isearch-lazy-highlight-end nil)
+ (defvar isearch-lazy-highlight-point nil)
+ (defvar isearch-lazy-highlight-shadowed nil)
+ (defvar isearch-lazy-highlight-other-end nil)
  (defvar isearch-lazy-highlight-timer nil)
  (defvar isearch-lazy-highlight-last-string nil)
  (defvar isearch-lazy-highlight-window nil)
***************
*** 2882,2891 ****
  `lazy-highlight-cleanup' is non-nil."
    (interactive '(t))
    (if (or force lazy-highlight-cleanup)
!       (while isearch-lazy-highlight-overlays
!         (delete-overlay (car isearch-lazy-highlight-overlays))
!         (setq isearch-lazy-highlight-overlays
!               (cdr isearch-lazy-highlight-overlays))))
    (when isearch-lazy-highlight-timer
      (cancel-timer isearch-lazy-highlight-timer)
      (setq isearch-lazy-highlight-timer nil)))
--- 2885,2898 ----
  `lazy-highlight-cleanup' is non-nil."
    (interactive '(t))
    (if (or force lazy-highlight-cleanup)
!       (progn
! 	(while isearch-lazy-highlight-overlays
! 	  (delete-overlay (car isearch-lazy-highlight-overlays))
! 	  (setq isearch-lazy-highlight-overlays
! 		(cdr isearch-lazy-highlight-overlays)))
! 	(setq isearch-lazy-highlight-shadowed nil))
!     (when isearch-lazy-highlight-shadowed
!       (overlay-put isearch-lazy-highlight-shadowed 'face lazy-highlight-face)))
    (when isearch-lazy-highlight-timer
      (cancel-timer isearch-lazy-highlight-timer)
      (setq isearch-lazy-highlight-timer nil)))
***************
*** 2894,2955 ****
                                  'lazy-highlight-cleanup
                                  "22.1")
  
  (defun isearch-lazy-highlight-new-loop (&optional beg end)
    "Cleanup any previous `lazy-highlight' loop and begin a new one.
  BEG and END specify the bounds within which highlighting should occur.
  This is called when `isearch-update' is invoked (which can cause the
  search string to change or the window to scroll).  It is also used
  by other Emacs features."
!   (when (and (null executing-kbd-macro)
!              (sit-for 0)         ;make sure (window-start) is credible
!              (or (not (equal isearch-string
!                              isearch-lazy-highlight-last-string))
!                  (not (eq (selected-window)
!                           isearch-lazy-highlight-window))
! 		 (not (eq isearch-lazy-highlight-case-fold-search
! 			  isearch-case-fold-search))
! 		 (not (eq isearch-lazy-highlight-regexp
! 			  isearch-regexp))
! 		 (not (eq isearch-lazy-highlight-word
! 			  isearch-word))
! 		 (not (eq isearch-lazy-highlight-lax-whitespace
! 			  isearch-lax-whitespace))
! 		 (not (eq isearch-lazy-highlight-regexp-lax-whitespace
! 			  isearch-regexp-lax-whitespace))
!                  (not (= (window-start)
!                          isearch-lazy-highlight-window-start))
!                  (not (= (window-end)   ; Window may have been split/joined.
! 			 isearch-lazy-highlight-window-end))
! 		 (not (eq isearch-forward
! 			  isearch-lazy-highlight-forward))
! 		 ;; In case we are recovering from an error.
! 		 (not (equal isearch-error
! 			     isearch-lazy-highlight-error))))
!     ;; something important did indeed change
!     (lazy-highlight-cleanup t) ;kill old loop & remove overlays
!     (setq isearch-lazy-highlight-error isearch-error)
!     ;; It used to check for `(not isearch-error)' here, but actually
!     ;; lazy-highlighting might find matches to highlight even when
!     ;; `isearch-error' is non-nil.  (Bug#9918)
!     (setq isearch-lazy-highlight-start-limit beg
! 	  isearch-lazy-highlight-end-limit end)
!     (setq isearch-lazy-highlight-window       (selected-window)
! 	  isearch-lazy-highlight-window-start (window-start)
! 	  isearch-lazy-highlight-window-end   (window-end)
! 	  isearch-lazy-highlight-start        (point)
! 	  isearch-lazy-highlight-end          (point)
! 	  isearch-lazy-highlight-wrapped      nil
! 	  isearch-lazy-highlight-last-string  isearch-string
! 	  isearch-lazy-highlight-case-fold-search isearch-case-fold-search
! 	  isearch-lazy-highlight-regexp       isearch-regexp
! 	  isearch-lazy-highlight-lax-whitespace   isearch-lax-whitespace
! 	  isearch-lazy-highlight-regexp-lax-whitespace isearch-regexp-lax-whitespace
! 	  isearch-lazy-highlight-word         isearch-word
! 	  isearch-lazy-highlight-forward      isearch-forward)
!       (unless (equal isearch-string "")
! 	(setq isearch-lazy-highlight-timer
! 	      (run-with-idle-timer lazy-highlight-initial-delay nil
! 				   'isearch-lazy-highlight-update)))))
  
  (defun isearch-lazy-highlight-search ()
    "Search ahead for the next or previous match, for lazy highlighting.
--- 2901,2984 ----
                                  'lazy-highlight-cleanup
                                  "22.1")
  
+ (defun isearch-lazy-highlight-move-shadow ()
+   "Move the lazy highlight \"shadow\" to the current match position."
+   (overlay-put isearch-lazy-highlight-shadowed 'face lazy-highlight-face)
+   (let ((ovs (if isearch-forward
+ 		 (overlays-in isearch-other-end (point))
+ 	       (overlays-in (point) isearch-other-end)))
+ 	ov)
+     (while ovs
+       (setq ov (car ovs)
+ 	    ovs (cdr ovs))
+       (when (memq ov isearch-lazy-highlight-overlays)
+ 	(overlay-put ov 'face nil)
+ 	(setq isearch-lazy-highlight-shadowed ov)))))
+ 
  (defun isearch-lazy-highlight-new-loop (&optional beg end)
    "Cleanup any previous `lazy-highlight' loop and begin a new one.
  BEG and END specify the bounds within which highlighting should occur.
  This is called when `isearch-update' is invoked (which can cause the
  search string to change or the window to scroll).  It is also used
  by other Emacs features."
!   (if (and (null executing-kbd-macro)
! 	   (sit-for 0)		 ;make sure (window-start) is credible
! 	   (or (not (equal isearch-string
! 			   isearch-lazy-highlight-last-string))
! 	       (not (eq (selected-window)
! 			isearch-lazy-highlight-window))
! 	       (not (eq isearch-lazy-highlight-case-fold-search
! 			isearch-case-fold-search))
! 	       (not (eq isearch-lazy-highlight-regexp
! 			isearch-regexp))
! 	       (not (eq isearch-lazy-highlight-word
! 			isearch-word))
! 	       (not (eq isearch-lazy-highlight-lax-whitespace
! 			isearch-lax-whitespace))
! 	       (not (eq isearch-lazy-highlight-regexp-lax-whitespace
! 			isearch-regexp-lax-whitespace))
! 	       (not (= (window-start)
! 		       isearch-lazy-highlight-window-start))
! 	       (not (= (window-end) ; Window may have been split/joined.
! 		       isearch-lazy-highlight-window-end))
! 	       (not (eq isearch-forward
! 			isearch-lazy-highlight-forward))
! 	       ;; In case we are recovering from an error.
! 	       (not (equal isearch-error
! 			   isearch-lazy-highlight-error))))
!       ;; something important did indeed change
!       (progn
! 	(lazy-highlight-cleanup t)    ;kill old loop & remove overlays
! 	(setq isearch-lazy-highlight-error isearch-error)
! 	;; It used to check for `(not isearch-error)' here, but actually
! 	;; lazy-highlighting might find matches to highlight even when
! 	;; `isearch-error' is non-nil.  (Bug#9918)
! 	(setq isearch-lazy-highlight-start-limit beg
! 	      isearch-lazy-highlight-end-limit end)
! 	(setq isearch-lazy-highlight-window       (selected-window)
! 	      isearch-lazy-highlight-window-start (window-start)
! 	      isearch-lazy-highlight-window-end   (window-end)
! 	      isearch-lazy-highlight-start        (point)
! 	      isearch-lazy-highlight-end          (point)
! 	      isearch-lazy-highlight-point        (point)
! 	      isearch-lazy-highlight-shadowed     nil
! 	      isearch-lazy-highlight-other-end    isearch-other-end
! 	      isearch-lazy-highlight-wrapped      nil
! 	      isearch-lazy-highlight-last-string  isearch-string
! 	      isearch-lazy-highlight-case-fold-search isearch-case-fold-search
! 	      isearch-lazy-highlight-regexp       isearch-regexp
! 	      isearch-lazy-highlight-lax-whitespace   isearch-lax-whitespace
! 	      isearch-lazy-highlight-regexp-lax-whitespace isearch-regexp-lax-whitespace
! 	      isearch-lazy-highlight-word         isearch-word
! 	      isearch-lazy-highlight-forward      isearch-forward)
! 	(unless (equal isearch-string "")
! 	  (setq isearch-lazy-highlight-timer
! 		(run-with-idle-timer lazy-highlight-initial-delay nil
! 				     'isearch-lazy-highlight-update))))
! 
!     ;; Swap the previously "shadowed" lazy highlight overlay for the new one.
!     (when isearch-lazy-highlight-shadowed
!       (isearch-lazy-highlight-move-shadow))))
  
  (defun isearch-lazy-highlight-search ()
    "Search ahead for the next or previous match, for lazy highlighting.
***************
*** 3033,3039 ****
  			  ;; 1000 is higher than ediff's 100+,
  			  ;; but lower than isearch main overlay's 1001
  			  (overlay-put ov 'priority 1000)
! 			  (overlay-put ov 'face lazy-highlight-face)
  			  (overlay-put ov 'window (selected-window))))
  		      (if isearch-lazy-highlight-forward
  			  (setq isearch-lazy-highlight-end (point))
--- 3062,3074 ----
  			  ;; 1000 is higher than ediff's 100+,
  			  ;; but lower than isearch main overlay's 1001
  			  (overlay-put ov 'priority 1000)
! 			  (if (if isearch-lazy-highlight-forward
! 				  (or (<= me isearch-lazy-highlight-other-end)
! 				      (>= mb isearch-lazy-highlight-point))
! 				(or (<= me isearch-lazy-highlight-point)
! 				    (>= mb isearch-lazy-highlight-other-end)))
! 			      (overlay-put ov 'face lazy-highlight-face)
! 			    (setq isearch-lazy-highlight-shadowed ov))
  			  (overlay-put ov 'window (selected-window))))
  		      (if isearch-lazy-highlight-forward
  			  (setq isearch-lazy-highlight-end (point))

=== modified file 'lisp/replace.el'
*** lisp/replace.el	2013-02-01 23:38:41 +0000
--- lisp/replace.el	2013-02-17 22:16:38 +0000
***************
*** 2198,2203 ****
--- 2198,2204 ----
  	     replace-regexp-lax-whitespace)
  	    (isearch-case-fold-search case-fold-search)
  	    (isearch-forward t)
+ 	    (isearch-other-end match-beg)
  	    (isearch-error nil))
  	(isearch-lazy-highlight-new-loop range-beg range-end))))
  

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch]

[Juri Linkov]

> I predict, if we don't fix it, there will be quite a few angry
> bug reports, a bit like when the "fringe" was introduced in 21.1
> with no way of disabling it, but probably not as bad.

Unlike fringes in 21.1, it's easy to disable this feature by customizing
`search-whitespace-regexp' to nil.

> And this _is_ a regression, maybe not in the code, but certainly from the
> point of view of a user.  It is true that this effect can be disabled by
> `isearch-toggle-lax-whitespace' but there is no way a normal user can
> find this out, apart from stumbling on it by luck.

I think you are overestimating the seriousness of the problem.
The highlighting effect that you don't like might seem peculiar
but it has a logical explanation.  Trying to change its logic to
more complicated will introduce other problems that might seem illogical
to other users.  For example, consider the following test case:

With `search-whitespace-regexp' customized to nil, try to put the cursor
to the second space character in the string "   Each entry" and type
`C-M-s SPC'.  It lazy-highlights the previous space character too
(it's the first space character in the string).  This is right.

Now with your patch applied, add the character "+" to the search regexp,
so that a complete search regexp is " +" and the cursor is on the second
space character in the string "   Each entry".  The result is that
the first space character is not lazy-highlighted.  I think this is wrong.

If you assume that the boundary of the current match should begin only
from the leading character of the search string, then you have to allow
the preceding match to be lazy-highlighted separately in its own right
(in the above case the first space character in the string) because
it matches the search regexp.

IOW, what is more logical to do according to your approach is not to hide
the lazy-highlighted match under point, but split it to two matches
where the match preceding the current search position is lazy-highlighted
and the second match under point is hidden.

> We can't really document this either.

Actually, documenting this effect is a very good idea
that in any case should be done in emacs-24.

> I'm surprised how difficult the fix is.

That's is why I prefer to refrain from making hasty changes that might
cause more problems in unexpected places making other users unhappy.

bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch]

[Alan Mackenzie]

Hello again, Juri.

On Wed, Feb 20, 2013 at 12:49:04PM +0200, Juri Linkov wrote:

> I think you are overestimating the seriousness of the problem.

It is going to be a big problem for a small number of users.  I dare say
the majority won't notice.

> The highlighting effect that you don't like might seem peculiar
> but it has a logical explanation.  Trying to change its logic to
> more complicated will introduce other problems that might seem illogical
> to other users.

Just as activating `search-whitespace-regexp' has done, you mean.  ;-)

> For example, consider the following test case:

> With `search-whitespace-regexp' customized to nil, try to put the cursor
> to the second space character in the string "   Each entry" and type
> `C-M-s SPC'.  It lazy-highlights the previous space character too
> (it's the first space character in the string).  This is right.

> Now with your patch applied, add the character "+" to the search regexp,
> so that a complete search regexp is " +" and the cursor is on the second
> space character in the string "   Each entry".  The result is that
> the first space character is not lazy-highlighted.  I think this is wrong.

:-).  I'm not sure.  It has a certain logic behind it.  I think a missing
lazy-highlight is much less disturbing than an obtrusive one.

> If you assume that the boundary of the current match should begin only
> from the leading character of the search string, then you have to allow
> the preceding match to be lazy-highlighted separately in its own right
> (in the above case the first space character in the string) because
> it matches the search regexp.

That match overlaps the current match.

> IOW, what is more logical to do according to your approach is not to hide
> the lazy-highlighted match under point, but split it to two matches
> where the match preceding the current search position is lazy-highlighted
> and the second match under point is hidden.

I don't agree.  I think a match should either be highlighted completely
or not at all.  But we might be splitting hairs at this point.

> > We can't really document this either.

> Actually, documenting this effect is a very good idea
> that in any case should be done in emacs-24.

> > I'm surprised how difficult the fix is.

> That's is why I prefer to refrain from making hasty changes that might
> cause more problems in unexpected places making other users unhappy.

I think I must now reluctantly agree with you here.  There just aren't
enough pretests left before Emacs 24.3 to test it properly.  I grepped -l
isearch, and it came up with over 70 matching files.  grepping for
isearch-lazy-highlight produced just 4 matches, the already identified
files plus .../lisp/cedet/semantic/senator.el.

How about installing the change in the trunk so that people can try it
out?

Anyhow, just for the sake of completeness, here is the part of the patch
which fixes ispell.e:



=== modified file 'lisp/textmodes/ispell.el'
*** lisp/textmodes/ispell.el	2013-01-01 09:11:05 +0000
--- lisp/textmodes/ispell.el	2013-02-20 21:11:47 +0000
***************
*** 2491,2506 ****
  	(delete-overlay ispell-overlay)))
    (if (and ispell-lazy-highlight (boundp 'lazy-highlight-cleanup))
        (if highlight
! 	  (let ((isearch-string
! 		 (concat
! 		  "\\b"
! 		  (regexp-quote (buffer-substring-no-properties start end))
! 		  "\\b"))
! 		(isearch-regexp t)
! 		(isearch-case-fold-search nil))
! 	    (isearch-lazy-highlight-new-loop
! 	     (if (boundp 'reg-start) reg-start)
! 	     (if (boundp 'reg-end)   reg-end)))
  	(lazy-highlight-cleanup lazy-highlight-cleanup)
  	(setq isearch-lazy-highlight-last-string nil))))
  
--- 2491,2509 ----
  	(delete-overlay ispell-overlay)))
    (if (and ispell-lazy-highlight (boundp 'lazy-highlight-cleanup))
        (if highlight
! 	  (save-excursion
! 	    (goto-char start)
! 	    (let ((isearch-string
! 		   (concat
! 		    "\\b"
! 		    (regexp-quote (buffer-substring-no-properties start end))
! 		    "\\b"))
! 		  (isearch-regexp t)
! 		  (isearch-case-fold-search nil)
! 		  (isearch-other-end end))
! 	      (isearch-lazy-highlight-new-loop
! 	       (if (boundp 'reg-start) reg-start)
! 	       (if (boundp 'reg-end)   reg-end))))
  	(lazy-highlight-cleanup lazy-highlight-cleanup)
  	(setq isearch-lazy-highlight-last-string nil))))
  

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch]

[Juri Linkov]

>> The result is that the first space character is not lazy-highlighted.
>> I think this is wrong.
>
> :-).  I'm not sure.  It has a certain logic behind it.  I think a missing
> lazy-highlight is much less disturbing than an obtrusive one.

Alan, I completely agree with you.  Your logic makes perfect sense.
An obtrusive lazy-highlight is too disturbing.

But I also think that my logic makes sense as well
since a missing lazy-highlight is too bad.

Fortunately, it is possible to combine both logics
with a small patch that you can see below.

The key point is that the lazy-highlighting loop should start
from the same position where the main search function found
the beginning of the match!

Currently the boundary of the lazy-highlighting loop starts at the end
of the found match (the value of `(point)').  But the match found by
the main search function (and highlighted by the primary face `isearch')
begins at `isearch-other-end'.  So currently they are mutually inconsistent.

This patch moves the boundary of lazy-highlighting from the end
of the found match to its beginning - like in the main search function.
Thus it brings the logic of `isearch-lazy-highlight-search' closer to
the logic of the main search function in `isearch-search'.

In the result of removing this inconsistency, this patch passes
the test case that you posted in your original bug report,
i.e. placing point at the start of the second paragraph
("   Each entry in a ....") and typing `C-s M-s C-e' (isearch-yank-line)
doesn't highlight the gap preceding the paragraph with lazy-highlight face.
And it also passes other tests that I tried.

=== modified file 'lisp/isearch.el'
--- lisp/isearch.el	2013-02-01 23:38:41 +0000
+++ lisp/isearch.el	2013-02-21 00:45:05 +0000
@@ -2936,8 +2936,8 @@ (defun isearch-lazy-highlight-new-loop (
     (setq isearch-lazy-highlight-window       (selected-window)
 	  isearch-lazy-highlight-window-start (window-start)
 	  isearch-lazy-highlight-window-end   (window-end)
-	  isearch-lazy-highlight-start        (point)
-	  isearch-lazy-highlight-end          (point)
+	  isearch-lazy-highlight-start        (or isearch-other-end (point))
+	  isearch-lazy-highlight-end          (or isearch-other-end (point))
 	  isearch-lazy-highlight-wrapped      nil
 	  isearch-lazy-highlight-last-string  isearch-string
 	  isearch-lazy-highlight-case-fold-search isearch-case-fold-search

bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch]

[Alan Mackenzie]

Hi, Juri.

On Thu, Feb 21, 2013 at 02:45:31AM +0200, Juri Linkov wrote:
> Fortunately, it is possible to combine both logics
> with a small patch that you can see below.

> The key point is that the lazy-highlighting loop should start
> from the same position where the main search function found
> the beginning of the match!

Hey, that's brilliant!  It simply works, without all the added (or
alternative) complexity of my patch.

> Currently the boundary of the lazy-highlighting loop starts at the end
> of the found match (the value of `(point)').  But the match found by
> the main search function (and highlighted by the primary face `isearch')
> begins at `isearch-other-end'.  So currently they are mutually inconsistent.

> This patch moves the boundary of lazy-highlighting from the end
> of the found match to its beginning - like in the main search function.
> Thus it brings the logic of `isearch-lazy-highlight-search' closer to
> the logic of the main search function in `isearch-search'.

> In the result of removing this inconsistency, this patch passes
> the test case that you posted in your original bug report,
> i.e. placing point at the start of the second paragraph
> ("   Each entry in a ....") and typing `C-s M-s C-e' (isearch-yank-line)
> doesn't highlight the gap preceding the paragraph with lazy-highlight face.
> And it also passes other tests that I tried.

It seems to work for ispell too.  What more could one ask for?

Can we install this into the emacs-24 branch?  It satisfies Glenn's
criterion of a week ago, "...and you are sure the fix is safe".

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch]

[Glenn Morris]

Alan Mackenzie wrote:

> Can we install this into the emacs-24 branch?  It satisfies Glenn's
> criterion of a week ago, "...and you are sure the fix is safe".

If you and Juri both agree on that, OK.

bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch]

[Juri Linkov]

>> Can we install this into the emacs-24 branch?  It satisfies Glenn's
>> criterion of a week ago, "...and you are sure the fix is safe".
>
> If you and Juri both agree on that, OK.

So I installed into the emacs-24 branch.  I had to add more comments
because in `isearch-lazy-highlight-start' there is a similar assignment
of `point' to `isearch-lazy-highlight-start' that shouldn't be changed,
because it's used for other purposes.  I've explained the difference in
the comments.  Also the 1-line change in replace.el like in Alan's patch
was still necessary.  The change in ispell.el is not necessary but I added
the let-bindings like in other places that use lazy-highlighting
as a precaution against potential problems.