Package: emacs;
Reported by: Alan Mackenzie <acm <at> muc.de>
Date: Thu, 10 Jan 2013 13:33:01 UTC
Severity: normal
Found in version 24.2.92
Done: Juri Linkov <juri <at> jurta.org>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 13402 in the body.
You can then email your comments to 13402 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
View this report as an mbox folder, status mbox, maintainer mbox
bug-gnu-emacs <at> gnu.org:bug#13402; Package emacs.
(Thu, 10 Jan 2013 13:33:02 GMT) Full text and rfc822 format available.Alan Mackenzie <acm <at> muc.de>:bug-gnu-emacs <at> gnu.org.
(Thu, 10 Jan 2013 13:33:02 GMT) Full text and rfc822 format available.Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
From: Alan Mackenzie <acm <at> muc.de> To: bug-gnu-emacs <at> gnu.org Subject: 24.2.92 pretest: bugs in isearch-yank-line in info page Date: Thu, 10 Jan 2013 13:25:30 +0000
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-gnu-emacs <at> gnu.org:bug#13402; Package emacs.
(Thu, 10 Jan 2013 18:22:02 GMT) Full text and rfc822 format available.Message #8 received at 13402 <at> debbugs.gnu.org (full text, mbox):
From: Andreas Schwab <schwab <at> linux-m68k.org> To: Alan Mackenzie <acm <at> muc.de> Cc: 13402 <at> debbugs.gnu.org Subject: Re: bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page Date: Thu, 10 Jan 2013 19:21:27 +0100
Alan Mackenzie <acm <at> 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 <at> linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
bug-gnu-emacs <at> gnu.org:bug#13402; Package emacs.
(Fri, 11 Jan 2013 01:00:02 GMT) Full text and rfc822 format available.Message #11 received at 13402 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> jurta.org> To: Alan Mackenzie <acm <at> muc.de> Cc: 13402 <at> debbugs.gnu.org Subject: Re: bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page Date: Fri, 11 Jan 2013 02:43:16 +0200
> 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-gnu-emacs <at> gnu.org:bug#13402; Package emacs.
(Wed, 13 Feb 2013 09:49:01 GMT) Full text and rfc822 format available.Message #14 received at 13402 <at> debbugs.gnu.org (full text, mbox):
From: Alan Mackenzie <acm <at> muc.de> To: Juri Linkov <juri <at> jurta.org> Cc: 13402 <at> debbugs.gnu.org Subject: Re: bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page Date: Wed, 13 Feb 2013 09:47:24 +0000
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-gnu-emacs <at> gnu.org:bug#13402; Package emacs.
(Wed, 13 Feb 2013 17:56:01 GMT) Full text and rfc822 format available.Message #17 received at 13402 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> jurta.org> To: Alan Mackenzie <acm <at> muc.de> Cc: 13402 <at> debbugs.gnu.org Subject: Re: bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page Date: Wed, 13 Feb 2013 19:53:28 +0200
> 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-gnu-emacs <at> gnu.org:bug#13402; Package emacs.
(Wed, 13 Feb 2013 22:01:02 GMT) Full text and rfc822 format available.Message #20 received at 13402 <at> debbugs.gnu.org (full text, mbox):
From: Alan Mackenzie <acm <at> muc.de> To: Juri Linkov <juri <at> jurta.org> Cc: 13402 <at> debbugs.gnu.org Subject: Re: bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page Date: Wed, 13 Feb 2013 21:59:36 +0000
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).
Juri Linkov <juri <at> jurta.org>:Alan Mackenzie <acm <at> muc.de>:Message #25 received at 13402-done <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> jurta.org> To: Alan Mackenzie <acm <at> muc.de> Cc: 13402-done <at> debbugs.gnu.org Subject: Re: bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page Date: Thu, 14 Feb 2013 11:16:16 +0200
>> > 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-gnu-emacs <at> gnu.org:bug#13402; Package emacs.
(Thu, 14 Feb 2013 20:11:02 GMT) Full text and rfc822 format available.Message #28 received at 13402 <at> debbugs.gnu.org (full text, mbox):
From: Alan Mackenzie <acm <at> muc.de> To: emacs-devel <at> gnu.org Cc: 13402 <at> debbugs.gnu.org Subject: Re: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch] Date: Thu, 14 Feb 2013 20:09:38 +0000
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-gnu-emacs <at> gnu.org:bug#13402; Package emacs.
(Thu, 14 Feb 2013 23:55:01 GMT) Full text and rfc822 format available.Message #31 received at 13402 <at> debbugs.gnu.org (full text, mbox):
From: Glenn Morris <rgm <at> gnu.org> To: Alan Mackenzie <acm <at> muc.de> Cc: 13402 <at> debbugs.gnu.org Subject: Re: bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch] Date: Thu, 14 Feb 2013 18:53:34 -0500
(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-gnu-emacs <at> gnu.org:bug#13402; Package emacs.
(Fri, 15 Feb 2013 07:51:02 GMT) Full text and rfc822 format available.Message #34 received at 13402 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> jurta.org> To: Alan Mackenzie <acm <at> muc.de> Cc: 13402 <at> debbugs.gnu.org Subject: Re: bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch] Date: Fri, 15 Feb 2013 09:47:10 +0200
> 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-gnu-emacs <at> gnu.org:bug#13402; Package emacs.
(Fri, 15 Feb 2013 10:33:02 GMT) Full text and rfc822 format available.Message #37 received at 13402 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> jurta.org> To: Alan Mackenzie <acm <at> muc.de> Cc: 13402 <at> debbugs.gnu.org Subject: Re: bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch] Date: Fri, 15 Feb 2013 12:10:45 +0200
>> 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-gnu-emacs <at> gnu.org:bug#13402; Package emacs.
(Fri, 15 Feb 2013 11:57:01 GMT) Full text and rfc822 format available.Message #40 received at 13402 <at> debbugs.gnu.org (full text, mbox):
From: Alan Mackenzie <acm <at> muc.de> To: Juri Linkov <juri <at> jurta.org> Cc: 13402 <at> debbugs.gnu.org Subject: Re: bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch] Date: Fri, 15 Feb 2013 11:55:02 +0000
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-gnu-emacs <at> gnu.org:bug#13402; Package emacs.
(Fri, 15 Feb 2013 13:22:02 GMT) Full text and rfc822 format available.Message #43 received at 13402 <at> debbugs.gnu.org (full text, mbox):
From: Alan Mackenzie <acm <at> muc.de> To: Juri Linkov <juri <at> jurta.org> Cc: 13402 <at> debbugs.gnu.org Subject: Re: bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch] Date: Fri, 15 Feb 2013 13:20:04 +0000
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-gnu-emacs <at> gnu.org:bug#13402; Package emacs.
(Sat, 16 Feb 2013 22:31:01 GMT) Full text and rfc822 format available.Message #46 received at 13402 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> jurta.org> To: Alan Mackenzie <acm <at> muc.de> Cc: 13402 <at> debbugs.gnu.org Subject: Re: bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch] Date: Sat, 16 Feb 2013 23:50:39 +0200
> 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-gnu-emacs <at> gnu.org:bug#13402; Package emacs.
(Sun, 17 Feb 2013 10:12:01 GMT) Full text and rfc822 format available.Message #49 received at 13402 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> jurta.org> To: Alan Mackenzie <acm <at> muc.de> Cc: 13402 <at> debbugs.gnu.org Subject: Re: bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch] Date: Sun, 17 Feb 2013 12:03:45 +0200
> 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-gnu-emacs <at> gnu.org:bug#13402; Package emacs.
(Tue, 19 Feb 2013 14:53:02 GMT) Full text and rfc822 format available.Message #52 received at 13402 <at> debbugs.gnu.org (full text, mbox):
From: Alan Mackenzie <acm <at> muc.de> To: Juri Linkov <juri <at> jurta.org> Cc: 13402 <at> debbugs.gnu.org Subject: Re: bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch] Date: Tue, 19 Feb 2013 14:50:57 +0000
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-gnu-emacs <at> gnu.org:bug#13402; Package emacs.
(Wed, 20 Feb 2013 11:49:02 GMT) Full text and rfc822 format available.Message #55 received at 13402 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> jurta.org> To: Alan Mackenzie <acm <at> muc.de> Cc: 13402 <at> debbugs.gnu.org Subject: Re: bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch] Date: Wed, 20 Feb 2013 12:49:04 +0200
> 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-gnu-emacs <at> gnu.org:bug#13402; Package emacs.
(Wed, 20 Feb 2013 22:40:01 GMT) Full text and rfc822 format available.Message #58 received at 13402 <at> debbugs.gnu.org (full text, mbox):
From: Alan Mackenzie <acm <at> muc.de> To: Juri Linkov <juri <at> jurta.org> Cc: 13402 <at> debbugs.gnu.org Subject: Re: bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch] Date: Wed, 20 Feb 2013 22:37:59 +0000
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-gnu-emacs <at> gnu.org:bug#13402; Package emacs.
(Thu, 21 Feb 2013 01:08:01 GMT) Full text and rfc822 format available.Message #61 received at 13402 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> jurta.org> To: Alan Mackenzie <acm <at> muc.de> Cc: 13402 <at> debbugs.gnu.org Subject: Re: bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch] Date: Thu, 21 Feb 2013 02:45:31 +0200
>> 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-gnu-emacs <at> gnu.org:bug#13402; Package emacs.
(Thu, 21 Feb 2013 12:30:02 GMT) Full text and rfc822 format available.Message #64 received at 13402 <at> debbugs.gnu.org (full text, mbox):
From: Alan Mackenzie <acm <at> muc.de> To: Juri Linkov <juri <at> jurta.org> Cc: 13402 <at> debbugs.gnu.org Subject: Re: bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch] Date: Thu, 21 Feb 2013 12:27:40 +0000
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-gnu-emacs <at> gnu.org:bug#13402; Package emacs.
(Thu, 21 Feb 2013 17:11:01 GMT) Full text and rfc822 format available.Message #67 received at 13402 <at> debbugs.gnu.org (full text, mbox):
From: Glenn Morris <rgm <at> gnu.org> To: Alan Mackenzie <acm <at> muc.de> Cc: Juri Linkov <juri <at> jurta.org>, 13402 <at> debbugs.gnu.org Subject: Re: bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch] Date: Thu, 21 Feb 2013 12:09:28 -0500
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-gnu-emacs <at> gnu.org:bug#13402; Package emacs.
(Thu, 21 Feb 2013 17:51:01 GMT) Full text and rfc822 format available.Message #70 received at 13402 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> jurta.org> To: Glenn Morris <rgm <at> gnu.org> Cc: Alan Mackenzie <acm <at> muc.de>, 13402 <at> debbugs.gnu.org Subject: Re: bug#13402: 24.2.92 pretest: bugs in isearch-yank-line in info page. [Patch] Date: Thu, 21 Feb 2013 19:48:04 +0200
>> 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.
Debbugs Internal Request <help-debbugs <at> gnu.org>
to internal_control <at> debbugs.gnu.org.
(Fri, 22 Mar 2013 11:24:03 GMT) Full text and rfc822 format available.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.