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

[npostavs@users.sourceforge.net]

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