; Merge: fixes and updates to scroll margin (Bug#5718)

- add new option `maximum-sroll-margin'
- refactor and fix scroll margin calculation

doc/emacs/display.texi

@@ -285,13 +285,17 @@ multiple variables, the order of priority is:
 @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.
+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.  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

doc/lispref/windows.texi

@@ -3924,6 +3924,21 @@ redisplay scrolls the text automatically (if possible) to move point
 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

etc/NEWS

@@ -307,6 +307,11 @@ 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
 

lisp/cus-start.el

@@ -511,6 +511,7 @@ since it could result in memory overflow and make Emacs crash."
 	     (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

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;

src/window.c

@@ -4790,6 +4790,36 @@ window_scroll (Lisp_Object window, EMACS_INT n, bool whole, bool noerror)
   XWINDOW (window)->window_end_valid = false;
 }
 
+/* Compute scroll margin for WINDOW.
+   We scroll when point is within this distance from the top or bottom
+   of the window.  The result is measured in lines or in pixels
+   depending on the second parameter.  */
+int
+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_lines = window_box_height (window) / frame_line_height;
+
+      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;
+}
+
 
 /* Implementation of window_scroll that works based on pixel line
    heights.  See the comment of window_scroll for parameter
@@ -4806,7 +4836,6 @@ window_scroll_pixel_based (Lisp_Object window, int n, bool whole, bool noerror)
   bool vscrolled = false;
   int x, y, rtop, rbot, rowh, vpos;
   void *itdata = NULL;
-  int window_total_lines;
   int frame_line_height = default_line_pixel_height (w);
   bool adjust_old_pointm = !NILP (Fequal (Fwindow_point (window),
 					  Fwindow_old_point (window)));
@@ -5062,12 +5091,7 @@ window_scroll_pixel_based (Lisp_Object window, int n, bool whole, bool noerror)
   /* Move PT out of scroll margins.
      This code wants current_y to be zero at the window start position
      even if there is a header line.  */
-  window_total_lines
-    = w->total_lines * WINDOW_FRAME_LINE_HEIGHT (w) / frame_line_height;
-  this_scroll_margin = max (0, scroll_margin);
-  this_scroll_margin
-    = min (this_scroll_margin, window_total_lines / 4);
-  this_scroll_margin *= frame_line_height;
+  this_scroll_margin = window_scroll_margin (w, MARGIN_IN_PIXELS);
 
   if (n > 0)
     {
@@ -5123,7 +5147,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);
 
@@ -5290,9 +5314,7 @@ window_scroll_line_based (Lisp_Object window, int n, bool whole, bool noerror)
 
   if (pos < ZV)
     {
-      /* Don't use a scroll margin that is negative or too large.  */
-      int this_scroll_margin =
-	max (0, min (scroll_margin, w->total_lines / 4));
+      int this_scroll_margin = window_scroll_margin (w, MARGIN_IN_LINES);
 
       set_marker_restricted_both (w->start, w->contents, pos, pos_byte);
       w->start_at_line_beg = !NILP (bolp);
@@ -5722,8 +5744,7 @@ and redisplay normally--don't erase and redraw the frame.  */)
 
   /* Do this after making BUF current
      in case scroll_margin is buffer-local.  */
-  this_scroll_margin
-    = max (0, min (scroll_margin, w->total_lines / 4));
+  this_scroll_margin = window_scroll_margin (w, MARGIN_IN_LINES);
 
   /* Don't use redisplay code for initial frames, as the necessary
      data structures might not be set up yet then.  */
@@ -5962,10 +5983,6 @@ from the top of the window.  */)
 
   lines = displayed_window_lines (w);
 
-#if false
-  this_scroll_margin = max (0, min (scroll_margin, lines / 4));
-#endif
-
   if (NILP (arg))
     XSETFASTINT (arg, lines / 2);
   else
@@ -5981,6 +5998,8 @@ from the top of the window.  */)
 	     it is probably better not to install it.  However, it is here
 	     inside #if false so as not to lose it.  -- rms.  */
 
+      this_scroll_margin = window_scroll_margin (w, MARGIN_IN_LINES);
+
       /* Don't let it get into the margin at either top or bottom.  */
       iarg = max (iarg, this_scroll_margin);
       iarg = min (iarg, lines - this_scroll_margin - 1);

src/window.h

@@ -1120,6 +1120,8 @@ extern bool compare_window_configurations (Lisp_Object, Lisp_Object, bool);
 extern void mark_window_cursors_off (struct window *);
 extern int window_internal_height (struct window *);
 extern int window_body_width (struct window *w, bool);
+enum margin_unit { MARGIN_IN_LINES, MARGIN_IN_PIXELS };
+extern int window_scroll_margin (struct window *, enum margin_unit);
 extern void temp_output_buffer_show (Lisp_Object);
 extern void replace_buffer_in_windows (Lisp_Object);
 extern void replace_buffer_in_windows_safely (Lisp_Object);

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
@@ -15316,7 +15342,6 @@ try_scrolling (Lisp_Object window, bool just_this_one_p,
 	       bool temp_scroll_step, bool last_line_misfit)
 {
   struct window *w = XWINDOW (window);
-  struct frame *f = XFRAME (w->frame);
   struct text_pos pos, startp;
   struct it it;
   int this_scroll_margin, scroll_max, rc, height;
@@ -15327,8 +15352,6 @@ try_scrolling (Lisp_Object window, bool just_this_one_p,
   /* We will never try scrolling more than this number of lines.  */
   int scroll_limit = SCROLL_LIMIT;
   int frame_line_height = default_line_pixel_height (w);
-  int window_total_lines
-    = WINDOW_TOTAL_LINES (w) * FRAME_LINE_HEIGHT (f) / frame_line_height;
 
 #ifdef GLYPH_DEBUG
   debug_method_add (w, "try_scrolling");
@@ -15336,13 +15359,7 @@ try_scrolling (Lisp_Object window, bool just_this_one_p,
 
   SET_TEXT_POS_FROM_MARKER (startp, w->start);
 
-  /* Compute scroll margin height in pixels.  We scroll when point is
-     within this distance from the top or bottom of the window.  */
-  if (scroll_margin > 0)
-    this_scroll_margin = min (scroll_margin, window_total_lines / 4)
-      * frame_line_height;
-  else
-    this_scroll_margin = 0;
+  this_scroll_margin = window_scroll_margin (w, MARGIN_IN_PIXELS);
 
   /* Force arg_scroll_conservatively to have a reasonable value, to
      avoid scrolling too far away with slow move_it_* functions.  Note
@@ -15377,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));
@@ -15816,23 +15834,12 @@ try_cursor_movement (Lisp_Object window, struct text_pos startp,
     {
       int this_scroll_margin, top_scroll_margin;
       struct glyph_row *row = NULL;
-      int frame_line_height = default_line_pixel_height (w);
-      int window_total_lines
-	= WINDOW_TOTAL_LINES (w) * FRAME_LINE_HEIGHT (f) / frame_line_height;
 
 #ifdef GLYPH_DEBUG
       debug_method_add (w, "cursor movement");
 #endif
 
-      /* Scroll if point within this distance from the top or bottom
-	 of the window.  This is a pixel value.  */
-      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;
+      this_scroll_margin = window_scroll_margin (w, MARGIN_IN_PIXELS);
 
       top_scroll_margin = this_scroll_margin;
       if (WINDOW_WANTS_HEADER_LINE_P (w))
@@ -16280,7 +16287,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;
 
@@ -16310,6 +16317,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
@@ -16614,10 +16623,7 @@ 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 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
@@ -16901,12 +16907,6 @@ 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;
       ptrdiff_t margin_pos = CHARPOS (startp);
       Lisp_Object aggressive;
       bool scrolling_up;
@@ -17150,10 +17150,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 =
-	    scroll_margin > 0
-	    ? min (scroll_margin, window_total_lines / 4)
-	    : 0;
 	  bool move_down = w->cursor.vpos >= window_total_lines / 2;
 
 	  move_it_by_lines (&it, move_down ? margin + 1 : -(margin + 1));
@@ -17359,7 +17355,6 @@ try_window (Lisp_Object window, struct text_pos pos, int flags)
   struct it it;
   struct glyph_row *last_text_row = NULL;
   struct frame *f = XFRAME (w->frame);
-  int frame_line_height = default_line_pixel_height (w);
 
   /* Make POS the new window start.  */
   set_marker_both (w->start, Qnil, CHARPOS (pos), BYTEPOS (pos));
@@ -17385,17 +17380,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
@@ -18679,15 +18664,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)
@@ -31569,6 +31547,14 @@ 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.  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).  */);

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