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: Sun, 11 Sep 2016 16:58:08 -0400
> 
> >> >>   this_scroll_margin = max (0, scroll_margin);
> >> >>   this_scroll_margin
> >> >>     = min (this_scroll_margin, window_total_lines / 4);
> >> >
> >> > Which reveals a subtle bug: the actual scroll margin should be 1 for 7
> >> > lines, 2 for 11, etc.  The problem is that the value of
> >> > window_total_lines includes the mode line, which it shouldn't.  Maybe
> >> > this should be fixed.
> 
> 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.

Thanks, LGTM.

However, I think we need to solve those glitches as part of
introducing the feature.  Setting a margin to half the window size
makes centering point difficult, but since some commands do succeed,
I'm guessing that those commands which succeed have a bug, i.e. they
leave point inside the margin.  Is that indeed so?

Also, did you test these changes with scroll-conservatively set to
101?  If not, please do, as that setting activates some code parts
that no other option does.

A few comments below.

> @@ -16527,10 +16507,8 @@ redisplay_window (Lisp_Object window, bool 
> just_this_one_p)
>         /* Some people insist on not letting point enter the scroll
>            margin, even though this part handles windows that didn't
>            scroll at all.  */
> -       int window_total_lines
> -         = WINDOW_TOTAL_LINES (w) * FRAME_LINE_HEIGHT (f) / 
> frame_line_height;
> -       int margin = min (scroll_margin, window_total_lines / 4);
> -       int pixel_margin = margin * frame_line_height;
> +          int margin = window_scroll_margin (w, MARGIN_IN_LINES);
> +          int pixel_margin = margin * frame_line_height;
>         bool header_line = WINDOW_WANTS_HEADER_LINE_P (w);
 
>         /* Note: We add an extra FRAME_LINE_HEIGHT, because the loop
> @@ -16814,12 +16792,7 @@ redisplay_window (Lisp_Object window, bool 
> just_this_one_p)
>    it.current_y = it.last_visible_y;
>    if (centering_position < 0)
>      {
> -      int window_total_lines
> -     = WINDOW_TOTAL_LINES (w) * FRAME_LINE_HEIGHT (f) / frame_line_height;
> -      int margin
> -     = scroll_margin > 0
> -     ? min (scroll_margin, window_total_lines / 4)
> -     : 0;
> +      int margin = window_scroll_margin (w, MARGIN_IN_LINES);
>        ptrdiff_t margin_pos = CHARPOS (startp);
>        Lisp_Object aggressive;
>        bool scrolling_up;
> @@ -17063,10 +17036,7 @@ redisplay_window (Lisp_Object window, bool 
> just_this_one_p)
>       {
>         int window_total_lines
>           = WINDOW_TOTAL_LINES (w) * FRAME_LINE_HEIGHT (f) / 
> frame_line_height;
> -       int margin =
> -         scroll_margin > 0
> -         ? min (scroll_margin, window_total_lines / 4)
> -         : 0;
> +          int margin = window_scroll_margin (w, MARGIN_IN_LINES);
>         bool move_down = w->cursor.vpos >= window_total_lines / 2;

Here you call window_scroll_margin 3 times in the same function.  Any
way of doing that only once and reusing the result?


> @@ -17298,17 +17267,7 @@ try_window (Lisp_Object window, struct text_pos pos, 
> int flags)
>    if ((flags & TRY_WINDOW_CHECK_MARGINS)
>        && !MINI_WINDOW_P (w))
>      {
> -      int this_scroll_margin;
> -      int window_total_lines
> -     = WINDOW_TOTAL_LINES (w) * FRAME_LINE_HEIGHT (f) / frame_line_height;
> -
> -      if (scroll_margin > 0)
> -     {
> -       this_scroll_margin = min (scroll_margin, window_total_lines / 4);
> -       this_scroll_margin *= frame_line_height;
> -     }
> -      else
> -     this_scroll_margin = 0;
> +      int this_scroll_margin = window_scroll_margin (w, MARGIN_IN_PIXELS);
 
>        if ((w->cursor.y >= 0  /* not vscrolled */
>          && w->cursor.y < this_scroll_margin
> @@ -18592,15 +18551,8 @@ try_window_id (struct window *w)
 
>    /* Don't let the cursor end in the scroll margins.  */
>    {
> -    int this_scroll_margin, cursor_height;
> -    int frame_line_height = default_line_pixel_height (w);
> -    int window_total_lines
> -      = WINDOW_TOTAL_LINES (w) * FRAME_LINE_HEIGHT (it.f) / 
> frame_line_height;
> -
> -    this_scroll_margin =
> -      max (0, min (scroll_margin, window_total_lines / 4));
> -    this_scroll_margin *= frame_line_height;
> -    cursor_height = MATRIX_ROW (w->desired_matrix, w->cursor.vpos)->height;
> +    int this_scroll_margin = window_scroll_margin (w, MARGIN_IN_PIXELS);
> +    int cursor_height = MATRIX_ROW (w->desired_matrix, 
> w->cursor.vpos)->height;
 
>      if ((w->cursor.y < this_scroll_margin
>        && CHARPOS (start) > BEGV)

Same here (in another function).

> diff --git a/src/window.c b/src/window.c
> index dbda435..20a7f3a 100644
> --- a/src/window.c
> +++ b/src/window.c
> @@ -4803,7 +4803,18 @@ window_scroll_margin (struct window *window, enum 
> margin_unit unit)
>          = (window->total_lines * WINDOW_FRAME_LINE_HEIGHT (window)
>             - WINDOW_MODE_LINE_HEIGHT (window))
>          / frame_line_height;
> -      int margin = min (scroll_margin, window_total_lines / 4);
> +
> +      int margin, max_margin;
> +      double ratio = 0.25;
> +      if (FLOATP (Vmaximum_scroll_margin))
> +        {
> +          ratio = XFLOAT_DATA (Vmaximum_scroll_margin);
> +          ratio = max (0.0, ratio);
> +          ratio = min (ratio, 0.5);
> +        }
> +      max_margin = (int) (window_total_lines * ratio);
> +      margin = max (0, scroll_margin);
> +      margin = min (scroll_margin, max_margin);
>        if (unit == MARGIN_IN_PIXELS)
>          return margin * frame_line_height;
>        else
> diff --git a/src/xdisp.c b/src/xdisp.c
> index 3602025..b22242a 100644
> --- a/src/xdisp.c
> +++ b/src/xdisp.c
> @@ -31451,6 +31451,11 @@ Recenter the window whenever point gets within this 
> many lines
>  of the top or bottom of the window.  */);
>    scroll_margin = 0;
>  
> +  DEFVAR_LISP ("maximum-scroll-margin", Vmaximum_scroll_margin,
> +    doc: /* Maximum effective value of `scroll-margin'.
> +Given as a fraction of the current window's lines.  */);

"as a fraction of the current window's height" sounds better, I
think.  (It doesn't matter if the height is in lines or in pixels,
for this purpose.)

> +  Vmaximum_scroll_margin = make_float (0.25);
> +

We usually call such variables "max-SOMETHING", not
"maximum-SOMETHING".

Also, the actual value is limited by 0.5, but the doc string doesn't
tell that.  It also doesn't say that any non-float value is ignored.

Finally, the new variable needs to be documented in the user manual
and in NEWS.

Thanks.