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.
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).
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.