[Emacs-diffs] master f0e7f39: Fix scrolling with partial line corner case (Bug#25792)

[Noam Postavsky]

branch: master
commit f0e7f39e0b4026a3d613416ad8ffc84e6b74242b
Author: Noam Postavsky <address@hidden>
Commit: Noam Postavsky <address@hidden>

    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.
---
 src/xdisp.c                 |  12 +++--
 test/manual/scroll-tests.el | 105 ++++++++++++++++++++++++++++++--------------
 2 files changed, 80 insertions(+), 37 deletions(-)

diff --git a/src/xdisp.c b/src/xdisp.c
index b0ff627..b064488 100644
--- 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 1167efd..95647ce 100644
--- 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))