Re: bug#17453: Isearch doesn't work properly with Follow Mode.

[Alan Mackenzie <acm@muc.de>]

Hello again, Artur

On Mon, Nov 02, 2015 at 02:18:59PM +0000, Artur Malabarba wrote:
> > > Lazy highlighting searches for matches on the current window.  It must
> > > be extended to search for matches on the Follow Mode group of windows.
> > > For that, it needs the details of the "window*-start" and "window*-end".

> > Yes, you're right, I see that now. This might still be solvable via
> > setting isearch-search-fun-function, I'll need to think on that for a
> > moment.

> Here's a new version, split into 3 short patches to make it easier to
> understand.

Thanks.  I understand what you're suggesting, now, and I think it's a
good idea, on the whole.

However, about the bug where, with lazy highlighting active, isearch
scrolls the window its on rather than moving to the next one: as I said,
this is caused by the "(sit-for 0)" near the beginning of
isearch-lazy-highlight-new-loop.  The purpose of this is to scroll the
display, in case point has moved off of the window.

The bug cause is now clear: ..-lazy-highlight-new-loop first scrolls the
window, then checks whether it needs to restart its lazy h. loop.  If it
does, it starts the timer which triggers ..-lazy-highlight-update after
the delay.

The bug is that all this checking happens in ...-lazy-h-new-loop rather
than in ...-lazy-h-update.  If we moved all the checking to
..l-h-update, then there would be no need for "(sit-for 0)" - the
command loop redisplay would already have scrolled the windows, if
necessary; and Follow Mode would already have shifted point to the right
window.

So how about us just moving all these checks to where they really
belong, in isearch-lazy-highlight-update?  I've a feeling that if we do
this, then your function follow--search-function becomes unneeded.

Juri?

#########################################################################

A related point in one of your patches below.  It explicitly selects one
of the Follow Mode windows.  This is bad.  Follow Mode itself choses the
right window in its post-command-hook.  Second-guessing it within the
command processing is liable to lead to confusion, maintenance
difficulties, and general pain.

Another point.  It is better to use follow-all-followers to get at the
windows rather than get-buffer-window-list.  The first of these is
guaranteed to get the windows in the right order, and also allows for
a future enhancement whereby only some of the windows displaying
<buffer> are part of the Follow Mode group.  Of course, anything using
follow-all-followers needs to be textually in follow.el.

> This makes 2 small changes to isearch (which I think are even
> justifiable on their own, regardless of follow-mode), and defines a
> new function in follow-mode which takes care of two things:

> Matches are highlighted on all windows

Here, the matches should be highlighted on all the windows in the FM
group without exception.  I think a new option you're providing allows
lazy highlighting to happen only in the current window.  This is a bad
idea when FM is active.



> When the user types something, and the next match is on another
> window, switch to that window instead of scrolling the current one.

> From caf5adfc0b7caf07dc74813242f9ecc664babc13 Mon Sep 17 00:00:00 2001
> From: Artur Malabarba <bruce.connor.am@gmail.com>
> Date: Mon, 2 Nov 2015 14:01:18 +0000
> Subject: [PATCH 3/3] * lisp/follow.el: Improve isearch compatibility
> 
> (follow--search-function): New function.
> (follow-mode): Configure `isearch-lazy-highlight' and
> `isearch-search-fun-function'.
> ---
>  lisp/follow.el | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/lisp/follow.el b/lisp/follow.el
> index 938c59e..0b80f04 100644
> --- a/lisp/follow.el
> +++ b/lisp/follow.el
> @@ -419,6 +419,9 @@ follow-mode
>    :keymap follow-mode-map
>    (if follow-mode
>        (progn
> +        (setq-local isearch-search-fun-function #'follow--search-function)
> +        (when isearch-lazy-highlight
> +          (setq-local isearch-lazy-highlight 'all-windows))
>  	(add-hook 'compilation-filter-hook 'follow-align-compilation-windows t t)
>  	(add-hook 'post-command-hook 'follow-post-command-hook t)
>  	(add-hook 'window-size-change-functions 'follow-window-size-change t))
> @@ -434,6 +437,36 @@ follow-mode
>  	(remove-hook 'window-size-change-functions 'follow-window-size-change)))
>      (remove-hook 'compilation-filter-hook 'follow-align-compilation-windows t)))
>  
> +(defun follow--search-function ()
> +  "Return a function suitable for `isearch-search-fun-function'."
> +  (lambda (string &optional bound noerror count)
> +    (let* ((search-function (isearch-search-fun-default))
> +           (all-followers (follow-all-followers))
> +           ;; If bound at the edge of the window, extend it to the
> +           ;; edges know by `follow-mode'.
> +           (bound (cond ((equal bound (window-end))
> +                         (window-end (car (last all-followers))))
> +                        ((equal bound (window-start))
> +                         (window-start (car all-followers)))
> +                        (t bound)))
> +           (matched (funcall search-function string bound noerror count)))
> +      ;; If this is a proper user-triggered search (and not just a
> +      ;; lazy-highlight search), and if the search matched, and if the
> +      ;; match is not visible on this window...
> +      (when (and matched
> +                 (not isearch-lazy-highlight-ongoing-search)
> +                 (not (and (pos-visible-in-window-p (match-beginning 0))
> +                           (pos-visible-in-window-p (match-end 0)))))
> +        ;; ... see if the match is visible on another window.
> +        (let ((win (seq-find (lambda (w)
> +                             (and (pos-visible-in-window-p (match-beginning 0) w)
> +                                  (pos-visible-in-window-p (match-end 0) w)))
> +                           all-followers)))
> +          ;; If so, select it.
> +          (when win
> +            (select-window win))))                       <===========================
> +      matched)))
> +
>  (defun follow-find-file-hook ()
>    "Find-file hook for Follow mode.  See the variable `follow-auto'."
>    (if follow-auto (follow-mode 1)))
> -- 
> 2.6.2
> 

> From 8a52f12872d2b99acb2ca5a2077ccd87779309fe Mon Sep 17 00:00:00 2001
> From: Artur Malabarba <bruce.connor.am@gmail.com>
> Date: Mon, 2 Nov 2015 13:54:15 +0000
> Subject: [PATCH 2/3] * lisp/isearch.el: Bind a variable while
>  lazy-highlighting
> 
> (isearch-lazy-highlight-ongoing-search): New variable.
> (isearch-lazy-highlight-search): Use it.
> ---
>  lisp/isearch.el | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/lisp/isearch.el b/lisp/isearch.el
> index 6442166..ef49650 100644
> --- a/lisp/isearch.el
> +++ b/lisp/isearch.el
> @@ -3080,11 +3080,18 @@ isearch-lazy-highlight-new-loop
>  	      (run-with-idle-timer lazy-highlight-initial-delay nil
>  				   'isearch-lazy-highlight-update)))))
>  
> +(defvar isearch-lazy-highlight-ongoing-search nil
> +  "Dynamically bound to t in `isearch-lazy-highlight-search'.
> +This can be used by `isearch-search-fun-function' to detect
> +whether its return value is being run for a proper user search or
> +a lazy highlight search.")
> +
>  (defun isearch-lazy-highlight-search ()
>    "Search ahead for the next or previous match, for lazy highlighting.
>  Attempt to do the search exactly the way the pending Isearch would."
>    (condition-case nil
>        (let ((case-fold-search isearch-lazy-highlight-case-fold-search)
> +            (isearch-lazy-highlight-ongoing-search t)
>  	    (isearch-regexp isearch-lazy-highlight-regexp)
>  	    (isearch-regexp-function isearch-lazy-highlight-regexp-function)
>  	    (isearch-lax-whitespace
> -- 
> 2.6.2
> 

> From 0c815fa21001b4a29f124e8eda58f7889560d701 Mon Sep 17 00:00:00 2001
> From: Artur Malabarba <bruce.connor.am@gmail.com>
> Date: Mon, 2 Nov 2015 12:09:16 +0000
> Subject: [PATCH 1/3] * lisp/isearch.el: Allow lazy-highlighting of all windows
> 
> (isearch-lazy-highlight): New possible value.
> (isearch-lazy-highlight-update): Use it.
> ---
>  lisp/isearch.el | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/lisp/isearch.el b/lisp/isearch.el
> index b762884..6442166 100644
> --- a/lisp/isearch.el
> +++ b/lisp/isearch.el
> @@ -279,8 +279,13 @@ isearch-lazy-highlight
>    "Controls the lazy-highlighting during incremental search.
>  When non-nil, all text in the buffer matching the current search
>  string is highlighted lazily (see `lazy-highlight-initial-delay'
> -and `lazy-highlight-interval')."
> -  :type 'boolean
> +and `lazy-highlight-interval').
> +
> +When multiple windows display the current buffer, the
> +highlighting is displayed only on the selected window, unless
> +this variable is set to the symbol `all-windows'."
> +  :type '(choice boolean
> +                 (const :tag "On, and applied to all windows" all-windows))
>    :group 'lazy-highlight
>    :group 'isearch)
>  
> @@ -3162,7 +3167,8 @@ isearch-lazy-highlight-update
>  			  ;; 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))))
> +                          (unless (eq isearch-lazy-highlight 'all-windows)
> +                            (overlay-put ov 'window (selected-window)))))
>  		      ;; Remember the current position of point for
>  		      ;; the next call of `isearch-lazy-highlight-update'
>  		      ;; when `lazy-highlight-max-at-a-time' is too small.
> -- 
> 2.6.2
> 



-- 
Alan Mackenzie (Nuremberg, Germany).