bug#17453: Framework extending window functions for Follow Mode (etc.).

bug#17453: Framework extending window functions for Follow Mode (etc.).

[Juri Linkov]

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

bug#17453: Framework extending window functions for Follow Mode (etc.).

[Alan Mackenzie]

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

bug#17453: Framework extending window functions for Follow Mode (etc.).

[Juri Linkov]

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

bug#17453: Framework extending window functions for Follow Mode (etc.).

[Alan Mackenzie]

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

bug#17453: Framework extending window functions for Follow Mode (etc.).

[Juri Linkov]

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

bug#17453: Framework extending window functions for Follow Mode (etc.).

[Alan Mackenzie]

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

bug#17453: Framework extending window functions for Follow Mode (etc.).

[Juri Linkov]

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

bug#17453: Framework extending window functions for Follow Mode (etc.).

[martin rudalics]

 > 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

bug#17453: Framework extending window functions for Follow Mode (etc.).

[Juri Linkov]

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

bug#17453: Framework extending window functions for Follow Mode (etc.).

[Alan Mackenzie]

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

bug#17453: Framework extending window functions for Follow Mode (etc.).

[Juri Linkov]

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

bug#17453: Framework extending window functions for Follow Mode (etc.).

[Alan Mackenzie]

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

bug#17453: Framework extending window functions for Follow Mode (etc.).

[Alan Mackenzie]

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

bug#17453: Framework extending window functions for Follow Mode (etc.).

[Juri Linkov]

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

bug#17453: Framework extending window functions for Follow Mode (etc.).

[Alan Mackenzie]

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

bug#17453: Framework extending window functions for Follow Mode (etc.).

[Juri Linkov]

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

bug#17453: Framework extending window functions for Follow Mode (etc.).

[Alan Mackenzie]

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

bug#17453: Framework extending window functions for Follow Mode (etc.).

[Juri Linkov]

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

bug#17453: Framework extending window functions for Follow Mode (etc.).

[Alan Mackenzie]

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

bug#17453: Framework extending window functions for Follow Mode (etc.).

[Juri Linkov]

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

bug#17453: Framework extending window functions for Follow Mode (etc.).

[Alan Mackenzie]

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

bug#17453: Framework extending window functions for Follow Mode (etc.).

[Juri Linkov]

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

bug#17453: Framework extending window functions for Follow Mode (etc.).

[Alan Mackenzie]

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