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

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

[Alan Mackenzie]

Hi, Emacs.

Isearch doesn't work properly with Follow Mode.  There follows a patch
which is a first attempt to fix this.

There are three main problems (there may be others):

1. Isearch commands stay in the same window when they ought to move
across the entire screen.  This is only actually a problem with lazy
highlighting enabled.

2. Lazy highlighting is only applied to the current follow window, not
all of them.

3. Scroll commands cannot scroll out of the current window.  They ought
to be able to scroll through any follow window.

Most of the exercise was straightforward.  The only trouble came where
isearch-message (or isearch-message-function) was called from
isearch-search.  There are circumstances (see comments in the code)
where calling isearch-message with follow-mode enabled can cause
unwanted vertical scrolling.  This happens when point is temporarily
moved to isearch-other-end and then calling isearch-search.

To get round this I refactored isearch-message out of isearch-search and
inserted into the places just before isearch-search is called.

Here's the patch:




--- emacs/emacs.bzr/trunk/lisp/isearch.el	2014-03-03 11:50:56.000000000 +0000
+++ /home/acm/isearch.el	2014-05-09 22:10:29.000000000 +0000
@@ -166,6 +166,21 @@
   :type 'boolean
   :group 'isearch)
 
+(defvar isearch-windows nil
+  "List of windows active in the incremental search.
+This is either the ordered list of active windows administered by
+`follow-mode' or a list of the single window involved in the
+search.")
+
+(defmacro isearch-windows-start (&optional wins)
+  "Get the start point of the windows involved in the isearch."
+  `(window-start (car ,(if wins wins 'isearch-windows))))
+
+(defmacro isearch-windows-end (&optional wins update)
+  "Get the end point of the windows involved in the isearch."
+  `(window-end (car (last ,(if wins wins 'isearch-windows)))
+	       ,@(if update `(,update))))
+
 (defvar isearch-mode-hook nil
   "Function(s) to call after starting up an incremental search.")
 
@@ -184,6 +199,11 @@
   "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,
@@ -203,7 +223,7 @@
 `isearch-message-prefix' advice property to specify the prefix string
 displayed in the search message.")
 
-;; Search ring.
+;;; Search ring.
 
 (defvar search-ring nil
   "List of search string sequences.")
@@ -860,6 +880,9 @@
 	isearch-regexp regexp
 	isearch-word word
 	isearch-op-fun op-fun
+	isearch-windows (if (and (boundp 'follow-mode) follow-mode)
+			    (follow-all-followers)
+			  (list (selected-window)))
 	isearch-last-case-fold-search isearch-case-fold-search
 	isearch-case-fold-search case-fold-search
 	isearch-invisible search-invisible
@@ -1254,13 +1277,6 @@
 	  (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
@@ -1273,7 +1289,17 @@
 		  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))
@@ -1417,16 +1443,20 @@
 	    (isearch-ring-adjust1 nil))
 	;; If already have what to search for, repeat it.
 	(or isearch-success
-	    (progn
-	      ;; Set isearch-wrapped before calling isearch-wrap-function
-	      (setq isearch-wrapped t)
-	      (if isearch-wrap-function
-		  (funcall isearch-wrap-function)
-	        (goto-char (if isearch-forward (point-min) (point-max)))))))
+	    ;; Set isearch-wrapped before calling isearch-wrap-function
+	    (setq isearch-wrapped t)))
     ;; C-s in reverse or C-r in forward, change direction.
     (setq isearch-forward (not isearch-forward)
 	  isearch-success t))
 
+  (isearch-call-message nil t)		; Call before moving point.
+  (if (and (eq isearch-forward (eq direction 'forward))
+	   (not (equal isearch-string ""))
+	   (not isearch-success))
+      (if isearch-wrap-function
+	  (funcall isearch-wrap-function)
+	(goto-char (if isearch-forward (point-min) (point-max)))))
+
   (setq isearch-barrier (point)) ; For subsequent \| if regexp.
 
   (if (equal isearch-string "")
@@ -1867,6 +1897,7 @@
 					    (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,
@@ -2041,6 +2072,7 @@
 	      (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
@@ -2207,10 +2239,12 @@
 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 (isearch-windows-start))
+	(w-end (isearch-windows-end nil t))
+        (w-L1 (with-selected-window (car isearch-windows)
+		(save-excursion (move-to-window-line 1) (point))))
+        (w-L-1 (with-selected-window (car (last isearch-windows))
+		 (save-excursion (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))
@@ -2236,14 +2270,18 @@
       (setq start isearch-point  end (or isearch-other-end isearch-point)))
     (if above
         (progn
-          (goto-char start)
+          (select-window (car isearch-windows))
+	  (goto-char start)
           (recenter 0)
-          (when (>= isearch-point (window-end nil t))
-            (goto-char isearch-point)
+          (when (>= isearch-point (isearch-windows-end nil t))
+            (select-window (car (last isearch-windows)))
+	    (goto-char isearch-point)
             (recenter -1)))
+      (select-window (car (last isearch-windows)))
       (goto-char end)
       (recenter -1)
-      (when (< isearch-point (window-start))
+      (when (< isearch-point (isearch-windows-start))
+	(select-window (car isearch-windows))
         (goto-char isearch-point)
         (recenter 0))))
   (goto-char isearch-point))
@@ -2390,6 +2428,7 @@
   (isearch-ring-adjust1 advance)
   (if search-ring-update
       (progn
+	(isearch-call-message nil t)
 	(isearch-search)
 	(isearch-push-state)
 	(isearch-update))
@@ -2462,6 +2501,13 @@
 
 (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)))
@@ -2630,9 +2676,6 @@
 
 (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)))
@@ -2936,8 +2979,9 @@
 (defvar isearch-lazy-highlight-timer nil)
 (defvar isearch-lazy-highlight-last-string nil)
 (defvar isearch-lazy-highlight-window nil)
-(defvar isearch-lazy-highlight-window-start nil)
-(defvar isearch-lazy-highlight-window-end nil)
+(defvar isearch-lazy-highlight-windows nil)
+(defvar isearch-lazy-highlight-windows-start 0)
+(defvar isearch-lazy-highlight-windows-end 0)
 (defvar isearch-lazy-highlight-case-fold-search nil)
 (defvar isearch-lazy-highlight-regexp nil)
 (defvar isearch-lazy-highlight-lax-whitespace nil)
@@ -2972,11 +3016,15 @@
 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
+             (if (and (boundp 'follow-mode) follow-mode)
+		 (progn (follow-adjust-window (selected-window))
+			t)
+	       (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)
+                          isearch-lazy-highlight-windows))
 		 (not (eq isearch-lazy-highlight-case-fold-search
 			  isearch-case-fold-search))
 		 (not (eq isearch-lazy-highlight-regexp
@@ -2987,10 +3035,10 @@
 			  isearch-lax-whitespace))
 		 (not (eq isearch-lazy-highlight-regexp-lax-whitespace
 			  isearch-regexp-lax-whitespace))
-                 (not (= (window-start)
-                         isearch-lazy-highlight-window-start))
-                 (not (= (window-end)   ; Window may have been split/joined.
-			 isearch-lazy-highlight-window-end))
+                 (not (= (isearch-windows-start isearch-lazy-highlight-windows)
+                         isearch-lazy-highlight-windows-start))
+                 (not (= (isearch-windows-end isearch-lazy-highlight-windows)   ; Window may have been split/joined.
+			 isearch-lazy-highlight-windows-end))
 		 (not (eq isearch-forward
 			  isearch-lazy-highlight-forward))
 		 ;; In case we are recovering from an error.
@@ -3005,8 +3053,14 @@
     (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-windows
+	  (if (and (boundp 'follow-mode) follow-mode)
+	      (follow-all-followers)
+	    (list (selected-window)))
+	  isearch-lazy-highlight-windows-start
+	  (isearch-windows-start isearch-lazy-highlight-windows)
+	  isearch-lazy-highlight-windows-end
+	  (isearch-windows-end isearch-lazy-highlight-windows)
 	  ;; 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)
@@ -3048,11 +3102,11 @@
 		       (min (or isearch-lazy-highlight-end-limit (point-max))
 			    (if isearch-lazy-highlight-wrapped
 				isearch-lazy-highlight-start
-			      (window-end)))
+			      isearch-lazy-highlight-windows-end))
 		     (max (or isearch-lazy-highlight-start-limit (point-min))
 			  (if isearch-lazy-highlight-wrapped
 			      isearch-lazy-highlight-end
-			    (window-start))))))
+			    isearch-lazy-highlight-windows-start)))))
 	;; Use a loop like in `isearch-search'.
 	(while retry
 	  (setq success (isearch-search-string
@@ -3068,6 +3122,24 @@
 	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
+`isearch-lazy-highlight-windows' 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))))
+	isearch-lazy-highlight-windows))
+
 (defun isearch-lazy-highlight-update ()
   "Update highlighting of other matches for current search."
   (let ((max lazy-highlight-max-at-a-time)
@@ -3096,23 +3168,15 @@
 			  (if isearch-lazy-highlight-forward
 			      (if (= mb (if isearch-lazy-highlight-wrapped
 					    isearch-lazy-highlight-start
-					  (window-end)))
+					  isearch-lazy-highlight-windows-end))
 				  (setq found nil)
 				(forward-char 1))
 			    (if (= mb (if isearch-lazy-highlight-wrapped
 					  isearch-lazy-highlight-end
-					(window-start)))
+					isearch-lazy-highlight-windows-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.
@@ -3128,12 +3192,12 @@
 		      (setq isearch-lazy-highlight-wrapped t)
 		      (if isearch-lazy-highlight-forward
 			  (progn
-			    (setq isearch-lazy-highlight-end (window-start))
+			    (setq isearch-lazy-highlight-end isearch-lazy-highlight-windows-start)
 			    (goto-char (max (or isearch-lazy-highlight-start-limit (point-min))
-					    (window-start))))
-			(setq isearch-lazy-highlight-start (window-end))
+					    isearch-lazy-highlight-windows-start)))
+			(setq isearch-lazy-highlight-start isearch-lazy-highlight-windows-end)
 			(goto-char (min (or isearch-lazy-highlight-end-limit (point-max))
-					(window-end))))))))
+					isearch-lazy-highlight-windows-end)))))))
 	    (unless nomore
 	      (setq isearch-lazy-highlight-timer
 		    (run-at-time lazy-highlight-interval nil
@@ -3151,6 +3215,7 @@
   (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).

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

[Stefan Monnier]

> +(defvar isearch-windows nil
> +  "List of windows active in the incremental search.
> +This is either the ordered list of active windows administered by
> +`follow-mode' or a list of the single window involved in the
> +search.")
> +
> +(defmacro isearch-windows-start (&optional wins)
> +  "Get the start point of the windows involved in the isearch."
> +  `(window-start (car ,(if wins wins 'isearch-windows))))
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                          (or wins 'isearch-windows)

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.

> +  `(if isearch-message-function
> +       (funcall isearch-message-function ,cqh ,ellip)
> +     (isearch-message ,cqh ,ellip)))

Aka (funcall (or isearch-message-function #'isearch-message) ,cqh ,ellip)

BTW, isearch-message-function should be changed to default to
isearch-message rather than to nil.

> @@ -2207,10 +2239,12 @@
>  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 (isearch-windows-start))
> +	(w-end (isearch-windows-end nil t))
> +        (w-L1 (with-selected-window (car isearch-windows)
> +		(save-excursion (move-to-window-line 1) (point))))
> +        (w-L-1 (with-selected-window (car (last isearch-windows))
> +		 (save-excursion (move-to-window-line -1) (point))))

This isearch-string-out-of-window+isearch-back-into-window business, for
example should be generalized to a function along the lines of
"bring-region-into-sight".  It's not useful only for isearch.
E.g. ediff-next-hunk would also benefit from it.
And of course, it should come with a bring-region-into-sight-function
hook (which does not default to nil but to the default implementation),
which follow-mode can then override.

> +		;; 1000 is higher than ediff's 100+,

[ Side note: Ediff doesn't use priorities any more.  ]


        Stefan

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

[Alan Mackenzie]

Hello, Stefan.

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.

Follow Mode knows nothing of Isearch.  The problem the other way round is
in Lisp files that themselves play with redisplay (including calling
`sit-for') and `set-window-start', and so on.  Between these invocations
and the actual redisplay, Follow Mode must get a chance to make its
adjustments.

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

For what?

Given that Follow Mode uses a list of windows where most elisp programs
(including Isearch up till now) are expecting to deal with a single
window, there is quite a large clash, possibly unbridgeable with any
reasonable amount of effort.  Some sort of abstraction that smoothes over
the difference between the window and the window list is difficult to
conceive of - Follow Mode itself is such an abstraction.

Maybe a useful hook here would be `redisplay-hook' where Follow Mode
could do its stuff more effectively than on `post-command-hook'.

The extreme solution would be to enhance the display code to handle
multiple column windows, rendering Follow Mode redundant.  I don't think
anybody with the expertise has the time for this.

> > @@ -2207,10 +2239,12 @@
> >  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 (isearch-windows-start))
> > +	(w-end (isearch-windows-end nil t))
> > +        (w-L1 (with-selected-window (car isearch-windows)
> > +		(save-excursion (move-to-window-line 1) (point))))
> > +        (w-L-1 (with-selected-window (car (last isearch-windows))
> > +		 (save-excursion (move-to-window-line -1) (point))))

> This isearch-string-out-of-window+isearch-back-into-window business, for
> example should be generalized to a function along the lines of
> "bring-region-into-sight".  It's not useful only for isearch.

This seems to be a different bug to the one I reported, and orthogonal to
it.

> E.g. ediff-next-hunk would also benefit from it.

Not necessarily.  isearch-back-into-window, in addition to doing
"bring-region-into-sight" ensures that it's the isearch-point end of it
which is visible when it is too big for the window.  This may be
problematic for other uses, like in ediff.

> And of course, it should come with a bring-region-into-sight-function
> hook (which does not default to nil but to the default implementation),
> which follow-mode can then override.

Yes.

Question is, what about the main bug?  The patch I wrote fixes a real
bug, and works (modulo any remaining bugs).  Do you have any concrete
suggestions as to how to improve the fix?

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Stefan Monnier]

> Follow Mode knows nothing of Isearch.

follow-mode.el doesn't, indeed, but since you're moving some follow mode
code to isearch.el, that means that follow mode (which is now spread
over follow-mode.el and isearch.el) knows something about Isearch.

> The problem the other way round is in Lisp files that themselves play
> with redisplay (including calling `sit-for') and `set-window-start',
> and so on.  Between these invocations and the actual redisplay, Follow
> Mode must get a chance to make its adjustments.

There's no doubt that follow mode's task is a tricky one, and I'm
willing to add special support for it.  I just want this special support
to be a bit more generic than what you provided.

It shouldn't be too difficult.  It's just a matter of refactoring:
change your patch so that on isearch.el's side it only adds some hooks,
which are then set follow-mode.el.

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

So that the same hooks can be used by other code than Isearch, for example.

> Maybe a useful hook here would be `redisplay-hook' where Follow Mode
> could do its stuff more effectively than on `post-command-hook'.

There's pre-redisplay-function, which I think it does get us closer but
is not sufficient yet.  And of course, it won't help with things
like "bring-region-into-sight".

>> > @@ -2207,10 +2239,12 @@
>> >  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 (isearch-windows-start))
>> > +	(w-end (isearch-windows-end nil t))
>> > +        (w-L1 (with-selected-window (car isearch-windows)
>> > +		(save-excursion (move-to-window-line 1) (point))))
>> > +        (w-L-1 (with-selected-window (car (last isearch-windows))
>> > +		 (save-excursion (move-to-window-line -1) (point))))
>> This isearch-string-out-of-window+isearch-back-into-window business, for
>> example should be generalized to a function along the lines of
>> "bring-region-into-sight".  It's not useful only for isearch.
> This seems to be a different bug to the one I reported, and orthogonal
> to it.

What bug?  I'm just pointing out that the code you're modifying is more
generally useful and that this generality is a good guide to help us
decide where to put needed hooks.

>> E.g. ediff-next-hunk would also benefit from it.
        ^^^^^
I meant "diff", sorry.  Tho it would also be useful to ediff and smerge-mode
as well, indeed.

> Not necessarily.  isearch-back-into-window, in addition to doing
> "bring-region-into-sight" ensures that it's the isearch-point end of it
> which is visible when it is too big for the window.  This may be
> problematic for other uses, like in ediff.

I doubt this will be problematic.  It seems quite natural for
"bring-region-into-sight" to take as arguments not just the region but
also some indication of the part to favor in case it can't all
be displayed.

> Question is, what about the main bug?  The patch I wrote fixes a real
> bug, and works (modulo any remaining bugs).  Do you have any concrete
> suggestions as to how to improve the fix?

Hmm... I'm sorry I got lost along the way.  I thought the whole patch is
the bug fix (and the bring-region-into-sight part does seem like
a bug-fix as well, tho maybe of top priority, admittedly).

Could you show which part of the patch addresses the main bug?


        Stefan

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

[Alan Mackenzie]

Hello, Stefan.

On Sun, May 11, 2014 at 12:09:38PM -0400, Stefan Monnier wrote:
> > Follow Mode knows nothing of Isearch.

> follow-mode.el doesn't, indeed, but since you're moving some follow mode
> code to isearch.el, ....

Am I?  No code that currently is in follow-mode.el will cease to be in
follow-mode.el.

> .... that means that follow mode (which is now spread
> over follow-mode.el and isearch.el) knows something about Isearch.

No, Follow Mode will know nothing about Isearch.  Isearch will know about
follow-mode's internal structures, namely its list of windows, and what
their sequencing means.  In that sense, the two libraries are coupled,
which isn't, other things being equal, good, but I can't see how to
avoid it.  Can you see a way of avoiding this coupling?

> > The problem the other way round is in Lisp files that themselves play
> > with redisplay (including calling `sit-for') and `set-window-start',
> > and so on.  Between these invocations and the actual redisplay, Follow
> > Mode must get a chance to make its adjustments.

> There's no doubt that follow mode's task is a tricky one, and I'm
> willing to add special support for it.  I just want this special support
> to be a bit more generic than what you provided.

I'm having trouble discerning WHAT, specifically, should be made more
generic (with the exception of the two functions discussed below).  Could
you talk more in specifics, please.  

> It shouldn't be too difficult.  It's just a matter of refactoring:
> change your patch so that on isearch.el's side it only adds some hooks,
> which are then set follow-mode.el.

Do you mean "set to a function in follow-mode.el".  I think you're
suggesting writing more functions in follow-mode.el.  Do you mean
something like a Follow Mode equivalent of `window-start'?

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

> So that the same hooks can be used by other code than Isearch, for example.

I know the motivation for the change.  For what functionalities should
code be come up with that will be incorporated into these hooks?  What
will the hooks do, specifically?

> > Maybe a useful hook here would be `redisplay-hook' where Follow Mode
> > could do its stuff more effectively than on `post-command-hook'.

> There's pre-redisplay-function, which I think it does get us closer but
> is not sufficient yet.  And of course, it won't help with things
> like "bring-region-into-sight".

I didn't know about pre-redisplay-function.  It looks new.  As an aside,
its documentation is unclear (what is a "set" of windows, for example,
and what sort of things are allowed/not allowed in a hook function
(deleting windows, for example)?)

Is it for "bring-region-into-sight" and friends that you envisage adding
hooks?

> >> > @@ -2207,10 +2239,12 @@
> >> >  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 (isearch-windows-start))
> >> > +	(w-end (isearch-windows-end nil t))
> >> > +        (w-L1 (with-selected-window (car isearch-windows)
> >> > +		(save-excursion (move-to-window-line 1) (point))))
> >> > +        (w-L-1 (with-selected-window (car (last isearch-windows))
> >> > +		 (save-excursion (move-to-window-line -1) (point))))
> >> This isearch-string-out-of-window+isearch-back-into-window business, for
> >> example should be generalized to a function along the lines of
> >> "bring-region-into-sight".  It's not useful only for isearch.
> > This seems to be a different bug to the one I reported, and orthogonal
> > to it.

> What bug?  I'm just pointing out that the code you're modifying is more
> generally useful and that this generality is a good guide to help us
> decide where to put needed hooks.

I meant that forming a more general subroutine out of
isearch-back-into-window is not properly part of bug #17453.  It is a
separate exercise, which can be done independently of #17453.

I'm not sure how well a hook would work for this functionality, since in
the "plain" hook function you'd want to pass it a window, whereas in the
"follow" hook function you'd want to pass it a list of windows.

> >> E.g. ediff-next-hunk would also benefit from it.
>         ^^^^^
> I meant "diff", sorry.  Tho it would also be useful to ediff and smerge-mode
> as well, indeed.

OK.  So you're thinking of somewhere like subr.el, with something like

    (defun scroll-region-into-window/s (start end window/s above
    opposite-important) ....)

where WINDOW/S is either a single window or a list of them, START END
bound the region, OPPOSITE-IMPORTANT is t when the region boundary
"nearest the window" is to be preferred for display when the region's too
big.  Or something like that.  Maybe we could add a parameter for desired
margin at the top/bottom of the window.

> > Not necessarily.  isearch-back-into-window, in addition to doing
> > "bring-region-into-sight" ensures that it's the isearch-point end of it
> > which is visible when it is too big for the window.  This may be
> > problematic for other uses, like in ediff.

> I doubt this will be problematic.  It seems quite natural for
> "bring-region-into-sight" to take as arguments not just the region but
> also some indication of the part to favor in case it can't all
> be displayed.

OK.  Formulating a good description of that parameter is tricky, though.

> > Question is, what about the main bug?  The patch I wrote fixes a real
> > bug, and works (modulo any remaining bugs).  Do you have any concrete
> > suggestions as to how to improve the fix?

> Hmm... I'm sorry I got lost along the way.  I thought the whole patch is
> the bug fix (and the bring-region-into-sight part does seem like
> a bug-fix as well, tho maybe of top priority, admittedly).

> Could you show which part of the patch addresses the main bug?

The whole patch addresses the main bug; forming generally useful
subroutines out of isearch-string-out-of-window and friends is the other
bug, which I'm suggesting be dealt with separately.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Stefan Monnier]

>> follow-mode.el doesn't, indeed, but since you're moving some follow mode
>> code to isearch.el, ....
> Am I?  No code that currently is in follow-mode.el will cease to be in
> follow-mode.el.

Please replace "moving" with "adding" in my phrase, then.

> No, Follow Mode will know nothing about Isearch.  Isearch will know about
> follow-mode's internal structures, namely its list of windows, and what
> their sequencing means.  In that sense, the two libraries are coupled,
> which isn't, other things being equal, good, but I can't see how to
> avoid it.  Can you see a way of avoiding this coupling?

I gave one suggestion to uncouple them in one specific spot, yes.
There's no general answer for such problems, so we have to look at
every part of the coupling and try to find solutions for each part.

>> It shouldn't be too difficult.  It's just a matter of refactoring:
>> change your patch so that on isearch.el's side it only adds some hooks,
>> which are then set follow-mode.el.
> Do you mean "set to a function in follow-mode.el".  I think you're
> suggesting writing more functions in follow-mode.el.  Do you mean
> something like a Follow Mode equivalent of `window-start'?

Right, I think introducing new functions/hooks to get the beginning/end of the
visible part of the buffer, which can then be overridden by follow-mode
would probably be part of the solution.

> I know the motivation for the change.  For what functionalities should
> code be come up with that will be incorporated into these hooks?  What
> will the hooks do, specifically?

I don't know.  You know the code better than I do, so hopefully you can
come up with good ideas.

> I didn't know about pre-redisplay-function.  It looks new.  As an aside,
> its documentation is unclear (what is a "set" of windows, for example,

As is common in (Emacs) Lisp, sets are represented as a list.

> and what sort of things are allowed/not allowed in a hook function
> (deleting windows, for example)?)

Noone knows, really.

> Is it for "bring-region-into-sight" and friends that you envisage adding
> hooks?

Yes.

> I'm not sure how well a hook would work for this functionality, since in
> the "plain" hook function you'd want to pass it a window, whereas in the
> "follow" hook function you'd want to pass it a list of windows.

I don't see why you'd need to pass anything like a window or a list
of windows.  All it needs is a region and a point.  The window (or set
thereof) would be passed implicitly via selected-window, as usual.

> OK.  So you're thinking of somewhere like subr.el, with something like
>     (defun scroll-region-into-window/s (start end window/s above
>     opposite-important) ....)
> where WINDOW/S is either a single window or a list of them, START END
> bound the region, OPPOSITE-IMPORTANT is t when the region boundary
> "nearest the window" is to be preferred for display when the region's too
> big.  Or something like that.  Maybe we could add a parameter for desired
> margin at the top/bottom of the window.

Something like that, yes.

> OK.  Formulating a good description of that parameter is tricky, though.

Right, designing an API is often tricky.

> The whole patch addresses the main bug; forming generally useful
> subroutines out of isearch-string-out-of-window and friends is the other
> bug, which I'm suggesting be dealt with separately.

I don't want to add code specific to follow-mode directly in Isearch.
So the way to fix the bug is in 2 steps:
- refactor Isearch so that it adds the hooks we need (some of this,
  may involve creating new functions in subr.el such as discussed above).
- change follow-mode.el to set those hooks as appropriate.
There's then a 3rd step which is to make other packages than Isearch use
those same new functions/hooks, but that one can be postponed.


        Stefan

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

[Alan Mackenzie]

Hello, Stefan.

On Sun, May 11, 2014 at 03:05:17PM -0400, Stefan Monnier wrote:
> >> It shouldn't be too difficult.  It's just a matter of refactoring:
> >> change your patch so that on isearch.el's side it only adds some hooks,
> >> which are then set follow-mode.el.
> > Do you mean "set to a function in follow-mode.el".  I think you're
> > suggesting writing more functions in follow-mode.el.  Do you mean
> > something like a Follow Mode equivalent of `window-start'?

> Right, I think introducing new functions/hooks to get the beginning/end of the
> visible part of the buffer, which can then be overridden by follow-mode
> would probably be part of the solution.

OK, but it really needs a window list as a parameter, and that needs to
be stored in a variable somewhere.

> > I know the motivation for the change.  For what functionalities should
> > code be come up with that will be incorporated into these hooks?  What
> > will the hooks do, specifically?

> I don't know.  You know the code better than I do, so hopefully you can
> come up with good ideas.

I don't really see opportunities for this whose costs don't outweigh
them.  We can make a variable called `window-start-function', but then
everybody has to start using

    (funcall window-start-function w)

which is a little more obfuscated and confusing than

    (window-start w)

just to save a (very) few calls of

    (window-start (car foo-windows))

.  The last form is typically going to be much faster when Follow Mode is
active (see below).

[ Snipped bit about pre-redisplay-function.  Thanks. ]

> > Is it for "bring-region-into-sight" and friends that you envisage adding
> > hooks?

> Yes.

> > I'm not sure how well a hook would work for this functionality, since in
> > the "plain" hook function you'd want to pass it a window, whereas in the
> > "follow" hook function you'd want to pass it a list of windows.

> I don't see why you'd need to pass anything like a window or a list
> of windows.  All it needs is a region and a point.  The window (or set
> thereof) would be passed implicitly via selected-window, as usual.

Because each time the Follow Mode window list would have to be
recalculated, and this is a moderately expensive calculation, involving
filtering and sorting the windows in a frame (`follow-all-followers').
This is no big deal once per command, but if done much more often will
start to drag.

[ .... ]

> Right, designing an API is often tricky.

:-)

> > The whole patch addresses the main bug; forming generally useful
> > subroutines out of isearch-string-out-of-window and friends is the other
> > bug, which I'm suggesting be dealt with separately.

> I don't want to add code specific to follow-mode directly in Isearch.

There is one place where I think it is unavoidable, and that is adding
lazy highlight overlays.  These are currently given a `window' property
to restrict their effect to the current window.  In my patch, a
particular match gets _two_ (or even several) overlays when it spans two
or more Follow Mode windows.  This can surely not be abstracted away in
any sensible way.

> So the way to fix the bug is in 2 steps:
> - refactor Isearch so that it adds the hooks we need (some of this,
>   may involve creating new functions in subr.el such as discussed above).
> - change follow-mode.el to set those hooks as appropriate.
> There's then a 3rd step which is to make other packages than Isearch use
> those same new functions/hooks, but that one can be postponed.


>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Stefan Monnier]

>> Right, I think introducing new functions/hooks to get the
>> beginning/end of the visible part of the buffer, which can then be
>> overridden by follow-mode would probably be part of the solution.
> OK, but it really needs a window list as a parameter, and that needs to
> be stored in a variable somewhere.

I don't understand.  Why would it need to be both in a parameter and in
a variable somewhere?  And isn't it already in some follow-mode
"variable" somewhere?

window-start does not need a window parameter (it does accept such
a parameter, optionally, but that's just to avoid some
with-selected-window), so I don't think such new function would need
such a parameter either.

> I don't really see opportunities for this whose costs don't outweigh
> them.  We can make a variable called `window-start-function', but then
> everybody has to start using

>     (funcall window-start-function w)

I see two options:
- either some/many calls need to be changed to use the new API.
- or follow-mode advises window-start.

And of course, it would be (funcall window-start-function), without the `w'.

> which is a little more obfuscated and confusing than
>     (window-start w)

To make it less obfuscated, we can provide a new function
`viewable-area-start' which does (funcall window-start-function).

> .  The last form is typically going to be much faster when Follow Mode is
> active (see below).

Not sure about "much".  My gut feeling would be rather around "not
noticeably".

>> I don't see why you'd need to pass anything like a window or a list
>> of windows.  All it needs is a region and a point.  The window (or set
>> thereof) would be passed implicitly via selected-window, as usual.
> Because each time the Follow Mode window list

I don't see why.  Isn't/can't the "Follow Mode window list" be kept as
a window-parameter, so you can get it quickly?

> would have to be recalculated, and this is a moderately expensive
> calculation, involving filtering and sorting the windows in a frame
> (`follow-all-followers').  This is no big deal once per command, but
> if done much more often will start to drag.

Indeed, recomputing it for every window-start call would make it
expensive.  I assume that it can be cached such that we only recompute
it when it changes (e.g. using window-configuration-change-hook).

> There is one place where I think it is unavoidable, and that is adding
> lazy highlight overlays.  These are currently given a `window' property
> to restrict their effect to the current window.  In my patch, a
> particular match gets _two_ (or even several) overlays when it spans two
> or more Follow Mode windows.  This can surely not be abstracted away in
> any sensible way.

You might be right.  As mentioned, I think follow-mode is important
enough to warrant special treatment, so we can probably leave a few
hacks in there, in case no useful/sensible abstraction can be invented.

In this case of isearch-vs-followmode, I think the way to find the
useful abstractions is to try and figure out "what would be
useful/useable for other packages than isearch".

Window-specific overlays are not used very often, the candidates seem to be:
- region highlighting.
- rectangular region highlighting.
- compare-w.el.


        Stefan

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

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

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

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

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

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

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

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

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.

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?

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.

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

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)

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

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.

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?

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

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

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.

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

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

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

[Artur Malabarba]

Hi Alan,

2015-11-02 15:44 GMT+00:00 Alan Mackenzie <acm@muc.de>:
> 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.

Yes, I think this might work too. And I like the idea of eliminating a
redisplay inside the command-loop.
Could you test it?

You'll probably still need a simpler version of that
follow--search-function to ensure that lazy-highlighting extends to
all windows (instead of stopping inside the selected-window), but at
least it won't need that `select-window' part that you said is bad.
:-)

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

No, currently lazy highlighting to happens only in the current window
(with or without follow-mode). This happens for 2 reasons. (1) Isearch
only highlights between window-start and window-end, and (2) isearch
limits the highlithing overlays to only be displayed on the selected
window.

The new option I introduced to isearch allows the highlighting
overlays to be displayed on all windows (solving (2)). Furthermore,
one of features of that follow--search-function function is to make
sure that highlighting doesn't stop `window-end' (solving (1)).

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

[Drew Adams]

> one of features of that follow--search-function function is to make
> sure that highlighting doesn't stop `window-end' (solving (1)).

I believe this was already taken care of by work that Juri did recently.
I don't have time to look this up now, but see recent bug reports.

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

[Artur Malabarba]

2015-11-02 16:35 GMT+00:00 Drew Adams <drew.adams@oracle.com>:
>> one of features of that follow--search-function function is to make
>> sure that highlighting doesn't stop `window-end' (solving (1)).
>
> I believe this was already taken care of by work that Juri did recently.
> I don't have time to look this up now, but see recent bug reports.

I ran a git log on isearch.el and I think you're referring to this change:

10ec0468dfbc0815a772cc46a031aca298af0985 Lazy-highlight the whole
string at point (debbugs:19353)

If so, that's just about extending the bound a little bit to ensure it
holds a whole symbol. follow-mode actually needs to extend the bound a
lot more (up 'till the `window-end' of the last window in the group).

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

[Drew Adams]

> > I believe this was already taken care of by work that Juri did recently.
> > I don't have time to look this up now, but see recent bug reports.
> 
> I ran a git log on isearch.el and I think you're referring to this change:
> 
> 10ec0468dfbc0815a772cc46a031aca298af0985 Lazy-highlight the whole
> string at point (debbugs:19353)
> 
> If so, that's just about extending the bound a little bit to ensure it
> holds a whole symbol. follow-mode actually needs to extend the bound a
> lot more (up 'till the `window-end' of the last window in the group).

Please check with Juri.  I'm pretty sure the discussion included
the possibility of having lazy highlighting throughout a buffer.

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

[Juri Linkov]

>> > I believe this was already taken care of by work that Juri did recently.
>> > I don't have time to look this up now, but see recent bug reports.
>>
>> I ran a git log on isearch.el and I think you're referring to this change:
>>
>> 10ec0468dfbc0815a772cc46a031aca298af0985 Lazy-highlight the whole
>> string at point (debbugs:19353)
>>
>> If so, that's just about extending the bound a little bit to ensure it
>> holds a whole symbol. follow-mode actually needs to extend the bound a
>> lot more (up 'till the `window-end' of the last window in the group).
>
> Please check with Juri.  I'm pretty sure the discussion included
> the possibility of having lazy highlighting throughout a buffer.

Yes, what I meant is:

diff --git a/lisp/follow.el b/lisp/follow.el
index 938c59e..fc970bb 100644
--- a/lisp/follow.el
+++ b/lisp/follow.el
@@ -419,6 +419,7 @@ (define-minor-mode follow-mode
   :keymap follow-mode-map
   (if follow-mode
       (progn
+	(setq-local lazy-highlight-buffer t)
 	(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))

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

[Alan Mackenzie]

Hello again, Artur.

On Mon, Nov 02, 2015 at 04:26:23PM +0000, Artur Malabarba wrote:
> Hi Alan,

> 2015-11-02 15:44 GMT+00:00 Alan Mackenzie <acm@muc.de>:
> > 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.

> Yes, I think this might work too. And I like the idea of eliminating a
> redisplay inside the command-loop.
> Could you test it?

Of course, nothing is ever that simple.  ;-).  I've got a working
version which doesn't use "(sit-for 0)", thus allowing point to move
properly over Follow Mode windows.

However, this meant the "old" highlighting not being erased until the
0.25 seconds had passed.  So I tried testing the conditions for a new
lazy-highlight loop (apart from window-start/end changing) in the
command loop bit.  This doesn't work brilliantly well when a C-s causes
a half-screen forward scroll: the "upper" lazy highlighting remains
there, and 0.25s later the "lower" lazy H. appears.

It struck me that instead of all this rigmarole, and instead of the
"(sit-for 0)", we could test whether or not a scroll is about to take
place.  The obvious candidate for this is `pos-visible-in-window-p' - if
this returns nil, obviously a scroll is about to happen.  Unfortunately,
even if p-v-i-w-p returns t, a scroll might still happen because of
`scroll-margin', and the like.

What we could do with is an interface which will tell us whether or not
redisplay would scroll if invoked immediately, with the current value of
point, window-start, etc.  Obviously it would be simple, but messy to
hack a lisp function together to do this, but a more general purpose
function might be apposite here.  Eli, what do you think?

All this is, just for the moment, disregarding any enhancements needed
to support Follow Mode properly.  Just for the moment, that is.

If you're interested, here is a (still scrappy) patch of where I am at
the moment.



diff --git a/lisp/isearch.el b/lisp/isearch.el
index b762884..b59a224 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -3012,13 +3012,52 @@ 'isearch-lazy-highlight-cleanup
                                 "22.1")
 
 (defun isearch-lazy-highlight-new-loop (&optional beg end)
-  "Cleanup any previous `lazy-highlight' loop and begin a new one.
-BEG and END specify the bounds within which highlighting should occur.
-This is called when `isearch-update' is invoked (which can cause the
-search string to change or the window to scroll).  It is also used
-by other Emacs features."
+  "Set an idle timer, which will trigger a new `lazy-highlight' loop.
+BEG and END specify the bounds within which highlighting should
+occur.  This is called when `isearch-update' is invoked (which
+can cause the search string to change or the window(s) to
+scroll).  It is also used by other Emacs features."
+  (setq isearch-lazy-highlight-start-limit beg
+        isearch-lazy-highlight-end-limit end)
+  (when (null executing-kbd-macro)
+    (when (or (not (equal isearch-string
+                          isearch-lazy-highlight-last-string))
+              (not (eq (selected-window)
+                       isearch-lazy-highlight-window))
+              (not (eq isearch-lazy-highlight-case-fold-search
+                       isearch-case-fold-search))
+              (not (eq isearch-lazy-highlight-regexp
+                       isearch-regexp))
+              (not (eq isearch-lazy-highlight-regexp-function
+                       isearch-regexp-function))
+              (not (eq isearch-lazy-highlight-lax-whitespace
+                       isearch-lax-whitespace))
+              (not (eq isearch-lazy-highlight-regexp-lax-whitespace
+                       isearch-regexp-lax-whitespace))
+              ;; (not (= (window-start)
+              ;;         isearch-lazy-highlight-window-start))
+              ;; (not (= (window-end) ; Window may have been split/joined.
+              ;;         isearch-lazy-highlight-window-end))
+              (not (eq isearch-forward
+                       isearch-lazy-highlight-forward))
+              ;; In case we are recovering from an error.
+              (not (equal isearch-error
+                          isearch-lazy-highlight-error)))
+      (lazy-highlight-cleanup t))    ;kill old loop & remove overlays.
+    (setq isearch-lazy-highlight-timer
+          (run-with-idle-timer lazy-highlight-initial-delay nil
+                               'isearch-lazy-highlight-maybe-new-loop))))
+
+(defun isearch-lazy-highlight-maybe-new-loop ()
+  "If needed, cleanup the previous `lazy-highlight' loop and begin a new one.
+Return t if we begin a loop, nil otherwise."
+;; "   Cleanup any previous `lazy-highlight' loop and begin a new one.
+;; BEG and END specify the bounds within which highlighting should occur.
+;; This is called when `isearch-update' is invoked (which can cause the
+;; search string to change or the window to scroll).  It is also used
+;; by other Emacs features."
   (when (and (null executing-kbd-macro)
-             (sit-for 0)         ;make sure (window-start) is credible
+             ;; (sit-for 0)         ;make sure (window-start) is credible
              (or (not (equal isearch-string
                              isearch-lazy-highlight-last-string))
                  (not (eq (selected-window)
@@ -3048,8 +3087,8 @@ isearch-lazy-highlight-new-loop
     ;; It used to check for `(not isearch-error)' here, but actually
     ;; lazy-highlighting might find matches to highlight even when
     ;; `isearch-error' is non-nil.  (Bug#9918)
-    (setq isearch-lazy-highlight-start-limit beg
-	  isearch-lazy-highlight-end-limit end)
+    ;; (setq isearch-lazy-highlight-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)
@@ -3070,10 +3109,12 @@ isearch-lazy-highlight-new-loop
 	  isearch-lazy-highlight-regexp-lax-whitespace isearch-regexp-lax-whitespace
 	  isearch-lazy-highlight-regexp-function  isearch-regexp-function
 	  isearch-lazy-highlight-forward      isearch-forward)
-      (unless (equal isearch-string "")
-	(setq isearch-lazy-highlight-timer
-	      (run-with-idle-timer lazy-highlight-initial-delay nil
-				   'isearch-lazy-highlight-update)))))
+      ;; (unless (equal isearch-string "")
+      ;;   (setq isearch-lazy-highlight-timer
+      ;;         (run-with-idle-timer lazy-highlight-initial-delay nil
+      ;;   			   'isearch-lazy-highlight-update)))
+    (unless (equal isearch-string "")
+      (isearch-lazy-highlight-update))))
 
 (defun isearch-lazy-highlight-search ()
   "Search ahead for the next or previous match, for lazy highlighting.



-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Artur Malabarba]

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

On 2 Nov 2015 10:09 pm, "Alan Mackenzie" <acm@muc.de> wrote:
> Of course, nothing is ever that simple.  ;-).  I've got a working
> version which doesn't use "(sit-for 0)", thus allowing point to move
> properly over Follow Mode windows.
>
> However, this meant the "old" highlighting not being erased until the
> 0.25 seconds had passed.  So I tried testing the conditions for a new
> lazy-highlight loop (apart from window-start/end changing) in the
> command loop bit.
> It struck me that instead of all this rigmarole, and instead of the
> "(sit-for 0)", we could test whether or not a scroll is about to take
> place.

Or we can cleanup the lazy highlighting _when_ scrolling takes place.

> What we could do with is an interface which will tell us whether or not
> redisplay would scroll if invoked immediately, with the current value of
> point, window-start, etc.

I don't think that's necessary. What if isearch just added
 (lazy-highlight-cleanup t) to window-scroll-functions?

I want to be very cautious about not increasing the complexity of isearch
code here, but so far these sound like good refactorings to me.

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

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

[Alan Mackenzie]

Hello, Artur.

On Mon, Nov 02, 2015 at 11:00:21PM +0000, Artur Malabarba wrote:
> On 2 Nov 2015 10:09 pm, "Alan Mackenzie" <acm@muc.de> wrote:
> > Of course, nothing is ever that simple.  ;-).  I've got a working
> > version which doesn't use "(sit-for 0)", thus allowing point to move
> > properly over Follow Mode windows.

> > However, this meant the "old" highlighting not being erased until the
> > 0.25 seconds had passed.  So I tried testing the conditions for a new
> > lazy-highlight loop (apart from window-start/end changing) in the
> > command loop bit.
> > It struck me that instead of all this rigmarole, and instead of the
> > "(sit-for 0)", we could test whether or not a scroll is about to take
> > place.

> Or we can cleanup the lazy highlighting _when_ scrolling takes place.

> > What we could do with is an interface which will tell us whether or not
> > redisplay would scroll if invoked immediately, with the current value of
> > point, window-start, etc.

> I don't think that's necessary. What if isearch just added
>  (lazy-highlight-cleanup t) to window-scroll-functions?

Now that's a good idea.  :-)

So stale lazy highlighting could be erased by the multitude of existing
tests for most cases, and by that hook function for when we scroll.  I'm
going to try it.

> I want to be very cautious about not increasing the complexity of isearch
> code here, but so far these sound like good refactorings to me.

I agree on both counts.

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Eli Zaretskii]

> Date: Mon, 2 Nov 2015 15:44:45 +0000
> From: Alan Mackenzie <acm@muc.de>
> Cc: Juri Linkov <juri@linkov.net>, 17453@debbugs.gnu.org,
> 	emacs-devel <emacs-devel@gnu.org>
> 
> 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.

You could call the same code that post-command-hook calls, then it
wouldn't be second-guessing.

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?

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

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

[Eli Zaretskii]

PLEASE let's keep emacs-devel out of this??

> Date: Tue, 3 Nov 2015 12:31:16 +0000
> From: Alan Mackenzie <acm@muc.de>
> Cc: 17453@debbugs.gnu.org, Artur Malabarba <bruce.connor.am@gmail.com>,
> 	emacs-devel <emacs-devel@gnu.org>
> 
> 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.

Once again, if some code in Isearch calls the same function that is
used in follow-post-command-hook, the above is not an issue.
Moreover, saving some calls to the hook will make Emacs more
responsive.  (Right now, using Follow mode is a pain due to the hook:
even a simple C-f is annoyingly slow.)

> 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[*]

I don't think you can count on that: if there's a ripe timer, it will
be processed before redisplay.  I think.

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

[Artur Malabarba]

2015-11-03 15:49 GMT+00:00 Eli Zaretskii <eliz@gnu.org>:
>> 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.
>
> Once again, if some code in Isearch calls the same function that is
> used in follow-post-command-hook, the above is not an issue.
> Moreover, saving some calls to the hook will make Emacs more
> responsive.

I agree with Eli and Juri on this. If there's a solution as simple as
calling a follow-mode function in isearch-post-update-hook, then this
sounds like a no-downside solution.
Of course it's not isearch's job to realign windows, but that's why
the hook exists: so that other packages can run their own functions
and do their own jobs at that point in time.

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

[Alan Mackenzie]

Hello, Arturn.

On Tue, Nov 03, 2015 at 04:18:45PM +0000, Artur Malabarba wrote:
> 2015-11-03 15:49 GMT+00:00 Eli Zaretskii <eliz@gnu.org>:
> >> 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.

> > Once again, if some code in Isearch calls the same function that is
> > used in follow-post-command-hook, the above is not an issue.
> > Moreover, saving some calls to the hook will make Emacs more
> > responsive.

> I agree with Eli and Juri on this. If there's a solution as simple as
> calling a follow-mode function in isearch-post-update-hook, then this
> sounds like a no-downside solution.

I'm wondering if we're still talking about the same problem.  ;-)  A
simpler solution is _not_ to call a FM function from that Isearch hook.
Unless we're talking at cross purposes, there is simply no need.  As
long as the Isearch command is allowed to go to completion without
forcibly redisplaying, FM will re-synchronise the windows (if needed)
and select an appropriate window for point, all on its own (in
follow-post-command-hook).

> Of course it's not isearch's job to realign windows, but that's why
> the hook exists: so that other packages can run their own functions
> and do their own jobs at that point in time.

Good night, and sleep well!

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Juri Linkov]

>> >> 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.
>
>> > Once again, if some code in Isearch calls the same function that is
>> > used in follow-post-command-hook, the above is not an issue.
>> > Moreover, saving some calls to the hook will make Emacs more
>> > responsive.
>
>> I agree with Eli and Juri on this. If there's a solution as simple as
>> calling a follow-mode function in isearch-post-update-hook, then this
>> sounds like a no-downside solution.
>
> I'm wondering if we're still talking about the same problem.  ;-)  A
> simpler solution is _not_ to call a FM function from that Isearch hook.
> Unless we're talking at cross purposes, there is simply no need.  As
> long as the Isearch command is allowed to go to completion without
> forcibly redisplaying, FM will re-synchronise the windows (if needed)
> and select an appropriate window for point, all on its own (in
> follow-post-command-hook).

It still might help to synchronise the windows from isearch-update-post-hook
if we'll call it before calling isearch-lazy-highlight-new-loop with sit-for.
I see no problem in changing the order of calls to isearch-update-post-hook and
isearch-lazy-highlight-new-loop in isearch-update.  Sure, follow-post-command-hook
will be called twice but at least this simple solution for follow-mode
doesn't require re-designing the whole lazy-highlighting machinery.

diff --git a/lisp/follow.el b/lisp/follow.el
index 938c59e..75c2788 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-post-command-hook 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-post-command-hook 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..1e4a1a5 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -1011,12 +1011,12 @@ (defun isearch-update ()
   (setq ;; quit-flag nil  not for isearch-mode
    isearch-adjusted nil
    isearch-yank-flag nil)
-  (when isearch-lazy-highlight
-    (isearch-lazy-highlight-new-loop))
   ;; We must prevent the point moving to the end of composition when a
   ;; part of the composition has just been searched.
   (setq disable-point-adjustment t)
-  (run-hooks 'isearch-update-post-hook))
+  (run-hooks 'isearch-update-post-hook)
+  (when isearch-lazy-highlight
+    (isearch-lazy-highlight-new-loop)))
 
 (defun isearch-done (&optional nopush edit)
   "Exit Isearch mode.

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

[Alan Mackenzie]

Hello, Juri.

On Wed, Nov 04, 2015 at 02:28:08AM +0200, Juri Linkov wrote:
> >> >> 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.

> >> > Once again, if some code in Isearch calls the same function that is
> >> > used in follow-post-command-hook, the above is not an issue.
> >> > Moreover, saving some calls to the hook will make Emacs more
> >> > responsive.

> >> I agree with Eli and Juri on this. If there's a solution as simple as
> >> calling a follow-mode function in isearch-post-update-hook, then this
> >> sounds like a no-downside solution.

> > I'm wondering if we're still talking about the same problem.  ;-)  A
> > simpler solution is _not_ to call a FM function from that Isearch hook.
> > Unless we're talking at cross purposes, there is simply no need.  As
> > long as the Isearch command is allowed to go to completion without
> > forcibly redisplaying, FM will re-synchronise the windows (if needed)
> > and select an appropriate window for point, all on its own (in
> > follow-post-command-hook).

> It still might help to synchronise the windows from isearch-update-post-hook
> if we'll call it before calling isearch-lazy-highlight-new-loop with sit-for.

I still say, wait until we really need it before we do anything so
drastic.  As Eli noted, follow-post-command-hook is SLOW, SLOW, SLOW.
If we call it twice per command, it will be twice as slow.

Also, why is the "(sit-for 0)" there at all?  As its comment says, It
is there for one purpose, and one purpose only: it is so that
(window-start) is valid, and the check

    (not (= (window-start)
            isearch-lazy-highlight-window-start))

will work.  This check means exactly "has the window scrolled?".

> I see no problem in changing the order of calls to isearch-update-post-hook and
> isearch-lazy-highlight-new-loop in isearch-update.  Sure, follow-post-command-hook
> will be called twice but at least this simple solution for follow-mode
> doesn't require re-designing the whole lazy-highlighting machinery.

In one of my mails yesterday, I proposed removing the (sit-for 0) and
replacing this check on (window-start) with

    (redisplay-would-scroll-window-p)

.  This would fix the bug without any further changes.  It would avoid
any far reaching change in design of the lazy highlighting, avoid
calling follow-post-command-hook twice, yet would work.

> diff --git a/lisp/follow.el b/lisp/follow.el
> index 938c59e..75c2788 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-post-command-hook 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-post-command-hook 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..1e4a1a5 100644
> --- a/lisp/isearch.el
> +++ b/lisp/isearch.el
> @@ -1011,12 +1011,12 @@ (defun isearch-update ()
>    (setq ;; quit-flag nil  not for isearch-mode
>     isearch-adjusted nil
>     isearch-yank-flag nil)
> -  (when isearch-lazy-highlight
> -    (isearch-lazy-highlight-new-loop))
>    ;; We must prevent the point moving to the end of composition when a
>    ;; part of the composition has just been searched.
>    (setq disable-point-adjustment t)
> -  (run-hooks 'isearch-update-post-hook))
> +  (run-hooks 'isearch-update-post-hook)
> +  (when isearch-lazy-highlight
> +    (isearch-lazy-highlight-new-loop)))
 
>  (defun isearch-done (&optional nopush edit)
>    "Exit Isearch mode.

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Artur Malabarba]

2015-11-04 9:01 GMT+00:00 Alan Mackenzie <acm@muc.de>:
>> It still might help to synchronise the windows from isearch-update-post-hook
>> if we'll call it before calling isearch-lazy-highlight-new-loop with sit-for.
>
> I still say, wait until we really need it before we do anything so
> drastic.  As Eli noted, follow-post-command-hook is SLOW, SLOW, SLOW.
> If we call it twice per command, it will be twice as slow.
>
> Also, why is the "(sit-for 0)" there at all?  As its comment says, It
> is there for one purpose, and one purpose only: it is so that
> (window-start) is valid, and the check
>
>     (not (= (window-start)
>             isearch-lazy-highlight-window-start))
>
> will work.  This check means exactly "has the window scrolled?"

Have you tested redisplay-would-scroll-window-p in a fully folded
org-mode buffer with 100 headlines where each headline has 100 lines
of content?

If you report that it works in this context and isn't slow then I'm ok
with going with this solution. Like I said, as long as it doesn't
cause regressions, this would be a fine refactoring to do in
isearch.el. So we might as well apply it now and give any possible
issues some time to surface (isearch is used by tons of people, so I'm
sure they'll surface if any exist).

While you're at it, if you could also refactor all those `(not (equal
...))' tests into a single `isearch--lazy-highlight-needs-update-p'
function that would be very welcome (and if you do, do it as a
separate commit before everything else so the other stuff is easy to
revert separately).

And if it looks like I'm saying "I'm fine with this" to everything
here, that's because I am. It sounds like we're debating two fine
options and bordering on bikeshedding. So I say: merge one and see how
it goes.


Cheers

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

[Alan Mackenzie]

Hello, Artur.

On Wed, Nov 04, 2015 at 10:17:09AM +0000, Artur Malabarba wrote:
> 2015-11-04 9:01 GMT+00:00 Alan Mackenzie <acm@muc.de>:
> >> It still might help to synchronise the windows from isearch-update-post-hook
> >> if we'll call it before calling isearch-lazy-highlight-new-loop with sit-for.

> > I still say, wait until we really need it before we do anything so
> > drastic.  As Eli noted, follow-post-command-hook is SLOW, SLOW, SLOW.
> > If we call it twice per command, it will be twice as slow.

> > Also, why is the "(sit-for 0)" there at all?  As its comment says, It
> > is there for one purpose, and one purpose only: it is so that
> > (window-start) is valid, and the check

> >     (not (= (window-start)
> >             isearch-lazy-highlight-window-start))

> > will work.  This check means exactly "has the window scrolled?"

> Have you tested redisplay-would-scroll-window-p in a fully folded
> org-mode buffer with 100 headlines where each headline has 100 lines
> of content?

Of course not.  :-)

By the way, what does Isearch do with such org-mode folded buffer?  Does
it unfold the content, ever?

> If you report that it works in this context and isn't slow then I'm ok
> with going with this solution. Like I said, as long as it doesn't
> cause regressions, this would be a fine refactoring to do in
> isearch.el. So we might as well apply it now and give any possible
> issues some time to surface (isearch is used by tons of people, so I'm
> sure they'll surface if any exist).

> While you're at it, if you could also refactor all those `(not (equal
> ...))' tests into a single `isearch--lazy-highlight-needs-update-p'
> function that would be very welcome (and if you do, do it as a
> separate commit before everything else so the other stuff is easy to
> revert separately).

I don't agree this is a good idea.  At the moment, all these tests are
in the same not too big defun that sets them.  This kind of keeps things
together - for instance if a new test was to be introduced, only this
one function would need amending.

> And if it looks like I'm saying "I'm fine with this" to everything
> here, that's because I am. It sounds like we're debating two fine
> options and bordering on bikeshedding. So I say: merge one and see how
> it goes.

I'm pretty much ready to do this.  But maybe you could answer my
question about unfolding org-mode buffers above, first.  Thanks!

> Cheers

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Artur Malabarba]

>> Have you tested redisplay-would-scroll-window-p in a fully folded
>> org-mode buffer with 100 headlines where each headline has 100 lines
>> of content?
>
> Of course not.  :-)
>
> By the way, what does Isearch do with such org-mode folded buffer?  Does
> it unfold the content, ever?

Isearch has an option for that. By default it searches inside hidden
text, and reveals the hidden text if the match is in there.

But that's not why I asked.

The 'count-screen-lines' function can be horribly slow on a large
number of lines. I just want to check whether 'move-to-window-line'
doesn't suffer from the same issue. A huge org buffer with lots of
folded headlines is just a common cause of slowness for
`count-screen-lines' because small visual movements could mean very
large actual movements.

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

[Alan Mackenzie]

Hello, Juri, Artur, Eli.

On Wed, Nov 04, 2015 at 02:28:08AM +0200, Juri Linkov wrote:
> >> >> 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.

> >> > Once again, if some code in Isearch calls the same function that is
> >> > used in follow-post-command-hook, the above is not an issue.
> >> > Moreover, saving some calls to the hook will make Emacs more
> >> > responsive.

> >> I agree with Eli and Juri on this. If there's a solution as simple as
> >> calling a follow-mode function in isearch-post-update-hook, then this
> >> sounds like a no-downside solution.

> > I'm wondering if we're still talking about the same problem.  ;-)  A
> > simpler solution is _not_ to call a FM function from that Isearch hook.
> > Unless we're talking at cross purposes, there is simply no need.  As
> > long as the Isearch command is allowed to go to completion without
> > forcibly redisplaying, FM will re-synchronise the windows (if needed)
> > and select an appropriate window for point, all on its own (in
> > follow-post-command-hook).

> It still might help to synchronise the windows from isearch-update-post-hook
> if we'll call it before calling isearch-lazy-highlight-new-loop with sit-for.
> I see no problem in changing the order of calls to isearch-update-post-hook and
> isearch-lazy-highlight-new-loop in isearch-update.  Sure, follow-post-command-hook
> will be called twice but at least this simple solution for follow-mode
> doesn't require re-designing the whole lazy-highlighting machinery.

The discussion of this bug seems to have stalled.  I'd very much like to
decide all the issues now, and to fix (this part of) this bug.

Bug summary: With follow mode active, and lazy highliting active, forward
Isearch scrolls the left hand window when it should instead move to to
the right hand window.

Immediate cause: the form "(sit-for 0)" in
isearch-lazy-highlight-new-loop scrolls the window before Follow Mode has
had a chance to resynchronise everything.

Proposed solutions:
1. Call follow-post-command-hook from isearch-update before calling
  isearch-lazy-highlight-new-loop (as described above).
2. Call the proposed function `redisplay-would-scroll-window' instead of
  the `sit-for'.
3. Make isearch-lazy-highlight-new-loop always set the idle timer, and
  test for the need for a new loop instead in the function it triggers.
  Remove the `sit-for'.

I now think solution 2. is not sensible or realistic.  Redisplay is just
too complicated to second-guess.

Solution 1. has the disadvantage that follow-post-command-hook would be
called twice for every command in Isearch.  It is not fast.

Solution 3. similarly might have the problem that if
lazy-highlight-initial-delay is set to zero, redisplay might not have
done its work when isearch-lazy-highlight-update runs.  (I haven't tried
it out, yet).

Personally, I am in favour of solution 3, but I'm willing to be persuaded
into solution 1.  But I'd like us to come to a decision quickly.

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Eli Zaretskii]

> Date: Sat, 7 Nov 2015 12:59:31 +0000
> Cc: Artur Malabarba <bruce.connor.am@gmail.com>, Eli Zaretskii <eliz@gnu.org>,
>   17453@debbugs.gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> Proposed solutions:
> 1. Call follow-post-command-hook from isearch-update before calling
>   isearch-lazy-highlight-new-loop (as described above).
> 2. Call the proposed function `redisplay-would-scroll-window' instead of
>   the `sit-for'.
> 3. Make isearch-lazy-highlight-new-loop always set the idle timer, and
>   test for the need for a new loop instead in the function it triggers.
>   Remove the `sit-for'.
> 
> I now think solution 2. is not sensible or realistic.  Redisplay is just
> too complicated to second-guess.
> 
> Solution 1. has the disadvantage that follow-post-command-hook would be
> called twice for every command in Isearch.  It is not fast.
> 
> Solution 3. similarly might have the problem that if
> lazy-highlight-initial-delay is set to zero, redisplay might not have
> done its work when isearch-lazy-highlight-update runs.  (I haven't tried
> it out, yet).
> 
> Personally, I am in favour of solution 3, but I'm willing to be persuaded
> into solution 1.  But I'd like us to come to a decision quickly.

How about using 1), but also adding some indication that could prevent
the post-command-hook from being called twice?

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

[Alan Mackenzie]

Hello, Eli.

On Sat, Nov 07, 2015 at 03:38:31PM +0200, Eli Zaretskii wrote:
> > Date: Sat, 7 Nov 2015 12:59:31 +0000
> > Cc: Artur Malabarba <bruce.connor.am@gmail.com>, Eli Zaretskii <eliz@gnu.org>,
> >   17453@debbugs.gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > Proposed solutions:
> > 1. Call follow-post-command-hook from isearch-update before calling
> >   isearch-lazy-highlight-new-loop (as described above).
> > 2. Call the proposed function `redisplay-would-scroll-window' instead of
> >   the `sit-for'.
> > 3. Make isearch-lazy-highlight-new-loop always set the idle timer, and
> >   test for the need for a new loop instead in the function it triggers.
> >   Remove the `sit-for'.

> > I now think solution 2. is not sensible or realistic.  Redisplay is just
> > too complicated to second-guess.

> > Solution 1. has the disadvantage that follow-post-command-hook would be
> > called twice for every command in Isearch.  It is not fast.

> > Solution 3. similarly might have the problem that if
> > lazy-highlight-initial-delay is set to zero, redisplay might not have
> > done its work when isearch-lazy-highlight-update runs.  (I haven't tried
> > it out, yet).

> > Personally, I am in favour of solution 3, but I'm willing to be persuaded
> > into solution 1.  But I'd like us to come to a decision quickly.

> How about using 1), but also adding some indication that could prevent
> the post-command-hook from being called twice?

Hmm.  Such artifices are not pretty.  Such a flag would have to be set in
isearch.el just after isearch-update-post-hook (which calls
follow-post-command-hook) has been invoked.  Something would have to
clear it, perhaps something at the end of the lazy highlight loop.  Also,
this would couple Isearch and Follow Mode undesirably: Isearch really
shouldn't have to know anything about the FM window setup.

Or, maybe just letting follow-post-command-hook run twice wouldn't be too
bad.  The second time, all the windows would already be synchronised and
point would be in the right window.  I've had a look at the code, and it
seems in this situation it actually does call the function to rearrange
the windows.  Maybe that is a bug which might be fixed.

Do you have any views on solution 3?

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Alan Mackenzie]

Hello, Eli.

On Tue, Nov 03, 2015 at 05:49:17PM +0200, Eli Zaretskii wrote:
> PLEASE let's keep emacs-devel out of this??

OK.

> > Date: Tue, 3 Nov 2015 12:31:16 +0000
> > From: Alan Mackenzie <acm@muc.de>
> > Cc: 17453@debbugs.gnu.org, Artur Malabarba <bruce.connor.am@gmail.com>,
> > 	emacs-devel <emacs-devel@gnu.org>

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

> Once again, if some code in Isearch calls the same function that is
> used in follow-post-command-hook, the above is not an issue.
> Moreover, saving some calls to the hook will make Emacs more
> responsive.  (Right now, using Follow mode is a pain due to the hook:
> even a simple C-f is annoyingly slow.)

The follow-post-command-hook will run anyway.  If Isearch directly
invokes that Follow Mode function, f-p-c-h will be run a second time.
That's one of the reasons I want to avoid it.

> > 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[*]

> I don't think you can count on that: if there's a ripe timer, it will
> be processed before redisplay.  I think.

Ah.  So if somebody sets isearch-lazy-highlighting-intial-delay to
0.00001 seconds, it will trigger before redisplay has happened.  Yes.

Here's another idea.  In place of that "(sit-for 0)", I add another
condition

    (redisplay-would-scroll-window-p)

, with the following in (probably) window.el:

(defun redisplay-would-scroll-window-p (&optional pos window)
  "Would redisplay scroll WINDOW vertically on an immediate redisplay?
WINDOW defaults to the current window.  POS, the position of point which
is to be tested defaults to the value of point in WINDOW.  The buffer
displayed by WINDOW is assumed to be the current buffer.

Return t if a vertical scroll would be triggered by redisplay, otherwise nil."
  (or window (setq window (selected-window)))
  (or pos (setq pos (window-point window)))
  (with-selected-window window
    (with-current-buffer (window-buffer)
      (not
       (and (pos-visible-in-window-p pos)
            (or (<= scroll-margin 0)
                (let* ((this-scroll-margin (min scroll-margin
                                                (/ (window-body-height) 4)))
                       (top-limit
                        (save-excursion
                         (move-to-window-line this-scroll-margin)
                         (point)))
                       (bottom-limit
                        (save-excursion
                          (move-to-window-line (- this-scroll-margin))
                          (point))))
                  (and (>= pos top-limit)
                       (< pos bottom-limit)))))))))

.

-- 
Alan Mackenzie (Nuremberg, Germany).

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

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

[Eli Zaretskii]

Lets' take emacs-devel out of this shall we?

> Date: Mon, 2 Nov 2015 12:35:12 +0000
> From: Alan Mackenzie <acm@muc.de>
> Cc: Juri Linkov <juri@linkov.net>, 17453@debbugs.gnu.org,
> 	Stefan Monnier <monnier@iro.umontreal.ca>,
> 	emacs-devel <emacs-devel@gnu.org>
> 
> > 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.

If this means that sit*-for does something other than redisplay and
wait, like switch to another window, I'd really suggest to rethink
that.  It is entirely counter-intuitive to have a sit-for family of
functions do anything other than some kind of redisplay and some kind
of waiting.

I think it is a better idea to have Isearch switch to another window
when the next hit is there, via some specialized movement command that
Follow mode could customize.  That'd be something expectable.

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

[Alan Mackenzie]

Hello, Eli.

On Mon, Nov 02, 2015 at 05:46:34PM +0200, Eli Zaretskii wrote:
> Lets' take emacs-devel out of this shall we?

OK.

> > Date: Mon, 2 Nov 2015 12:35:12 +0000
> > From: Alan Mackenzie <acm@muc.de>
> > Cc: Juri Linkov <juri@linkov.net>, 17453@debbugs.gnu.org,
> > 	Stefan Monnier <monnier@iro.umontreal.ca>,
> > 	emacs-devel <emacs-devel@gnu.org>

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

> If this means that sit*-for does something other than redisplay and
> wait, like switch to another window, I'd really suggest to rethink
> that.  It is entirely counter-intuitive to have a sit-for family of
> functions do anything other than some kind of redisplay and some kind
> of waiting.

sit*-for's synchronising windows, switching to the appropriate window,
etc., is conceptually an extension of redisplay's scrolling to get point
on screen.  But I think sit*-for could well be not needed here, anyway.

> I think it is a better idea to have Isearch switch to another window
> when the next hit is there, via some specialized movement command that
> Follow mode could customize.  That'd be something expectable.

No.  The right fix is not to do "(sit-for 0)" at that point in the
processing.  As I explained at length to Artur in another post today, if
we move lazy-highlight's checking whether window-start/end have changed,
into the function triggered by the timer, redisplay will already have
happened and so we won't need that offending sit-for.

Then Follow Mode will quite naturally select the next window (via its
post-command-hook), rather than isearch forcibly scrolling the first
one.  As currently happens when lazy highlighting is disabled.

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Eli Zaretskii]

> Date: Mon, 2 Nov 2015 16:09:17 +0000
> Cc: bruce.connor.am@gmail.com, juri@linkov.net, 17453@debbugs.gnu.org,
>   monnier@iro.umontreal.ca
> From: Alan Mackenzie <acm@muc.de>
> 
> > If this means that sit*-for does something other than redisplay and
> > wait, like switch to another window, I'd really suggest to rethink
> > that.  It is entirely counter-intuitive to have a sit-for family of
> > functions do anything other than some kind of redisplay and some kind
> > of waiting.
> 
> sit*-for's synchronising windows, switching to the appropriate window,
> etc., is conceptually an extension of redisplay's scrolling to get point
> on screen.

No, redisplay doesn't necessarily scroll.  It does so only if needed.
So sit-for should not be thought as a way to scroll the window, even
if sometimes it does.

> But I think sit*-for could well be not needed here, anyway.

That's okay, but my point is: please do not add to established
functions code that does on behalf of Follow mode something that
conceptually doesn't belong to those functions.  OK?

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

[John Wiegley]

>>>>> Eli Zaretskii <eliz@gnu.org> writes:

> That's okay, but my point is: please do not add to established functions
> code that does on behalf of Follow mode something that conceptually doesn't
> belong to those functions. OK?

I'd like to second this request.

John

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

[Alan Mackenzie]

On Mon, Nov 02, 2015 at 07:49:43PM +0200, Eli Zaretskii wrote:
> > Date: Mon, 2 Nov 2015 16:09:17 +0000
> > Cc: bruce.connor.am@gmail.com, juri@linkov.net, 17453@debbugs.gnu.org,
> >   monnier@iro.umontreal.ca
> > From: Alan Mackenzie <acm@muc.de>

> > > If this means that sit*-for does something other than redisplay and
> > > wait, like switch to another window, I'd really suggest to rethink
> > > that.  It is entirely counter-intuitive to have a sit-for family of
> > > functions do anything other than some kind of redisplay and some kind
> > > of waiting.

> > sit*-for's synchronising windows, switching to the appropriate window,
> > etc., is conceptually an extension of redisplay's scrolling to get point
> > on screen.

> No, redisplay doesn't necessarily scroll.  It does so only if needed.
> So sit-for should not be thought as a way to scroll the window, even
> if sometimes it does.

Redisplay scrolls sometimes, and it moves point sometimes.  But it does
those things only when needed for the actual display.  Similarly the
sit*-for, invoking display would do other things strictly necessary for
successful redisplay.  That sometimes would include resynchronising the
windows, for example.

> > But I think sit*-for could well be not needed here, anyway.

> That's okay, but my point is: please do not add to established
> functions code that does on behalf of Follow mode something that
> conceptually doesn't belong to those functions.  OK?

Fine!

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Drew Adams]

> Lets' take emacs-devel out of this shall we?

Enfin !

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.

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

[Alan Mackenzie]

Bug fixed in emacs-25 branch.

-- 
Alan Mackenzie (Nuremberg, Germany).