Fix scrolling with partial line corner case (Bug#25792)

Also fix up the scrolling tests so that they don't make so many
assumptions about the current window configuration.

* src/xdisp.c (try_window): Take partial line height into account when
comparing cursor position against scroll margin.

* test/manual/scroll-tests.el (scroll-tests-with-buffer-window): Add
HEIGHT argument, to allow setting up window with exact height and
partial line.
(scroll-tests-display-buffer-with-height): New display-buffer action
function.
(scroll-tests-scroll-margin-over-max):
(scroll-tests--scroll-margin-whole-window): Pass HEIGHT to
`scroll-tests--scroll-margin-whole-window'.
(scroll-tests-conservative-show-trailing-whitespace): New test.
(scroll-tests-scroll-margin-negative): Fix line counting.
(scroll-tests--point-in-middle-of-window-p): Set window height
properly.

diff --git a/src/xdisp.c b/src/xdisp.c
index b0ff627c70e8e0b3bc4ec46085c291751e45de2d..b0644882bdc6a1403e2d118b94f713af49d24e87 100644 (file)
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -17380,21 +17380,27 @@ try_window (Lisp_Object window, struct text_pos pos, int flags)
        return 0;
     }
 
+  /* Save the character position of 'it' before we call
+     'start_display' again.  */
+  ptrdiff_t it_charpos = IT_CHARPOS (it);
+
   /* Don't let the cursor end in the scroll margins.  */
   if ((flags & TRY_WINDOW_CHECK_MARGINS)
       && !MINI_WINDOW_P (w))
     {
       int this_scroll_margin = window_scroll_margin (w, MARGIN_IN_PIXELS);
+      start_display (&it, w, pos);
 
       if ((w->cursor.y >= 0    /* not vscrolled */
           && w->cursor.y < this_scroll_margin
           && CHARPOS (pos) > BEGV
-          && IT_CHARPOS (it) < ZV)
+           && it_charpos < ZV)
          /* rms: considering make_cursor_line_fully_visible_p here
             seems to give wrong results.  We don't want to recenter
             when the last line is partly visible, we want to allow
             that case to be handled in the usual way.  */
-         || w->cursor.y > it.last_visible_y - this_scroll_margin - 1)
+          || w->cursor.y > (it.last_visible_y - partial_line_height (&it)
+                            - this_scroll_margin - 1))
        {
          w->cursor.vpos = -1;
          clear_glyph_matrix (w->desired_matrix);
@@ -17403,7 +17409,7 @@ try_window (Lisp_Object window, struct text_pos pos, int flags)
     }
 
   /* If bottom moved off end of frame, change mode line percentage.  */
-  if (w->window_end_pos <= 0 && Z != IT_CHARPOS (it))
+  if (w->window_end_pos <= 0 && Z != it_charpos)
     w->update_mode_line = true;
 
   /* Set window_end_pos to the offset of the last character displayed

diff --git a/test/manual/scroll-tests.el b/test/manual/scroll-tests.el
index 1167efd6a6696c314a060b271b8b0586c43412da..95647ce0c40f8b3854811a3ad16767697c3887ba 100644 (file)
--- a/test/manual/scroll-tests.el
+++ b/test/manual/scroll-tests.el
@@ -53,41 +53,79 @@
     (sit-for 0)
     (should (= 1 (window-start)))))
 
-(defmacro scroll-tests-with-buffer-window (&rest body)
-  (declare (debug t))
+(defun scroll-tests-display-buffer-with-height (buffer alist)
+  (let ((height (alist-get 'window-height alist)))
+    (when height
+      (let* ((window (or (get-buffer-window buffer) (selected-window)))
+             (lines (floor height))
+             (partial (round (* (- height lines) (default-line-height)))))
+        (setq window (cond ((window-in-direction 'above window nil +1))
+                           ((or (window-in-direction 'below window nil -1)
+                                (split-window-below lines))
+                            window)))
+        (set-window-buffer window buffer)
+        (set-window-text-height window lines)
+        (adjust-window-trailing-edge window partial nil t)
+        window))))
+
+(defmacro scroll-tests-with-buffer-window (&optional height &rest body)
+  (declare (debug t) (indent defun))
   `(with-temp-buffer
-     (with-selected-window (display-buffer (current-buffer))
+     (with-selected-window (display-buffer (current-buffer)
+                                           '(scroll-tests-display-buffer-with-height
+                                             . ,(if (numberp height)
+                                                    `((window-height . ,height))
+                                                  (push height body)
+                                                  nil)))
        ,@body)))
 
 (ert-deftest scroll-tests-scroll-margin-0 ()
   (skip-unless (not noninteractive))
   (scroll-tests-with-buffer-window
-   (scroll-tests-up-and-down 0)))
+    (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)))
+    (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))))
+    (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))))
+  (scroll-tests-with-buffer-window 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))))
+
+(ert-deftest scroll-tests-conservative-show-trailing-whitespace ()
+  "Test for Bug#25792."
+  ;; Note: requires partial line to trigger problem.
+  (scroll-tests-with-buffer-window 20.5
+    (let ((show-trailing-whitespace t)
+          (scroll-conservatively 101)
+          (scroll-margin 5))
+      (insert (mapconcat #'number-to-string
+                         (number-sequence 1 200) "\n"))
+      (goto-char 1)
+      (forward-line 15)
+      (sit-for 0)
+      (let ((window-line (count-lines (window-start) (window-point))))
+        (dotimes (_ 10)
+          (call-interactively 'next-line)
+          (sit-for 0)
+          (should (= window-line (count-lines (window-start)
+                                              (window-point)))))))))
 
 (defun scroll-tests--point-in-middle-of-window-p ()
   (= (count-lines (window-start) (window-point))
-     (/ (1- (window-text-height)) 2)))
+     (/ (1- (floor (window-screen-lines))) 2)))
 
 (cl-defun scroll-tests--scroll-margin-whole-window (&key with-line-spacing)
   "Test `maximum-scroll-margin' at 0.5.
@@ -95,27 +133,26 @@ 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)))))
+    ;; Choose an odd number of lines, so there is a middle line.
+    (scroll-tests-with-buffer-window 7
+      (setq-local line-spacing with-line-spacing)
+      ;; `set-window-text-height' doesn't count `line-spacing'.
+      (when with-line-spacing
+        (window-resize nil (* line-spacing 8) 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))