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

[npostavs]

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

I'm not sure what you mean by "succeed" here.  `next-line',
`previous-line', and `scroll-up-command' are able to keep point ouside
of the margin (thus keeping it centered); `scroll-down-command' leaves
it one line down from where it should be.

Actually, the above applies to windows with an odd number of lines, I
realized I didn't account for the case of an even number of lines, which
currently has 0 lines that are outside the margin.  I need to change the
cap on `max-scroll-margin' to account for this.

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

I hadn't; trying it out now, it seems to cause `next-line' to also have
the bad behaviour of `scroll-down-command' where point is one line too
far down.

>
> 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?

Yes, this seems to work:

@@ -16173,7 +16173,7 @@ redisplay_window (Lisp_Object window, bool 
just_this_one_p)
   int centering_position = -1;
   bool last_line_misfit = false;
   ptrdiff_t beg_unchanged, end_unchanged;
-  int frame_line_height;
+  int frame_line_height, margin;
   bool use_desired_matrix;
   void *itdata = NULL;
 
@@ -16203,6 +16203,8 @@ redisplay_window (Lisp_Object window, bool 
just_this_one_p)
  restart:
   reconsider_clip_changes (w);
   frame_line_height = default_line_pixel_height (w);
+  margin = window_scroll_margin (w, MARGIN_IN_LINES);
+
 
   /* Has the mode line to be updated?  */
   update_mode_line = (w->update_mode_line
@@ -16507,7 +16509,6 @@ 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 margin = window_scroll_margin (w, MARGIN_IN_LINES);
           int pixel_margin = margin * frame_line_height;
          bool header_line = WINDOW_WANTS_HEADER_LINE_P (w);
 
@@ -16792,7 +16793,6 @@ redisplay_window (Lisp_Object window, bool 
just_this_one_p)
   it.current_y = it.last_visible_y;
   if (centering_position < 0)
     {
-      int margin = window_scroll_margin (w, MARGIN_IN_LINES);
       ptrdiff_t margin_pos = CHARPOS (startp);
       Lisp_Object aggressive;
       bool scrolling_up;
@@ -17036,7 +17036,6 @@ 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 = window_scroll_margin (w, MARGIN_IN_LINES);
          bool move_down = w->cursor.vpos >= window_total_lines / 2;
 
          move_it_by_lines (&it, move_down ? margin + 1 : -(margin + 1));


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

These are 2 different functions: try_window and try_window_id.

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

Makes sense.

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

Okay.