bug#34723: 27.0.50; customize and improve diff-mode recentering

bug#34723: 27.0.50; customize and improve diff-mode recentering

[Charles A. Roelli]

In Bug#32991 we touched on diff-mode conditionally recentering when
you use diff-hunk-next or diff-hunk-prev.  We found that this behavior
does not respect scroll-related variables (though Stefan suggested we
might be able to recenter while taking into account these variables).
It would be nice to allow disabling the behavior.

There are also cases where the diff-mode recentering ends up showing
less of the hunk than would have been displayed without the
recentering, which must be a bug.  In general, I don't understand why
recentering is thought to "show more" of a hunk in a buffer, when it
might show either less or more of a hunk in a buffer.

For example, if you look at the diff of commit 7523a9e in a
single-window GUI frame in master (emacs -Q), and type

M-: (set-frame-height (selected-frame) 15) RET
M-g c 450 RET
C-l C-l n

Emacs recenters and shows less of the hunk that you moved to.  This is
contrived, but is something you might come across a lot in small
windows, especially with longer hunks.

As a solution we could make C-M-l (reposition-window) work in
diff-mode and call that instead from diff-hunk-next and
diff-hunk-prev, with some customizable variable to enable or disable
the behavior.  Or fix the recentering code to stop recentering in
cases like the above.

bug#34723: 27.0.50; customize and improve diff-mode recentering

[Juri Linkov]

> As a solution we could make C-M-l (reposition-window) work in
> diff-mode and call that instead from diff-hunk-next and
> diff-hunk-prev, with some customizable variable to enable or disable
> the behavior.  Or fix the recentering code to stop recentering in
> cases like the above.

I use `C-M-l' (reposition-window) to recenter during search,
and it works quite well to recenter diff hunks with

  (setq isearch-push-state-function
        (lambda ()
          (when (and isearch-success (not (pos-visible-in-window-p)))
            (reposition-window))
          `(lambda (cmd)
             (when isearch-success
               (set-window-start nil ,(window-start))))))

but the problem is that I also have to change the default definition
of reposition boundaries used by `C-M-l' (reposition-window)

   (add-hook 'diff-mode-hook
             (lambda ()
               (set (make-local-variable 'beginning-of-defun-function)
                    #'diff-beginning-of-hunk)
               (set (make-local-variable 'end-of-defun-function)
                    #'diff-end-of-hunk)))

to recenter at diff hunk boundaries, not at diff file boundaries
as defined in `diff-mode' by default, because usually there are
much more lines covered by all file changes, and when reposition-window
tries to fit all file changes into the screen, it puts the current line
at the bottom line of the window, thus not showing the lower part
of the current hunk.

bug#34723: 27.0.50; customize and improve diff-mode recentering

[Charles A. Roelli]

> From: Juri Linkov <juri@linkov.net>
> Date: Sun, 03 Mar 2019 23:33:16 +0200
> 
> I use `C-M-l' (reposition-window) to recenter during search,
> and it works quite well to recenter diff hunks with
> 
>   (setq isearch-push-state-function
>         (lambda ()
>           (when (and isearch-success (not (pos-visible-in-window-p)))
>             (reposition-window))
>           `(lambda (cmd)
>              (when isearch-success
>                (set-window-start nil ,(window-start))))))

Can we add something like this to isearch.el, maybe as a defcustom?
Or maybe we could add it in a way such that other packages that show
search results (grep, xref) could use the same function to reposition
the window.  The function could inspect a defcustom in simple.el to
decide whether (or how) to reposition the window.

> but the problem is that I also have to change the default definition
> of reposition boundaries used by `C-M-l' (reposition-window)
> 
>    (add-hook 'diff-mode-hook
>              (lambda ()
>                (set (make-local-variable 'beginning-of-defun-function)
>                     #'diff-beginning-of-hunk)
>                (set (make-local-variable 'end-of-defun-function)
>                     #'diff-end-of-hunk)))
> 
> to recenter at diff hunk boundaries, not at diff file boundaries
> as defined in `diff-mode' by default, because usually there are
> much more lines covered by all file changes, and when reposition-window
> tries to fit all file changes into the screen, it puts the current line
> at the bottom line of the window, thus not showing the lower part
> of the current hunk.

I noticed this too.  If it makes sense, we should make these values of
beginning-of-defun-function and end-of-defun-function the default.

bug#34723: 27.0.50; customize and improve diff-mode recentering

[Juri Linkov]

> Can we add something like this to isearch.el, maybe as a defcustom?
> Or maybe we could add it in a way such that other packages that show
> search results (grep, xref) could use the same function to reposition
> the window.  The function could inspect a defcustom in simple.el to
> decide whether (or how) to reposition the window.

Yes, a more common customization would be preferable.
Something at the same low level as `recenter',
maybe adding a new defcustom like `recenter-function'
to run a customized function e.g. `reposition-window'
instead of the default recentering to the middle of the screen.

bug#34723: 27.0.50; customize and improve diff-mode recentering

[Eli Zaretskii]

> Date: Sun, 03 Mar 2019 21:38:22 +0100
> From: charles@aurox.ch (Charles A. Roelli)
> 
> In Bug#32991 we touched on diff-mode conditionally recentering when
> you use diff-hunk-next or diff-hunk-prev.  We found that this behavior
> does not respect scroll-related variables (though Stefan suggested we
> might be able to recenter while taking into account these variables).
> It would be nice to allow disabling the behavior.
> 
> There are also cases where the diff-mode recentering ends up showing
> less of the hunk than would have been displayed without the
> recentering, which must be a bug.  In general, I don't understand why
> recentering is thought to "show more" of a hunk in a buffer, when it
> might show either less or more of a hunk in a buffer.
> 
> For example, if you look at the diff of commit 7523a9e in a
> single-window GUI frame in master (emacs -Q), and type
> 
> M-: (set-frame-height (selected-frame) 15) RET
> M-g c 450 RET
> C-l C-l n
> 
> Emacs recenters and shows less of the hunk that you moved to.  This is
> contrived, but is something you might come across a lot in small
> windows, especially with longer hunks.
> 
> As a solution we could make C-M-l (reposition-window) work in
> diff-mode and call that instead from diff-hunk-next and
> diff-hunk-prev, with some customizable variable to enable or disable
> the behavior.  Or fix the recentering code to stop recentering in
> cases like the above.

I've read this description and that in bug#32991, several times, and I
still cannot figure out whether you are saying that there's a problem
in recentering per se (and in the related redisplay code), or you are
saying that diff-mode does something in its application code that
indirectly and inadvertently causes unwarranted recentering.  Could
you please clarify which is the case here?  Depending on that, the
solution should be either in diff-mode or in the display engine
(however, I have hard time believing that we have use cases where the
display engine ignores scroll-related variables).

Thanks.

bug#34723: 27.0.50; customize and improve diff-mode recentering

[Eli Zaretskii]

> Date: Mon, 04 Mar 2019 20:06:13 +0100
> From: charles@aurox.ch (Charles A. Roelli)
> Cc: 34723@debbugs.gnu.org
> 
> >   (setq isearch-push-state-function
> >         (lambda ()
> >           (when (and isearch-success (not (pos-visible-in-window-p)))
> >             (reposition-window))
> >           `(lambda (cmd)
> >              (when isearch-success
> >                (set-window-start nil ,(window-start))))))
> 
> Can we add something like this to isearch.el, maybe as a defcustom?
> Or maybe we could add it in a way such that other packages that show
> search results (grep, xref) could use the same function to reposition
> the window.  The function could inspect a defcustom in simple.el to
> decide whether (or how) to reposition the window.

Isn't it easier to temporarily bind scroll-margin to a non-zero value,
then force redisplay?

Or maybe I'm missing something in this discussion.

bug#34723: 27.0.50; customize and improve diff-mode recentering

[Eli Zaretskii]

> Date: Tue, 05 Mar 2019 20:49:13 +0100
> From: charles@aurox.ch (Charles A. Roelli)
> CC: 34723@debbugs.gnu.org
> 
> The function diff-hunk-next is defined using the macro
> easy-mmode-define-navigation, and its definition calls (recenter '(0))
> too liberally.  The call also ignores settings like
> 'scroll-conservatively'.  The display engine is not at fault, and we
> should be able to fix the issue either in easy-mmode or in diff-mode.

I don't understand what calling 'recenter' has to do with
scroll-conservatively.  And scroll-conservatively is implemented in
the display engine, so I'm afraid I'm still in the dark regarding the
nature of the problem you are discussing.

Of course, if you know what to do to fix the problem, there's no need
for me to understand the issue, and I should just shut up and let you
do what you think is right.

Thanks.

bug#34723: 27.0.50; customize and improve diff-mode recentering

[Charles A. Roelli]

> Date: Tue, 05 Mar 2019 17:46:11 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> CC: 34723@debbugs.gnu.org
> 
> I've read this description and that in bug#32991, several times, and I
> still cannot figure out whether you are saying that there's a problem
> in recentering per se (and in the related redisplay code), or you are
> saying that diff-mode does something in its application code that
> indirectly and inadvertently causes unwarranted recentering.  Could
> you please clarify which is the case here?  Depending on that, the
> solution should be either in diff-mode or in the display engine
> (however, I have hard time believing that we have use cases where the
> display engine ignores scroll-related variables).
> 
> Thanks.

The function diff-hunk-next is defined using the macro
easy-mmode-define-navigation, and its definition calls (recenter '(0))
too liberally.  The call also ignores settings like
'scroll-conservatively'.  The display engine is not at fault, and we
should be able to fix the issue either in easy-mmode or in diff-mode.

bug#34723: 27.0.50; customize and improve diff-mode recentering

[Charles A. Roelli]

> Date: Tue, 05 Mar 2019 18:11:57 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> CC: juri@linkov.net, 34723@debbugs.gnu.org
>
> > >   (setq isearch-push-state-function
> > >         (lambda ()
> > >           (when (and isearch-success (not (pos-visible-in-window-p)))
> > >             (reposition-window))
> > >           `(lambda (cmd)
> > >              (when isearch-success
> > >                (set-window-start nil ,(window-start))))))
> > 
> > Can we add something like this to isearch.el, maybe as a defcustom?
> > Or maybe we could add it in a way such that other packages that show
> > search results (grep, xref) could use the same function to reposition
> > the window.  The function could inspect a defcustom in simple.el to
> > decide whether (or how) to reposition the window.
> 
> Isn't it easier to temporarily bind scroll-margin to a non-zero value,
> then force redisplay?
> 
> Or maybe I'm missing something in this discussion.

I'm not sure how binding scroll-margin would change the behavior here.

IIUC the aim of this snippet is to reposition the window to show as
much of the function definition (or diff hunk, or some other text
structure) that includes the search hit as possible.  For example, if
you evaluate the snippet from emacs -Q, then type, for example

M-x find-library RET simple.el RET
C-s quit-flag

Then Emacs positions the window to show the definition (from the
beginning) of the function containing the search hit.  When you try
that again in another session without evaluating the snippet, Emacs
centers the search hit in the window, giving you less useful context.

bug#34723: 27.0.50; customize and improve diff-mode recentering

[Eli Zaretskii]

> Date: Tue, 05 Mar 2019 21:11:13 +0100
> From: charles@aurox.ch (Charles A. Roelli)
> CC: juri@linkov.net, 34723@debbugs.gnu.org
> 
> > Isn't it easier to temporarily bind scroll-margin to a non-zero value,
> > then force redisplay?
> > 
> > Or maybe I'm missing something in this discussion.
> 
> I'm not sure how binding scroll-margin would change the behavior here.

It keeps point closer to the center of the window and away from the
window edges.  I though that's what you wanted, because doing that
will show more context around the match.

bug#34723: 27.0.50; customize and improve diff-mode recentering

[Charles A. Roelli]

> Date: Tue, 05 Mar 2019 21:44:01 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> CC: 34723@debbugs.gnu.org
> 
> I don't understand what calling 'recenter' has to do with
> scroll-conservatively.  And scroll-conservatively is implemented in
> the display engine, so I'm afraid I'm still in the dark regarding the
> nature of the problem you are discussing.

Does the following scenario make some sense?  diff-hunk-next moves
point to a position beyond (window-end), and immediately calls
(recenter '(0)).  Scrolling (which would otherwise respect
scroll-conservatively) gets preempted by diff-hunk-next's call to
recenter.

> Of course, if you know what to do to fix the problem, there's no need
> for me to understand the issue, and I should just shut up and let you
> do what you think is right.

(Please don't, I will almost certainly screw it up. ;-)

bug#34723: 27.0.50; customize and improve diff-mode recentering

[Eli Zaretskii]

> Date: Tue, 05 Mar 2019 21:37:32 +0100
> From: charles@aurox.ch (Charles A. Roelli)
> CC: 34723@debbugs.gnu.org
> 
> > I don't understand what calling 'recenter' has to do with
> > scroll-conservatively.  And scroll-conservatively is implemented in
> > the display engine, so I'm afraid I'm still in the dark regarding the
> > nature of the problem you are discussing.
> 
> Does the following scenario make some sense?  diff-hunk-next moves
> point to a position beyond (window-end), and immediately calls
> (recenter '(0)).  Scrolling (which would otherwise respect
> scroll-conservatively) gets preempted by diff-hunk-next's call to
> recenter.

Why does it call 'recenter'?  It must have a reason, doesn't it?  Does
the history of that code gives a clue?

Note that without recentering, if you just move point to some location
in the diffs, when scroll-conservatively > 100, point will wind up
either on the last screen line of the window or its first screen line,
depending on whether you move forward or back in the buffer.  The
latter might be okay, but the former will most probably hide most of
the hunk, which might be the reason for recentering (I'm just guessing
here).

bug#34723: 27.0.50; customize and improve diff-mode recentering

[Charles A. Roelli]

> Date: Tue, 05 Mar 2019 22:22:41 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> It keeps point closer to the center of the window and away from the
> window edges.  I though that's what you wanted, because doing that
> will show more context around the match.

Right.  I spoke earlier about calling reposition-window from
diff-hunk-next as a possible replacement for recenter, and Juri
suggested an equivalent behavior for Isearch, and the aim of these
changes is to show the most useful context around point -- which does
not necessarily mean keeping point close to the center of the window.
For example, with the Isearch example I gave, point can end up
anywhere in the window after searching, since reposition-window tries
to position the first line of the function definition on the first
screen line.

bug#34723: 27.0.50; customize and improve diff-mode recentering

[Charles A. Roelli]

> Date: Wed, 06 Mar 2019 18:06:14 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> Why does it call 'recenter'?  It must have a reason, doesn't it?  Does
> the history of that code gives a clue?

The doc of easy-mmode-define-navigation says:

  BASE-next also tries to make sure that the whole entry is visible by
  searching for its end (by calling ENDFUN if provided or by looking for
  the next entry) and recentering if necessary.

There are cases where recentering does "make sure that the whole entry
is visible", but as we saw in the first post of this report, there is
at least one case where recentering makes less useful context visible.
The current implementation seems a little too eager.  I'd like to fix
that, and at least allow to toggle the auto-recentering.

> Note that without recentering, if you just move point to some location
> in the diffs, when scroll-conservatively > 100, point will wind up
> either on the last screen line of the window or its first screen line,
> depending on whether you move forward or back in the buffer.  The
> latter might be okay, but the former will most probably hide most of
> the hunk, which might be the reason for recentering (I'm just guessing
> here).

Yes, I think you are right.  Maybe diff-mode could have just set
scroll-conservatively (or scroll-margin) in a buffer-local variable to
get this auto-recentering behavior, although that would also step on
user customizations.

bug#34723: 27.0.50; customize and improve diff-mode recentering

[Stefan Monnier]

[ Author responsible for the call to recenter speaking ;-) ]

>   BASE-next also tries to make sure that the whole entry is visible by
>   searching for its end (by calling ENDFUN if provided or by looking for
>   the next entry) and recentering if necessary.

That's indeed the purpose.  When I move to the next hunk, I almost
always want to see that hunk, and that often requires some kind of
"recentering".

> There are cases where recentering does "make sure that the whole entry
> is visible", but as we saw in the first post of this report, there is
> at least one case where recentering makes less useful context visible.

That's clearly undesirable, indeed.  I think it's a plain bug.

Basically when moving in one direction, diff-hunk-next/prev should never
scroll the buffer in the other direction.

> The current implementation seems a little too eager.

Sometimes it's also not eager enough (if the hunk is longer than half of
the screen, it shouldn't just recenter but it should likely scroll
further so that more than half of the screen shows the hunk).

>> Note that without recentering, if you just move point to some location
>> in the diffs, when scroll-conservatively > 100, point will wind up
>> either on the last screen line of the window or its first screen line,
>> depending on whether you move forward or back in the buffer.  The
>> latter might be okay, but the former will most probably hide most of
>> the hunk, which might be the reason for recentering (I'm just guessing
>> here).
>
> Yes, I think you are right.  Maybe diff-mode could have just set
> scroll-conservatively (or scroll-margin) in a buffer-local variable to
> get this auto-recentering behavior, although that would also step on
> user customizations.

Not sure what you mean.  I have scroll-conservatively set to 0.
How should I set it to get the recentering that I want?
As for scroll-margin, I don't like using it.  Also it would affect all
cursor movement rather than only diff-hunk-* and would force point
closer to the middle which is not what I want when the hunk is longer
than half the window.

In your original message you said:
> We found that this behavior does not respect scroll-related variables

Did that only refer to `scroll-conservatively`?  If not, could you
clarify which scroll-related variable setting is not respected,
and when?


        Stefan

bug#34723: 27.0.50; customize and improve diff-mode recentering

[Stefan Monnier]

>> There are cases where recentering does "make sure that the whole entry
>> is visible", but as we saw in the first post of this report, there is
>> at least one case where recentering makes less useful context visible.
> That's clearly undesirable, indeed.  I think it's a plain bug.
> Basically when moving in one direction, diff-hunk-next/prev should never
> scroll the buffer in the other direction.

I installed the patch below which should fix this most glaring problem.


        Stefan


diff --git a/lisp/emacs-lisp/easy-mmode.el b/lisp/emacs-lisp/easy-mmode.el
index b866a95443..57cf5c86f4 100644
--- a/lisp/emacs-lisp/easy-mmode.el
+++ b/lisp/emacs-lisp/easy-mmode.el
@@ -625,6 +625,7 @@ easy-mmode-define-navigation
                  ,body
                  (when was-narrowed (funcall #',narrowfun)))))))
     (unless name (setq name base-name))
+    ;; FIXME: Move most of those functions's bodies to helper functions!
     `(progn
        (defun ,next-sym (&optional count)
 	 ,(format "Go to the next COUNT'th %s.
@@ -646,7 +647,11 @@ easy-mmode-define-navigation
                                         `(re-search-forward ,re nil t 2)))
                                    (point-max))))
                     (unless (pos-visible-in-window-p endpt nil t)
-                      (recenter '(0)))))))
+                      (let ((ws (window-start)))
+                        (recenter '(0))
+                        (if (< (window-start) ws)
+                            ;; recenter scrolled in the wrong direction!
+                            (set-window-start nil ws))))))))
            ,@body))
        (put ',next-sym 'definition-name ',base)
        (defun ,prev-sym (&optional count)

bug#34723: 27.0.50; customize and improve diff-mode recentering

[Charles A. Roelli]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Wed, 13 Mar 2019 15:56:50 -0400
> 
> >> There are cases where recentering does "make sure that the whole entry
> >> is visible", but as we saw in the first post of this report, there is
> >> at least one case where recentering makes less useful context visible.
> > That's clearly undesirable, indeed.  I think it's a plain bug.
> > Basically when moving in one direction, diff-hunk-next/prev should never
> > scroll the buffer in the other direction.
> 
> I installed the patch below which should fix this most glaring problem.
> 
> 
>         Stefan

Thanks!  I noticed that I had to "touch lisp/vc/diff-mode.el" before
the change took effect in my local tree, since the change in the macro
does not trigger the recompilation of the callers.  Could "make"
somehow detect this?

bug#34723: 27.0.50; customize and improve diff-mode recentering

[Charles A. Roelli]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Wed, 13 Mar 2019 15:40:16 -0400

> > The current implementation seems a little too eager.
> 
> Sometimes it's also not eager enough (if the hunk is longer than half of
> the screen, it shouldn't just recenter but it should likely scroll
> further so that more than half of the screen shows the hunk).

Yes.  In that specific case, we could call (recenter 0) to make more
(or all of) the hunk visible.
 
> > Yes, I think you are right.  Maybe diff-mode could have just set
> > scroll-conservatively (or scroll-margin) in a buffer-local variable to
> > get this auto-recentering behavior, although that would also step on
> > user customizations.
> 
> Not sure what you mean.  I have scroll-conservatively set to 0.
> How should I set it to get the recentering that I want?

Setting scroll-conservatively to 0 in diff-mode would of course not be
enough to recenter in all the cases where diff-hunk-next currently
does, but it would be a bit cleaner.  The part of the code that
recenters based on the visibility of the end of the hunk at point
could instead go in a post-command-hook (or in a display hook?), which
would make it work after all movement commands, not just
diff-hunk-next.  I don't know if that's desirable, though.

> As for scroll-margin, I don't like using it.  Also it would affect all
> cursor movement rather than only diff-hunk-* and would force point
> closer to the middle which is not what I want when the hunk is longer
> than half the window.

Agreed.

> In your original message you said:
> > We found that this behavior does not respect scroll-related variables
> 
> Did that only refer to `scroll-conservatively`?  If not, could you
> clarify which scroll-related variable setting is not respected,
> and when?

Hm, I'm not sure if there was another variable that the behavior
affects (can't find one now).  This is no big issue; it's an argument
for making the behavior customizable.

bug#34723: 27.0.50; customize and improve diff-mode recentering

[Stefan Monnier]

> Thanks!  I noticed that I had to "touch lisp/vc/diff-mode.el" before
> the change took effect in my local tree, since the change in the macro
> does not trigger the recompilation of the callers.  Could "make"
> somehow detect this?

Depends if you want the theoretical answer or the practical one ;-)


        Stefan

bug#34723: 27.0.50; customize and improve diff-mode recentering

[Stefan Monnier]

>> Sometimes it's also not eager enough (if the hunk is longer than half of
>> the screen, it shouldn't just recenter but it should likely scroll
>> further so that more than half of the screen shows the hunk).
> Yes.  In that specific case, we could call (recenter 0) to make more
> (or all of) the hunk visible.

I think in some cases I'd like it, yes.  But would you?
I thought you didn't want recentering in next-hunk at all‽
Also I'm not sure if it would annoy me more often than not, which is
probably why I haven't bothered to try and code it up.

>> Did that only refer to `scroll-conservatively`?  If not, could you
>> clarify which scroll-related variable setting is not respected,
>> and when?
> Hm, I'm not sure if there was another variable that the behavior
> affects (can't find one now).  This is no big issue; it's an argument
> for making the behavior customizable.

Yes, feel free to add such a custom var.


        Stefan