bug#5718: scroll-margin in buffer with small line count.

[Eli Zaretskii]

> From: npostavs@users.sourceforge.net
> Cc: 5718@debbugs.gnu.org,  ahyatt@gmail.com,  gavenkoa@gmail.com
> Date: Mon, 02 Jan 2017 19:48:03 -0500
> 
> 
> [1:text/plain Hide]
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: npostavs@users.sourceforge.net
> >> Cc: 5718@debbugs.gnu.org,  ahyatt@gmail.com,  gavenkoa@gmail.com
> >> Date: Tue, 13 Sep 2016 22:40:29 -0400
> >> 
> >> Eli Zaretskii <eliz@gnu.org> writes:
> >> >> I have a patch set for fixing this and allowing the user to change the
> >> >> maximum margin from 0.25.  The latter doesn't quite work perfectly, for
> >> >> some reason when setting the maximum margin to 0.5 and scroll-margin to
> >> >> 100, `scroll-down-command' doesn't keep point centered in the window,
> >> >> even though other commands (e.g. `scroll-up-command') do.
> [...]
> >
> > ... given the above, I now understand that your interpretation of 0.5
> > is "half the window minus one line", which leaves the center line for
> > point.  Is that so?
> 
> Yes, I hadn't quite thought it through, but that was my intention.  I've
> updated the code to clarify this.

Thanks.

I think the remaining issues are minor (modulo the couple of questions
below), so I think you should install your changes

> The issues with `scroll-down-command' (and `next-line', below) remain.
> I find the following change fixes the problem for `scroll-down-command',
> though I'm not sure whether it's the right thing to do.
> 
> --- a/src/window.c
> +++ b/src/window.c
> @@ -5148,7 +5148,7 @@ window_scroll_pixel_based (Lisp_Object window, int n, 
> bool whole, bool noerror)
>          in the scroll margin at the bottom.  */
>        move_it_to (&it, PT, -1,
>                   (it.last_visible_y - WINDOW_HEADER_LINE_HEIGHT (w)
> -                  - this_scroll_margin - 1),
> +                  - this_scroll_margin - 1 - frame_line_height),
>                   -1,
>                   MOVE_TO_POS | MOVE_TO_Y);

Can you explain why this fixes the problem?  IOW, what was the problem
with the original code, and why moving to the previous screen line
here solves the problem?

Also, does this correction need to be conditional on the new variable
in some way, or is it correct in the general case as well?

>     (defun line-move (arg &optional noerror _to-end try-vscroll)
>     ...
>         (unless (and auto-window-vscroll try-vscroll
>                      ;; Only vscroll for single line moves
>                      (= (abs arg) 1)
>                      ;; Under scroll-conservatively, the display engine
>                      ;; does this better.
>                      (zerop scroll-conservatively)
>                      ...
>                      (line-move-partial arg noerror))
>           ...
>               (prog1 (line-move-visual arg noerror)
> 
> The problem involves partial lines.  In a window where
> (window-screen-lines) returns 7.222, doing M-: (vertical-motion '(0.0
> . 1)) does not scroll the window, which lets point end up 1 line away
> from the center.  Doing M-: (vertical-motion '(0.0 . -1)) does scroll
> the window, keeping the point in the center (as expected).
> 
> Adjusting the window so that (window-screen-lines) returns 7.0, there is
> no discrepancy between up and down motion.
> 
> I guess there is some incorrect boundary condition in try_scrolling,
> though I haven't worked out where.

Could be some off-by-one error when comparing with the window height.
This sounds like a minor issue in an extreme (and thus rare)
situation, so I'm not sure it should prevent installing this.

Thanks.