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

[npostavs@users.sourceforge.net]

[-- 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"