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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 5718 in the body.
You can then email your comments to 5718 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Oleksandr Gavenko <gavenkoa <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: scroll-margin in buffer with small line count.
Date: Sun, 14 Mar 2010 19:26:57 +0200

I have official Emacs 22.3 from FSF ftp server for Windows.

When set

(setq scroll-margin 4)

in buffer with window width 6 lines real margin value is 1
(so real line scrolling done when press <up> on second line
or when press <down> on fifth line).

Experiment show such dependence of real margin on line count:

lines      real-scroll-margin
3,4,5,6         1
7,8,9,10        2
11,12,13,14     3
>15             4

I count from 3 as when try make less lines Emacs warn.

I think that whose modify scroll-margin want to see as many
as possible margin value up to its customization value.

This formula produce such values:

best-scroll-margin = min( (line_count - 1)/2,  scroll-margin)

So previous table changed to:

lines      best-scroll-margin
3,4             1
5,6             2
7,8             3
>9              4

Also as you can see in proposal case
user switch start work from 9 lines in window, not 15!

Please implement described behaviour.

-- 
Best regards!

Message #8 received at 5718 <at> debbugs.gnu.org (full text, mbox):

From: Andrew Hyatt <ahyatt <at> gmail.com>
To: Oleksandr Gavenko <gavenkoa <at> gmail.com>
Cc: 5718 <at> debbugs.gnu.org
Subject: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Thu, 11 Aug 2016 00:11:22 -0400

Oleksandr Gavenko <gavenkoa <at> gmail.com> writes:

> I have official Emacs 22.3 from FSF ftp server for Windows.
>
> When set
>
> (setq scroll-margin 4)
>
> in buffer with window width 6 lines real margin value is 1
> (so real line scrolling done when press <up> on second line
> or when press <down> on fifth line).
>
> Experiment show such dependence of real margin on line count:
>
> lines      real-scroll-margin
> 3,4,5,6         1
> 7,8,9,10        2
> 11,12,13,14     3
>>15             4
>
> I count from 3 as when try make less lines Emacs warn.
>
> I think that whose modify scroll-margin want to see as many
> as possible margin value up to its customization value.
>
> This formula produce such values:
>
> best-scroll-margin = min( (line_count - 1)/2,  scroll-margin)
>
> So previous table changed to:
>
> lines      best-scroll-margin
> 3,4             1
> 5,6             2
> 7,8             3
>>9              4
>
> Also as you can see in proposal case
> user switch start work from 9 lines in window, not 15!
>
> Please implement described behaviour.

I'm having a hard time understanding this report.  However, it seems
like more like a wishlist request than a bug: you want to change the
scrolling behavior.  I'm going to change this to wishlist for now, but
let me know if I've misunderstood, please.

Message #13 received at 5718 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Andrew Hyatt <ahyatt <at> gmail.com>
Cc: 5718 <at> debbugs.gnu.org, Oleksandr Gavenko <gavenkoa <at> gmail.com>
Subject: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Thu, 11 Aug 2016 08:03:18 -0400

Andrew Hyatt <ahyatt <at> gmail.com> writes:

> Oleksandr Gavenko <gavenkoa <at> gmail.com> writes:
>
>> I have official Emacs 22.3 from FSF ftp server for Windows.
>>
>> When set
>>
>> (setq scroll-margin 4)
>>
>> in buffer with window width 6 lines real margin value is 1
>> (so real line scrolling done when press <up> on second line
>> or when press <down> on fifth line).
>>
>> Experiment show such dependence of real margin on line count:
>>
>> lines      real-scroll-margin
>> 3,4,5,6         1
>> 7,8,9,10        2
>> 11,12,13,14     3
>>>15             4
>>
>> I count from 3 as when try make less lines Emacs warn.
>>
>> I think that whose modify scroll-margin want to see as many
>> as possible margin value up to its customization value.
>>
>> This formula produce such values:
>>
>> best-scroll-margin = min( (line_count - 1)/2,  scroll-margin)
>>
>> So previous table changed to:
>>
>> lines      best-scroll-margin
>> 3,4             1
>> 5,6             2
>> 7,8             3
>>>9              4
>>
>> Also as you can see in proposal case
>> user switch start work from 9 lines in window, not 15!
>>
>> Please implement described behaviour.
>
> I'm having a hard time understanding this report.  However, it seems
> like more like a wishlist request than a bug: you want to change the
> scrolling behavior.  I'm going to change this to wishlist for now, but
> let me know if I've misunderstood, please.

I think the complaint is that the `scroll-margin' effective value is
capped at a 1/4 of the window height, as seen in this
window_scroll_pixel_based (window.c):

  this_scroll_margin = max (0, scroll_margin);
  this_scroll_margin
    = min (this_scroll_margin, window_total_lines / 4);

Whereas, it seems more logical to cap it at half window height.

I seem to recall another feature request that I can't find now about
always keeping the cursor in the middle of the window.  If this
restriction were loosened it might be possible to implement by setting
scroll-margin to a very high number.  There might well be some important
reason to divide by 4 though (I haven't looked at the following code).

Message #16 received at 5718 <at> debbugs.gnu.org (full text, mbox):

From: Oleksandr Gavenko <gavenkoa <at> gmail.com>
To: npostavs <at> users.sourceforge.net
Cc: Andrew Hyatt <ahyatt <at> gmail.com>, 5718 <at> debbugs.gnu.org
Subject: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Thu, 11 Aug 2016 16:05:57 +0300

On 2016-08-11, npostavs <at> users.sourceforge.net wrote:
> I think the complaint is that the `scroll-margin' effective value is
> capped at a 1/4 of the window height, as seen in this
> window_scroll_pixel_based (window.c):
>
>   this_scroll_margin = max (0, scroll_margin);
>   this_scroll_margin
>     = min (this_scroll_margin, window_total_lines / 4);
>
> Whereas, it seems more logical to cap it at half window height.
>

Yes, that was what I meant!

Setting this_scroll_margin to 'window_total_lines / 2' keeps current line
centered for short buffers.

I think it is most desired behavior because it takes more surrounding context
in small buffer.

Many recipes suggest to set 'scroll-margin' to very large number to keep
current line always centered.

================================================================

On reporting day I removed:

  (setq-default scroll-margin 4)

from ~/.emacs because in "M-x calendar" UP/DOWN navigation become broken.

Even in Emacs 24.5.1 setting

  (setq scroll-margin 4)

in Calendar buffer hide month names on UP/DOWN moves.

Since that time I selectively set scroll-margin:

  (make-variable-buffer-local 'scroll-margin)

  (defun my-set-scroll-margin () (setq scroll-margin 4))
  (mapc (lambda (hook) (add-hook hook #'my-set-scroll-margin))
       (delete-dups (append my-text-mode-hook-list my-devel-mode-hook-list)) )

to avoid setting 'scroll-margin' in Calendar.

In order just to use:

  (setq-default scroll-margin 4)

Emacs should not take in a count 'scroll-margin' when all lines are visible.

Those are all my discoveries about 'scroll-margin' that makes Emacs usage
experience less delightful.

-- 
http://defun.work/

Message #19 received at 5718 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> users.sourceforge.net>
To: Oleksandr Gavenko <gavenkoa <at> gmail.com>
Cc: Andrew Hyatt <ahyatt <at> gmail.com>, 5718 <at> debbugs.gnu.org
Subject: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Thu, 11 Aug 2016 09:24:59 -0400

On Thu, Aug 11, 2016 at 9:05 AM, Oleksandr Gavenko <gavenkoa <at> gmail.com> wrote:
> On reporting day I removed:
>
>   (setq-default scroll-margin 4)
>
> from ~/.emacs because in "M-x calendar" UP/DOWN navigation become broken.
>
> Even in Emacs 24.5.1 setting
>
>   (setq scroll-margin 4)
>
> in Calendar buffer hide month names on UP/DOWN moves.

This sounds like Bug #10379 which should be fixed as of 24.1 by
calendar mode resetting scroll-margin locally to 0.

calendar.el:1748:

(define-derived-mode calendar-mode nil "Calendar"
...
  (set (make-local-variable 'scroll-margin) 0) ; bug#10379

Message #22 received at 5718 <at> debbugs.gnu.org (full text, mbox):

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: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Thu, 11 Aug 2016 18:28:32 +0300

> From: npostavs <at> users.sourceforge.net
> Date: Thu, 11 Aug 2016 08:03:18 -0400
> Cc: 5718 <at> debbugs.gnu.org, Oleksandr Gavenko <gavenkoa <at> gmail.com>
> 
> >> lines      real-scroll-margin
> >> 3,4,5,6         1
> >> 7,8,9,10        2
> >> 11,12,13,14     3
> >>>15             4
> >>
> >> I count from 3 as when try make less lines Emacs warn.
> >>
> >> I think that whose modify scroll-margin want to see as many
> >> as possible margin value up to its customization value.
> >>
> >> This formula produce such values:
> >>
> >> best-scroll-margin = min( (line_count - 1)/2,  scroll-margin)
> >>
> >> So previous table changed to:
> >>
> >> lines      best-scroll-margin
> >> 3,4             1
> >> 5,6             2
> >> 7,8             3
> >>>9              4
> >>
> >> Also as you can see in proposal case
> >> user switch start work from 9 lines in window, not 15!
> >>
> >> Please implement described behaviour.
> >
> > I'm having a hard time understanding this report.  However, it seems
> > like more like a wishlist request than a bug: you want to change the
> > scrolling behavior.  I'm going to change this to wishlist for now, but
> > let me know if I've misunderstood, please.
> 
> I think the complaint is that the `scroll-margin' effective value is
> capped at a 1/4 of the window height, as seen in this
> window_scroll_pixel_based (window.c):
> 
>   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.

> Whereas, it seems more logical to cap it at half window height.

No, I think it would leave too few lines for moving the cursor.  This
has been Emacs behavior since time immemoriam, so if we want to have a
different behavior, it should be implemented an opt-in option, not the
default.

Message #25 received at 5718 <at> debbugs.gnu.org (full text, mbox):

From: Oleksandr Gavenko <gavenkoa <at> gmail.com>
To: Noam Postavsky <npostavs <at> users.sourceforge.net>
Cc: Andrew Hyatt <ahyatt <at> gmail.com>, 5718 <at> debbugs.gnu.org
Subject: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Fri, 12 Aug 2016 10:54:37 +0300

On 2016-08-11, Noam Postavsky wrote:

> On Thu, Aug 11, 2016 at 9:05 AM, Oleksandr Gavenko <gavenkoa <at> gmail.com> wrote:
>> On reporting day I removed:
>>
>>   (setq-default scroll-margin 4)
>>
>> from ~/.emacs because in "M-x calendar" UP/DOWN navigation become broken.
>>
>> Even in Emacs 24.5.1 setting
>>
>>   (setq scroll-margin 4)
>>
>> in Calendar buffer hide month names on UP/DOWN moves.
>
> This sounds like Bug #10379 which should be fixed as of 24.1 by
> calendar mode resetting scroll-margin locally to 0.
>
> calendar.el:1748:
>
> (define-derived-mode calendar-mode nil "Calendar"
> ...
>   (set (make-local-variable 'scroll-margin) 0) ; bug#10379

I am agree, this was fixed. I made improperly test.

-- 
http://defun.work/

Message #28 received at 5718 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: ahyatt <at> gmail.com, 5718 <at> debbugs.gnu.org, gavenkoa <at> gmail.com
Subject: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Sat, 13 Aug 2016 18:01:43 -0400

Eli Zaretskii <eliz <at> gnu.org> writes:
>> 
>> I think the complaint is that the `scroll-margin' effective value is
>> capped at a 1/4 of the window height, as seen in this
>> window_scroll_pixel_based (window.c):
>> 
>>   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 was looking at fixing it, but I got confused by this comment in
window.h:

/* Height in pixels, and in lines, of the mode line.
   May be zero if W doesn't have a mode line.  */
#define WINDOW_MODE_LINE_HEIGHT(W)	\

How is the height "in pixels, and in lines"?  Doesn't it have to be one
or the other?

>
>> Whereas, it seems more logical to cap it at half window height.
>
> No, I think it would leave too few lines for moving the cursor.  This
> has been Emacs behavior since time immemoriam, so if we want to have a
> different behavior, it should be implemented an opt-in option, not the
> default.

Yes, sure.  This is about Emacs' behaviour after the user has customized
scroll-margin, so the default isn't in question anyway.  I imagine
something like this:

  DEFVAR_LISP ("minimum-non-scroll-lines", Vminimum_non_scroll_lines,
    doc: /* Lines around window's center where `scoll-margin' doesn't apply.
If point is within this many lines from the window's center, it will
not cause scrolling regardless of the value of `scroll-margin'.  If
this is a float then it represents a fraction of the current window's
lines.  */);
  Vminimum_non_scroll_lines = make_float (1.0/4.0);

And then setting this to 1 would make scroll-margin have the effect that
the OP expects.

Message #31 received at 5718 <at> debbugs.gnu.org (full text, mbox):

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: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Sun, 14 Aug 2016 05:36:19 +0300

> From: npostavs <at> users.sourceforge.net
> Cc: 5718 <at> debbugs.gnu.org,  ahyatt <at> gmail.com,  gavenkoa <at> gmail.com
> Date: Sat, 13 Aug 2016 18:01:43 -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 was looking at fixing it, but I got confused by this comment in
> window.h:
> 
> /* Height in pixels, and in lines, of the mode line.
>    May be zero if W doesn't have a mode line.  */
> #define WINDOW_MODE_LINE_HEIGHT(W)	\
> 
> How is the height "in pixels, and in lines"?  Doesn't it have to be one
> or the other?

It's in pixels.  The comment should be fixed.

Thanks.

Message #34 received at 5718 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: ahyatt <at> gmail.com, 5718 <at> debbugs.gnu.org, gavenkoa <at> gmail.com
Subject: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Sun, 11 Sep 2016 16:58:08 -0400

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: npostavs <at> users.sourceforge.net
>> Cc: 5718 <at> debbugs.gnu.org,  ahyatt <at> gmail.com,  gavenkoa <at> gmail.com
>> Date: Sat, 13 Aug 2016 18:01:43 -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.  The patches
come with tests demonstrating this (the tests only work in interactive
mode).

[v1-0001-Refactor-uses-of-scroll_margin-to-a-function.patch.gz (application/octet-stream, attachment)]

[v1-0002-Don-t-count-mode-line-for-scroll-margin-limit.patch (text/plain, attachment)]

[v1-0003-BROKEN-Make-limit-on-scroll-margin-variable.patch (text/plain, attachment)]

[Message part 5 (text/plain, inline)]

>> 
>> 
>> /* Height in pixels, and in lines, of the mode line.
>>    May be zero if W doesn't have a mode line.  */
>> #define WINDOW_MODE_LINE_HEIGHT(W)	\
>> 
>> How is the height "in pixels, and in lines"?  Doesn't it have to be one
>> or the other?
>
> It's in pixels.  The comment should be fixed.

Fixed pushed as ea0f750e, "Fix comments on window height macros"

Message #37 received at 5718 <at> debbugs.gnu.org (full text, mbox):

From: martin rudalics <rudalics <at> gmx.at>
To: npostavs <at> users.sourceforge.net, Eli Zaretskii <eliz <at> gnu.org>
Cc: ahyatt <at> gmail.com, 5718 <at> debbugs.gnu.org, gavenkoa <at> gmail.com
Subject: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Mon, 12 Sep 2016 08:19:59 +0200

> 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.  The patches
> come with tests demonstrating this (the tests only work in interactive
> mode).

Thank you.

       int window_total_lines
-        = window->total_lines * WINDOW_FRAME_LINE_HEIGHT (window)
+        = (window->total_lines * WINDOW_FRAME_LINE_HEIGHT (window)
+           - WINDOW_MODE_LINE_HEIGHT (window))

Please use another name instead of "window_total_lines" here.  And
please explain why you can't use Fwindow_text_height here (i.e., why
header lines, horizontal scrollbars and window dividers apparently don't
count).

+(defmacro window-with-test-buffer-window (&rest body)

Please call it ‘window-test-with-test-buffer-window’ to consistentlyy
keep the ‘window-test-’ prefix on everything defined in this file.

martin

Message #40 received at 5718 <at> debbugs.gnu.org (full text, mbox):

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

Message #43 received at 5718 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: martin rudalics <rudalics <at> gmx.at>
Cc: ahyatt <at> gmail.com, Eli Zaretskii <eliz <at> gnu.org>, 5718 <at> debbugs.gnu.org,
 gavenkoa <at> gmail.com
Subject: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Tue, 13 Sep 2016 22:23:07 -0400

martin rudalics <rudalics <at> gmx.at> writes:

>        int window_total_lines
> -        = window->total_lines * WINDOW_FRAME_LINE_HEIGHT (window)
> +        = (window->total_lines * WINDOW_FRAME_LINE_HEIGHT (window)
> +           - WINDOW_MODE_LINE_HEIGHT (window))
>
> Please use another name instead of "window_total_lines" here.  And
> please explain why you can't use Fwindow_text_height here (i.e., why
> header lines, horizontal scrollbars and window dividers apparently don't
> count).

Oh, I just didn't think of those, I don't think there is a reason not to
use it.  How about this (I use window_box_height instead of
Fwindow_text_height just to save the bother of struct window* to
Lisp_Object conversion):

@@ -4799,10 +4799,7 @@ window_scroll_margin (struct window *window, enum margin_unit unit)
   if (scroll_margin > 0)
     {
       int frame_line_height = default_line_pixel_height (window);
-      int window_total_lines
-        = (window->total_lines * WINDOW_FRAME_LINE_HEIGHT (window)
-           - WINDOW_MODE_LINE_HEIGHT (window))
-        / frame_line_height;
+      int window_lines = window_box_height (window) / frame_line_height;

       int margin, max_margin;
       double ratio = 0.25;
@@ -4812,7 +4809,7 @@ window_scroll_margin (struct window *window, enum margin_unit unit)
           ratio = max (0.0, ratio);
           ratio = min (ratio, 0.5);
         }
-      max_margin = (int) (window_total_lines * ratio);
+      max_margin = (int) (window_lines * ratio);
       margin = max (0, scroll_margin);
       margin = min (scroll_margin, max_margin);


>
> +(defmacro window-with-test-buffer-window (&rest body)
>
> Please call it ‘window-test-with-test-buffer-window’ to consistentlyy
> keep the ‘window-test-’ prefix on everything defined in this file.

Oops, right.

Message #46 received at 5718 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: ahyatt <at> gmail.com, 5718 <at> debbugs.gnu.org, gavenkoa <at> gmail.com
Subject: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Tue, 13 Sep 2016 22:40:29 -0400

Eli Zaretskii <eliz <at> 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.

Message #49 received at 5718 <at> debbugs.gnu.org (full text, mbox):

From: martin rudalics <rudalics <at> gmx.at>
To: npostavs <at> users.sourceforge.net
Cc: ahyatt <at> gmail.com, Eli Zaretskii <eliz <at> gnu.org>, 5718 <at> debbugs.gnu.org,
 gavenkoa <at> gmail.com
Subject: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Wed, 14 Sep 2016 07:30:53 +0200

> How about this (I use window_box_height instead of
> Fwindow_text_height just to save the bother of struct window* to
> Lisp_Object conversion):

No more complaints.

Thank you, martin

Message #52 received at 5718 <at> debbugs.gnu.org (full text, mbox):

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: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Wed, 14 Sep 2016 20:26:57 +0300

> From: npostavs <at> users.sourceforge.net
> Cc: 5718 <at> debbugs.gnu.org,  ahyatt <at> gmail.com,  gavenkoa <at> gmail.com
> Date: Tue, 13 Sep 2016 22:40:29 -0400
> 
> Eli Zaretskii <eliz <at> 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.

I assumed that when maximum margin is set to 0.5, there's no way Emacs
can center point in the window, because wherever it puts point will be
inside the margin.  So therefore, if it sometimes does succeed to
position point, there must be a bug.

However, ...

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

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

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

Some code involved in this probably assumes the margin cannot be that
large.

> > Same here (in another function).
> 
> These are 2 different functions: try_window and try_window_id.

Oops.

Thanks.

Message #55 received at 5718 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: ahyatt <at> gmail.com, 5718 <at> debbugs.gnu.org, gavenkoa <at> gmail.com
Subject: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Mon, 02 Jan 2017 19:48:03 -0500

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: npostavs <at> users.sourceforge.net
>> Cc: 5718 <at> debbugs.gnu.org,  ahyatt <at> gmail.com,  gavenkoa <at> gmail.com
>> Date: Tue, 13 Sep 2016 22:40:29 -0400
>> 
>> Eli Zaretskii <eliz <at> 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.

[v2-0001-Refactor-uses-of-scroll_margin-to-a-function.patch.gz (application/octet-stream, attachment)]

[v2-0002-Don-t-count-mode-line-for-scroll-margin-limit.patch (text/plain, attachment)]

[v2-0003-Make-limit-on-scroll-margin-variable.patch (text/plain, attachment)]

[Message part 5 (text/plain, inline)]

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);

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

For `next-line', the initial difference seems to be in `line-move'.
When `scroll-conservatively' is non-zero (I found no change at 101, in
particular), it calls `vertical-motion` (via `line-move-visual'),
otherwise `line-move-partial'.

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

Message #58 received at 5718 <at> debbugs.gnu.org (full text, mbox):

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: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Sat, 07 Jan 2017 10:17:28 +0200

> From: npostavs <at> users.sourceforge.net
> Cc: 5718 <at> debbugs.gnu.org,  ahyatt <at> gmail.com,  gavenkoa <at> gmail.com
> Date: Mon, 02 Jan 2017 19:48:03 -0500
> 
> 
> [1:text/plain Hide]
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >> From: npostavs <at> users.sourceforge.net
> >> Cc: 5718 <at> debbugs.gnu.org,  ahyatt <at> gmail.com,  gavenkoa <at> gmail.com
> >> Date: Tue, 13 Sep 2016 22:40:29 -0400
> >> 
> >> Eli Zaretskii <eliz <at> 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.

Message #61 received at 5718 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: ahyatt <at> gmail.com, 5718 <at> debbugs.gnu.org, gavenkoa <at> gmail.com
Subject: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Fri, 13 Jan 2017 23:18:56 -0500

Eli Zaretskii <eliz <at> gnu.org> writes:

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

Actually, this is also a problem with partial lines.  I didn't notice at
first, because Emacs produces a window with a partial line by default
when I do `C-x 2'.  AFAICT, it's not related with the new variable,
except that the problem becomes more obvious with maximal margins.  In
Emacs 25.1, using a window where (window-screen-lines) => 20.22, and a
scroll-margin of 100, doing `scroll-down-command' leaves 5 full lines
above point, while `scroll-up-command' leaves 4 full lines (and the
partial one) below point.

So in the code, I think the problem is that it.last_visible_y includes
the partial line, while this_scroll_margin is just the integer
scroll_margin multiplied by pixels per line.  I guess to fix it,
subtracting the partial line height would be more correct than
frame_line_height.

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

Looks like the same kind of problem as the other case, I can fix it
again by subtracting frame_line_height, though again, subtracting the
partial height is probably more correct.

---   i/src/xdisp.c
+++   w/src/xdisp.c
@@ -15369,7 +15369,7 @@ try_scrolling (Lisp_Object window, bool just_this_one_p,
         either that ypos or PT, whichever comes first.  */
       start_display (&it, w, startp);
       scroll_margin_y = it.last_visible_y - this_scroll_margin
-        - frame_line_height * extra_scroll_margin_lines;
+        - frame_line_height * (1 + extra_scroll_margin_lines);
       move_it_to (&it, PT, -1, scroll_margin_y - 1, -1,
                  (MOVE_TO_POS | MOVE_TO_Y));

Message #64 received at 5718 <at> debbugs.gnu.org (full text, mbox):

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: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Sat, 14 Jan 2017 09:58:38 +0200

> From: npostavs <at> users.sourceforge.net
> Cc: 5718 <at> debbugs.gnu.org,  ahyatt <at> gmail.com,  gavenkoa <at> gmail.com
> Date: Fri, 13 Jan 2017 23:18:56 -0500
> 
> Looks like the same kind of problem as the other case, I can fix it
> again by subtracting frame_line_height, though again, subtracting the
> partial height is probably more correct.

I think you are right on both counts.  Computing the partial height of
the last visible line shouldn't be hard, I think.  Let me know if you
need help with that.

Thanks.

Message #67 received at 5718 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: ahyatt <at> gmail.com, 5718 <at> debbugs.gnu.org, gavenkoa <at> gmail.com
Subject: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Sun, 15 Jan 2017 16:43:34 -0500

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: npostavs <at> users.sourceforge.net
>> Cc: 5718 <at> debbugs.gnu.org,  ahyatt <at> gmail.com,  gavenkoa <at> gmail.com
>> Date: Fri, 13 Jan 2017 23:18:56 -0500
>> 
>> Looks like the same kind of problem as the other case, I can fix it
>> again by subtracting frame_line_height, though again, subtracting the
>> partial height is probably more correct.
>
> I think you are right on both counts.  Computing the partial height of
> the last visible line shouldn't be hard, I think.  Let me know if you
> need help with that.

Yeah, I'm still a bit lost in all the different structures.  It seems
(it.last_visible_y % frame_line_height) might work.  The matrix row
stuff in xdisp seems a more direct way of getting it, though I guess
that's not really usable from window.c?  What would be the idiomatic way
of doing this?

#define MR_PARTIALLY_VISIBLE(ROW)	\
  ((ROW)->height != (ROW)->visible_height)


By the way, in window_scroll_pixel_based (lines 5139 to 5158) the
position of "it" is saved twice, and it looks like the first one of
these can never be used (because the second overwrites it), correct?

      ptrdiff_t charpos, bytepos;
      bool partial_p;

      /* Save our position, for the   <---------------- Useless?
	 window_scroll_pixel_based_preserve_y case.  */
      charpos = IT_CHARPOS (it);
      bytepos = IT_BYTEPOS (it);

      /* We moved the window start towards BEGV, so PT may be now
	 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 - frame_line_height),
		  -1,
		  MOVE_TO_POS | MOVE_TO_Y);

      /* Save our position, in case it's correct.  */
      charpos = IT_CHARPOS (it);
      bytepos = IT_BYTEPOS (it);

Message #70 received at 5718 <at> debbugs.gnu.org (full text, mbox):

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: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Mon, 16 Jan 2017 19:08:37 +0200

> From: npostavs <at> users.sourceforge.net
> Cc: 5718 <at> debbugs.gnu.org,  ahyatt <at> gmail.com,  gavenkoa <at> gmail.com
> Date: Sun, 15 Jan 2017 16:43:34 -0500
> 
> Yeah, I'm still a bit lost in all the different structures.  It seems
> (it.last_visible_y % frame_line_height) might work.

That would only be correct if all the screen lines use the default
font.  Otherwise, the last line could be only partially visible even
if the above is zero.

> The matrix row stuff in xdisp seems a more direct way of getting it,
> though I guess that's not really usable from window.c?

If by "matrix row" you meant the glyph matrix prepared by the previous
redisplay, then using that is unreliable, because that matrix might
not be up to date.

You can see that window_scroll_pixel_based already uses the move_it_*
family of functions, which is the right way of doing this stuff --
these functions simulate redisplay without actually displaying
anything, so you can compute the metrics of anything on display using
them.

> What would be the idiomatic way of doing this?
> 
> #define MR_PARTIALLY_VISIBLE(ROW)	\
>   ((ROW)->height != (ROW)->visible_height)

There's already such a calculation in window_scroll_pixel_based:

      /* See if point is on a partially visible line at the end.  */
      if (it.what == IT_EOB)
	partial_p = it.current_y + it.ascent + it.descent > it.last_visible_y;
      else
	{
	  move_it_by_lines (&it, 1);
	  partial_p = it.current_y > it.last_visible_y;
	}

(This is preceded by moving the iterator to the point's screen line or
to EOB, whichever comes first.)  The value it.current_y is the Y
coordinate of the top edge of a glyph row (the value is zero for the
first screen line), so if it.last_visible_y is farther away from that
than the height of the glyph row, that glyph row is fully visible;
otherwise it isn't.

> By the way, in window_scroll_pixel_based (lines 5139 to 5158) the
> position of "it" is saved twice, and it looks like the first one of
> these can never be used (because the second overwrites it), correct?

Yep, good catch.

Thanks.

Message #73 received at 5718 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: ahyatt <at> gmail.com, 5718 <at> debbugs.gnu.org, gavenkoa <at> gmail.com
Subject: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Sat, 21 Jan 2017 13:46:37 -0500

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

>
> There's already such a calculation in window_scroll_pixel_based:
>
>       /* See if point is on a partially visible line at the end.  */
>       if (it.what == IT_EOB)
> 	partial_p = it.current_y + it.ascent + it.descent > it.last_visible_y;
>       else
> 	{
> 	  move_it_by_lines (&it, 1);
> 	  partial_p = it.current_y > it.last_visible_y;
> 	}
>
> (This is preceded by moving the iterator to the point's screen line or
> to EOB, whichever comes first.)  The value it.current_y is the Y
> coordinate of the top edge of a glyph row (the value is zero for the
> first screen line), so if it.last_visible_y is farther away from that
> than the height of the glyph row, that glyph row is fully visible;
> otherwise it isn't.

I'm not entirely clear why there is a branch in that code.  I came up
with this; it works, though I'm not sure if it's the best way:

[v3-0001-Fix-scrolling-with-partial-lines.patch (text/x-diff, inline)]

From 6944c7badade2477f381acf3b0922e5309e063d8 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs <at> gmail.com>
Date: Sat, 21 Jan 2017 13:24:47 -0500
Subject: [PATCH v3] Fix scrolling with partial lines

* src/xdisp.c (partial_line_height): New function.
(try_scrolling):
* src/window.c (window_scroll_pixel_based): Use it for calculating the
pixel scroll margin correctly in a window with partial lines.
---
 src/dispextern.h |  1 +
 src/window.c     |  2 +-
 src/xdisp.c      | 14 +++++++++++++-
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/src/dispextern.h b/src/dispextern.h
index 51222e6..470330a 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3263,6 +3263,7 @@ void move_it_past_eol (struct it *);
 void move_it_in_display_line (struct it *it,
 			      ptrdiff_t to_charpos, int to_x,
 			      enum move_operation_enum op);
+int partial_line_height (const struct it *it_origin);
 bool in_display_vector_p (struct it *);
 int frame_mode_line_height (struct frame *);
 extern bool redisplaying_p;
diff --git a/src/window.c b/src/window.c
index f664597..300472e 100644
--- 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),
+                   - partial_line_height (&it)- this_scroll_margin - 1),
 		  -1,
 		  MOVE_TO_POS | MOVE_TO_Y);
 
diff --git a/src/xdisp.c b/src/xdisp.c
index 55bb34a..9dc65b8 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -9859,6 +9859,17 @@ move_it_by_lines (struct it *it, ptrdiff_t dvpos)
     }
 }
 
+int
+partial_line_height (const struct it *it_origin)
+{
+  struct it it = *it_origin;
+  move_it_to (&it, ZV, -1, it.last_visible_y, -1,
+              MOVE_TO_POS | MOVE_TO_Y);
+  int vis_height = it.last_visible_y - it.current_y;
+  int log_height = it.ascent + it.descent;
+  return max (0, log_height - vis_height);
+}
+
 /* Return true if IT points into the middle of a display vector.  */
 
 bool
@@ -15368,7 +15379,8 @@ try_scrolling (Lisp_Object window, bool just_this_one_p,
       /* Compute the pixel ypos of the scroll margin, then move IT to
 	 either that ypos or PT, whichever comes first.  */
       start_display (&it, w, startp);
-      scroll_margin_y = it.last_visible_y - this_scroll_margin
+      scroll_margin_y = it.last_visible_y - partial_line_height (&it)
+        - this_scroll_margin
 	- frame_line_height * extra_scroll_margin_lines;
       move_it_to (&it, PT, -1, scroll_margin_y - 1, -1,
 		  (MOVE_TO_POS | MOVE_TO_Y));
-- 
2.9.3

Message #76 received at 5718 <at> debbugs.gnu.org (full text, mbox):

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: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Sat, 21 Jan 2017 21:17:05 +0200

> From: npostavs <at> users.sourceforge.net
> Cc: 5718 <at> debbugs.gnu.org,  ahyatt <at> gmail.com,  gavenkoa <at> gmail.com
> Date: Sat, 21 Jan 2017 13:46:37 -0500
> 
> >       /* See if point is on a partially visible line at the end.  */
> >       if (it.what == IT_EOB)
> > 	partial_p = it.current_y + it.ascent + it.descent > it.last_visible_y;
> >       else
> > 	{
> > 	  move_it_by_lines (&it, 1);
> > 	  partial_p = it.current_y > it.last_visible_y;
> > 	}
> >
> > (This is preceded by moving the iterator to the point's screen line or
> > to EOB, whichever comes first.)  The value it.current_y is the Y
> > coordinate of the top edge of a glyph row (the value is zero for the
> > first screen line), so if it.last_visible_y is farther away from that
> > than the height of the glyph row, that glyph row is fully visible;
> > otherwise it isn't.
> 
> I'm not entirely clear why there is a branch in that code.

Because of line-spacing, perhaps?  Did you test your code when
line-spacing is at non-default value?  In general, it is safer to go
to the next screen line than reason about where it will start.

> +int
> +partial_line_height (const struct it *it_origin)
> +{
> +  struct it it = *it_origin;

When you copy the iterator structure and modify the copy, you need to
save and restore the bidi cache, using SAVE_IT and RESTORE_IT macros.
Otherwise, the code which calls this function will work incorrectly if
it uses the original iterator after the call, because the bidi cache
is not restored to its state before the call.

Message #79 received at 5718 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: ahyatt <at> gmail.com, 5718 <at> debbugs.gnu.org, gavenkoa <at> gmail.com
Subject: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Sun, 22 Jan 2017 12:21:20 -0500

npostavs <at> users.sourceforge.net writes:
>
> I came up with this; it works

Actually, that doesn't work, I must have not compiled before testing.

Eli Zaretskii <eliz <at> gnu.org> writes:
>> 
>> I'm not entirely clear why there is a branch in that code.
>
> Because of line-spacing, perhaps?  Did you test your code when
> line-spacing is at non-default value?  In general, it is safer to go
> to the next screen line than reason about where it will start.
>
>> +int
>> +partial_line_height (const struct it *it_origin)
>> +{
>> +  struct it it = *it_origin;
>
> When you copy the iterator structure and modify the copy, you need to
> save and restore the bidi cache, using SAVE_IT and RESTORE_IT macros.
> Otherwise, the code which calls this function will work incorrectly if
> it uses the original iterator after the call, because the bidi cache
> is not restored to its state before the call.

I came up with this modified version of partial_line_height, which does
actually work.  Having to do RESTORE_IT (&it, &it, it_data) is a bit
unintuitive though.  I don't understand what the typical use case of
this macro is, if it's going to copy back the new state into the
original `it', then why use a separate `it' in the first place?

By the way, while testing I noticed that `set-window-text-height'
doesn't take `line-spacing' into account, should it?


int
partial_line_height (struct it *it_origin)
{
  int partial_height;
  void *it_data = NULL;
  struct it it;
  SAVE_IT (it, *it_origin, it_data);
  move_it_to (&it, ZV, -1, it.last_visible_y, -1,
              MOVE_TO_POS | MOVE_TO_Y);
  if (it.what == IT_EOB)
    {
      int vis_height = it.last_visible_y - it.current_y;
      int height = it.ascent + it.descent;
      partial_height = (vis_height < height) ? vis_height : 0;
    }
  else
    {
      int last_line_y = it.current_y;
      move_it_by_lines (&it, 1);
      partial_height = (it.current_y > it.last_visible_y)
        ? it.last_visible_y - last_line_y : 0;
    }
  RESTORE_IT (&it, &it, it_data);
  return partial_height;
}

Message #82 received at 5718 <at> debbugs.gnu.org (full text, mbox):

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: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Sun, 22 Jan 2017 19:58:32 +0200

> From: npostavs <at> users.sourceforge.net
> Cc: 5718 <at> debbugs.gnu.org,  ahyatt <at> gmail.com,  gavenkoa <at> gmail.com
> Date: Sun, 22 Jan 2017 12:21:20 -0500
> 
> > When you copy the iterator structure and modify the copy, you need to
> > save and restore the bidi cache, using SAVE_IT and RESTORE_IT macros.
> > Otherwise, the code which calls this function will work incorrectly if
> > it uses the original iterator after the call, because the bidi cache
> > is not restored to its state before the call.
> 
> I came up with this modified version of partial_line_height, which does
> actually work.

It looks good to me, thanks.

> Having to do RESTORE_IT (&it, &it, it_data) is a bit
> unintuitive though.  I don't understand what the typical use case of
> this macro is, if it's going to copy back the new state into the
> original `it', then why use a separate `it' in the first place?

If you don't modify the original iterator object, then you indeed
don't need to copy back, you only need to restore the bidi cache
state, by calling bidi_unshelve_cache directly, as some code fragments
actually do.  (The macro refrains from copying if the two arguments
point to the same object, so it transparently does this for you.)

More generally, two separate arguments are needed because some of the
macro uses choose one of several copies to copy back.  For example:

  if (result == MOVE_LINE_CONTINUED
      && it->line_wrap == WORD_WRAP
      && wrap_it.sp >= 0
      && ((atpos_it.sp >= 0 && wrap_it.current_x < atpos_it.current_x)
	  || (atx_it.sp >= 0 && wrap_it.current_x < atx_it.current_x)))
    RESTORE_IT (it, &wrap_it, wrap_data);
  else if (atpos_it.sp >= 0)
    RESTORE_IT (it, &atpos_it, atpos_data);
  else if (atx_it.sp >= 0)
    RESTORE_IT (it, &atx_it, atx_data);

The general paradigm is:

  . save 'it' in some local variable
  . modify 'it'
  . restore 'it' from the local variable
  . continue using 'it' as if the modification never happened

> By the way, while testing I noticed that `set-window-text-height'
> doesn't take `line-spacing' into account, should it?

No, because it returns its value in canonical line units, which means
no line-spacing.

Message #85 received at 5718 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: ahyatt <at> gmail.com, 5718 <at> debbugs.gnu.org, gavenkoa <at> gmail.com
Subject: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Sat, 28 Jan 2017 19:57:21 -0500

[Message part 1 (text/plain, inline)]

tags 5718 patch
quit

Eli Zaretskii <eliz <at> gnu.org> writes:

>> 
>> I came up with this modified version of partial_line_height, which does
>> actually work.
>
> It looks good to me, thanks.

Okay, I think this is ready now, I'm posting the final patchset for
reference.  It's basically the same as the previous stuff, but I've made
maximum-scroll-margin customizable, and renamed window-tests.el to
test/manual/scroll-tests.el (since it only works in an interactive
session).

I'll push to master in a couple of days.

[v4-0001-Refactor-uses-of-scroll_margin-to-a-function.patch.gz (application/octet-stream, attachment)]

[v4-0002-Don-t-count-mode-line-for-scroll-margin-limit.patch (text/x-diff, attachment)]

[v4-0003-Make-limit-on-scroll-margin-variable.patch (text/x-diff, attachment)]

[v4-0004-Fix-scrolling-with-partial-lines.patch (text/x-diff, attachment)]

[v4-0005-Add-tests-for-scrolling.patch (text/x-diff, attachment)]

Message #90 received at 5718 <at> debbugs.gnu.org (full text, mbox):

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: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Mon, 30 Jan 2017 17:29:26 +0200

> From: npostavs <at> users.sourceforge.net
> Cc: 5718 <at> debbugs.gnu.org,  ahyatt <at> gmail.com,  gavenkoa <at> gmail.com
> Date: Sat, 28 Jan 2017 19:57:21 -0500
> 
> Okay, I think this is ready now, I'm posting the final patchset for
> reference.  It's basically the same as the previous stuff, but I've made
> maximum-scroll-margin customizable, and renamed window-tests.el to
> test/manual/scroll-tests.el (since it only works in an interactive
> session).
> 
> I'll push to master in a couple of days.

Thanks, it LGTM.  Would you mind to mention the new option in the user
manual?

Message #93 received at 5718 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: ahyatt <at> gmail.com, 5718 <at> debbugs.gnu.org, gavenkoa <at> gmail.com
Subject: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Mon, 30 Jan 2017 23:52:22 -0500

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: npostavs <at> users.sourceforge.net
>> Cc: 5718 <at> debbugs.gnu.org,  ahyatt <at> gmail.com,  gavenkoa <at> gmail.com
>> Date: Sat, 28 Jan 2017 19:57:21 -0500
>> 
>> Okay, I think this is ready now, I'm posting the final patchset for
>> reference.  It's basically the same as the previous stuff, but I've made
>> maximum-scroll-margin customizable, and renamed window-tests.el to
>> test/manual/scroll-tests.el (since it only works in an interactive
>> session).
>> 
>> I'll push to master in a couple of days.
>
> Thanks, it LGTM.  Would you mind to mention the new option in the user
> manual?

Oh right, forgot about that.  Does this look okay?

diff --git c/doc/emacs/display.texi i/doc/emacs/display.texi
index c6e990d..4c4d696 100644
--- c/doc/emacs/display.texi
+++ i/doc/emacs/display.texi
@@ -285,13 +285,17 @@ Auto Scrolling
 @code{scroll-up-aggressively} / @code{scroll-down-aggressively}.
 
 @vindex scroll-margin
+@vindex maximum-scroll-margin
   The variable @code{scroll-margin} restricts how close point can come
 to the top or bottom of a window (even if aggressive scrolling
 specifies a fraction @var{f} that is larger than the window portion
 between the top and the bottom margins).  Its value is a number of screen
 lines; if point comes within that many lines of the top or bottom of
 the window, Emacs performs automatic scrolling.  By default,
-@code{scroll-margin} is 0.
+@code{scroll-margin} is 0.  The effective margin size is limited to a
+quarter of the window height by default, but this limit can be
+increased up to half (or decreased down to zero) by customizing
+@code{maximum-scroll-margin}.
 
 @node Horizontal Scrolling
 @section Horizontal Scrolling
diff --git c/doc/lispref/windows.texi i/doc/lispref/windows.texi
index 6f3de0c..affa28c 100644
--- c/doc/lispref/windows.texi
+++ i/doc/lispref/windows.texi
@@ -3924,6 +3924,21 @@ Textual Scrolling
 out of the margin, closer to the center of the window.
 @end defopt
 
+@defopt maximum-scroll-margin
+This variable limits the effective value of @code{scroll-margin} to a
+fraction of the current window line height.  For example, if the
+current window has 20 lines and @code{maximum-scroll-margin} is 0.1,
+then the scroll margins will never be larger than 2 lines, no matter
+how big @code{scroll-margin} is.
+
+@code{maximum-scroll-margin} itself has a maximum value of 0.5, which
+allows setting margins large to keep the cursor at the middle line of
+the window (or two middle lines if the window has an even number of
+lines).  If it's set to a larger value (or any value other than a
+float between 0.0 and 0.5) then the default value of 0.25 will be used
+instead.
+@end defopt
+
 @defopt scroll-conservatively
 This variable controls how scrolling is done automatically when point
 moves off the screen (or into the scroll margin).  If the value is a

Message #96 received at 5718 <at> debbugs.gnu.org (full text, mbox):

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: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Tue, 31 Jan 2017 17:33:21 +0200

> From: npostavs <at> users.sourceforge.net
> Cc: 5718 <at> debbugs.gnu.org,  ahyatt <at> gmail.com,  gavenkoa <at> gmail.com
> Date: Mon, 30 Jan 2017 23:52:22 -0500
> 
> > Thanks, it LGTM.  Would you mind to mention the new option in the user
> > manual?
> 
> Oh right, forgot about that.  Does this look okay?

Yes, thanks.

Message #99 received at 5718 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: ahyatt <at> gmail.com, 5718 <at> debbugs.gnu.org, gavenkoa <at> gmail.com
Subject: Re: bug#5718: scroll-margin in buffer with small line count.
Date: Thu, 02 Feb 2017 21:40:57 -0500

tags 5718 fixed
close 5718 26.1
quit

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: npostavs <at> users.sourceforge.net
>> Cc: 5718 <at> debbugs.gnu.org,  ahyatt <at> gmail.com,  gavenkoa <at> gmail.com
>> Date: Mon, 30 Jan 2017 23:52:22 -0500
>> 
>> > Thanks, it LGTM.  Would you mind to mention the new option in the user
>> > manual?
>> 
>> Oh right, forgot about that.  Does this look okay?
>
> Yes, thanks.

Pushed to master [1: ce88155].

1: 2017-02-02 21:35:51 -0500 ce88155d83ba84e84321ed69a39c82f40117dd1f
  ; Merge: fixes and updates to scroll margin (Bug#5718)

Previous Next

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