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).