GNU bug report logs - #17453
Isearch doesn't work properly with Follow Mode.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 17453 in the body.
You can then email your comments to 17453 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: bug-gnu-emacs <at> gnu.org
Subject: Isearch doesn't work properly with Follow Mode.
Date: Fri, 9 May 2014 22:44:58 +0000

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

Message #8 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Fri, 09 May 2014 22:40:07 -0400

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

Message #11 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 17453 <at> debbugs.gnu.org
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Sun, 11 May 2014 12:58:42 +0000

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

Message #14 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Sun, 11 May 2014 12:09:38 -0400

> 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

Message #17 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 17453 <at> debbugs.gnu.org
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Sun, 11 May 2014 18:18:32 +0000

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

Message #20 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Sun, 11 May 2014 15:05:17 -0400

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

Message #23 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 17453 <at> debbugs.gnu.org
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Sun, 11 May 2014 20:40:17 +0000

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

Message #26 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Sun, 11 May 2014 17:46:53 -0400

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

Message #29 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 17453 <at> debbugs.gnu.org, emacs-devel <at> gnu.org
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Thu, 29 Oct 2015 23:23:02 +0000

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

Message #32 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: "John Wiegley" <johnw <at> newartisans.com>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 emacs-devel <at> gnu.org
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Sat, 31 Oct 2015 15:35:22 -0700

>>>>> Alan Mackenzie <acm <at> 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

Message #35 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 emacs-devel <emacs-devel <at> gnu.org>
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Sat, 31 Oct 2015 23:13:34 +0000

[Message part 1 (text/plain, inline)]

On 29 Oct 2015 11:23 pm, "Alan Mackenzie" <acm <at> 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?

[Message part 2 (text/html, inline)]

Message #38 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>, 17453 <at> debbugs.gnu.org,
 emacs-devel <at> gnu.org
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Sat, 31 Oct 2015 23:25:38 +0000

Hello, John.

On Sat, Oct 31, 2015 at 03:35:22PM -0700, John Wiegley wrote:
> >>>>> Alan Mackenzie <acm <at> 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).

Message #41 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Artur Malabarba <bruce.connor.am <at> gmail.com>
Cc: 17453 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 emacs-devel <emacs-devel <at> gnu.org>
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Sat, 31 Oct 2015 23:32:25 +0000

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

Message #44 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 emacs-devel <at> gnu.org
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Sun, 01 Nov 2015 01:35:53 +0200

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

Message #47 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: "John Wiegley" <johnw <at> newartisans.com>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 emacs-devel <at> gnu.org
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Sat, 31 Oct 2015 16:41:01 -0700

>>>>> Alan Mackenzie <acm <at> 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

Message #50 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Juri Linkov <juri <at> linkov.net>
Cc: 17453 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 emacs-devel <at> gnu.org
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Sat, 31 Oct 2015 23:56:51 +0000

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

Message #53 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: John Wiegley <johnw <at> newartisans.com>, Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 emacs-devel <at> gnu.org
Subject: RE: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Sat, 31 Oct 2015 17:17:41 -0700 (PDT)

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

Message #56 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>, 17453 <at> debbugs.gnu.org,
 emacs-devel <at> gnu.org
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Sun, 1 Nov 2015 11:59:26 +0000

Hello, John.

On Sat, Oct 31, 2015 at 04:41:01PM -0700, John Wiegley wrote:
> >>>>> Alan Mackenzie <acm <at> 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).

Message #59 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 emacs-devel <emacs-devel <at> gnu.org>
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Sun, 1 Nov 2015 12:20:19 +0000

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.

Message #62 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 emacs-devel <emacs-devel <at> gnu.org>
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Sun, 1 Nov 2015 12:23:52 +0000

2015-11-01 12:20 GMT+00:00 Artur Malabarba <bruce.connor.am <at> 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?

Message #65 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 emacs-devel <at> gnu.org
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Mon, 02 Nov 2015 02:14:47 +0200

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

Message #68 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Juri Linkov <juri <at> linkov.net>
Cc: acm <at> muc.de, 17453 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca,
 emacs-devel <at> gnu.org
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Mon, 02 Nov 2015 05:35:27 +0200

> From: Juri Linkov <juri <at> linkov.net>
> Date: Mon, 02 Nov 2015 02:14:47 +0200
> Cc: 17453 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
> 	emacs-devel <at> 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.

Message #71 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Juri Linkov <juri <at> linkov.net>
Cc: 17453 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 emacs-devel <at> gnu.org
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Mon, 2 Nov 2015 09:28:53 +0000

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

Message #74 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org, emacs-devel <emacs-devel <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Mon, 2 Nov 2015 11:53:10 +0000

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

Message #77 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org, emacs-devel <emacs-devel <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Mon, 2 Nov 2015 12:14:12 +0000

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

Message #80 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Artur Malabarba <bruce.connor.am <at> gmail.com>
Cc: Juri Linkov <juri <at> linkov.net>, 17453 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, emacs-devel <emacs-devel <at> gnu.org>
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Mon, 2 Nov 2015 12:35:12 +0000

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

Message #83 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Artur Malabarba <bruce.connor.am <at> gmail.com>
Cc: Juri Linkov <juri <at> linkov.net>, 17453 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, emacs-devel <emacs-devel <at> gnu.org>
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Mon, 2 Nov 2015 12:39:49 +0000

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

Message #86 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Alan Mackenzie <acm <at> muc.de>
Cc: Juri Linkov <juri <at> linkov.net>, 17453 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, emacs-devel <emacs-devel <at> gnu.org>
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Mon, 2 Nov 2015 13:10:24 +0000

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

Message #89 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Alan Mackenzie <acm <at> muc.de>
Cc: Juri Linkov <juri <at> linkov.net>, 17453 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, emacs-devel <emacs-devel <at> gnu.org>
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Mon, 2 Nov 2015 14:18:59 +0000

[Message part 1 (text/plain, inline)]

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

[0003-lisp-follow.el-Improve-isearch-compatibility.patch (text/x-patch, attachment)]

[0002-lisp-isearch.el-Bind-a-variable-while-lazy-highlight.patch (text/x-patch, attachment)]

[0001-lisp-isearch.el-Allow-lazy-highlighting-of-all-windo.patch (text/x-patch, attachment)]

Message #92 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Artur Malabarba <bruce.connor.am <at> gmail.com>
Cc: Juri Linkov <juri <at> linkov.net>, 17453 <at> debbugs.gnu.org,
 emacs-devel <emacs-devel <at> gnu.org>
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Mon, 2 Nov 2015 15:44:45 +0000

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

Message #95 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca, bruce.connor.am <at> gmail.com,
 juri <at> linkov.net
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Mon, 02 Nov 2015 17:46:34 +0200

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

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

Message #98 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Eli Zaretskii <eliz <at> gnu.org>, Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca, bruce.connor.am <at> gmail.com,
 juri <at> linkov.net
Subject: RE: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Mon, 2 Nov 2015 08:05:32 -0800 (PST)

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

Enfin !

Message #101 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 17453 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca, bruce.connor.am <at> gmail.com,
 juri <at> linkov.net
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Mon, 2 Nov 2015 16:09:17 +0000

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 <at> muc.de>
> > Cc: Juri Linkov <juri <at> linkov.net>, 17453 <at> debbugs.gnu.org,
> > 	Stefan Monnier <monnier <at> iro.umontreal.ca>,
> > 	emacs-devel <emacs-devel <at> 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).

Message #104 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org, Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Mon, 2 Nov 2015 16:26:23 +0000

Hi Alan,

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

Message #107 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: bruce.connor.am <at> gmail.com, Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org, Juri Linkov <juri <at> linkov.net>
Subject: RE: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Mon, 2 Nov 2015 08:35:24 -0800 (PST)

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

Message #110 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org, bruce.connor.am <at> gmail.com, juri <at> linkov.net
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Mon, 02 Nov 2015 19:45:41 +0200

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

Message #113 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca, bruce.connor.am <at> gmail.com,
 juri <at> linkov.net
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Mon, 02 Nov 2015 19:49:43 +0200

> Date: Mon, 2 Nov 2015 16:09:17 +0000
> Cc: bruce.connor.am <at> gmail.com, juri <at> linkov.net, 17453 <at> debbugs.gnu.org,
>   monnier <at> iro.umontreal.ca
> From: Alan Mackenzie <acm <at> 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?

Message #116 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: Alan Mackenzie <acm <at> muc.de>, 17453 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Mon, 2 Nov 2015 19:18:37 +0000

2015-11-02 16:35 GMT+00:00 Drew Adams <drew.adams <at> 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).

Message #119 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: bruce.connor.am <at> gmail.com
Cc: Alan Mackenzie <acm <at> muc.de>, 17453 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: RE: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Mon, 2 Nov 2015 11:28:18 -0800 (PST)

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

Message #122 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: "John Wiegley" <jwiegley <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Alan Mackenzie <acm <at> muc.de>, 17453 <at> debbugs.gnu.org,
 monnier <at> iro.umontreal.ca, bruce.connor.am <at> gmail.com, juri <at> linkov.net
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Mon, 02 Nov 2015 15:35:51 -0500

>>>>> Eli Zaretskii <eliz <at> 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

Message #125 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Artur Malabarba <bruce.connor.am <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 17453 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Mon, 2 Nov 2015 22:09:27 +0000

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

Message #128 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Alan Mackenzie <acm <at> muc.de>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 17453 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Mon, 2 Nov 2015 23:00:21 +0000

[Message part 1 (text/plain, inline)]

On 2 Nov 2015 10:09 pm, "Alan Mackenzie" <acm <at> 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.

[Message part 2 (text/html, inline)]

Message #131 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org, Artur Malabarba <bruce.connor.am <at> gmail.com>,
 emacs-devel <emacs-devel <at> gnu.org>
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Tue, 03 Nov 2015 01:22:03 +0200

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

Message #134 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Artur Malabarba <bruce.connor.am <at> gmail.com>
Cc: Alan Mackenzie <acm <at> muc.de>, 17453 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, emacs-devel <emacs-devel <at> gnu.org>
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Tue, 03 Nov 2015 01:28:40 +0200

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

Message #137 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 emacs-devel <at> gnu.org
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Tue, 03 Nov 2015 01:33:06 +0200

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

Message #140 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: Alan Mackenzie <acm <at> muc.de>, 17453 <at> debbugs.gnu.org,
 bruce.connor.am <at> gmail.com
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Tue, 03 Nov 2015 01:45:54 +0200

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

Message #143 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 17453 <at> debbugs.gnu.org, bruce.connor.am <at> gmail.com, juri <at> linkov.net
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Tue, 3 Nov 2015 08:35:02 +0000

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 <at> gmail.com, juri <at> linkov.net, 17453 <at> debbugs.gnu.org,
> >   monnier <at> iro.umontreal.ca
> > From: Alan Mackenzie <acm <at> 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).

Message #146 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Artur Malabarba <bruce.connor.am <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 17453 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Tue, 3 Nov 2015 09:18:47 +0000

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

Message #149 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Juri Linkov <juri <at> linkov.net>
Cc: 17453 <at> debbugs.gnu.org, Artur Malabarba <bruce.connor.am <at> gmail.com>,
 emacs-devel <emacs-devel <at> gnu.org>
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Tue, 3 Nov 2015 12:31:16 +0000

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

Message #152 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org, bruce.connor.am <at> gmail.com, juri <at> linkov.net
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Tue, 03 Nov 2015 17:49:17 +0200

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

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

Message #155 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Alan Mackenzie <acm <at> muc.de>, 17453 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Tue, 3 Nov 2015 16:18:45 +0000

2015-11-03 15:49 GMT+00:00 Eli Zaretskii <eliz <at> 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.

Message #158 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 17453 <at> debbugs.gnu.org, bruce.connor.am <at> gmail.com, juri <at> linkov.net
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Tue, 3 Nov 2015 16:39:55 +0000

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 <at> muc.de>
> > Cc: 17453 <at> debbugs.gnu.org, Artur Malabarba <bruce.connor.am <at> gmail.com>,
> > 	emacs-devel <emacs-devel <at> 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).

Message #161 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Artur Malabarba <bruce.connor.am <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 17453 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Tue, 3 Nov 2015 22:11:31 +0000

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

Message #164 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Alan Mackenzie <acm <at> muc.de>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 17453 <at> debbugs.gnu.org,
 Artur Malabarba <bruce.connor.am <at> gmail.com>
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Wed, 04 Nov 2015 02:28:08 +0200

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

Message #167 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Juri Linkov <juri <at> linkov.net>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 17453 <at> debbugs.gnu.org,
 Artur Malabarba <bruce.connor.am <at> gmail.com>
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Wed, 4 Nov 2015 09:01:06 +0000

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

Message #170 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Alan Mackenzie <acm <at> muc.de>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 17453 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Wed, 4 Nov 2015 10:17:09 +0000

2015-11-04 9:01 GMT+00:00 Alan Mackenzie <acm <at> 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

Message #173 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Artur Malabarba <bruce.connor.am <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 17453 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Thu, 5 Nov 2015 12:38:27 +0000

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

Message #176 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Artur Malabarba <bruce.connor.am <at> gmail.com>
To: Alan Mackenzie <acm <at> muc.de>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 17453 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Thu, 5 Nov 2015 17:13:11 +0000

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

Message #179 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Juri Linkov <juri <at> linkov.net>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 17453 <at> debbugs.gnu.org,
 Artur Malabarba <bruce.connor.am <at> gmail.com>
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Sat, 7 Nov 2015 12:59:31 +0000

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

Message #182 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org, bruce.connor.am <at> gmail.com, juri <at> linkov.net
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Sat, 07 Nov 2015 15:38:31 +0200

> Date: Sat, 7 Nov 2015 12:59:31 +0000
> Cc: Artur Malabarba <bruce.connor.am <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>,
>   17453 <at> debbugs.gnu.org
> From: Alan Mackenzie <acm <at> 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?

Message #185 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 17453 <at> debbugs.gnu.org, bruce.connor.am <at> gmail.com, juri <at> linkov.net
Subject: Re: bug#17453: Isearch doesn't work properly with Follow Mode.
Date: Sun, 8 Nov 2015 10:32:32 +0000

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 <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>,
> >   17453 <at> debbugs.gnu.org
> > From: Alan Mackenzie <acm <at> 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).

Message #188 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org, emacs-devel <emacs-devel <at> gnu.org>
Subject: Re: Framework extending window functions for Follow Mode (etc.).
Date: Mon, 09 Nov 2015 02:50:04 +0200

>> >> I admit I am confused. I thought we had narrowed down three possible
>> >> solutions to this issue (which you just listed over at the bug thread), all
>> >> of which are simpler than this code.
>
>> > Those solutions are to merely one part of the bug, namely C-s wrongly
>> > scrolling a window instead of moving onto the next one.
>
>> > The other parts of #17453 are:
>> > 2: lazy highlighting is confined to one Follow Mode window (I'm a bit
>> >   confused as to the status of this, though);
>
>> This problem is already solved by enabling lazy-highlighting of the whole
>> follow-mode buffer, but I postponed installing that patch to not create merge
>> conflicts with your work in the same functions.
>
> My solution is to lazily highlight just the part visible in windows (but
> in all pertinent windows).  Isn't highlighting all of it going to be a
> bit slow on a large buffer?

It shouldn't be slow with a sufficiently low value of lazy-highlight-max-at-a-time.

>> > 3: With isearch-allow-scroll enabled, it is not possible to scroll point
>> >   to the next or previous Follow Mode window;
>> > , in addition to which I have a fix for ...
>
>> That's because isearch-post-command-hook uses isearch-pre-scroll-point
>> to move it back, so it's possible to nullify isearch-pre-scroll-point
>> in follow-mode, but I see that it leaves the highlighted found string
>> at its old position because the isearch-allow-scroll feature doesn't support
>> finding the next search hit after scrolling, or something like this
>> that would make sense.  IOW, this is a limitation of isearch-allow-scroll.
>
> In my personal copy of Emacs, I've had the isearch scrolling working
> properly with Follow Mode for ~18 months.  It was me that wrote the
> isearch scrolling code in the first place, back in 2003.

Yes, I remember, and appreciate your efforts to develop it further.

> Part 3 of the bug is most assuredly NOT an intrinsic limitation of
> isearch-allow-scroll.  It's caused by the variables w-L1 and w-L-1, the
> bounds for the permissible scrolling range in
> isearch-string-out-of-window, being set to the top and botom of the
> _single_ window.  When these variables are set to the top of bottom of
> the entire FM group of windows, the bug is solved.  This requires my new
> framework, or something like it.

I tried to not use isearch-string-out-of-window/isearch-back-into-window
at all, but I can't get a useful behavior in such situation of scrolling
out of the window with the current search hit.  Could you show how you see
it should work in this case in follow-mode?

>> > 4: With point near the bottom of a Follow Mode window, start an Isearch,
>> >   and repeatedly do M-s C-e, until the highlighted match continues on to
>> >   the next window.  Continue doing M-s C-e until the string in the
>> >   minibuffer expands by a line.  At this point the top of the RH window
>> >   gets spuriously scrolled into the middle of the window, leaving the FM
>> >   windows unsynchronised.
>
>> I see the same behavior even without Isearch.
>
> The buggy behaviour I've described is explicitly and essentially an
> Isearch scenario.  What do you mean by "the same behavior" and what
> sequence of commands generates it?
>
> The solution to problem 4 involves making sure point is at the right
> place at the right time when invoking isearch-message.  I have made
> changes to fix it.

I meant such things as 'M-: (message "x\ny\nz") RET' causes jumping too.

>> Honestly, I see here nothing that requires a new multi-window framework.
>
> I want these bugs to be solved.  What, then, are the alternatives to
> this framework (or something like it)?  Isearch needs information about
> the Follow Mode windows, or it can't work properly.
>
> So far, I've written three solutions for these bugs, as I outlined at
> length in an email to Martin R. today.  The first of these solutions was
> (justifiably) rejected by Stefan because it was a quick and dirty fix,
> and he explicitly requested the new framework that I have now built.
> Eli didn't like the second attempt and explicitly suggested the way for
> my third attempt.  You and Martin dislike this most recent third
> attempt.
>
> It seems to me I've spent more time discussing this bug on the bug list
> and emacs-devel, and reformulating the fix, than actually tracking down
> and fixing the bugs in the first place.  At the moment I feel like I'm
> trying to hack down a wall of constant negativity.  I don't recall
> anybody else saying positively they want this bug fixed, and I certainly
> don't feel I've had much encouragement wrt this bug, in the last few
> days and weeks.
>
> I see Follow Mode as being a critically important component of Emacs,
> the more so since very wide (240 characters and more) screens displaced
> the fairly narrow CRT monitors.  I would like every Emacs user to be
> able to use FM as easily as I can.  Right at the moment there is no
> suitable interface to FM for libraries that require to do their own
> window manipulation.  Such an interface is what Stefan wanted, and it's
> what I want, too.  As of yet, there's been practically no discussion of
> this interface I've written, beyond Eli suggesting the current version
> and John suggesting a name change for some hooks.
>
> So, where do we go from here?  I would like these bugs fixed for 25.1.

The problem is that you are trying to design a new framework,
but not demonstrated yet how it would be useful generally in cases
other than a specific combination of isearch/follow-mode.

Message #191 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Juri Linkov <juri <at> linkov.net>
Cc: 17453 <at> debbugs.gnu.org, emacs-devel <emacs-devel <at> gnu.org>
Subject: Re: Framework extending window functions for Follow Mode (etc.).
Date: Mon, 9 Nov 2015 15:41:24 +0000

Hello, Juri.

On Mon, Nov 09, 2015 at 02:50:04AM +0200, Juri Linkov wrote:
> >> This problem is already solved by enabling lazy-highlighting of the whole
> >> follow-mode buffer, but I postponed installing that patch to not create merge
> >> conflicts with your work in the same functions.

> > My solution is to lazily highlight just the part visible in windows (but
> > in all pertinent windows).  Isn't highlighting all of it going to be a
> > bit slow on a large buffer?

> It shouldn't be slow with a sufficiently low value of lazy-highlight-max-at-a-time.

OK, I see it: it would be slow if it had to go to completion, but any
key/mouse/other event will terminate it.  So the only place it might be a
problem is somewhere where CPU cycles are at a premium.

> > Part 3 of the bug is most assuredly NOT an intrinsic limitation of
> > isearch-allow-scroll.  It's caused by the variables w-L1 and w-L-1, the
> > bounds for the permissible scrolling range in
> > isearch-string-out-of-window, being set to the top and botom of the
> > _single_ window.  When these variables are set to the top of bottom of
> > the entire FM group of windows, the bug is solved.  This requires my new
> > framework, or something like it.

> I tried to not use isearch-string-out-of-window/isearch-back-into-window
> at all, but I can't get a useful behavior in such situation of scrolling
> out of the window with the current search hit.  Could you show how you see
> it should work in this case in follow-mode?

To start with, set

  (global-set-key [next] 'follow-scroll-up)
  (global-set-key [prior] 'follow-scroll-down)
  (setq isearch-allow-scroll t)

.  Then start an Isearch not too close to the start of a buffer with
Follow Mode enabled with at least two windows.  Type something to get a
search match highlighted.  Now <PageUp> and <PageDown> should scroll that
match between Follow Mode windows, the boundaries of that scrolling being
the top of the LH window and the bottom of the RH window.

To make this work properly, the four variables in
isearch-string-out-of-window, w-start, w-end, w-L1, w-L-1, are set to the
positions in the entire group of windows, by setting the proposed
&optional argument GROUP to t in the calls to certain window functions,
e.g.

     (let ((w-start (window-start nil t))
                                      ^

> >> > 4: With point near the bottom of a Follow Mode window, start an Isearch,
> >> >   and repeatedly do M-s C-e, until the highlighted match continues on to
> >> >   the next window.  Continue doing M-s C-e until the string in the
> >> >   minibuffer expands by a line.  At this point the top of the RH window
> >> >   gets spuriously scrolled into the middle of the window, leaving the FM
> >> >   windows unsynchronised.

> >> I see the same behavior even without Isearch.

> > The buggy behaviour I've described is explicitly and essentially an
> > Isearch scenario.  What do you mean by "the same behavior" and what
> > sequence of commands generates it?

> > The solution to problem 4 involves making sure point is at the right
> > place at the right time when invoking isearch-message.  I have made
> > changes to fix it.

> I meant such things as 'M-: (message "x\ny\nz") RET' causes jumping too.

It causes some scrolling of windows to keep them properly aligned when
the echo area expands to three lines.  This is expected and proper.
However, when the echo area goes back to one line, the windows don't
scroll back again, leaving them unaligned.  I raised a bug for this this
morning.  It seems it is a known problem which is tricky to solve.

> >> Honestly, I see here nothing that requires a new multi-window framework.

> > I want these bugs to be solved.  What, then, are the alternatives to
> > this framework (or something like it)?  Isearch needs information about
> > the Follow Mode windows, or it can't work properly.

[ .... ]

> > So, where do we go from here?  I would like these bugs fixed for 25.1.

> The problem is that you are trying to design a new framework,
> but not demonstrated yet how it would be useful generally in cases
> other than a specific combination of isearch/follow-mode.

Any elisp software which manipulates windows will be able to work better
by considering entire FM groups.  I did some grepping: here are some
examples which the new framework could improve:

`kill-ring-save' should blink the cursor on the matching other-end, if
that is visible.  With FM active, and other end being in a different
window, currently it wrongly outputs a message instead.

`window-screen-lines' could be enhanced (optionally) to return the number
of screen lines in the entire group.

`blink-matching-open' could be enhanced to handle FM windows properly.

`ns-scroll-bar-move' (probably) should `set-window-start' for the entire
window group.  This (might) avoid scrolling such that the RH follow
window is displaying empty space.


One point about this new framework is that it is "harmless" in the sense
that anything which worked (or failed to work) prior to it will continue
unchanged unless specifically amended.  It's disadvantage is that it adds
a little to the bulk of things people need to know or to look up in
manuals.

I think Follow Mode should become easier to use, both for users and for
hackers, especially given that wide screens (>200 characters) are now the
norm rather than the exception.  At the moment Follow Mode is
ridiculously difficult to start (manually splitting windows, doing M-x
follow-mode) and even more difficult to hack for - just what interfaces
are currently available for making a package work with FM?  Answer: none
at all.  This new framework is intended to go some way to closing the
gap.

The alternative to a framework is a bugfix which is specific to Isearch.
Even this would require some sort of interface definition and
abstraction.  At a minimum, for "part 3" of the bug, Isearch would need
somehow to obtain the bounds of the group of windows.

The last alternative is a quick and dirty fix where Isearch would just
call Follow Mode functions.  I don't think anybody really wants this.

Would it help if I actually made the source available?  If so, where?  I
don't really think it would be appropriate to dump a patch of this size
on emacs-devel, and the time to commit the changes to master has clearly
not yet arrived.

-- 
Alan Mackenzie (Nuremberg, Germany).

Message #194 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org
Subject: Re: Framework extending window functions for Follow Mode (etc.).
Date: Tue, 10 Nov 2015 02:51:41 +0200

>> I tried to not use isearch-string-out-of-window/isearch-back-into-window
>> at all, but I can't get a useful behavior in such situation of scrolling
>> out of the window with the current search hit.  Could you show how you see
>> it should work in this case in follow-mode?
>
> To start with, set
>
>   (global-set-key [next] 'follow-scroll-up)
>   (global-set-key [prior] 'follow-scroll-down)
>   (setq isearch-allow-scroll t)
>
> .  Then start an Isearch not too close to the start of a buffer with
> Follow Mode enabled with at least two windows.  Type something to get a
> search match highlighted.  Now <PageUp> and <PageDown> should scroll that
> match between Follow Mode windows, the boundaries of that scrolling being
> the top of the LH window and the bottom of the RH window.
>
> To make this work properly, the four variables in
> isearch-string-out-of-window, w-start, w-end, w-L1, w-L-1, are set to the
> positions in the entire group of windows, by setting the proposed
> &optional argument GROUP to t in the calls to certain window functions,
> e.g.
>
>      (let ((w-start (window-start nil t))
>                                       ^

Could you provide the shortest patch to test the behavior that you describe?
For now I tried the following, is this what you want to generalise with
a new framework?

diff --git a/lisp/isearch.el b/lisp/isearch.el
index b762884..3b61505 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -2237,10 +2237,19 @@ (defun isearch-string-out-of-window (isearch-point)
 together with as much of the search string as will fit; the symbol
 `above' if we need to scroll the text downwards; the symbol `below',
 if upwards."
-  (let ((w-start (window-start))
-        (w-end (window-end nil t))
-        (w-L1 (save-excursion (move-to-window-line 1) (point)))
-        (w-L-1 (save-excursion (move-to-window-line -1) (point)))
+  (let ((w-start (window-start (and (fboundp 'follow-all-followers)
+                                    (car (follow-all-followers)))))
+        (w-end (window-end (and (fboundp 'follow-all-followers)
+                                (car (last (follow-all-followers))))
+                           t))
+        (w-L1 (save-excursion
+                (when (fboundp 'follow-all-followers)
+                  (select-window (car (follow-all-followers))))
+                (move-to-window-line 1) (point)))
+        (w-L-1 (save-excursion
+                 (when (fboundp 'follow-all-followers)
+                   (select-window (car (last (follow-all-followers)))))
+                 (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))

>> The problem is that you are trying to design a new framework,
>> but not demonstrated yet how it would be useful generally in cases
>> other than a specific combination of isearch/follow-mode.
>
> Any elisp software which manipulates windows will be able to work better
> by considering entire FM groups.  I did some grepping: here are some
> examples which the new framework could improve:
>
> `kill-ring-save' should blink the cursor on the matching other-end, if
> that is visible.  With FM active, and other end being in a different
> window, currently it wrongly outputs a message instead.
>
> `window-screen-lines' could be enhanced (optionally) to return the number
> of screen lines in the entire group.
>
> `blink-matching-open' could be enhanced to handle FM windows properly.
>
> `ns-scroll-bar-move' (probably) should `set-window-start' for the entire
> window group.  This (might) avoid scrolling such that the RH follow
> window is displaying empty space.
>
>
> One point about this new framework is that it is "harmless" in the sense
> that anything which worked (or failed to work) prior to it will continue
> unchanged unless specifically amended.  It's disadvantage is that it adds
> a little to the bulk of things people need to know or to look up in
> manuals.
>
> I think Follow Mode should become easier to use, both for users and for
> hackers, especially given that wide screens (>200 characters) are now the
> norm rather than the exception.  At the moment Follow Mode is
> ridiculously difficult to start (manually splitting windows, doing M-x
> follow-mode) and even more difficult to hack for - just what interfaces
> are currently available for making a package work with FM?  Answer: none
> at all.  This new framework is intended to go some way to closing the
> gap.
>
> The alternative to a framework is a bugfix which is specific to Isearch.
> Even this would require some sort of interface definition and
> abstraction.  At a minimum, for "part 3" of the bug, Isearch would need
> somehow to obtain the bounds of the group of windows.
>
> The last alternative is a quick and dirty fix where Isearch would just
> call Follow Mode functions.  I don't think anybody really wants this.
>
> Would it help if I actually made the source available?  If so, where?  I
> don't really think it would be appropriate to dump a patch of this size
> on emacs-devel, and the time to commit the changes to master has clearly
> not yet arrived.

You are trying to do everything at once.  To successfully achieve your goals
it would be much more clear for us to see the progress step by step, i.e.
if at first you demonstrated how to fix the co-operation between Isearch and
Follow Mode by a quick and dirty fix like in the patch above then we could see
how well your fixes work, and also what places need generalisation,
and how your new framework would be useful here and for other packages
that might benefit from it.  By such inductive method we could quickly
arrive to a conclusion without much friction.

Message #197 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Juri Linkov <juri <at> linkov.net>
Cc: 17453 <at> debbugs.gnu.org
Subject: Re: Framework extending window functions for Follow Mode (etc.).
Date: Tue, 10 Nov 2015 11:08:24 +0000

Hello, Juri.

On Tue, Nov 10, 2015 at 02:51:41AM +0200, Juri Linkov wrote:
> >> I tried to not use isearch-string-out-of-window/isearch-back-into-window
> >> at all, but I can't get a useful behavior in such situation of scrolling
> >> out of the window with the current search hit.  Could you show how you see
> >> it should work in this case in follow-mode?

> > To start with, set

> >   (global-set-key [next] 'follow-scroll-up)
> >   (global-set-key [prior] 'follow-scroll-down)
> >   (setq isearch-allow-scroll t)

> > .  Then start an Isearch not too close to the start of a buffer with
> > Follow Mode enabled with at least two windows.  Type something to get a
> > search match highlighted.  Now <PageUp> and <PageDown> should scroll that
> > match between Follow Mode windows, the boundaries of that scrolling being
> > the top of the LH window and the bottom of the RH window.

> > To make this work properly, the four variables in
> > isearch-string-out-of-window, w-start, w-end, w-L1, w-L-1, are set to the
> > positions in the entire group of windows, by setting the proposed
> > &optional argument GROUP to t in the calls to certain window functions,
> > e.g.

> >      (let ((w-start (window-start nil t))
> >                                       ^

> Could you provide the shortest patch to test the behavior that you describe?

Can I ask you here to look at the initial patch in the archive for bug
#17453 (see below)?

> For now I tried the following, is this what you want to generalise with
> a new framework?

More or less, yes.

> diff --git a/lisp/isearch.el b/lisp/isearch.el
> index b762884..3b61505 100644
> --- a/lisp/isearch.el
> +++ b/lisp/isearch.el
> @@ -2237,10 +2237,19 @@ (defun isearch-string-out-of-window (isearch-point)
>  together with as much of the search string as will fit; the symbol
>  `above' if we need to scroll the text downwards; the symbol `below',
>  if upwards."
> -  (let ((w-start (window-start))
> -        (w-end (window-end nil t))
> -        (w-L1 (save-excursion (move-to-window-line 1) (point)))
> -        (w-L-1 (save-excursion (move-to-window-line -1) (point)))
> +  (let ((w-start (window-start (and (fboundp 'follow-all-followers)
> +                                    (car (follow-all-followers)))))
> +        (w-end (window-end (and (fboundp 'follow-all-followers)
> +                                (car (last (follow-all-followers))))
> +                           t))
> +        (w-L1 (save-excursion
> +                (when (fboundp 'follow-all-followers)
> +                  (select-window (car (follow-all-followers))))
> +                (move-to-window-line 1) (point)))
> +        (w-L-1 (save-excursion
> +                 (when (fboundp 'follow-all-followers)
> +                   (select-window (car (last (follow-all-followers)))))
> +                 (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))

As a small point, I think you'd want a `save-selected-window' around the
forms which bind w-L1 and w-L-1.

[ .... ]

> > The last alternative is a quick and dirty fix where Isearch would just
> > call Follow Mode functions.  I don't think anybody really wants this.

> > Would it help if I actually made the source available?  If so, where?  I
> > don't really think it would be appropriate to dump a patch of this size
> > on emacs-devel, and the time to commit the changes to master has clearly
> > not yet arrived.

> You are trying to do everything at once.  To successfully achieve your goals
> it would be much more clear for us to see the progress step by step, i.e.
> if at first you demonstrated how to fix the co-operation between Isearch and
> Follow Mode by a quick and dirty fix like in the patch above then we could see
> how well your fixes work, and also what places need generalisation,
> and how your new framework would be useful here and for other packages
> that might benefit from it.

I posted the quick and dirty fix on 2014-05-09 in the opening post for
bug #17453 (the bug which is still current and has around 55 posts).
This post is, naturally, still available on http://debbugs.gnu.org.  I
was encouraged by Stefan instead to formulate the change as a more
general framework, removing the direct access to follow-mode.el from
isearch.el.  I first posted a description of the framework on 2015-10-29,
in the bug #17453 thread.  This was criticised by Eli, and I amended it
substantially in response.

> By such inductive method we could quickly arrive to a conclusion
> without much friction.

I posted the essence of the framework, as it now is, in a (cut down)
patch at the beginning of this thread.  I have complete patches for both
the framework and isearch.el available.  Together, they are really too
big to post on emacs-devel.

-- 
Alan Mackenzie (Nuremberg, Germany).

Message #200 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org
Subject: Re: Framework extending window functions for Follow Mode (etc.).
Date: Wed, 11 Nov 2015 02:12:17 +0200

> Can I ask you here to look at the initial patch in the archive for bug
> #17453 (see below)?

Thanks, I see.

> I posted the essence of the framework, as it now is, in a (cut down)
> patch at the beginning of this thread.  I have complete patches for both
> the framework and isearch.el available.  Together, they are really too
> big to post on emacs-devel.

Could you put them on a branch then?

Message #203 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Juri Linkov <juri <at> linkov.net>
Cc: 17453 <at> debbugs.gnu.org
Subject: Re: Framework extending window functions for Follow Mode (etc.).
Date: Wed, 11 Nov 2015 16:19:14 +0000

Hello, Juri.

On Wed, Nov 11, 2015 at 02:12:17AM +0200, Juri Linkov wrote:
> > Can I ask you here to look at the initial patch in the archive for bug
> > #17453 (see below)?

> Thanks, I see.

> > I posted the essence of the framework, as it now is, in a (cut down)
> > patch at the beginning of this thread.  I have complete patches for both
> > the framework and isearch.el available.  Together, they are really too
> > big to post on emacs-devel.

> Could you put them on a branch then?

I've created the branch scratch/follow in savannah, with both the new
framework and the proposed changes to isearch.el.

-- 
Alan Mackenzie (Nuremberg, Germany).

Message #206 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Alan Mackenzie <acm <at> muc.de>
Cc: martin rudalics <rudalics <at> gmx.at>, 17453 <at> debbugs.gnu.org
Subject: Re: Framework extending window functions for Follow Mode (etc.).
Date: Thu, 12 Nov 2015 02:52:56 +0200

>> > I posted the essence of the framework, as it now is, in a (cut down)
>> > patch at the beginning of this thread.  I have complete patches for both
>> > the framework and isearch.el available.  Together, they are really too
>> > big to post on emacs-devel.
>
>> Could you put them on a branch then?
>
> I've created the branch scratch/follow in savannah, with both the new
> framework and the proposed changes to isearch.el.

Thanks, I'm looking at your changes, and also Cc'ing Martin in the hope
that Martin could review your window-related changes as well.

PS: one immediate question - why do you need a new function isearch-call-message
when at the top of this thread Stefan asked you to replace this code with
(funcall (or isearch-message-function #'isearch-message) ,cqh ,ellip)
and even to change isearch-message-function to default to isearch-message
rather than to nil.

Message #209 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: martin rudalics <rudalics <at> gmx.at>
To: Juri Linkov <juri <at> linkov.net>, Alan Mackenzie <acm <at> muc.de>
Cc: 17453 <at> debbugs.gnu.org
Subject: Re: Framework extending window functions for Follow Mode (etc.).
Date: Thu, 12 Nov 2015 09:22:32 +0100

> Thanks, I'm looking at your changes, and also Cc'ing Martin in the hope
> that Martin could review your window-related changes as well.

Glimpsing on the window-related changes I think I have already said
everything in this thread.  Usurpating the term "group" in the sense
that all windows belonging to a group have to show the same buffer seems
overly restrictive but I don't want to continue the earlier discussion.

martin

Message #212 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: martin rudalics <rudalics <at> gmx.at>
Cc: Alan Mackenzie <acm <at> muc.de>, 17453 <at> debbugs.gnu.org
Subject: Re: Framework extending window functions for Follow Mode (etc.).
Date: Thu, 12 Nov 2015 22:14:22 +0200

>> Thanks, I'm looking at your changes, and also Cc'ing Martin in the hope
>> that Martin could review your window-related changes as well.
>
> Glimpsing on the window-related changes I think I have already said
> everything in this thread.  Usurpating the term "group" in the sense
> that all windows belonging to a group have to show the same buffer seems
> overly restrictive but I don't want to continue the earlier discussion.

I agree that the cleanest way to add a new feature is not to change window
primitives, but to add it on the top of existing window primitives
by defining a new functional layer with a set of functions having a common
name prefix either 'window-group-' or 'windows-' (used by follow.el in some
places).  Here are existing primitives and two variants of new functions:

(window-start &optional WINDOW)
(windows-start &optional WINDOW)
(window-group-start &optional WINDOW)

(set-window-start WINDOW POS &optional NOFORCE)
(set-windows-start WINDOW POS &optional NOFORCE)
(set-window-group-start WINDOW POS &optional NOFORCE)

(move-to-window-line COUNT)
(move-to-windows-line COUNT)
(move-to-window-group-line COUNT)

(pos-visible-in-window-p &optional POS WINDOW PARTIALLY)
(pos-visible-in-windows-p &optional POS WINDOW PARTIALLY)
(pos-visible-in-window-group-p &optional POS WINDOW PARTIALLY)

These new functions could be defined in a new core package with a name like
window-group.el or windows.el, or maybe added to the end of window.el,
and have no follow-specific code.

Then there is no need in a set of functions like window-start-group-function,
because one function should be enough for a package like follow.el
to declare an active group of windows via follow-all-followers,
or packages other than follow.el using window parameters.
Then window-group/windows functions should take care about all
aspects of handling multiple windows.

So e.g. new function follow-window-start will be window-group-start
taking only follow-all-followers from follow.el, etc.

This means that while adapting other packages to multi-window configurations,
instead of adding the 't' arg to core primitives, we need to add 's' (window -> windows)
or '-group' to the names of the existing function calls.

The key point is not touching core primitives until we'll be able to
implement a proper display abstraction for window groups such as proposed
a while ago under the name "framelettes".  This comment in follow.el also
gives a hint in this direction:

;; Almost like the real thing, except when the cursor ends up outside
;; the top or bottom...  In our case however, we end up outside the
;; window and hence we are recentered.  Should we let `recenter' handle
;; the point position we would never leave the selected window.  To do
;; it ourselves we would need to do our own redisplay, which is easier
;; said than done.  (Why didn't I do a real display abstraction from
;; the beginning?)

Message #215 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Juri Linkov <juri <at> linkov.net>
Cc: martin rudalics <rudalics <at> gmx.at>, 17453 <at> debbugs.gnu.org
Subject: Re: Framework extending window functions for Follow Mode (etc.).
Date: Thu, 12 Nov 2015 22:15:24 +0000

Hello, Juri.

On Thu, Nov 12, 2015 at 02:52:56AM +0200, Juri Linkov wrote:
> >> > I posted the essence of the framework, as it now is, in a (cut down)
> >> > patch at the beginning of this thread.  I have complete patches for both
> >> > the framework and isearch.el available.  Together, they are really too
> >> > big to post on emacs-devel.

> >> Could you put them on a branch then?

> > I've created the branch scratch/follow in savannah, with both the new
> > framework and the proposed changes to isearch.el.

> Thanks, I'm looking at your changes, and also Cc'ing Martin in the hope
> that Martin could review your window-related changes as well.

Thanks.

> PS: one immediate question - why do you need a new function isearch-call-message
> when at the top of this thread Stefan asked you to replace this code with
> (funcall (or isearch-message-function #'isearch-message) ,cqh ,ellip)
> and even to change isearch-message-function to default to isearch-message
> rather than to nil.

isearch-call-message is actually a macro.  It's exists because it's
invoked several times from the rest of the code, and is cleaner this way.

Stefan's request just slipped my memory.  It is a relatively minor
stylistic thing, of little consequence.  At the time he requested it, he
had lots of far more important criticisms to make.  But I could change
it now, no problem.

-- 
Alan Mackenzie (Nuremberg, Germany).

Message #218 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Juri Linkov <juri <at> linkov.net>
Cc: martin rudalics <rudalics <at> gmx.at>, 17453 <at> debbugs.gnu.org
Subject: Re: Framework extending window functions for Follow Mode (etc.).
Date: Tue, 17 Nov 2015 22:55:58 +0000

Hello, Juri.

On Thu, Nov 12, 2015 at 10:14:22PM +0200, Juri Linkov wrote:
> >> Thanks, I'm looking at your changes, and also Cc'ing Martin in the hope
> >> that Martin could review your window-related changes as well.

> > Glimpsing on the window-related changes I think I have already said
> > everything in this thread.  Usurpating the term "group" in the sense
> > that all windows belonging to a group have to show the same buffer seems
> > overly restrictive but I don't want to continue the earlier discussion.

> I agree that the cleanest way to add a new feature is not to change window
> primitives, but to add it on the top of existing window primitives
> by defining a new functional layer with a set of functions having a common
> name prefix either 'window-group-' or 'windows-' (used by follow.el in some
> places).

I am coming around to the idea that the new functions are a better way of
implementing this than adding an extra optional parameter to the
primitives.  The reason is, I have a vision that some time in the medium
future, the Follow Mode functionality will come to be implemented
directly in the redisplay engine.  If we add the extra parameter, it
will then be redundant and stick out like a sore thumb.  But with
functions like `window-group-start', these functions could quietly merge
with `window-start', etc., as soon as the display engine stuff is
working.

I have already implemented (though not committed) the extra functional
layer.  The names I used were `window*-start', ....,
`move-to-window*-line'.  Thinking about Anders's comment today that the
"*" could easily get lost, ....

> Here are existing primitives and two variants of new functions:

> (window-start &optional WINDOW)
> (windows-start &optional WINDOW)
> (window-group-start &optional WINDOW)

...., I now think the best name would be the last of these three,
window-group-start.  It is not too long, and the phrase "window_group"
fits in with all the primitives apart from `recenter'.  So we'd need some
different naming ideas, perhaps "recenter-group" or "group-recenter".

> These new functions could be defined in a new core package with a name like
> window-group.el or windows.el, or maybe added to the end of window.el,
> and have no follow-specific code.

I put them into window.el.  It seems like a good place.

> Then there is no need in a set of functions like window-start-group-function,
> because one function should be enough for a package like follow.el
> to declare an active group of windows via follow-all-followers,
> or packages other than follow.el using window parameters.
> Then window-group/windows functions should take care about all
> aspects of handling multiple windows.

We don't want Follow Mode to become too tightly coupled with "its
users".

> So e.g. new function follow-window-start will be window-group-start
> taking only follow-all-followers from follow.el, etc.

> This means that while adapting other packages to multi-window configurations,
> instead of adding the 't' arg to core primitives, we need to add 's' (window -> windows)
> or '-group' to the names of the existing function calls.

I say the '-group' variant.

> The key point is not touching core primitives until we'll be able to
> implement a proper display abstraction for window groups such as proposed
> a while ago under the name "framelettes".  This comment in follow.el also
> gives a hint in this direction:

> ;; Almost like the real thing, except when the cursor ends up outside
> ;; the top or bottom...  In our case however, we end up outside the
> ;; window and hence we are recentered.  Should we let `recenter' handle
> ;; the point position we would never leave the selected window.  To do
> ;; it ourselves we would need to do our own redisplay, which is easier
> ;; said than done.  (Why didn't I do a real display abstraction from
> ;; the beginning?)

Yes.

By the way, have you had the chance to look at the changes I made to
isearch.el in the scratch/follow branch?

-- 
Alan Mackenzie (Nuremberg, Germany).

Message #221 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Alan Mackenzie <acm <at> muc.de>
Cc: martin rudalics <rudalics <at> gmx.at>, 17453 <at> debbugs.gnu.org
Subject: Re: Framework extending window functions for Follow Mode (etc.).
Date: Wed, 18 Nov 2015 02:38:49 +0200

> ...., I now think the best name would be the last of these three,
> window-group-start.  It is not too long, and the phrase "window_group"
> fits in with all the primitives apart from `recenter'.  So we'd need some
> different naming ideas, perhaps "recenter-group" or "group-recenter".

For consistency with other related names it could have the same prefix, e.g.
"window-group-recenter".

> By the way, have you had the chance to look at the changes I made to
> isearch.el in the scratch/follow branch?

One thing in the isearch-related part of your patch that I'm not excited about
is the macro isearch-call-message.  With Stefan's request there is no need in it
since it becomes one-liner: (funcall isearch-message-function).

Another thing that disturbs me is moving lazy-highlighting checks
to a new function isearch-lazy-highlight-maybe-new-loop.

What do you think about moving only window-checking into new function
i.e. only checks for window-start/window-end/etc that need (sit-for 0)
and leaving other checks in isearch-lazy-highlight-new-loop?
Would this help to avoid new problems such as un-highlighting
postponed until the timer fires?

Message #224 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Juri Linkov <juri <at> linkov.net>
Cc: martin rudalics <rudalics <at> gmx.at>, 17453 <at> debbugs.gnu.org
Subject: Re: Framework extending window functions for Follow Mode (etc.).
Date: Wed, 18 Nov 2015 17:58:14 +0000

Hello, Juri.

On Wed, Nov 18, 2015 at 02:38:49AM +0200, Juri Linkov wrote:
> > ...., I now think the best name would be the last of these three,
> > window-group-start.  It is not too long, and the phrase "window_group"
> > fits in with all the primitives apart from `recenter'.  So we'd need some
> > different naming ideas, perhaps "recenter-group" or "group-recenter".

> For consistency with other related names it could have the same prefix, e.g.
> "window-group-recenter".

Or even "recenter-window-group", since "window-group" isn't strictly a
prefix in a lot of the other cases anyway.

> > By the way, have you had the chance to look at the changes I made to
> > isearch.el in the scratch/follow branch?

> One thing in the isearch-related part of your patch that I'm not excited about
> is the macro isearch-call-message.  With Stefan's request there is no need in it
> since it becomes one-liner: (funcall isearch-message-function).

A minor thing, which I will amend this evening.

> Another thing that disturbs me is moving lazy-highlighting checks
> to a new function isearch-lazy-highlight-maybe-new-loop.

What disturbs you about this change in particular?

> What do you think about moving only window-checking into new function
> i.e. only checks for window-start/window-end/etc that need (sit-for 0)
> and leaving other checks in isearch-lazy-highlight-new-loop?

Actually, I think that "(sit-for 0)" isn't needed - I left it in to deal
with isearch-lazy-highlight-initial-delay being set to 0, thinking that
Follow Mode's post-command-hook might "not have had time" to run.  But
surely post-command-hook will be called before checking timers.  I'll
need to look at the source code (probably keyboard.c) to be absolutely
sure of this.

I don't think splitting the checks between
isearch-lazy-highlight-new-loop (in the command loop) and a function
triggered by a timer is a good idea.  I tried it out, and it kind of
jars when the lazy highlighting sometimes stays 0.25s, sometimes goes
instantly.  How about maybe ...

How about maybe putting the test for new highlighting in Isearch's
post-command-hook (and using the APPEND argument of `add-hook' to make
sure Isearch's post-hook-function comes after Follow Modes's)?  That
way, all the lazy h. would be removed immediately after the command, as
at present.

> Would this help to avoid new problems such as un-highlighting
> postponed until the timer fires?

Is this actually a problem?  I played with Emacs a little with that
un-highlighting strategy, and didn't find it any more disturbing than
the 0.25s gap between old highlighting going and new highlighting
arriving.

-- 
Alan Mackenzie (Nuremberg, Germany).

Message #227 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Juri Linkov <juri <at> linkov.net>
Cc: martin rudalics <rudalics <at> gmx.at>, 17453 <at> debbugs.gnu.org
Subject: Re: Framework extending window functions for Follow Mode (etc.).
Date: Wed, 18 Nov 2015 21:28:22 +0000

Hello again, Juri.

On Wed, Nov 18, 2015 at 05:58:14PM +0000, Alan Mackenzie wrote:
> > One thing in the isearch-related part of your patch that I'm not excited about
> > is the macro isearch-call-message.  With Stefan's request there is no need in it
> > since it becomes one-liner: (funcall isearch-message-function).

> A minor thing, which I will amend this evening.

DONE.  I have committed the patch to the scratch/follow branch of the
repository.

-- 
Alan Mackenzie (Nuremberg, Germany).

Message #230 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Alan Mackenzie <acm <at> muc.de>
Cc: martin rudalics <rudalics <at> gmx.at>, 17453 <at> debbugs.gnu.org
Subject: Re: Framework extending window functions for Follow Mode (etc.).
Date: Thu, 19 Nov 2015 02:45:43 +0200

>> For consistency with other related names it could have the same prefix, e.g.
>> "window-group-recenter".
>
> Or even "recenter-window-group", since "window-group" isn't strictly a
> prefix in a lot of the other cases anyway.

Maybe "recenter-window-group" in the sense of "recenter a window in the
window group" as opposed to "window-group-recenter" in the sense of
"recenter the whole window group"?

>> Another thing that disturbs me is moving lazy-highlighting checks
>> to a new function isearch-lazy-highlight-maybe-new-loop.
>
> What disturbs you about this change in particular?

The same consequences you mentioned earlier: various time periods
of lazy-highlighting staying active.

>> What do you think about moving only window-checking into new function
>> i.e. only checks for window-start/window-end/etc that need (sit-for 0)
>> and leaving other checks in isearch-lazy-highlight-new-loop?
>
> Actually, I think that "(sit-for 0)" isn't needed - I left it in to deal
> with isearch-lazy-highlight-initial-delay being set to 0, thinking that
> Follow Mode's post-command-hook might "not have had time" to run.  But
> surely post-command-hook will be called before checking timers.  I'll
> need to look at the source code (probably keyboard.c) to be absolutely
> sure of this.
>
> I don't think splitting the checks between
> isearch-lazy-highlight-new-loop (in the command loop) and a function
> triggered by a timer is a good idea.  I tried it out, and it kind of
> jars when the lazy highlighting sometimes stays 0.25s, sometimes goes
> instantly.  How about maybe ...
>
> How about maybe putting the test for new highlighting in Isearch's
> post-command-hook (and using the APPEND argument of `add-hook' to make
> sure Isearch's post-hook-function comes after Follow Modes's)?  That
> way, all the lazy h. would be removed immediately after the command, as
> at present.

You could try to append a new follow+isearch specific hook
to post-command-hook in follow.el, if this helps.

>> Would this help to avoid new problems such as un-highlighting
>> postponed until the timer fires?
>
> Is this actually a problem?  I played with Emacs a little with that
> un-highlighting strategy, and didn't find it any more disturbing than
> the 0.25s gap between old highlighting going and new highlighting
> arriving.

We need to test the same in all modes that use lazy-highlighting too.

Message #233 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Juri Linkov <juri <at> linkov.net>
Cc: martin rudalics <rudalics <at> gmx.at>, 17453 <at> debbugs.gnu.org
Subject: Re: Framework extending window functions for Follow Mode (etc.).
Date: Wed, 25 Nov 2015 19:33:01 +0000

Hello, Juri

It's been nearly a week since your last email.  In that time, I tried
one of the approaches (see below) that worked just fine.... except it
wouldn't work with replace.el.  I got discouraged by this, and moved
onto other things for a while.  Now I'm back.

On Thu, Nov 19, 2015 at 02:45:43AM +0200, Juri Linkov wrote:

> >> Another thing that disturbs me is moving lazy-highlighting checks
> >> to a new function isearch-lazy-highlight-maybe-new-loop.

> > What disturbs you about this change in particular?

> The same consequences you mentioned earlier: various time periods
> of lazy-highlighting staying active.

OK.

> >> What do you think about moving only window-checking into new function
> >> i.e. only checks for window-start/window-end/etc that need (sit-for 0)
> >> and leaving other checks in isearch-lazy-highlight-new-loop?

> > Actually, I think that "(sit-for 0)" isn't needed - I left it in to deal
> > with isearch-lazy-highlight-initial-delay being set to 0, thinking that
> > Follow Mode's post-command-hook might "not have had time" to run.  But
> > surely post-command-hook will be called before checking timers.  I'll
> > need to look at the source code (probably keyboard.c) to be absolutely
> > sure of this.

The "(sit-for 0)" is absolutely not needed: post-command-hook is indeed
always invoked before Emacs checks for timer expiry; I tried this out.

> > I don't think splitting the checks between
> > isearch-lazy-highlight-new-loop (in the command loop) and a function
> > triggered by a timer is a good idea.  I tried it out, and it kind of
> > jars when the lazy highlighting sometimes stays 0.25s, sometimes goes
> > instantly.  How about maybe ...

> > How about maybe putting the test for new highlighting in Isearch's
> > post-command-hook (and using the APPEND argument of `add-hook' to make
> > sure Isearch's post-hook-function comes after Follow Modes's)?  That
> > way, all the lazy h. would be removed immediately after the command, as
> > at present.

This is precisely what I tried.  It works very well in Isearch.
However, the problem is in replace.el (the replace functionality, not
`occur'): `query-replace', instead of using the Command Loop like
Isearch does, implements its own command loop.  This seems suboptimal:
it doesn't invoke post-command-hook (or pre-command-hook) until the
entire replace session is over.

This means the use of `query-replace' whilst Follow Mode is enabled is
not going to work properly, without some radical change in replace.el.

Probably the smallest change would be to invoke new hooks
`pre-replace-command-hook' and `post-replace-command-hook' from
`query-replace''s command loop.

A more satisfying change would be to get rid of `perform-replace' and
use Emacs's command loop the way Isearch does.  This would probably not
be all that difficult.  Do you know if there's any special reason
`query-replace' implements its own command loop?

What do you think?

[ .... ]

> We need to test the same in all modes that use lazy-highlighting too.

Yes.  That's a problem, right now.

-- 
Alan Mackenzie (Nuremberg, Germany).

Message #236 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Alan Mackenzie <acm <at> muc.de>
Cc: martin rudalics <rudalics <at> gmx.at>, 17453 <at> debbugs.gnu.org
Subject: Re: Framework extending window functions for Follow Mode (etc.).
Date: Fri, 27 Nov 2015 01:03:43 +0200

> This means the use of `query-replace' whilst Follow Mode is enabled is
> not going to work properly, without some radical change in replace.el.
>
> Probably the smallest change would be to invoke new hooks
> `pre-replace-command-hook' and `post-replace-command-hook' from
> `query-replace''s command loop.
>
> A more satisfying change would be to get rid of `perform-replace' and
> use Emacs's command loop the way Isearch does.  This would probably not
> be all that difficult.  Do you know if there's any special reason
> `query-replace' implements its own command loop?

The patch in bug#20430 awaits the possibility of helping to fix this
problem.  It adds a new hook replace-update-post-hook that is like
its isearch counterpart hook isearch-update-post-hook is the right way
to handle display updates like syncing follow windows, etc.

Together with changing the order of calling isearch-update-post-hook
and isearch-lazy-highlight-new-loop in isearch-update, adding
follow-post-command-hook to isearch-update-post-hook, and adding
follow-post-command-hook to replace-update-post-hook to handle
follow-mode in query-replace will comprise the least radical change
just before the next release.

Do you see a shortcoming of this course of action?

Message #239 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Juri Linkov <juri <at> linkov.net>
Cc: martin rudalics <rudalics <at> gmx.at>, 17453 <at> debbugs.gnu.org
Subject: Re: Framework extending window functions for Follow Mode (etc.).
Date: Mon, 30 Nov 2015 20:37:23 +0000

Hello, Juri.

On Fri, Nov 27, 2015 at 01:03:43AM +0200, Juri Linkov wrote:
> > This means the use of `query-replace' whilst Follow Mode is enabled is
> > not going to work properly, without some radical change in replace.el.

> > Probably the smallest change would be to invoke new hooks
> > `pre-replace-command-hook' and `post-replace-command-hook' from
> > `query-replace''s command loop.

> > A more satisfying change would be to get rid of `perform-replace' and
> > use Emacs's command loop the way Isearch does.  This would probably not
> > be all that difficult.  Do you know if there's any special reason
> > `query-replace' implements its own command loop?

> The patch in bug#20430 awaits the possibility of helping to fix this
> problem.  It adds a new hook replace-update-post-hook that is like
> its isearch counterpart hook isearch-update-post-hook is the right way
> to handle display updates like syncing follow windows, etc.

Does this patch exist, yet?

It bothers me a little that we might be adding hook after hook into
Emacs, each one for a single special purpose.

Would it not perhaps be better to call `isearch-update-post-hook' also
from `perform-replace', since that would be more economical with hooks;
the meaning of the hook invocation would be "the same" in Isearch and
`perform-replace' - "hook called after having moved to the next match".

> Together with changing the order of calling isearch-update-post-hook
> and isearch-lazy-highlight-new-loop in isearch-update, adding
> follow-post-command-hook to isearch-update-post-hook, and adding
> follow-post-command-hook to replace-update-post-hook to handle
> follow-mode in query-replace will comprise the least radical change
> just before the next release.

This sounds like a good idea.  Though, again, I think calling
isearch-update-post-hook from `query-replace' would be better than
implementing a new hook.

> Do you see a shortcoming of this course of action?

Only that it is working around the problems in replace.el rather than
fixing them.  But to fix them properly would mean a radical redesign of
`perform-replace', or superseding it altogether, which is probably best
postponed until 25.2 or later.  I still think `post-command-hook' is the
best hook for us to use - but it isn't called from `query-replace'.

Would it still be possible to mark `isearch-update-post-hook' as "for
internal use only", so that we could get rid of it later?

-- 
Alan Mackenzie (Nuremberg, Germany).

Message #242 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Alan Mackenzie <acm <at> muc.de>
Cc: martin rudalics <rudalics <at> gmx.at>, 17453 <at> debbugs.gnu.org
Subject: Re: Framework extending window functions for Follow Mode (etc.).
Date: Tue, 01 Dec 2015 02:07:31 +0200

>> The patch in bug#20430 awaits the possibility of helping to fix this
>> problem.  It adds a new hook replace-update-post-hook that is like
>> its isearch counterpart hook isearch-update-post-hook is the right way
>> to handle display updates like syncing follow windows, etc.
>
> Does this patch exist, yet?

Yes, you can find the patch in http://debbugs.gnu.org/20430#8

> It bothers me a little that we might be adding hook after hook into
> Emacs, each one for a single special purpose.

Fortunately, a new hook is not for a single special purpose -
it's a general type of hook, requested for different user needs.

> Would it not perhaps be better to call `isearch-update-post-hook' also
> from `perform-replace', since that would be more economical with hooks;
> the meaning of the hook invocation would be "the same" in Isearch and
> `perform-replace' - "hook called after having moved to the next match".

Logically, it makes sense to reuse isearch hooks in query-replace
since query-replace searches for matches like isearch does, but
practically users might have such customizations in .emacs that
would break query-replace in unpredictable ways.  This is why
a separate query-replace hook would be much safer.

I see no problem for follow-mode to add follow-post-command-hook
to both hooks.

>> Together with changing the order of calling isearch-update-post-hook
>> and isearch-lazy-highlight-new-loop in isearch-update, adding
>> follow-post-command-hook to isearch-update-post-hook, and adding
>> follow-post-command-hook to replace-update-post-hook to handle
>> follow-mode in query-replace will comprise the least radical change
>> just before the next release.
>
> This sounds like a good idea.  Though, again, I think calling
> isearch-update-post-hook from `query-replace' would be better than
> implementing a new hook.

Adding a new hook is just a one-liner, but we have to find the right place
in perform-replace to call it.  I think replace-update-pre-hook should be
called before (read-event), and replace-update-post-hook after (read-event).
I'm not yet sure which one is needed for follow-mode to sync windows?

> Would it still be possible to mark `isearch-update-post-hook' as "for
> internal use only", so that we could get rid of it later?

isearch-update-post-hook is a first-class hook added 5 years ago,
so there is no need to remove it.

Message #245 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Juri Linkov <juri <at> linkov.net>
Cc: martin rudalics <rudalics <at> gmx.at>, 17453 <at> debbugs.gnu.org
Subject: Re: Framework extending window functions for Follow Mode (etc.).
Date: Sat, 5 Dec 2015 16:40:32 +0000

Hello, Juri.

On Tue, Dec 01, 2015 at 02:07:31AM +0200, Juri Linkov wrote:
> >> The patch in bug#20430 awaits the possibility of helping to fix this
> >> problem.  It adds a new hook replace-update-post-hook that is like
> >> its isearch counterpart hook isearch-update-post-hook is the right way
> >> to handle display updates like syncing follow windows, etc.

> > Does this patch exist, yet?

> Yes, you can find the patch in http://debbugs.gnu.org/20430#8

OK, I've got it.

> > It bothers me a little that we might be adding hook after hook into
> > Emacs, each one for a single special purpose.

> Fortunately, a new hook is not for a single special purpose -
> it's a general type of hook, requested for different user needs.

I'll stop arguing the philosophy right now (and take it up again a bit
lower down).  ;-)

> > Would it not perhaps be better to call `isearch-update-post-hook' also
> > from `perform-replace', since that would be more economical with hooks;
> > the meaning of the hook invocation would be "the same" in Isearch and
> > `perform-replace' - "hook called after having moved to the next match".

> Logically, it makes sense to reuse isearch hooks in query-replace
> since query-replace searches for matches like isearch does, but
> practically users might have such customizations in .emacs that
> would break query-replace in unpredictable ways.  This is why
> a separate query-replace hook would be much safer.

> I see no problem for follow-mode to add follow-post-command-hook
> to both hooks.

Because this ties Follow Mode to implementation details of isearch.el,
replace.el, and ispell.el.  It is plain ugly for Follow Mode to have to
add-hook an obscure hook for every package that attempts lazy
highlighting.

    (add-hook 'post-command-hook follow-post-command-hook nil t)

should be the only pertinent 'add-hook necessary in follow-mode (and it
would be if isearch.el were the only library to adpat for - I had it
working at one point).  It's not, because replace.el and ispell.el both
attempt to implement their own command loops rather than using Emacs's
standard one.  These design decisions were Bad.

> >> Together with changing the order of calling isearch-update-post-hook
> >> and isearch-lazy-highlight-new-loop in isearch-update, adding
> >> follow-post-command-hook to isearch-update-post-hook, and adding
> >> follow-post-command-hook to replace-update-post-hook to handle
> >> follow-mode in query-replace will comprise the least radical change
> >> just before the next release.

> > This sounds like a good idea.  Though, again, I think calling
> > isearch-update-post-hook from `query-replace' would be better than
> > implementing a new hook.

> Adding a new hook is just a one-liner, but we have to find the right place
> in perform-replace to call it.  I think replace-update-pre-hook should be
> called before (read-event), and replace-update-post-hook after (read-event).
> I'm not yet sure which one is needed for follow-mode to sync windows?

At the moment there is only replace-update-post-hook.  It is called after
point has been moved, but before i-l-highlight-new-loop is called.  I had
to move it slightly from where your patch had put it.

> > Would it still be possible to mark `isearch-update-post-hook' as "for
> > internal use only", so that we could get rid of it later?

> isearch-update-post-hook is a first-class hook added 5 years ago,
> so there is no need to remove it.

Sorry, I made a typo there.  I really meant replace-update-post-hook.
Can we somehow keep this "internal use only", so that we are not bound
somehow to keep supporting it should the `query-replace' command loop be
reformulated (as believe it should, ASAP)?  The same applies to
ispell-update-post-hook, which I've been forced to introduce into
ispell.el for the same reason.

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

Anyhow, here's a status update with where I am on making isearch.el and
follow.el work together:

(i) Yesterday I rebased the scratch/follow branch on the emacs-25 branch.
(ii) I haven't yet replaced the GROUP parameter in the windows primitives
  with (e.g.) `window-group-start'.
(iii) isearch.el now appears to work properly.  For this, I had to swap
  the order of invocation of isearch-update-post-hook and i-l-h-new-loop,
  like you said.  I restored i-l-h-new-loop pretty much to the way it was
  prior to my experimentations.
(iv) replace.el now appears to work properly.
(v) ispell.el is more troublesome.  See bug #22097.  I have a problem
  with ispell putting its *Choices* window at the top of the left hand
  Follow Mode window.  Because of FM's sorting algorithm, this causes the
  two windows to be logically swapped, leading to confusing results.  I
  think the neatest solution would be to put *Choices* at the top of the
  rightmost window, preventing this.

Here's a diff of my current state, based off of the rebased
scratch/follow branch mentioned above in (i):



diff --git a/.gitignore b/.gitignore
index 34b0c02..5ef5a5c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -261,6 +261,10 @@ ChangeLog
 [0-9]*.txt
 .dir-locals?.el
 /vc-dwim-log-*
+*.diff
+
+.gitattributes
+*.acm
 
 # Built by 'make install'.
 etc/emacs.tmpdesktop
diff --git a/lisp/follow.el b/lisp/follow.el
index 2cbf0f2..609b29f 100644
--- a/lisp/follow.el
+++ b/lisp/follow.el
@@ -423,6 +423,9 @@ follow-mode
 	(add-hook 'post-command-hook 'follow-post-command-hook t)
 	(add-hook 'window-size-change-functions 'follow-window-size-change t)
         (add-hook 'after-change-functions 'follow-after-change nil t)
+        (add-hook 'isearch-update-post-hook 'follow-post-command-hook nil t)
+        (add-hook 'replace-update-post-hook 'follow-post-command-hook nil t)
+        (add-hook 'ispell-update-post-hook 'follow-post-command-hook nil t)
 
         (setq window-start-group-function 'follow-window-start)
         (setq window-end-group-function 'follow-window-end)
@@ -431,8 +434,7 @@ follow-mode
         (setq pos-visible-in-window-p-group-function
               'follow-pos-visible-in-window-p)
         (setq selected-window-group-function 'follow-all-followers)
-        (setq move-to-window-line-group-function 'follow-move-to-window-line)
-        (setq sit*-for-function 'follow-sit-for))
+        (setq move-to-window-line-group-function 'follow-move-to-window-line))
 
     ;; Remove globally-installed hook functions only if there is no
     ;; other Follow mode buffer.
@@ -445,7 +447,6 @@ follow-mode
 	(remove-hook 'post-command-hook 'follow-post-command-hook)
 	(remove-hook 'window-size-change-functions 'follow-window-size-change)))
 
-    (kill-local-variable 'sit*-for-function)
     (kill-local-variable 'move-to-window-line-group-function)
     (kill-local-variable 'selected-window-group-function)
     (kill-local-variable 'pos-visible-in-window-p-group-function)
@@ -454,6 +455,9 @@ follow-mode
     (kill-local-variable 'window-end-group-function)
     (kill-local-variable 'window-start-group-function)
 
+    (remove-hook 'ispell-update-post-hook 'follow-post-command-hook t)
+    (remove-hook 'replace-update-post-hook 'follow-post-command-hook t)
+    (remove-hook 'isearch-update-post-hook 'follow-post-command-hook t)
     (remove-hook 'after-change-functions 'follow-after-change t)
     (remove-hook 'compilation-filter-hook 'follow-align-compilation-windows t)))
 
diff --git a/lisp/isearch.el b/lisp/isearch.el
index 12ded02..e43d860 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -1018,12 +1018,12 @@ 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.
@@ -3068,21 +3068,7 @@ 'isearch-lazy-highlight-cleanup
                                 "22.1")
 
 (defun isearch-lazy-highlight-new-loop (&optional beg end)
-  "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.  Do not start
-the loop when we are executing a keyboard macro."
-  (setq isearch-lazy-highlight-start-limit beg
-        isearch-lazy-highlight-end-limit end)
-  (when (null executing-kbd-macro)
-    (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 any previous `lazy-highlight' loop and begin a new one.
+  "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
@@ -3118,6 +3104,8 @@ isearch-lazy-highlight-maybe-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-window       (selected-window)
           isearch-lazy-highlight-window-group (selected-window-group)
 	  isearch-lazy-highlight-window-start (window-start nil t)
@@ -3140,7 +3128,9 @@ isearch-lazy-highlight-maybe-new-loop
 	  isearch-lazy-highlight-regexp-function  isearch-regexp-function
 	  isearch-lazy-highlight-forward      isearch-forward)
     (unless (equal isearch-string "")
-      (isearch-lazy-highlight-update))))
+      (setq isearch-lazy-highlight-timer
+            (run-with-idle-timer lazy-highlight-initial-delay nil
+                                 'isearch-lazy-highlight-update)))))
 
 (defun isearch-lazy-highlight-search ()
   "Search ahead for the next or previous match, for lazy highlighting.
diff --git a/lisp/replace.el b/lisp/replace.el
index 54b3a71..d48f4f3 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -2011,6 +2011,9 @@ replace-match-maybe-edit
   (when backward (goto-char (nth 0 match-data)))
   noedit)
 
+(defvar replace-update-post-hook nil
+  "Function(s) to call after query-replace has found a match in the buffer.")
+
 (defvar replace-search-function nil
   "Function to use when searching for strings to replace.
 It is used by `query-replace' and `replace-string', and is called
@@ -2264,7 +2267,7 @@ perform-replace
 		(and nonempty-match
 		     (or (not regexp-flag)
 			 (and (if backward
-				  (looking-back search-string)
+				  (looking-back search-string nil)
 				(looking-at search-string))
 			      (let ((match (match-data)))
 				(and (/= (nth 0 match) (nth 1 match))
@@ -2318,7 +2321,8 @@ perform-replace
 		;; `real-match-data'.
 		(while (not done)
 		  (set-match-data real-match-data)
-		  (replace-highlight
+                  (run-hooks 'replace-update-post-hook) ; Before `replace-highlight'.
+                  (replace-highlight
 		   (match-beginning 0) (match-end 0)
 		   start end search-string
 		   regexp-flag delimited-flag case-fold-search backward)
diff --git a/lisp/textmodes/ispell.el b/lisp/textmodes/ispell.el
index fe27f0f..fae0549 100644
--- a/lisp/textmodes/ispell.el
+++ b/lisp/textmodes/ispell.el
@@ -2248,6 +2248,11 @@ ispell-pdict-save
   (setq ispell-pdict-modified-p nil))
 
 
+(defvar ispell-update-post-hook nil
+  "A normal hook invoked from the ispell command loop.
+It is called once per iteration, before displaying a prompt to
+the user.")
+
 (defun ispell-command-loop (miss guess word start end)
   "Display possible corrections from list MISS.
 GUESS lists possibly valid affix construction of WORD.
@@ -2315,8 +2320,10 @@ ispell-command-loop
 	      count (ispell-int-char (1+ count))))
       (setq count (ispell-int-char (- count ?0 skipped))))
 
+    (run-hooks 'ispell-update-post-hook)
+
     ;; ensure word is visible
-    (if (not (pos-visible-in-window-p end))
+    (if (not (pos-visible-in-window-p end nil nil t))
 	(sit-for 0))
 
     ;; Display choices for misspelled word.


-- 
Alan Mackenzie (Nuremberg, Germany).

Message #248 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Alan Mackenzie <acm <at> muc.de>
Cc: martin rudalics <rudalics <at> gmx.at>, 17453 <at> debbugs.gnu.org
Subject: Re: Framework extending window functions for Follow Mode (etc.).
Date: Sun, 06 Dec 2015 01:06:05 +0200

>> I see no problem for follow-mode to add follow-post-command-hook
>> to both hooks.
>
> Because this ties Follow Mode to implementation details of isearch.el,
> replace.el, and ispell.el.  It is plain ugly for Follow Mode to have to
> add-hook an obscure hook for every package that attempts lazy
> highlighting.

A hook is needed not only for lazy highlighting (in its current state),
but also for every package that doesn't use the standard command loop.

>> Adding a new hook is just a one-liner, but we have to find the right place
>> in perform-replace to call it.  I think replace-update-pre-hook should be
>> called before (read-event), and replace-update-post-hook after (read-event).
>> I'm not yet sure which one is needed for follow-mode to sync windows?
>
> At the moment there is only replace-update-post-hook.  It is called after
> point has been moved, but before i-l-highlight-new-loop is called.  I had
> to move it slightly from where your patch had put it.

Yes, you've moved it to a better place.

> Sorry, I made a typo there.  I really meant replace-update-post-hook.
> Can we somehow keep this "internal use only", so that we are not bound
> somehow to keep supporting it should the `query-replace' command loop be
> reformulated (as believe it should, ASAP)?  The same applies to
> ispell-update-post-hook, which I've been forced to introduce into
> ispell.el for the same reason.

isearch-update-post-hook, replace-update-post-hook, ispell-update-post-hook
are not just a hack, they will stay as useful hooks even after we'll
rewrite query-replace/ispell to use the standard command loop.  These hooks
are for convenience, for the users to be able to set in ~/.emacs, e.g.:

  (add-hook 'isearch-update-post-hook 'recenter)
  (add-hook 'replace-update-post-hook 'recenter)

How would you do the same without these hooks, using only post-command-hook?

> #########################################################################
>
> Anyhow, here's a status update with where I am on making isearch.el and
> follow.el work together:
>
> (i) Yesterday I rebased the scratch/follow branch on the emacs-25 branch.
> (ii) I haven't yet replaced the GROUP parameter in the windows primitives
>   with (e.g.) `window-group-start'.
> (iii) isearch.el now appears to work properly.  For this, I had to swap
>   the order of invocation of isearch-update-post-hook and i-l-h-new-loop,
>   like you said.  I restored i-l-h-new-loop pretty much to the way it was
>   prior to my experimentations.
> (iv) replace.el now appears to work properly.
> (v) ispell.el is more troublesome.  See bug #22097.  I have a problem
>   with ispell putting its *Choices* window at the top of the left hand
>   Follow Mode window.  Because of FM's sorting algorithm, this causes the
>   two windows to be logically swapped, leading to confusing results.  I
>   think the neatest solution would be to put *Choices* at the top of the
>   rightmost window, preventing this.

I believe bug#22097 is easy to fix for lazy highlighting, but what you
described above is more troublesome, and might require adding support for
window groups to ispell.el (like you're adding it to isearch.el).

Message #251 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Juri Linkov <juri <at> linkov.net>
Cc: martin rudalics <rudalics <at> gmx.at>, 17453 <at> debbugs.gnu.org
Subject: Re: Framework extending window functions for Follow Mode (etc.).
Date: Mon, 7 Dec 2015 19:15:40 +0000

Hello, Juri.

On Sun, Dec 06, 2015 at 01:06:05AM +0200, Juri Linkov wrote:
> >> I see no problem for follow-mode to add follow-post-command-hook
> >> to both hooks.

> > Because this ties Follow Mode to implementation details of isearch.el,
> > replace.el, and ispell.el.  It is plain ugly for Follow Mode to have to
> > add-hook an obscure hook for every package that attempts lazy
> > highlighting.

> A hook is needed not only for lazy highlighting (in its current state),
> but also for every package that doesn't use the standard command loop.

Yes, I think we're in violent agreement here.

[ .... ]

> > Sorry, I made a typo there.  I really meant replace-update-post-hook.
> > Can we somehow keep this "internal use only", so that we are not
> > bound somehow to keep supporting it should the `query-replace'
> > command loop be reformulated (as believe it should, ASAP)?  The same
> > applies to ispell-update-post-hook, which I've been forced to
> > introduce into ispell.el for the same reason.

> isearch-update-post-hook, replace-update-post-hook, ispell-update-post-hook
> are not just a hack, they will stay as useful hooks even after we'll
> rewrite query-replace/ispell to use the standard command loop.  These hooks
> are for convenience, for the users to be able to set in ~/.emacs, e.g.:

>   (add-hook 'isearch-update-post-hook 'recenter)
>   (add-hook 'replace-update-post-hook 'recenter)

First comment: this sort of thing will wreck Follow Mode, scrolling text
in the current window rather than point moving forward to the next
window.

> How would you do the same without these hooks, using only post-command-hook?

By putting #'recenter into `post-command-hook'?  This should work for
Isearch, and would work for Replace and Ispell if these two used the
Emacs command loop.

> > #########################################################################

> > Anyhow, here's a status update with where I am on making isearch.el and
> > follow.el work together:

> > (i) Yesterday I rebased the scratch/follow branch on the emacs-25 branch.
> > (ii) I haven't yet replaced the GROUP parameter in the windows primitives
> >   with (e.g.) `window-group-start'.
> > (iii) isearch.el now appears to work properly.  For this, I had to swap
> >   the order of invocation of isearch-update-post-hook and i-l-h-new-loop,
> >   like you said.  I restored i-l-h-new-loop pretty much to the way it was
> >   prior to my experimentations.
> > (iv) replace.el now appears to work properly.
> > (v) ispell.el is more troublesome.  See bug #22097.  I have a problem
> >   with ispell putting its *Choices* window at the top of the left hand
> >   Follow Mode window.  Because of FM's sorting algorithm, this causes the
> >   two windows to be logically swapped, leading to confusing results.  I
> >   think the neatest solution would be to put *Choices* at the top of the
> >   rightmost window, preventing this.

I have indeed put *Choices* at the top of the RH window, to preserve
sanity.

I committed those changes, including changes to Ispell this afternoon.  I
think they're close to working fully.

> I believe bug#22097 is easy to fix for lazy highlighting, but what you
> described above is more troublesome, and might require adding support for
> window groups to ispell.el (like you're adding it to isearch.el).

I've put the window group stuff into ispell.el - there wasn't a lot to
change.

This afternoon, lack of lazy highlighting in Ispell (bug #22097) made
itself noticeable again.  I'll see what I can do, here.

-- 
Alan Mackenzie (Nuremberg, Germany).

Message #254 received at 17453 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Alan Mackenzie <acm <at> muc.de>
Cc: martin rudalics <rudalics <at> gmx.at>, 17453 <at> debbugs.gnu.org
Subject: Re: Framework extending window functions for Follow Mode (etc.).
Date: Tue, 08 Dec 2015 02:42:39 +0200

>>   (add-hook 'isearch-update-post-hook 'recenter)
>>   (add-hook 'replace-update-post-hook 'recenter)
>
> First comment: this sort of thing will wreck Follow Mode, scrolling text
> in the current window rather than point moving forward to the next
> window.

I guess a lot of other customizations will wreck Follow Mode too.

>> How would you do the same without these hooks, using only post-command-hook?
>
> By putting #'recenter into `post-command-hook'?  This should work for
> Isearch, and would work for Replace and Ispell if these two used the
> Emacs command loop.

This will call #'recenter on every command.

> I have indeed put *Choices* at the top of the RH window, to preserve
> sanity.
>
> I committed those changes, including changes to Ispell this afternoon.  I
> think they're close to working fully.

Yes, pretty close.

Message #259 received at 17453-done <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: 17453-done <at> debbugs.gnu.org
Subject: Re: bug#17453: Acknowledgement (Isearch doesn't work properly with
 Follow Mode.)
Date: Sun, 20 Dec 2015 12:59:07 +0000

Bug fixed in emacs-25 branch.

-- 
Alan Mackenzie (Nuremberg, Germany).

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.