GNU bug report logs - #5718
scroll-margin in buffer with small line count.

Previous Next

Full log

View this message in rfc822 format

From: Eli Zaretskii <eliz <at> gnu.org>
To: npostavs <at> users.sourceforge.net
Cc: ahyatt <at> gmail.com, 5718 <at> debbugs.gnu.org, gavenkoa <at> gmail.com
Subject: bug#5718: scroll-margin in buffer with small line count.
Date: Mon, 12 Sep 2016 20:36:14 +0300

> From: npostavs <at> users.sourceforge.net
> Cc: 5718 <at> debbugs.gnu.org,  ahyatt <at> gmail.com,  gavenkoa <at> 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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.