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

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

[Oleksandr Gavenko]

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!

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

[Andrew Hyatt]

Oleksandr Gavenko <gavenkoa@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.

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

[npostavs]

Andrew Hyatt <ahyatt@gmail.com> writes:

> Oleksandr Gavenko <gavenkoa@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).

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

[Oleksandr Gavenko]

On 2016-08-11, npostavs@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/

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

[Noam Postavsky]

On Thu, Aug 11, 2016 at 9:05 AM, Oleksandr Gavenko <gavenkoa@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

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

[Eli Zaretskii]

> From: npostavs@users.sourceforge.net
> Date: Thu, 11 Aug 2016 08:03:18 -0400
> Cc: 5718@debbugs.gnu.org, Oleksandr Gavenko <gavenkoa@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.

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

[Oleksandr Gavenko]

On 2016-08-11, Noam Postavsky wrote:

> On Thu, Aug 11, 2016 at 9:05 AM, Oleksandr Gavenko <gavenkoa@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/

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

[npostavs]

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

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

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

[npostavs]

[-- Attachment #1: Type: text/plain, Size: 1013 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

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


[-- Attachment #2: compressed patch --]
[-- Type: application/octet-stream, Size: 3399 bytes --]

[-- Attachment #3: patch --]
[-- Type: text/plain, Size: 4819 bytes --]

From 0f3a50fa311e6ec1ccb1a0d03d521cc2ef9b309a Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sun, 28 Aug 2016 17:23:04 -0400
Subject: [PATCH v1 2/3] Don't count mode line for scroll-margin limit

* src/window.c (window_scroll_margin): Subtract mode line height from
upper limit applied to scroll margin (Bug #5718).
* test/src/window-tests.el: New tests for scroll-margin behavior.
---
 src/window.c             |  3 +-
 test/src/window-tests.el | 86 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 88 insertions(+), 1 deletion(-)
 create mode 100644 test/src/window-tests.el

diff --git a/src/window.c b/src/window.c
index 2046fe7..dbda435 100644
--- a/src/window.c
+++ b/src/window.c
@@ -4800,7 +4800,8 @@ window_scroll_margin (struct window *window, enum margin_unit unit)
     {
       int frame_line_height = default_line_pixel_height (window);
       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))
         / frame_line_height;
       int margin = min (scroll_margin, window_total_lines / 4);
       if (unit == MARGIN_IN_PIXELS)
diff --git a/test/src/window-tests.el b/test/src/window-tests.el
new file mode 100644
index 0000000..88ded18
--- /dev/null
+++ b/test/src/window-tests.el
@@ -0,0 +1,86 @@
+;;; window-tests.el -- tests for window.c -*- lexical-binding: t -*-
+
+;; Copyright (C) 2016 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Code:
+
+(require 'ert)
+
+(defun window-test-scrolling (margin &optional effective-margin)
+  (unless effective-margin
+    (setq effective-margin margin))
+  (erase-buffer)
+  (insert (mapconcat #'number-to-string
+                     (number-sequence 1 200) "\n"))
+  (goto-char 1)
+  (sit-for 0)
+  (let ((scroll-margin margin)
+        (mode-lines (/ (window-mode-line-height) (line-pixel-height)))
+        (wstart (window-start)))
+    ;; Stopping before `scroll-margin' so we shouldn't have
+    ;; scrolled.
+    (let ((current-prefix-arg (- (window-height)
+                                 mode-lines (window-start) effective-margin)))
+      (call-interactively 'next-line))
+    (sit-for 0)
+    (should (= wstart (window-start)))
+    ;; Passing `scroll-margin' should trigger scrolling.
+    (call-interactively 'next-line)
+    (sit-for 0)
+    (should (/= wstart (window-start)))
+    ;; Scroll back to top.
+    (let ((current-prefix-arg (window-start)))
+      (call-interactively 'scroll-down-command))
+    (sit-for 0)
+    (should (= 1 (window-start)))))
+
+(defmacro window-with-test-buffer-window (&rest body)
+  (let ((bufvar (make-symbol "buf")))
+    `(let ((,bufvar (get-buffer-create "*test*")))
+       (with-selected-window (display-buffer ,bufvar)
+         (with-current-buffer ,bufvar
+           ,@body)))))
+
+(ert-deftest window-test-scroll-margin-0 ()
+  (skip-unless (not noninteractive))
+  (window-with-test-buffer-window
+   (window-test-scrolling 0)))
+
+(ert-deftest window-test-scroll-margin-negative ()
+  "A negative `scroll-margin' should be the same as 0."
+  (skip-unless (not noninteractive))
+  (window-with-test-buffer-window
+   (window-test-scrolling -10 0)))
+
+(ert-deftest window-test-scroll-margin-max ()
+  (skip-unless (not noninteractive))
+  (window-with-test-buffer-window
+   (let* ((mode-lines (/ (window-mode-line-height) (line-pixel-height)))
+          (max-margin (/ (- (window-height) mode-lines) 4)))
+     (window-test-scrolling max-margin))))
+
+(ert-deftest window-test-scroll-margin-over-max ()
+  "A `scroll-margin' more than max should be the same as max."
+  (skip-unless (not noninteractive))
+  (window-with-test-buffer-window
+   ;; Check that mode line is not counted for determining max margin.
+   (set-window-text-height nil 7)
+   (let* ((mode-lines (/ (window-mode-line-height) (line-pixel-height)))
+          (max-margin (/ (- (window-height) mode-lines) 4)))
+     (window-test-scrolling (+ max-margin 1) max-margin)
+     (window-test-scrolling (+ max-margin 2) max-margin))))
-- 
2.9.3


[-- Attachment #4: patch --]
[-- Type: text/plain, Size: 4137 bytes --]

From 0697341a9cdc1f9962a7b984e2a8ffe5150831f5 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sun, 11 Sep 2016 11:09:57 -0400
Subject: [PATCH v1 3/3] [BROKEN] Make limit on scroll-margin variable

[BROKEN]: The `scroll-down-command' doesn't stay in the middle of the
window.

* src/xdisp.c (maximum-scroll-margin): New variable.
* src/window.c (window_scroll_pixel_based): Use it instead of hardcoding
division by 4 (Bug #5718).
* test/src/window-tests.el (window-test-scroll-margin-whole-window):
Test it.
---
 src/window.c             | 13 ++++++++++++-
 src/xdisp.c              |  5 +++++
 test/src/window-tests.el | 30 ++++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+), 1 deletion(-)

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.  */);
+  Vmaximum_scroll_margin = make_float (0.25);
+
   DEFVAR_LISP ("display-pixels-per-inch",  Vdisplay_pixels_per_inch,
     doc: /* Pixels per inch value for non-window system displays.
 Value is a number or a cons (WIDTH-DPI . HEIGHT-DPI).  */);
diff --git a/test/src/window-tests.el b/test/src/window-tests.el
index 88ded18..c6152c8 100644
--- a/test/src/window-tests.el
+++ b/test/src/window-tests.el
@@ -50,6 +50,7 @@ window-test-scrolling
     (should (= 1 (window-start)))))
 
 (defmacro window-with-test-buffer-window (&rest body)
+  (declare (debug t))
   (let ((bufvar (make-symbol "buf")))
     `(let ((,bufvar (get-buffer-create "*test*")))
        (with-selected-window (display-buffer ,bufvar)
@@ -84,3 +85,32 @@ window-with-test-buffer-window
           (max-margin (/ (- (window-height) mode-lines) 4)))
      (window-test-scrolling (+ max-margin 1) max-margin)
      (window-test-scrolling (+ max-margin 2) max-margin))))
+
+(defun window-test--point-in-middle-of-window-p ()
+  (= (count-lines (window-start) (window-point))
+     (- (count-lines (window-point) (window-end)) 1
+        (if (pos-visible-in-window-p (window-end)) 0 1))))
+
+(ert-deftest window-test-scroll-margin-whole-window ()
+  "Test `maximum-scroll-margin' at 0.5.
+With a high `scroll-margin', this should keep cursor in the
+middle of the window."
+  (skip-unless (not noninteractive))
+  (let ((maximum-scroll-margin 0.5)
+        (scroll-margin 100))
+    (window-with-test-buffer-window
+     (set-window-text-height nil 7)
+     (erase-buffer)
+     (insert (mapconcat #'number-to-string
+                        (number-sequence 1 200) "\n"))
+     (goto-char 1)
+     (sit-for 0)
+     (call-interactively 'scroll-up-command)
+     (sit-for 0)
+     (should (window-test--point-in-middle-of-window-p))
+     (call-interactively 'scroll-up-command)
+     (sit-for 0)
+     (should (window-test--point-in-middle-of-window-p))
+     (call-interactively 'scroll-down-command)
+     (sit-for 0)
+     (should (window-test--point-in-middle-of-window-p)))))
-- 
2.9.3


[-- Attachment #5: Type: text/plain, Size: 371 bytes --]



>> 
>> 
>> /* 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"

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

[martin rudalics]

 > 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

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.

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

[npostavs]

martin rudalics <rudalics@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.

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.

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

[martin rudalics]

 > 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

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: Tue, 13 Sep 2016 22:40:29 -0400
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> >> I have a patch set for fixing this and allowing the user to change the
> >> maximum margin from 0.25.  The latter doesn't quite work perfectly, for
> >> some reason when setting the maximum margin to 0.5 and scroll-margin to
> >> 100, `scroll-down-command' doesn't keep point centered in the window,
> >> even though other commands (e.g. `scroll-up-command') do.
> >
> > 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.

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

[npostavs]

[-- Attachment #1: Type: text/plain, Size: 887 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

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

Yes, I hadn't quite thought it through, but that was my intention.  I've
updated the code to clarify this.


[-- Attachment #2: patch --]
[-- Type: application/octet-stream, Size: 3553 bytes --]

[-- Attachment #3: patch --]
[-- Type: text/plain, Size: 4647 bytes --]

From 4293c63254fe9ae38391aed1896436c79ebb8f00 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sun, 28 Aug 2016 17:23:04 -0400
Subject: [PATCH v2 2/3] Don't count mode line for scroll-margin limit

* src/window.c (window_scroll_margin): Use window_box_height to avoid
counting header line, scrollbars for scroll-margin limit (Bug #5718).
* test/src/window-tests.el: New tests for scroll-margin behavior.
---
 src/window.c             |  6 ++--
 test/src/window-tests.el | 84 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 86 insertions(+), 4 deletions(-)
 create mode 100644 test/src/window-tests.el

diff --git a/src/window.c b/src/window.c
index 3cea9d0..b710333 100644
--- a/src/window.c
+++ b/src/window.c
@@ -4801,10 +4801,8 @@ 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)
-        / frame_line_height;
-      int margin = min (scroll_margin, window_total_lines / 4);
+      int window_lines = window_box_height (window) / frame_line_height;
+      int margin = min (scroll_margin, window_lines / 4);
       if (unit == MARGIN_IN_PIXELS)
         return margin * frame_line_height;
       else
diff --git a/test/src/window-tests.el b/test/src/window-tests.el
new file mode 100644
index 0000000..f5cca73
--- /dev/null
+++ b/test/src/window-tests.el
@@ -0,0 +1,84 @@
+;;; window-tests.el -- tests for window.c -*- lexical-binding: t -*-
+
+;; Copyright (C) 2016 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Code:
+
+(require 'ert)
+
+(defun window-tests-scrolling (margin &optional effective-margin)
+  (unless effective-margin
+    (setq effective-margin margin))
+  (erase-buffer)
+  (insert (mapconcat #'number-to-string
+                     (number-sequence 1 200) "\n"))
+  (goto-char 1)
+  (sit-for 0)
+  (let ((scroll-margin margin)
+        (wstart (window-start)))
+    ;; Stopping before `scroll-margin' so we shouldn't have
+    ;; scrolled.
+    (let ((current-prefix-arg (- (window-text-height) 1 effective-margin)))
+      (call-interactively 'next-line))
+    (sit-for 0)
+    (should (= wstart (window-start)))
+    ;; Passing `scroll-margin' should trigger scrolling.
+    (call-interactively 'next-line)
+    (sit-for 0)
+    (should (/= wstart (window-start)))
+    ;; Scroll back to top.
+    (let ((current-prefix-arg (window-start)))
+      (call-interactively 'scroll-down-command))
+    (sit-for 0)
+    (should (= 1 (window-start)))))
+
+(defmacro window-tests-with-buffer-window (&rest body)
+  (declare (debug t))
+  (let ((bufvar (make-symbol "buf")))
+    `(let ((,bufvar (get-buffer-create "*test*")))
+       (with-selected-window (display-buffer ,bufvar)
+         (with-current-buffer ,bufvar
+           ,@body)))))
+
+(ert-deftest window-tests-scroll-margin-0 ()
+  (skip-unless (not noninteractive))
+  (window-tests-with-buffer-window
+   (window-tests-scrolling 0)))
+
+(ert-deftest window-tests-scroll-margin-negative ()
+  "A negative `scroll-margin' should be the same as 0."
+  (skip-unless (not noninteractive))
+  (window-tests-with-buffer-window
+   (window-tests-scrolling -10 0)))
+
+(ert-deftest window-tests-scroll-margin-max ()
+  (skip-unless (not noninteractive))
+  (window-tests-with-buffer-window
+   (let ((max-margin (/ (window-text-height) 4)))
+     (window-tests-scrolling max-margin))))
+
+(ert-deftest window-tests-scroll-margin-over-max ()
+  "A `scroll-margin' more than max should be the same as max."
+  (skip-unless (not noninteractive))
+  (window-tests-with-buffer-window
+   (set-window-text-height nil 7)
+   (let ((max-margin (/ (window-text-height) 4)))
+     (window-tests-scrolling (+ max-margin 1) max-margin)
+     (window-tests-scrolling (+ max-margin 2) max-margin))))
+
+;;; window-tests.el ends here
-- 
2.9.3


[-- Attachment #4: patch --]
[-- Type: text/plain, Size: 4735 bytes --]

From 87ad96cc13ca45eb5c78e3773cfb1458001f9d83 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sun, 11 Sep 2016 11:09:57 -0400
Subject: [PATCH v2 3/3] Make limit on scroll-margin variable

TODO: `scroll-down-command' doesn't stay in the middle of the window.

* src/xdisp.c (maximum-scroll-margin): New variable.
* etc/NEWS: Mention it.
* src/window.c (window_scroll_pixel_based): Use it instead of hardcoding
division by 4 (Bug #5718).
* test/src/window-tests.el (window-tests-scroll-margin-whole-window):
(window-tests--point-in-middle-of-window-p): New test and helper
function.
---
 etc/NEWS                 |  4 ++++
 src/window.c             | 19 ++++++++++++++-----
 src/xdisp.c              |  8 ++++++++
 test/src/window-tests.el | 27 +++++++++++++++++++++++++++
 4 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index d91204b..f4fa98a 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -298,6 +298,10 @@ local part of a remote file name.  Thus, if you have a directory named
 "/~" on the remote host "foo", you can prevent it from being
 substituted by a home directory by writing it as "/foo:/:/~/file".
 
+** The new variable 'maximum-scroll-margin' allows having effective
+settings of 'scroll-margin' up to half the window size, instead of
+always restricting the margin to a quarter of the window.
+
 \f
 * Editing Changes in Emacs 26.1
 
diff --git a/src/window.c b/src/window.c
index b710333..f664597 100644
--- a/src/window.c
+++ b/src/window.c
@@ -4802,11 +4802,20 @@ window_scroll_margin (struct window *window, enum margin_unit unit)
     {
       int frame_line_height = default_line_pixel_height (window);
       int window_lines = window_box_height (window) / frame_line_height;
-      int margin = min (scroll_margin, window_lines / 4);
-      if (unit == MARGIN_IN_PIXELS)
-        return margin * frame_line_height;
-      else
-        return 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);
+        }
+      int max_margin = min ((window_lines - 1)/2,
+                            (int) (window_lines * ratio));
+      int margin = clip_to_bounds (0, scroll_margin, max_margin);
+      return (unit == MARGIN_IN_PIXELS)
+        ? margin * frame_line_height
+        : margin;
     }
   else
     return 0;
diff --git a/src/xdisp.c b/src/xdisp.c
index 0fd2a7c..55bb34a 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -31520,6 +31520,14 @@ syms_of_xdisp (void)
 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.  The value should
+be a floating point number between 0.0 and 0.5.  The effective maximum
+is limited to (/ (1- window-lines) 2).  Non-float values for this
+variable are ignored and the default 0.25 is used instead.  */);
+  Vmaximum_scroll_margin = make_float (0.25);
+
   DEFVAR_LISP ("display-pixels-per-inch",  Vdisplay_pixels_per_inch,
     doc: /* Pixels per inch value for non-window system displays.
 Value is a number or a cons (WIDTH-DPI . HEIGHT-DPI).  */);
diff --git a/test/src/window-tests.el b/test/src/window-tests.el
index f5cca73..1dcebef 100644
--- a/test/src/window-tests.el
+++ b/test/src/window-tests.el
@@ -81,4 +81,31 @@ window-tests-with-buffer-window
      (window-tests-scrolling (+ max-margin 1) max-margin)
      (window-tests-scrolling (+ max-margin 2) max-margin))))
 
+(defun window-tests--point-in-middle-of-window-p ()
+  (= (count-lines (window-start) (window-point))
+     (/ (1- (window-text-height)) 2)))
+
+(ert-deftest window-tests-scroll-margin-whole-window ()
+  "Test `maximum-scroll-margin' at 0.5.
+With a high `scroll-margin', this should keep cursor in the
+middle of the window."
+  (skip-unless (not noninteractive))
+  (let ((maximum-scroll-margin 0.5)
+        (scroll-margin 100))
+    (window-tests-with-buffer-window
+     (set-window-text-height nil 7)
+     (erase-buffer)
+     (insert (mapconcat #'number-to-string
+                        (number-sequence 1 200) "\n"))
+     (goto-char 1)
+     (sit-for 0)
+     (call-interactively 'scroll-up-command)
+     (sit-for 0)
+     (should (window-tests--point-in-middle-of-window-p))
+     (call-interactively 'scroll-up-command)
+     (sit-for 0)
+     (should (window-tests--point-in-middle-of-window-p))
+     (call-interactively 'scroll-down-command)
+     (sit-for 0)
+     (should (window-tests--point-in-middle-of-window-p)))))
 ;;; window-tests.el ends here
-- 
2.9.3


[-- Attachment #5: Type: text/plain, Size: 2331 bytes --]



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.

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

[Eli Zaretskii]

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

Thanks.

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

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

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

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

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

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

Thanks.

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

[npostavs]

Eli Zaretskii <eliz@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));

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

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

[npostavs]

Eli Zaretskii <eliz@gnu.org> writes:

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

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

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

[npostavs]

[-- Attachment #1: Type: text/plain, Size: 902 bytes --]

Eli Zaretskii <eliz@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:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 2682 bytes --]

From 6944c7badade2477f381acf3b0922e5309e063d8 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@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

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

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

[npostavs]

npostavs@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@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;
}

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

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

[npostavs]

[-- Attachment #1: Type: text/plain, Size: 519 bytes --]

tags 5718 patch
quit

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


[-- Attachment #2: patch --]
[-- Type: application/octet-stream, Size: 3554 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: patch --]
[-- Type: text/x-diff, Size: 1170 bytes --]

From 775da8da80acd65033624c57057015628a8f2bba Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sun, 28 Aug 2016 17:23:04 -0400
Subject: [PATCH v4 2/5] Don't count mode line for scroll-margin limit

* src/window.c (window_scroll_margin): Use window_box_height to avoid
counting header line, scrollbars for scroll-margin limit (Bug #5718).
---
 src/window.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/window.c b/src/window.c
index 5333a6f..e499dfb 100644
--- a/src/window.c
+++ b/src/window.c
@@ -4802,10 +4802,8 @@ 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)
-        / frame_line_height;
-      int margin = min (scroll_margin, window_total_lines / 4);
+      int window_lines = window_box_height (window) / frame_line_height;
+      int margin = min (scroll_margin, window_lines / 4);
       if (unit == MARGIN_IN_PIXELS)
         return margin * frame_line_height;
       else
-- 
2.9.3


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: patch --]
[-- Type: text/x-diff, Size: 3603 bytes --]

From ae56bb9600019e2c2a6a5f8c22c36bd08a737d1c Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sun, 11 Sep 2016 11:09:57 -0400
Subject: [PATCH v4 3/5] Make limit on scroll-margin variable

* src/xdisp.c (maximum-scroll-margin): New variable.
* lisp/cus-start.el: Make it customizable.
* etc/NEWS: Mention it.
* src/window.c (window_scroll_pixel_based): Use it instead of hardcoding
division by 4 (Bug #5718).
---
 etc/NEWS          |  4 ++++
 lisp/cus-start.el |  1 +
 src/window.c      | 19 ++++++++++++++-----
 src/xdisp.c       |  8 ++++++++
 4 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index b89e8e4..07b8051 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -307,6 +307,10 @@ local part of a remote file name.  Thus, if you have a directory named
 "/~" on the remote host "foo", you can prevent it from being
 substituted by a home directory by writing it as "/foo:/:/~/file".
 
+** The new variable 'maximum-scroll-margin' allows having effective
+settings of 'scroll-margin' up to half the window size, instead of
+always restricting the margin to a quarter of the window.
+
 \f
 * Editing Changes in Emacs 26.1
 
diff --git a/lisp/cus-start.el b/lisp/cus-start.el
index a790419..51c43c7 100644
--- a/lisp/cus-start.el
+++ b/lisp/cus-start.el
@@ -511,6 +511,7 @@ minibuffer-prompt-properties--setter
 	     (scroll-step windows integer)
 	     (scroll-conservatively windows integer)
 	     (scroll-margin windows integer)
+             (maximum-scroll-margin windows float "26.1")
 	     (hscroll-margin windows integer "22.1")
 	     (hscroll-step windows number "22.1")
 	     (truncate-partial-width-windows
diff --git a/src/window.c b/src/window.c
index e499dfb..7d86d9d 100644
--- a/src/window.c
+++ b/src/window.c
@@ -4803,11 +4803,20 @@ window_scroll_margin (struct window *window, enum margin_unit unit)
     {
       int frame_line_height = default_line_pixel_height (window);
       int window_lines = window_box_height (window) / frame_line_height;
-      int margin = min (scroll_margin, window_lines / 4);
-      if (unit == MARGIN_IN_PIXELS)
-        return margin * frame_line_height;
-      else
-        return 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);
+        }
+      int max_margin = min ((window_lines - 1)/2,
+                            (int) (window_lines * ratio));
+      int margin = clip_to_bounds (0, scroll_margin, max_margin);
+      return (unit == MARGIN_IN_PIXELS)
+        ? margin * frame_line_height
+        : margin;
     }
   else
     return 0;
diff --git a/src/xdisp.c b/src/xdisp.c
index 8a450b7..134ef6c 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -31520,6 +31520,14 @@ syms_of_xdisp (void)
 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.  The value should
+be a floating point number between 0.0 and 0.5.  The effective maximum
+is limited to (/ (1- window-lines) 2).  Non-float values for this
+variable are ignored and the default 0.25 is used instead.  */);
+  Vmaximum_scroll_margin = make_float (0.25);
+
   DEFVAR_LISP ("display-pixels-per-inch",  Vdisplay_pixels_per_inch,
     doc: /* Pixels per inch value for non-window system displays.
 Value is a number or a cons (WIDTH-DPI . HEIGHT-DPI).  */);
-- 
2.9.3


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: patch --]
[-- Type: text/x-diff, Size: 3090 bytes --]

From 239fe08b7667e397913842fe4a7fc5784dd5ea27 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sat, 21 Jan 2017 13:24:47 -0500
Subject: [PATCH v4 4/5] 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      | 29 ++++++++++++++++++++++++++++-
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/src/dispextern.h b/src/dispextern.h
index 51222e6..eb71a82 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 (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 7d86d9d..9feeede 100644
--- a/src/window.c
+++ b/src/window.c
@@ -5149,7 +5149,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 134ef6c..0e329df 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -9859,6 +9859,32 @@ move_it_by_lines (struct it *it, ptrdiff_t dvpos)
     }
 }
 
+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;
+}
+
 /* Return true if IT points into the middle of a display vector.  */
 
 bool
@@ -15368,7 +15394,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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: patch --]
[-- Type: text/x-diff, Size: 5495 bytes --]

From 198d9ec8bbcc93ba676b09627b08ace1cd611a74 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sat, 28 Jan 2017 16:54:33 -0500
Subject: [PATCH v4 5/5] Add tests for scrolling

* test/manual/scroll-tests.el: New tests for scroll-margin behavior.
---
 test/manual/scroll-tests.el | 130 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 130 insertions(+)
 create mode 100644 test/manual/scroll-tests.el

diff --git a/test/manual/scroll-tests.el b/test/manual/scroll-tests.el
new file mode 100644
index 0000000..1167efd
--- /dev/null
+++ b/test/manual/scroll-tests.el
@@ -0,0 +1,130 @@
+;;; scroll-tests.el -- tests for scrolling -*- lexical-binding: t -*-
+
+;; Copyright (C) 2017 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; These are mostly automated ert tests, but they don't work in batch
+;; mode which is why they are under test/manual.
+
+;;; Code:
+
+(require 'ert)
+(eval-when-compile (require 'cl-lib))
+
+(defun scroll-tests-up-and-down (margin &optional effective-margin)
+  (unless effective-margin
+    (setq effective-margin margin))
+  (erase-buffer)
+  (insert (mapconcat #'number-to-string
+                     (number-sequence 1 200) "\n"))
+  (goto-char 1)
+  (sit-for 0)
+  (let ((scroll-margin margin)
+        (wstart (window-start)))
+    ;; Stopping before `scroll-margin' so we shouldn't have
+    ;; scrolled.
+    (let ((current-prefix-arg (- (window-text-height) 1 effective-margin)))
+      (call-interactively 'next-line))
+    (sit-for 0)
+    (should (= wstart (window-start)))
+    ;; Passing `scroll-margin' should trigger scrolling.
+    (call-interactively 'next-line)
+    (sit-for 0)
+    (should (/= wstart (window-start)))
+    ;; Scroll back to top.
+    (let ((current-prefix-arg (window-start)))
+      (call-interactively 'scroll-down-command))
+    (sit-for 0)
+    (should (= 1 (window-start)))))
+
+(defmacro scroll-tests-with-buffer-window (&rest body)
+  (declare (debug t))
+  `(with-temp-buffer
+     (with-selected-window (display-buffer (current-buffer))
+       ,@body)))
+
+(ert-deftest scroll-tests-scroll-margin-0 ()
+  (skip-unless (not noninteractive))
+  (scroll-tests-with-buffer-window
+   (scroll-tests-up-and-down 0)))
+
+(ert-deftest scroll-tests-scroll-margin-negative ()
+  "A negative `scroll-margin' should be the same as 0."
+  (skip-unless (not noninteractive))
+  (scroll-tests-with-buffer-window
+   (scroll-tests-up-and-down -10 0)))
+
+(ert-deftest scroll-tests-scroll-margin-max ()
+  (skip-unless (not noninteractive))
+  (scroll-tests-with-buffer-window
+   (let ((max-margin (/ (window-text-height) 4)))
+     (scroll-tests-up-and-down max-margin))))
+
+(ert-deftest scroll-tests-scroll-margin-over-max ()
+  "A `scroll-margin' more than max should be the same as max."
+  (skip-unless (not noninteractive))
+  (scroll-tests-with-buffer-window
+   (set-window-text-height nil 7)
+   (let ((max-margin (/ (window-text-height) 4)))
+     (scroll-tests-up-and-down (+ max-margin 1) max-margin)
+     (scroll-tests-up-and-down (+ max-margin 2) max-margin))))
+
+(defun scroll-tests--point-in-middle-of-window-p ()
+  (= (count-lines (window-start) (window-point))
+     (/ (1- (window-text-height)) 2)))
+
+(cl-defun scroll-tests--scroll-margin-whole-window (&key with-line-spacing)
+  "Test `maximum-scroll-margin' at 0.5.
+With a high `scroll-margin', this should keep cursor in the
+middle of the window."
+  (let  ((maximum-scroll-margin 0.5)
+         (scroll-margin 100))
+    (scroll-tests-with-buffer-window
+     (setq-local line-spacing with-line-spacing)
+     ;; Choose an odd number, so there is one line in the middle.
+     (set-window-text-height nil 7)
+     ;; `set-window-text-height' doesn't count `line-spacing'.
+     (when with-line-spacing
+       (window-resize nil (* line-spacing 7) nil nil 'pixels))
+     (erase-buffer)
+     (insert (mapconcat #'number-to-string
+                        (number-sequence 1 200) "\n"))
+     (goto-char 1)
+     (sit-for 0)
+     (call-interactively 'scroll-up-command)
+     (sit-for 0)
+     (should (scroll-tests--point-in-middle-of-window-p))
+     (call-interactively 'scroll-up-command)
+     (sit-for 0)
+     (should (scroll-tests--point-in-middle-of-window-p))
+     (call-interactively 'scroll-down-command)
+     (sit-for 0)
+     (should (scroll-tests--point-in-middle-of-window-p)))))
+
+(ert-deftest scroll-tests-scroll-margin-whole-window ()
+  (skip-unless (not noninteractive))
+  (scroll-tests--scroll-margin-whole-window))
+
+(ert-deftest scroll-tests-scroll-margin-whole-window-line-spacing ()
+  ;; `line-spacing' has no effect on tty displays.
+  (skip-unless (display-graphic-p))
+  (scroll-tests--scroll-margin-whole-window :with-line-spacing 3))
+
+
+;;; scroll-tests.el ends here
-- 
2.9.3

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

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

[npostavs]

Eli Zaretskii <eliz@gnu.org> writes:

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

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

[Eli Zaretskii]

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

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

[npostavs]

tags 5718 fixed
close 5718 26.1
quit

Eli Zaretskii <eliz@gnu.org> writes:

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