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

[Alan Mackenzie <acm@muc.de>]

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