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

From: [email-masked]
Date: Mon, 02 Jan 2017 19:48:03 -0500

Eli Zaretskii <[email-masked]> writes:

>> From: [email-masked]
>> Cc: [email-masked],  [email-masked],  [email-masked]
>> Date: Tue, 13 Sep 2016 22:40:29 -0400
>> 
>> Eli Zaretskii <[email-masked]> 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.

>From 4293c63254fe9ae38391aed1896436c79ebb8f00 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <[email-masked]>
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

>From 87ad96cc13ca45eb5c78e3773cfb1458001f9d83 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <[email-masked]>
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.
+
 
 * 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

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.