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

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

[Alan Mackenzie]

Hello, Stefan, hello, Emacs.

Resuming a conversation from a year and a half ago, in it I had proposed
a patch which made isearch work properly in a Follow Mode set of
windows.  In the end, you rejected my patch (in effect), because...

On Fri, May 09, 2014 at 10:40:07PM -0400, Stefan Monnier wrote:

> I must say I really dislike this hard-coding of follow-mode support in
> isearch.el.  Isearch should not know so much about follow-mode and
> follow-mode should not know so much about Isearch either.

> IOW we should try harder to come up with more general hooks.

That irritated me at the time, but you were right - putting Follow Mode
support directly into isearch would not have been a good idea.

I've recently started working on the bug again, and although it's not
your job anymore, I'd still appreciate you giving me some feedback on my
new approach.

What I propose is that several functions which operate on windows have
extended versions which operate on @dfn{groups of windows}, this meaning
(for the moment) Follow Mode groups of windows.  These new functions are
flexible enough to work with methods other than Follow Mode of grouping
windows, should such ever be implemented.

The new functions are window*-start, window*-end, set-window*-start,
recenter*, pos-visible-in-window*-p, move-to-window*-line, and sit*-for.

window*-start returns the window-start of the _first_ window in the
group, window*-end returns the window-end of the _last_ window in the
group, and so on, all these functions doing the Right Thing for a group
of windows.  sit*-for exists, because Follow Mode needs a chance to
resynchronise its windows before redisplay happens.

If Follow Mode is not enabled, all the above functions do the Right
Thing on the single window.

In addition, there is a function window*-windows, which returns an
ordered list of the windows involved (or a list of the single window
when F.M. is not active).

I have a trial implementation of the above, and a trial adaptation of
isearch.el which uses it.  It works.

What do you think?

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[John Wiegley]

>>>>> Alan Mackenzie <acm@muc.de> writes:

> The new functions are window*-start, window*-end, set-window*-start,
> recenter*, pos-visible-in-window*-p, move-to-window*-line, and sit*-for.
> 
> window*-start returns the window-start of the _first_ window in the group,
> window*-end returns the window-end of the _last_ window in the group, and so
> on, all these functions doing the Right Thing for a group of windows.
> sit*-for exists, because Follow Mode needs a chance to resynchronise its
> windows before redisplay happens.

What is the reason for having separate functions such as window*-start,
instead of just taking the car of a list of windows?  I may be missing some
context here, but this sounds like special-casing general behavior, and I'm
wondering why it's necessary...

John

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

[Artur Malabarba]

[-- Attachment #1: Type: text/plain, Size: 889 bytes --]

On 29 Oct 2015 11:23 pm, "Alan Mackenzie" <acm@muc.de> wrote:
>
> Hello, Stefan, hello, Emacs.
>
> Resuming a conversation from a year and a half ago, in it I had proposed
> a patch which made isearch work properly in a Follow Mode set of
> windows.  In the end, you rejected my patch (in effect), because...
>
> On Fri, May 09, 2014 at 10:40:07PM -0400, Stefan Monnier wrote:
>
> > I must say I really dislike this hard-coding of follow-mode support in
> > isearch.el.  Isearch should not know so much about follow-mode and
> > follow-mode should not know so much about Isearch either.
>
> > IOW we should try harder to come up with more general hooks.

What if isearch just took into account all windows displaying
current-buffer, instead of just the selected one?
This wouldn’t involve anything specific to follow mode, and I believe it
would solve the issue, no?

[-- Attachment #2: Type: text/html, Size: 1085 bytes --]

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

[Alan Mackenzie]

Hello, John.

On Sat, Oct 31, 2015 at 03:35:22PM -0700, John Wiegley wrote:
> >>>>> Alan Mackenzie <acm@muc.de> writes:

> > The new functions are window*-start, window*-end, set-window*-start,
> > recenter*, pos-visible-in-window*-p, move-to-window*-line, and sit*-for.

> > window*-start returns the window-start of the _first_ window in the group,
> > window*-end returns the window-end of the _last_ window in the group, and so
> > on, all these functions doing the Right Thing for a group of windows.
> > sit*-for exists, because Follow Mode needs a chance to resynchronise its
> > windows before redisplay happens.

> What is the reason for having separate functions such as window*-start,
> instead of just taking the car of a list of windows?  I may be missing some
> context here, but this sounds like special-casing general behavior, and I'm
> wondering why it's necessary...

The prime use case is in isearch.  At the moment isearch doesn't work
well when Follow Mode is active (try it!).

My first attempt at amending isearch for this involved manipulating
Follow Mode's list of windows directly inside isearch.el.  The result
was too close a coupling between isearch and Follow Mode, and was also
rather ragged.  Stefan rejected this patch, asking for a more abstract
solution.

What I am proposing now is a solution where any library which needs to
manipulate things like window positions will be trivially upgradable to
working with Follow Mode, merely by replacing `window-start' by
`window*-start', etc.

The fact that the "group" of windows is represented by a list is an
implementation detail to be encapsulated within follow.el.  In the
(fairly distant) future, this might perhaps be superseded by code in
redisplay.  Perhaps.

> John

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Alan Mackenzie]

Hello, Artur

On Sat, Oct 31, 2015 at 11:13:34PM +0000, Artur Malabarba wrote:
> On 29 Oct 2015 11:23 pm, "Alan Mackenzie" <acm@muc.de> wrote:

> > Hello, Stefan, hello, Emacs.

> > Resuming a conversation from a year and a half ago, in it I had proposed
> > a patch which made isearch work properly in a Follow Mode set of
> > windows.  In the end, you rejected my patch (in effect), because...

> > On Fri, May 09, 2014 at 10:40:07PM -0400, Stefan Monnier wrote:

> > > I must say I really dislike this hard-coding of follow-mode support in
> > > isearch.el.  Isearch should not know so much about follow-mode and
> > > follow-mode should not know so much about Isearch either.

> > > IOW we should try harder to come up with more general hooks.

> What if isearch just took into account all windows displaying
> current-buffer, instead of just the selected one?
> This wouldn’t involve anything specific to follow mode, and I believe it
> would solve the issue, no?

I don't think so, really.  What exactly does "took into account" mean?
With Follow Mode active, on a forward search you explicitly want point to
move into the next window when the search target is visible there.  When
FM is not active, you most definitely don't want this to happen.

I have working code which make isearch and FM work together nicely.  I
think it's a sufficiently "nice" implementation to commit.

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Juri Linkov]

> If Follow Mode is not enabled, all the above functions do the Right
> Thing on the single window.

From another perspective, settings lazy-highlight-buffer to t
(implemented in bug#21092) and removing the current restriction of
(overlay-put ov 'window (selected-window)) will lazy-highlight matches
in all follow windows with no effort.

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

[John Wiegley]

>>>>> Alan Mackenzie <acm@muc.de> writes:

> What I am proposing now is a solution where any library which needs to
> manipulate things like window positions will be trivially upgradable to
> working with Follow Mode, merely by replacing `window-start' by
> `window*-start', etc.

Ah, I see. How many libraries do you think would need this change? Would using
window-start become bad practice under this regime?

John

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

[Alan Mackenzie]

Hello, Juri.

On Sun, Nov 01, 2015 at 01:35:53AM +0200, Juri Linkov wrote:
> > If Follow Mode is not enabled, all the above functions do the Right
> > Thing on the single window.

> >>From another perspective, settings lazy-highlight-buffer to t
> (implemented in bug#21092) and removing the current restriction of
> (overlay-put ov 'window (selected-window)) will lazy-highlight matches
> in all follow windows with no effort.

I wasn't actually aware of that fix.

There were three main problems my patch fixed:
1) Searching commands were restricted to a single follow window.  This
was caused by the lazy highlighting mechanism, as you say.
2) Lazy highlighting was only being done in a single window.
3) In scrolling commands, point was restricted to the singled window,
rather than being able to move freely throughout all the windows.

There were also some subtle things which could go wrong, in particular
when the current isearch match spans two windows, and the next command
causes the echo area to expand a line.  This caused spurious scrolling
of the windows.

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Drew Adams]

It's really not great when people cross-post a bug thread
to emacs-devel (or vice versa).

It can sometimes be appropriate to post a message to one
or the other that references the other, to make people aware
of the thread in the other list.  And it can sometimes be
appropriate to move a discussion from one list to the other.

But this needless multiplication is a bother.

(And yes, I've left both lists in this message to both.)

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

[Alan Mackenzie]

Hello, John.

On Sat, Oct 31, 2015 at 04:41:01PM -0700, John Wiegley wrote:
> >>>>> Alan Mackenzie <acm@muc.de> writes:

> > What I am proposing now is a solution where any library which needs to
> > manipulate things like window positions will be trivially upgradable to
> > working with Follow Mode, merely by replacing `window-start' by
> > `window*-start', etc.

> Ah, I see. How many libraries do you think would need this change?

I honestly don't know.  At a guess, I'd say several rather than many.
As a quick benchmark, there are 127 occurrences of set-window-start in
our lisp sources, in 59 files.

> Would using window-start become bad practice under this regime?

I hadn't actually thought of that.  Thinking about set-window-start
(rather than simply window-start), there will be lots of places where a
mode is explicitly handling its own windows (? speedbar.el, for example),
and set-window*-start would be the wrong thing.  But there are also
surely examples where set-window-start is currently used just to scroll
the text to a specific position.  Here set-window*-start would be
better.

> John

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Artur Malabarba]

Hi Alan,

>> What if isearch just took into account all windows displaying
>> current-buffer, instead of just the selected one?
>> This wouldn’t involve anything specific to follow mode, and I believe it
>> would solve the issue, no?
>
> I don't think so, really.  What exactly does "took into account" mean?
> With Follow Mode active, on a forward search you explicitly want point to
> move into the next window when the search target is visible there. When
> FM is not active, you most definitely don't want this to happen.

Well, one of the reasons I suggested it is because I think I actually
would want that. But, of course, it would be conditioned on a variable
(disabled by default), and follow-mode would simply have to enable it.

> I have working code which make isearch and FM work together nicely.  I
> think it's a sufficiently "nice" implementation to commit.

Sure. Don't take this as me trying to push you in a different
direction. I brought it up because it sounded like it would be
simpler.
You have a better understanding of the issue and the code than I do,
don't feel obliged to try my suggestion.

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

[Artur Malabarba]

2015-11-01 12:20 GMT+00:00 Artur Malabarba <bruce.connor.am@gmail.com>:
> You have a better understanding of the issue and the code than I do,
> don't feel obliged to try my suggestion.

Anyway, could we see this code? :-)
Can you send it as patch or push it to a scratch/window-groups branch?

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

[Alan Mackenzie]

Hello, Artur.

On Sun, Nov 01, 2015 at 12:23:52PM +0000, Artur Malabarba wrote:
> 2015-11-01 12:20 GMT+00:00 Artur Malabarba <bruce.connor.am@gmail.com>:
> > You have a better understanding of the issue and the code than I do,
> > don't feel obliged to try my suggestion.

> Anyway, could we see this code? :-)
> Can you send it as patch or push it to a scratch/window-groups branch?

No problem!  The patch below doesn't actually apply at the moment, due
to some (?very) recent change in isearch.el.  But it wouldn't run
anyway, because window*-start and friends aren't there yet.



diff --git a/lisp/isearch.el b/lisp/isearch.el
index 4fc9b38..07ec534 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -184,6 +184,11 @@ isearch-message-function
   "Function to call to display the search prompt.
 If nil, use function `isearch-message'.")
 
+(defmacro isearch-call-message (&optional cqh ellip)
+  `(if isearch-message-function
+       (funcall isearch-message-function ,cqh ,ellip)
+     (isearch-message ,cqh ,ellip)))
+
 (defvar isearch-wrap-function nil
   "Function to call to wrap the search when search is failed.
 If nil, move point to the beginning of the buffer for a forward search,
@@ -615,6 +620,7 @@ isearch-adjusted
 (defvar isearch-slow-terminal-mode nil)
 ;; If t, using a small window.
 (defvar isearch-small-window nil)
+(defvar isearch-suspended-follow-mode nil) ; only used with a small window.
 (defvar isearch-opoint 0)
 ;; The window configuration active at the beginning of the search.
 (defvar isearch-window-configuration nil)
@@ -885,6 +891,7 @@ isearch-mode
 					      (abs search-slow-window-lines))))
 	isearch-other-end nil
 	isearch-small-window nil
+        isearch-suspended-follow-mode nil
 	isearch-just-started t
 	isearch-start-hscroll (window-hscroll)
 
@@ -971,14 +978,15 @@ isearch-update
 	   (null executing-kbd-macro))
       (progn
         (if (not (input-pending-p))
-	    (if isearch-message-function
-		(funcall isearch-message-function)
-	      (isearch-message)))
+            (isearch-call-message))
         (if (and isearch-slow-terminal-mode
                  (not (or isearch-small-window
-                          (pos-visible-in-window-p))))
+                          (pos-visible-in-window*-p))))
             (let ((found-point (point)))
               (setq isearch-small-window t)
+              (when (and (boundp 'follow-mode) follow-mode)
+                (follow-mode 0)
+                (setq isearch-suspended-follow-mode t))
               (move-to-window-line 0)
               (let ((window-min-height 1))
                 (split-window nil (if (< search-slow-window-lines 0)
@@ -997,7 +1005,7 @@ isearch-update
 	  (let ((current-scroll (window-hscroll))
 		visible-p)
 	    (set-window-hscroll (selected-window) isearch-start-hscroll)
-	    (setq visible-p (pos-visible-in-window-p nil nil t))
+	    (setq visible-p (pos-visible-in-window*-p nil nil t))
 	    (if (or (not visible-p)
 		    ;; When point is not visible because of hscroll,
 		    ;; pos-visible-in-window-p returns non-nil, but
@@ -1051,17 +1059,20 @@ isearch-done
   (setq minibuffer-message-timeout isearch-original-minibuffer-message-timeout)
   (isearch-dehighlight)
   (lazy-highlight-cleanup lazy-highlight-cleanup)
-  (let ((found-start (window-start))
+  (let ((found-start (window*-start))
 	(found-point (point)))
     (when isearch-window-configuration
       (set-window-configuration isearch-window-configuration)
       (if isearch-small-window
-	  (goto-char found-point)
+          (progn
+            (when isearch-suspended-follow-mode
+              (follow-mode 1))
+            (goto-char found-point))
 	;; set-window-configuration clobbers window-start; restore it.
 	;; This has an annoying side effect of clearing the last_modiff
 	;; field of the window, which can cause unwanted scrolling,
 	;; so don't do it unless truly necessary.
-	(set-window-start (selected-window) found-start t))))
+	(set-window*-start (selected-window) found-start t))))
 
   (setq isearch-mode nil)
   (if isearch-input-method-local-p
@@ -1284,13 +1295,6 @@ with-isearch-suspended
 	  (unwind-protect
 	      (progn ,@body)
 
-	    ;; Set point at the start (end) of old match if forward (backward),
-	    ;; so after exiting minibuffer isearch resumes at the start (end)
-	    ;; of this match and can find it again.
-	    (if (and old-other-end (eq old-point (point))
-		     (eq isearch-forward isearch-new-forward))
-		(goto-char old-other-end))
-
 	    ;; Always resume isearching by restarting it.
 	    (isearch-mode isearch-forward
 			  isearch-regexp
@@ -1303,7 +1307,17 @@ with-isearch-suspended
 		  isearch-message isearch-new-message
 		  isearch-forward isearch-new-forward
 		  isearch-word isearch-new-word
-		  isearch-case-fold-search isearch-new-case-fold))
+		  isearch-case-fold-search isearch-new-case-fold)
+
+	    ;; Restore the minibuffer message before moving point.
+	    (isearch-call-message nil t)
+
+	    ;; Set point at the start (end) of old match if forward (backward),
+	    ;; so after exiting minibuffer isearch resumes at the start (end)
+	    ;; of this match and can find it again.
+	    (if (and old-other-end (eq old-point (point))
+		     (eq isearch-forward isearch-new-forward))
+		(goto-char old-other-end)))
 
 	  ;; Empty isearch-string means use default.
 	  (when (= 0 (length isearch-string))
@@ -1898,6 +1912,7 @@ isearch-del-char
 					    (length isearch-string))))
           isearch-message (mapconcat 'isearch-text-char-description
                                      isearch-string "")))
+  (isearch-call-message nil t) 		; Do this before moving point.
   ;; Use the isearch-other-end as new starting point to be able
   ;; to find the remaining part of the search string again.
   ;; This is like what `isearch-search-and-update' does,
@@ -2074,6 +2089,7 @@ isearch-search-and-update
 	      (setq isearch-case-fold-search
 		    (isearch-no-upper-case-p isearch-string isearch-regexp))))
       ;; Not regexp, not reverse, or no match at point.
+      (isearch-call-message nil t)	; Do this before moving point.
       (if (and isearch-other-end (not isearch-adjusted))
 	  (goto-char (if isearch-forward isearch-other-end
 		       (min isearch-opoint
@@ -2234,16 +2250,23 @@ isearch-allow-prefix
   :type 'boolean
   :group 'isearch)
 
+;; (defmacro save-excursion-and-window (&rest body)
+;;   "FIXME!!!"
+;;   `(let ((-old-win- (selected-window)))
+;;      (save-excursion
+;;        (unwind-protect (progn ,@body)
+;;          (select-window -old-win- t)))))
+
 (defun isearch-string-out-of-window (isearch-point)
   "Test whether the search string is currently outside of the window.
 Return nil if it's completely visible, or if point is visible,
 together with as much of the search string as will fit; the symbol
 `above' if we need to scroll the text downwards; the symbol `below',
 if upwards."
-  (let ((w-start (window-start))
-        (w-end (window-end nil t))
-        (w-L1 (save-excursion (move-to-window-line 1) (point)))
-        (w-L-1 (save-excursion (move-to-window-line -1) (point)))
+  (let ((w-start (window*-start))
+        (w-end (window*-end nil t))
+        (w-L1 (save-selected-window (move-to-window*-line 1) (point)))
+        (w-L-1 (save-selected-window (move-to-window*-line -1) (point)))
         start end)                  ; start and end of search string in buffer
     (if isearch-forward
         (setq end isearch-point  start (or isearch-other-end isearch-point))
@@ -2270,15 +2293,15 @@ isearch-back-into-window
     (if above
         (progn
           (goto-char start)
-          (recenter 0)
-          (when (>= isearch-point (window-end nil t))
+          (recenter* 0)
+          (when (>= isearch-point (window*-end nil t))
             (goto-char isearch-point)
-            (recenter -1)))
+            (recenter* -1)))
       (goto-char end)
-      (recenter -1)
-      (when (< isearch-point (window-start))
+      (recenter* -1)
+      (when (< isearch-point (window*-start))
         (goto-char isearch-point)
-        (recenter 0))))
+        (recenter* 0))))
   (goto-char isearch-point))
 
 (defvar isearch-pre-scroll-point nil)
@@ -2425,6 +2448,7 @@ isearch-ring-adjust
   (isearch-ring-adjust1 advance)
   (if search-ring-update
       (progn
+	(isearch-call-message nil t)
 	(isearch-search)
 	(isearch-push-state)
 	(isearch-update))
@@ -2497,6 +2521,13 @@ isearch-complete-edit
 
 (defun isearch-message (&optional c-q-hack ellipsis)
   ;; Generate and print the message string.
+
+  ;; N.B.: This function should always be called with point at the
+  ;; search point, because in certain (rare) circumstances, undesired
+  ;; scrolling can happen when point is elsewhere.  These
+  ;; circumstances are when follow-mode is active, the search string
+  ;; spans two (or several) windows, and the message about to be
+  ;; displayed will cause the echo area to expand.
   (let ((cursor-in-echo-area ellipsis)
 	(m isearch-message)
 	(fail-pos (isearch-fail-pos t)))
@@ -2679,9 +2710,6 @@ isearch-search-string
 
 (defun isearch-search ()
   ;; Do the search with the current search string.
-  (if isearch-message-function
-      (funcall isearch-message-function nil t)
-    (isearch-message nil t))
   (if (and (eq isearch-case-fold-search t) search-upper-case)
       (setq isearch-case-fold-search
 	    (isearch-no-upper-case-p isearch-string isearch-regexp)))
@@ -3015,11 +3043,13 @@ isearch-lazy-highlight-new-loop
 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
+             ;; make sure (window*-start) is credible and windows are
+             ;; properly synchronised.
+             (sit*-for 0)
              (or (not (equal isearch-string
                              isearch-lazy-highlight-last-string))
-                 (not (eq (selected-window)
-                          isearch-lazy-highlight-window))
+                 (not (memq (selected-window)
+                            (window*-windows isearch-lazy-highlight-window)))
 		 (not (eq isearch-lazy-highlight-case-fold-search
 			  isearch-case-fold-search))
 		 (not (eq isearch-lazy-highlight-regexp
@@ -3030,9 +3060,9 @@ isearch-lazy-highlight-new-loop
 			  isearch-lax-whitespace))
 		 (not (eq isearch-lazy-highlight-regexp-lax-whitespace
 			  isearch-regexp-lax-whitespace))
-                 (not (= (window-start)
+                 (not (= (window*-start)
                          isearch-lazy-highlight-window-start))
-                 (not (= (window-end)   ; Window may have been split/joined.
+                 (not (= (window*-end)   ; Window may have been split/joined.
 			 isearch-lazy-highlight-window-end))
 		 (not (eq isearch-forward
 			  isearch-lazy-highlight-forward))
@@ -3048,8 +3078,8 @@ isearch-lazy-highlight-new-loop
     (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-window-start (window*-start)
+	  isearch-lazy-highlight-window-end   (window*-end)
 	  ;; Start lazy-highlighting at the beginning of the found
 	  ;; match (`isearch-other-end').  If no match, use point.
 	  ;; One of the next two variables (depending on search direction)
@@ -3093,13 +3123,13 @@ isearch-lazy-highlight-search
 				(+ isearch-lazy-highlight-start
 				   ;; Extend bound to match whole string at point
 				   (1- (length isearch-lazy-highlight-last-string)))
-			      (window-end)))
+			      (window*-end)))
 		     (max (or isearch-lazy-highlight-start-limit (point-min))
 			  (if isearch-lazy-highlight-wrapped
 			      (- isearch-lazy-highlight-end
 				 ;; Extend bound to match whole string at point
 				 (1- (length isearch-lazy-highlight-last-string)))
-			    (window-start))))))
+			    (window*-start))))))
 	;; Use a loop like in `isearch-search'.
 	(while retry
 	  (setq success (isearch-search-string
@@ -3115,6 +3145,24 @@ isearch-lazy-highlight-search
 	success)
     (error nil)))
 
+(defun isearch-lazy-highlight-put-overlays (mb me)
+  "Put a highlighting overlay on the buffer for each pertinent window.
+
+An overlay is put over the positions (MB ME) in each window in the group
+of windows (e.g. in Follow Mode) which (at least partially) contains them."
+  ;; non-zero-length match
+  (mapc (lambda (w)
+	  (if (and (< mb (window-end w))
+		   (> me (window-start w)))
+	      (let ((ov (make-overlay mb me)))
+		(push ov isearch-lazy-highlight-overlays)
+		;; 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 w))))
+	(window*-windows)))
+
 (defun isearch-lazy-highlight-update ()
   "Update highlighting of other matches for current search."
   (let ((max lazy-highlight-max-at-a-time)
@@ -3143,23 +3191,15 @@ isearch-lazy-highlight-update
 			  (if isearch-lazy-highlight-forward
 			      (if (= mb (if isearch-lazy-highlight-wrapped
 					    isearch-lazy-highlight-start
-					  (window-end)))
+					  (window*-end)))
 				  (setq found nil)
 				(forward-char 1))
 			    (if (= mb (if isearch-lazy-highlight-wrapped
 					  isearch-lazy-highlight-end
-					(window-start)))
+					(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+,
-			  ;; 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))))
+			(isearch-lazy-highlight-put-overlays mb me))
 		      ;; 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.
@@ -3175,12 +3215,12 @@ isearch-lazy-highlight-update
 		      (setq isearch-lazy-highlight-wrapped t)
 		      (if isearch-lazy-highlight-forward
 			  (progn
-			    (setq isearch-lazy-highlight-end (window-start))
+			    (setq isearch-lazy-highlight-end (window*-start))
 			    (goto-char (max (or isearch-lazy-highlight-start-limit (point-min))
-					    (window-start))))
-			(setq isearch-lazy-highlight-start (window-end))
+					    (window*-start))))
+			(setq isearch-lazy-highlight-start (window*-end))
 			(goto-char (min (or isearch-lazy-highlight-end-limit (point-max))
-					(window-end))))))))
+					(window*-end))))))))
 	    (unless nomore
 	      (setq isearch-lazy-highlight-timer
 		    (run-at-time lazy-highlight-interval nil
@@ -3198,6 +3238,7 @@ isearch-resume
   (setq isearch-string string
 	isearch-message message
 	isearch-case-fold-search case-fold)
+  (isearch-call-message nil t)
   (isearch-search)
   (isearch-update))
 


-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Eli Zaretskii]

> Date: Sun, 1 Nov 2015 13:52:53 +0000
> From: Alan Mackenzie <acm@muc.de>
> Cc: emacs-devel <emacs-devel@gnu.org>
> 
> No problem!  The patch below doesn't actually apply at the moment, due
> to some (?very) recent change in isearch.el.  But it wouldn't run
> anyway, because window*-start and friends aren't there yet.
> 
> 
> 
> diff --git a/lisp/isearch.el b/lisp/isearch.el
> index 4fc9b38..07ec534 100644
> --- a/lisp/isearch.el
> +++ b/lisp/isearch.el

How is this "not having isearch know about Follow mode"?  I see that
knowledge on every step of this patch, whenever you call the new
functions.

So maybe I'm missing something, but I really don't see why this
variant is significantly better than the one Stefan didn't like.  Can
you explain what am I missing here?

I thought you will come up with some more generic framework for
commands to "scroll" the display by switching to the next window (when
under Follow mode), if possible.  But unless I'm missing something
very important, this isn't that framework, is it?

Btw, I see no reason to introduce new functions.  Instead, we could
have the original ones accept an additional optional argument.

As yet another comment, wrt this exchange between you and John:

> > What is the reason for having separate functions such as window*-start,
> > instead of just taking the car of a list of windows?  I may be missing some
> > context here, but this sounds like special-casing general behavior, and I'm
> > wondering why it's necessary...
> [...]
> The fact that the "group" of windows is represented by a list is an
> implementation detail to be encapsulated within follow.el.  In the
> (fairly distant) future, this might perhaps be superseded by code in
> redisplay.  Perhaps.

You are right about not relying on the list, but window-next-sibling
and window-prev-sibling are available, and always will be, so you can
"trivially" use them instead of relying on a list.  If you agree, then
John's question still stands, I think.

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

[Alan Mackenzie]

Hello, Eli.

On Sun, Nov 01, 2015 at 06:50:00PM +0200, Eli Zaretskii wrote:
> > Date: Sun, 1 Nov 2015 13:52:53 +0000
> > From: Alan Mackenzie <acm@muc.de>
> > Cc: emacs-devel <emacs-devel@gnu.org>
> > 
> > No problem!  The patch below doesn't actually apply at the moment, due
> > to some (?very) recent change in isearch.el.  But it wouldn't run
> > anyway, because window*-start and friends aren't there yet.
> > 
> > 
> > 
> > diff --git a/lisp/isearch.el b/lisp/isearch.el
> > index 4fc9b38..07ec534 100644
> > --- a/lisp/isearch.el
> > +++ b/lisp/isearch.el

> How is this "not having isearch know about Follow mode"?  I see that
> knowledge on every step of this patch, whenever you call the new
> functions.

It has no knowledge of the internals of Follow Mode.  window*-start is
merely an abstract interface, currently with two implementations
associated with it, a default one for normal windows and a Follow Mode
one.  More implementations could be added, the Follow Mode one could be
taken away.

I'm not sure I really understand your question.  At several points in
isearch, functions need to be called which eventually end up calling
Follow Mode functions.

> So maybe I'm missing something, but I really don't see why this
> variant is significantly better than the one Stefan didn't like.  Can
> you explain what am I missing here?

The new patch doesn't insert things like this into isearch.el:

+             (if (and (boundp 'follow-mode) follow-mode)
+                (progn (follow-adjust-window (selected-window))
+                       t)

, though the old one from eighteen months ago did.  The new patch
doesn't use any FM internal interfaces, indeed doesn't do anything
directly with FM, beyond temporarily disabling it when a "small window"
comes into effect because of a "slow terminal".

> I thought you will come up with some more generic framework for
> commands to "scroll" the display by switching to the next window (when
> under Follow mode), if possible.  But unless I'm missing something
> very important, this isn't that framework, is it?

Follow Mode itself choses which window to leave point in.  For example,
if isearch started in the left hand window, FM will place point in the
RH window if the first match is there.

window*-start and friends do constitute a generic framework.  Any Elisp
library can replace window-start by window*-start, etc., and it then
operates on the entire group of FM windows rather than just one window.

> Btw, I see no reason to introduce new functions.  Instead, we could
> have the original ones accept an additional optional argument.

We could.  But that might make these functions less "pure": as well as
doing what they do, they would also have to execute some call-out to
Follow Mode in some fashion.  The Follow Mode hook would then tell
window-start which window it _really_ should return the start of.  Maybe
this would be better, maybe it would be more messy.

> As yet another comment, wrt this exchange between you and John:

> > > What is the reason for having separate functions such as window*-start,
> > > instead of just taking the car of a list of windows?  I may be missing some
> > > context here, but this sounds like special-casing general behavior, and I'm
> > > wondering why it's necessary...
> > [...]
> > The fact that the "group" of windows is represented by a list is an
> > implementation detail to be encapsulated within follow.el.  In the
> > (fairly distant) future, this might perhaps be superseded by code in
> > redisplay.  Perhaps.

> You are right about not relying on the list, but window-next-sibling
> and window-prev-sibling are available, and always will be, so you can
> "trivially" use them instead of relying on a list.

The next/previous sibling might be displaying a different buffer.  This
happens quite frequently when you look up a tag with M-..  But to use
window-next/previous-sibling properly would mean some fairly extensive
changes to the innards of Follow Mode - FM has its own left-to-right,
top-to-bottom algorithm for ordering its windows.  (I'm not defending
this, by the way - it's just the way things are at the moment.)

> If you agree, then John's question still stands, I think.

The "car of a list of windows" he was talking about means that list of
windows needs to come from somewhere.  The list is maintained by Follow
Mode, really only available using an internal FM function.  As soon as
we use this, we have coupled isearch with Follow Mode more tightly.
This is undesirable.

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Artur Malabarba]

>> How is this "not having isearch know about Follow mode"?  I see that
>> knowledge on every step of this patch, whenever you call the new
>> functions.
>
> It has no knowledge of the internals of Follow Mode.  window*-start is
> merely an abstract interface, currently with two implementations
> associated with it, a default one for normal windows and a Follow Mode
> one. [...]
> I'm not sure I really understand your question.  At several points in
> isearch, functions need to be called which eventually end up calling
> Follow Mode functions.

This is not about the window*- functions.
There are several places where your patch checks whether follow-mode
is on, turns it off, or turns it back on. I think this is still too
attached to follow mode.
Isearch really shouldn't worry about disabling or enabling other libraries.

The idiomatic Emacs way to do this is to add hooks to isearch, and run
them at useful places. Then packages like follow-mode can add stuff to
these hooks and do what they want (like, enable/disable itself).

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

[Alan Mackenzie]

Hello, Artur.

On Sun, Nov 01, 2015 at 07:46:12PM +0000, Artur Malabarba wrote:
> >> How is this "not having isearch know about Follow mode"?  I see that
> >> knowledge on every step of this patch, whenever you call the new
> >> functions.
> >
> > It has no knowledge of the internals of Follow Mode.  window*-start is
> > merely an abstract interface, currently with two implementations
> > associated with it, a default one for normal windows and a Follow Mode
> > one. [...]
> > I'm not sure I really understand your question.  At several points in
> > isearch, functions need to be called which eventually end up calling
> > Follow Mode functions.

> This is not about the window*- functions.

> There are several places where your patch checks whether follow-mode
> is on, turns it off, or turns it back on. I think this is still too
> attached to follow mode.
> Isearch really shouldn't worry about disabling or enabling other libraries.

Have a look more closely at that point.  It is where an old (possibly
obsolete) facility of the "small window" is used.  The idea is that if
a terminal's comms line is very slow, isearch will just display a single
line of text instead of scrolling an entire large window.

However, if Follow Mode is enabled, it would completely negate the point
of the "small window" since it would scroll all the other windows
displaying the buffer anyway.

Now, it's highly unlikely anybody on a 1200 Baud terminal is going to be
using Follow Mode anyway, but if they are, they'll still get the benefit
of the "small window".

Set your `baud-rate' to 100, and try this facility out!

> The idiomatic Emacs way to do this is to add hooks to isearch, and run
> them at useful places. Then packages like follow-mode can add stuff to
> these hooks and do what they want (like, enable/disable itself).

But the "small window" facility is hard coded, and probably has been for
30 years.  Just up until now, nobody even considered the interaction
between "small window" and Follow Mode.

Still, this part of my patch is hardly essential to the main thrust.  So
if people really think it inappropriate, I can just take this bit out.

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Artur Malabarba]

2015-11-01 19:46 GMT+00:00 Artur Malabarba <bruce.connor.am@gmail.com>:
> The idiomatic Emacs way to do this is to add hooks to isearch, and run
> them at useful places. Then packages like follow-mode can add stuff to
> these hooks and do what they want (like, enable/disable itself).

After playing around a bit, here's a quick solution that works for me.
The patch adds 4 lines to isearch.el, and defines a function in
follow-mode.

I only tried it supperficially, so you might find issues with this
solution that I couldn't

---
 lisp/follow.el  | 14 ++++++++++++++
 lisp/isearch.el |  4 ++++
 2 files changed, 18 insertions(+)

diff --git a/lisp/follow.el b/lisp/follow.el
index 938c59e..e9a60e5 100644
--- a/lisp/follow.el
+++ b/lisp/follow.el
@@ -203,6 +203,8 @@

 (require 'easymenu)
 (eval-when-compile (require 'cl-lib))
+(require 'seq)
+(require 'subr-x)

 ;;; Variables

@@ -419,11 +421,13 @@ follow-mode
   :keymap follow-mode-map
   (if follow-mode
       (progn
+        (setq-local isearch-search-fun-function #'follow--search-function)
     (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))
     ;; Remove globally-installed hook functions only if there is no
     ;; other Follow mode buffer.
+    (setq-local isearch-search-fun-function #'isearch-search-fun-default)
     (let ((buffers (buffer-list))
       following)
       (while (and (not following) buffers)
@@ -434,6 +438,16 @@ follow-mode
     (remove-hook 'window-size-change-functions 'follow-window-size-change)))
     (remove-hook 'compilation-filter-hook
'follow-align-compilation-windows t)))

+(defvar isearch-lazy-highlight-ongoing-search)
+(defun follow--search-function ()
+  (lambda (&rest args)
+    (prog1 (apply (isearch-search-fun-default) args)
+      (unless (or isearch-lazy-highlight-ongoing-search
+                  (pos-visible-in-window-p))
+        (when-let ((win (seq-find (lambda (w)
(pos-visible-in-window-p (point) w))
+                                (get-buffer-window-list))))
+          (select-window win))))))
+
 (defun follow-find-file-hook ()
   "Find-file hook for Follow mode.  See the variable `follow-auto'."
   (if follow-auto (follow-mode 1)))
diff --git a/lisp/isearch.el b/lisp/isearch.el
index b762884..daecdb5 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -3075,11 +3075,15 @@ 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
+  "Bound to t while doing 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.5.3

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

[Eli Zaretskii]

> Date: Sun, 1 Nov 2015 18:27:33 +0000
> Cc: bruce.connor.am@gmail.com, emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > How is this "not having isearch know about Follow mode"?  I see that
> > knowledge on every step of this patch, whenever you call the new
> > functions.
> 
> It has no knowledge of the internals of Follow Mode.

But the window*-* functions do.  And any code that calls them _knows_
about Follow mode.

Maybe it's just me, but I see no real difference in having the Follow
mode explicitly called out in a separate function, instead in
isearch.el itself.

> The new patch doesn't insert things like this into isearch.el:
> 
> +             (if (and (boundp 'follow-mode) follow-mode)
> +                (progn (follow-adjust-window (selected-window))
> +                       t)

It inserts them indirectly via calling special functions.  Which means
a package cannot really be oblivious to Follow mode's existence.

> > I thought you will come up with some more generic framework for
> > commands to "scroll" the display by switching to the next window (when
> > under Follow mode), if possible.  But unless I'm missing something
> > very important, this isn't that framework, is it?
> 
> Follow Mode itself choses which window to leave point in.  For example,
> if isearch started in the left hand window, FM will place point in the
> RH window if the first match is there.

That's not what I see.  What I see is that starting Isearch in the
left window scrolls that window, and then the right window scrolls
accordingly.  What I'd expect (if I were a user of Follow mode) us an
automatic switch to the right window, and scroll only when that window
is also exhausted.  That doesn't happen, at least not with the current
master.

In any case, what I thought you'd do is introduce a bunch of
variables, such as window-selection-for-scroll-function, that will
allow Follow mode to override the default (trivial) value and dictate
its own decisions to its clients.  Or something like that.

> window*-start and friends do constitute a generic framework.

No, a generic framework would be, for example, to have a variable
whose value is a function, and let Follow mode override the default
value.

> > Btw, I see no reason to introduce new functions.  Instead, we could
> > have the original ones accept an additional optional argument.
> 
> We could.  But that might make these functions less "pure": as well as
> doing what they do, they would also have to execute some call-out to
> Follow Mode in some fashion.

Isn't that what window*-* functions do anyway?  Where's the benefit?

> > You are right about not relying on the list, but window-next-sibling
> > and window-prev-sibling are available, and always will be, so you can
> > "trivially" use them instead of relying on a list.
> 
> The next/previous sibling might be displaying a different buffer.

But that's very easy to test, no?

> to use window-next/previous-sibling properly would mean some fairly
> extensive changes to the innards of Follow Mode - FM has its own
> left-to-right, top-to-bottom algorithm for ordering its windows.

What algorithm could that be, if not something that traverses the
window tree in some order?

> > If you agree, then John's question still stands, I think.
> 
> The "car of a list of windows" he was talking about means that list of
> windows needs to come from somewhere.  The list is maintained by Follow
> Mode, really only available using an internal FM function.  As soon as
> we use this, we have coupled isearch with Follow Mode more tightly.
> This is undesirable.

Not if you go through a variable whose default value Follow mode can
override.  Or some other decoupling technique like that, I'm sure
there are more, perhaps more elegant ones.

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

[Artur Malabarba]

2015-11-01 20:15 GMT+00:00 Alan Mackenzie <acm@muc.de>:
>> There are several places where your patch checks whether follow-mode
>> is on, turns it off, or turns it back on. [...] >
> Have a look more closely at that point.  It is where an old (possibly
> obsolete) facility of the "small window" is used. [...] > Set your `baud-rate' to 100, and try this facility out

You learn something every day, I had no idea this facility existed. I
tried it and it's pretty neat. :-) Still...

> [...] > Still, this part of my patch is hardly essential to the main thrust.  So
> if people really think it inappropriate, I can just take this bit out.

Yes, I think that's best. It's quite a bit of complexity to add just
to slightly improve the interplay with follow-mode on very old
terminals.

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

[Alan Mackenzie]

Hello, Eli.

On Sun, Nov 01, 2015 at 10:54:21PM +0200, Eli Zaretskii wrote:
> > Date: Sun, 1 Nov 2015 18:27:33 +0000
> > Cc: bruce.connor.am@gmail.com, emacs-devel@gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > > How is this "not having isearch know about Follow mode"?  I see that
> > > knowledge on every step of this patch, whenever you call the new
> > > functions.
> > 
> > It has no knowledge of the internals of Follow Mode.

> > Follow Mode itself choses which window to leave point in.  For example,
> > if isearch started in the left hand window, FM will place point in the
> > RH window if the first match is there.

> That's not what I see.  What I see is that starting Isearch in the
> left window scrolls that window, and then the right window scrolls
> accordingly.

That is one of the bugs that my patch to isearch.el solves.  But I think
Juri has knocked that one down already.  It is caused by a glitch in
lazy highlighting.  Disable lazy highlighting, and try it again.  Then
C-s will move onto the next window rather than scrolling the one it
started on.

> In any case, what I thought you'd do is introduce a bunch of
> variables, such as window-selection-for-scroll-function, that will
> allow Follow mode to override the default (trivial) value and dictate
> its own decisions to its clients.  Or something like that.

> > window*-start and friends do constitute a generic framework.

> No, a generic framework would be, for example, to have a variable
> whose value is a function, and let Follow mode override the default
> value.

That's precisely what I've done (but haven't submitted for review, yet).
In window.el, I've got code like this:

  (defvar window*-start-function #'window-start)
  (make-variable-buffer-local 'window*-start-function)
  (put 'window*-start-function 'permanent-local t)
  (defun window*-start (&optional window)
    "Return position at which display currently starts in the group of
  windows containing WINDOW.  When a grouping mode (such as Follow Mode)
  is not active, this function is identical to `window-start'.

  WINDOW must be a live window and defaults to the selected one.
  This is updated by redisplay or by calling `set-window*-start'."
    (funcall window*-start-function window))

, and `follow-mode' sets window*-start-function to its own value when FM
is started, and kills the buffer local value when FM is disabled.

I think there's been some miscommunication over our last few posts.
Sorry about that.

> > > Btw, I see no reason to introduce new functions.  Instead, we could
> > > have the original ones accept an additional optional argument.

> > We could.  But that might make these functions less "pure": as well as
> > doing what they do, they would also have to execute some call-out to
> > Follow Mode in some fashion.

> Isn't that what window*-* functions do anyway?  Where's the benefit?

Maybe there isn't really any.  I don't feel too strongly about it.  But
what would the optional extra parameter to window-start be called?
"group"?

It would mean the facility having to be coded in C rather than Lisp.  I
suppose that's not too difficult.  It would save having to have the
rather ugly names with "window*" in them.

> > > You are right about not relying on the list, but window-next-sibling
> > > and window-prev-sibling are available, and always will be, so you can
> > > "trivially" use them instead of relying on a list.

> > The next/previous sibling might be displaying a different buffer.

> But that's very easy to test, no?

Yes, but it would be duplicating what Follow Mode has already done in
one of its hooks (post-command-hook, I think).  FM caters for any
arrangement of windows in a frame (not only side by side, but over
eachother, or a mixture) just like Emacs does.  Maybe they have the same
notion of order of windows, maybe they don't.  In the general case
(which surely doesn't happen in practice much), we'd have to scan the
frame's window tree more carefully.

> > to use window-next/previous-sibling properly would mean some fairly
> > extensive changes to the innards of Follow Mode - FM has its own
> > left-to-right, top-to-bottom algorithm for ordering its windows.

> What algorithm could that be, if not something that traverses the
> window tree in some order?

IIRC, FM takes the coordinates of the top left corner of each pertinent
window, and sorts them top to bottom first, then left to right for any
windows with the same Y coordinate.  Or something like that.

> > > If you agree, then John's question still stands, I think.

> > The "car of a list of windows" he was talking about means that list of
> > windows needs to come from somewhere.  The list is maintained by Follow
> > Mode, really only available using an internal FM function.  As soon as
> > we use this, we have coupled isearch with Follow Mode more tightly.
> > This is undesirable.

> Not if you go through a variable whose default value Follow mode can
> override.  Or some other decoupling technique like that, I'm sure
> there are more, perhaps more elegant ones.

I suppose so.  But why should isearch have to know that it is the car of
the list which is the "first" window (for window*-start)?

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Juri Linkov]

>> >>From another perspective, settings lazy-highlight-buffer to t
>> (implemented in bug#21092) and removing the current restriction of
>> (overlay-put ov 'window (selected-window)) will lazy-highlight matches
>> in all follow windows with no effort.
>
> I wasn't actually aware of that fix.
>
> There were three main problems my patch fixed:
> 1) Searching commands were restricted to a single follow window.  This
> was caused by the lazy highlighting mechanism, as you say.
> 2) Lazy highlighting was only being done in a single window.
> 3) In scrolling commands, point was restricted to the singled window,
> rather than being able to move freely throughout all the windows.

IIUC, lazy highlighting is not a problem anymore, so the remaining
problem is how to switch windows when the next search position
happens to be in an adjacent window.  Since there is no other
core library that require so much changes to adapt to follow-mode,
and there are already some mode-specific hooks in follow.el like
compilation-filter-hook, indicates that it's the responsibility of follow.el
to support isearch in follow-mode.  So I believe the right way to do
this is like Artur presented in a short patch, and maybe it's possible
to simplify it even more by using isearch-update-post-hook with
a follow-align-compilation-windows like hook.  I mean something like
(add-hook 'isearch-update-post-hook follow-align-isearch-windows t t)

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Mon, 02 Nov 2015 02:14:47 +0200
> Cc: 17453@debbugs.gnu.org, Stefan Monnier <monnier@iro.umontreal.ca>,
> 	emacs-devel@gnu.org
> 
> Since there is no other core library that require so much changes to
> adapt to follow-mode, and there are already some mode-specific hooks
> in follow.el like compilation-filter-hook, indicates that it's the
> responsibility of follow.el to support isearch in follow-mode.  So I
> believe the right way to do this is like Artur presented in a short
> patch, and maybe it's possible to simplify it even more by using
> isearch-update-post-hook with a follow-align-compilation-windows
> like hook.  I mean something like (add-hook
> 'isearch-update-post-hook follow-align-isearch-windows t t)

I agree.

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

[Alan Mackenzie]

Hi, Juri.

On Mon, Nov 02, 2015 at 02:14:47AM +0200, Juri Linkov wrote:
> >> >>From another perspective, settings lazy-highlight-buffer to t
> >> (implemented in bug#21092) and removing the current restriction of
> >> (overlay-put ov 'window (selected-window)) will lazy-highlight matches
> >> in all follow windows with no effort.

> > I wasn't actually aware of that fix.

> > There were three main problems my patch fixed:
> > 1) Searching commands were restricted to a single follow window.  This
> > was caused by the lazy highlighting mechanism, as you say.
> > 2) Lazy highlighting was only being done in a single window.
> > 3) In scrolling commands, point was restricted to the singled window,
> > rather than being able to move freely throughout all the windows.

> IIUC, lazy highlighting is not a problem anymore, ....

It is.  With Follow Mode enabled, lazy highlighting is only occurring in
the one window, not all of them.

> .... so the remaining problem is how to switch windows when the next
> search position happens to be in an adjacent window.

That problem only shows itself when lazy-highlighting is enabled.  This
suggests that the solution is to fix the lazy-highliting code so as not
to mess up the window arrangement.

The other problem which still exists is that when isearch-allow-scroll
is enabled, point cannot be scrolled into a neighbouring Follow Mode
window.

> Since there is no other core library that require so much changes to
> adapt to follow-mode, ....

I don't think we know this.

> .... and there are already some mode-specific hooks in follow.el like
> compilation-filter-hook, indicates that it's the responsibility of
> follow.el to support isearch in follow-mode.

There is nothing Follow Mode can do about isearch restricting its
lazy-highlighting and bounds for scrolling to one single window.
Isearch itself must be adapted for Follow Mode to work with it.

> So I believe the right way to do this is like Artur presented in a
> short patch, ....

I don't actually understand that patch, I'll need to study the wierd/new
constructs used in it, like `when-let'.

However, I'm certain that that patch will not fix all the problems
discussed in this post.  One way or another, isearch MUST work with the
window boundaries of the entire Follow Mode group.

> .... and maybe it's possible to simplify it even more by using
> isearch-update-post-hook with a follow-align-compilation-windows like
> hook.  I mean something like (add-hook 'isearch-update-post-hook
> follow-align-isearch-windows t t)

Hmm.  I see that more as complicatifying rather than simplifying.  The
imperative, set out by Stefan 18 months ago, was to keep internal Follow
Mode stuff out of isearch.

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Artur Malabarba]

> I don't actually understand that patch, I'll need to study the wierd/new
> constructs used in it, like `when-let'.

Here's a more thoroughly explained version of this function, that
doesn't use when-let.

seq-find is equivalent to cl-find-if, it returns the first element
that matches the provided predicate.

(defun follow--search-function ()
  (lambda (&rest args)
    (let ((search-function (isearch-search-fun-default))
          (matched (apply search-function args)))
      ;; 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)))
                             (follow-all-followers))))
          ;; If so, select it.
          (when win
            (select-window win))))
      matched)))

I also changed it to use (follow-all-followers).

> However, I'm certain that that patch will not fix all the problems
> discussed in this post.

The patch as provided doesn't fix the “highlighting matches on all
windows” issue. But that's trivial to solve by removing that
`(overlay-put ov 'window (selected-window))' line. (which I have half
a mind to do right now because I just think it's a generally useful
improvement.)

>  One way or another, isearch MUST work with the
> window boundaries of the entire Follow Mode group.

Maybe I missed part of the issue. I thought you wanted Isearch to
switch to another window if that window contains the next match
(instead of scrolling the current window). For that, you only need
pos-visible-in-window-p, you don't need to mess with boundaries.

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

[Artur Malabarba]

>>  One way or another, isearch MUST work with the
>> window boundaries of the entire Follow Mode group.
>
> Maybe I missed part of the issue. I thought you wanted Isearch to
> switch to another window if that window contains the next match
> (instead of scrolling the current window). For that, you only need
> pos-visible-in-window-p, you don't need to mess with boundaries.

I think I see what I'm missing. Lazy-highlighting stops at the end of
the window, so the matches on the second window won't be highlighted
until we switch to it, right?

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

[Alan Mackenzie]

Hello, Artur.

On Mon, Nov 02, 2015 at 11:53:10AM +0000, Artur Malabarba wrote:
> > I don't actually understand that patch, I'll need to study the wierd/new
> > constructs used in it, like `when-let'.

> Here's a more thoroughly explained version of this function, that
> doesn't use when-let.

> seq-find is equivalent to cl-find-if, it returns the first element
> that matches the provided predicate.

OK.

[ .... ]

> I also changed it to use (follow-all-followers).

That's unwanted.  ;-)  Isearch should not be using internal Follow Mode
functions.  Some more abstraction would be needed.

> > However, I'm certain that that patch will not fix all the problems
> > discussed in this post.

> The patch as provided doesn't fix the “highlighting matches on all
> windows” issue. But that's trivial to solve by removing that
> `(overlay-put ov 'window (selected-window))' line. (which I have half
> a mind to do right now because I just think it's a generally useful
> improvement.)

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

> >  One way or another, isearch MUST work with the
> > window boundaries of the entire Follow Mode group.

> Maybe I missed part of the issue. I thought you wanted Isearch to
> switch to another window if that window contains the next match
> (instead of scrolling the current window). For that, you only need
> pos-visible-in-window-p, you don't need to mess with boundaries.

What is causing the unwanted scrolling rather than moving to the next
window, is the form "(sit-for 0)" near the start of
isearch-lazy-highlight-new-loop.  When point is outside the window, this
form causes redisplay, which scrolls point back into the window -
without Follow Mode getting a look in.  In my patch, I replaced this
with "(sit*-for 0)", where Follow Mode can do its thing before the
redisplay happens.

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Alan Mackenzie]

Hello, Artur.

On Mon, Nov 02, 2015 at 12:14:12PM +0000, Artur Malabarba wrote:
> >>  One way or another, isearch MUST work with the
> >> window boundaries of the entire Follow Mode group.

> > Maybe I missed part of the issue. I thought you wanted Isearch to
> > switch to another window if that window contains the next match
> > (instead of scrolling the current window). For that, you only need
> > pos-visible-in-window-p, you don't need to mess with boundaries.

> I think I see what I'm missing. Lazy-highlighting stops at the end of
> the window, so the matches on the second window won't be highlighted
> until we switch to it, right?

Absolutely!  Not forgetting, of course, matches which start near the
bottom of one window, and carry on at the top of the next window.

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Artur Malabarba]

>> I also changed it to use (follow-all-followers).
>
> That's unwanted.  ;-)  Isearch should not be using internal Follow Mode
> functions.  Some more abstraction would be needed.

That function goes in follow.el. ;-)

> 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.
I see nothing wrong with the window*-start/end functionality you
suggest, but if we can solve this problem via configuration points
that are already provided (like isearch-search-fun-function) then all
the better.

>> >  One way or another, isearch MUST work with the
>> > window boundaries of the entire Follow Mode group.
>
>> Maybe I missed part of the issue. I thought you wanted Isearch to
>> switch to another window if that window contains the next match
>> (instead of scrolling the current window). For that, you only need
>> pos-visible-in-window-p, you don't need to mess with boundaries.
>
> What is causing the unwanted scrolling rather than moving to the next
> window, is the form "(sit-for 0)" near the start of
> isearch-lazy-highlight-new-loop.

I think the patch I provided switches to the second (or third or
whatever) window before that sit-for runs. So I believe it solves this
problem.

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

[Artur Malabarba]

[-- Attachment #1: Type: text/plain, Size: 828 bytes --]

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

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

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

[-- Attachment #2: 0003-lisp-follow.el-Improve-isearch-compatibility.patch --]
[-- Type: text/x-patch, Size: 2951 bytes --]

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


[-- Attachment #3: 0002-lisp-isearch.el-Bind-a-variable-while-lazy-highlight.patch --]
[-- Type: text/x-patch, Size: 1458 bytes --]

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


[-- Attachment #4: 0001-lisp-isearch.el-Allow-lazy-highlighting-of-all-windo.patch --]
[-- Type: text/x-patch, Size: 1763 bytes --]

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

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

[Alan Mackenzie]

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

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

[Juri Linkov]

> 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?

Right, without (sit-for 0) it's possible to switch focus to adjacent windows
with just adding 2 lines to follow.el, i.e. I get the desired behavior with:

diff --git a/lisp/follow.el b/lisp/follow.el
index 938c59e..0433854 100644
--- a/lisp/follow.el
+++ b/lisp/follow.el
@@ -420,6 +420,7 @@ (define-minor-mode follow-mode
   (if follow-mode
       (progn
 	(add-hook 'compilation-filter-hook 'follow-align-compilation-windows t t)
+	(add-hook 'isearch-update-post-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))
     ;; Remove globally-installed hook functions only if there is no
@@ -432,6 +433,7 @@ (define-minor-mode follow-mode
       (unless following
 	(remove-hook 'post-command-hook 'follow-post-command-hook)
 	(remove-hook 'window-size-change-functions 'follow-window-size-change)))
+    (remove-hook 'isearch-update-post-hook 'follow-align-compilation-windows t)
     (remove-hook 'compilation-filter-hook 'follow-align-compilation-windows t)))
 
 (defun follow-find-file-hook ()
diff --git a/lisp/isearch.el b/lisp/isearch.el
index b762884..8edf8b0 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -3018,7 +3018,7 @@ (defun isearch-lazy-highlight-new-loop (&optional beg end)
 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
+             ; (sit-for 0)         ;make sure (window-start) is credible
              (or (not (equal isearch-string
                              isearch-lazy-highlight-last-string))
                  (not (eq (selected-window)


So what remains to do is to fix this bug, but I don't understand the logic
you proposed: how checks could be moved to isearch-lazy-highlight-update
if isearch-lazy-highlight-update is scheduled by a timer conditionally
depending on these checks?

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

[Juri Linkov]

> +(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

Please note that this is unnecessary because all functions that override
isearch-search-fun-function use its ‘bounds’ arg to detect whether it's
called from isearch-search or isearch-lazy-highlight-search with the value
either nil or non-nil correspondingly.

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

This is a good option that follow-mode could set (or users to customize).

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

[Juri Linkov]

>> IIUC, lazy highlighting is not a problem anymore, ....
>
> It is.  With Follow Mode enabled, lazy highlighting is only occurring in
> the one window, not all of them.

Perhaps you missed my patch in the previous message
that will highlight matches in all windows after Artur will add
a new option ‘all-windows’ to ‘isearch-lazy-highlight’ to lift
the selected-window-only restriction from lazy overlays.

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

[Alan Mackenzie]

Hello, Juri.

On Tue, Nov 03, 2015 at 01:22:03AM +0200, Juri Linkov wrote:
> > 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?

> Right, without (sit-for 0) it's possible to switch focus to adjacent windows
> with just adding 2 lines to follow.el, i.e. I get the desired behavior with:

> diff --git a/lisp/follow.el b/lisp/follow.el
> index 938c59e..0433854 100644
> --- a/lisp/follow.el
> +++ b/lisp/follow.el
> @@ -420,6 +420,7 @@ (define-minor-mode follow-mode
>    (if follow-mode
>        (progn
>  	(add-hook 'compilation-filter-hook 'follow-align-compilation-windows t t)
> +	(add-hook 'isearch-update-post-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))
>      ;; Remove globally-installed hook functions only if there is no
> @@ -432,6 +433,7 @@ (define-minor-mode follow-mode
>        (unless following
>  	(remove-hook 'post-command-hook 'follow-post-command-hook)
>  	(remove-hook 'window-size-change-functions 'follow-window-size-change)))
> +    (remove-hook 'isearch-update-post-hook 'follow-align-compilation-windows t)
>      (remove-hook 'compilation-filter-hook 'follow-align-compilation-windows t)))
 
>  (defun follow-find-file-hook ()

This is complicated.  Ideally, the Follow Mode windows should be
synchronised in FM's post-command-hook, not isearch's.  It is not
isearch's job to realign windows.  follow-post-command-hook both realigns
windows and choses an appropriate window to put point in.  We should let
it.

> diff --git a/lisp/isearch.el b/lisp/isearch.el
> index b762884..8edf8b0 100644
> --- a/lisp/isearch.el
> +++ b/lisp/isearch.el
> @@ -3018,7 +3018,7 @@ (defun isearch-lazy-highlight-new-loop (&optional beg end)
>  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
> +             ; (sit-for 0)         ;make sure (window-start) is credible
>               (or (not (equal isearch-string
>                               isearch-lazy-highlight-last-string))
>                   (not (eq (selected-window)


> So what remains to do is to fix this bug, but I don't understand the logic
> you proposed: how checks could be moved to isearch-lazy-highlight-update
> if isearch-lazy-highlight-update is scheduled by a timer conditionally
> depending on these checks?

What I'm proposing is to schedule the timer always, and do the checks
(for whether we need to start a new lazy highlight loop) in the function
that the timer triggers.  The advantage is that when the timer triggers,
redisplay will already have taken place[*], and thus we can use
`window-start' and `window-end' without forcing an artificial redisplay
with "(sit-for 0)".

[*] and an appropriate FM window will have been selected by FM's
post-command-hook

The disadvantage is that stale lazy highlights are erased after
isearch-lazy-highlight-initial-delay, not immediately, as at present.
I'm not sure if this is really a disadvantage or not.  In my scrappy
patch last night, I attempted to ameliorate this "problem" by repeating
(most of) the tests in the command-loop bit.

-- 
Alan Mackenzie (Nuremberg, Germany).