From f0e7f39e0b4026a3d613416ad8ffc84e6b74242b Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Mon, 20 Feb 2017 13:34:39 -0500
Subject: [PATCH] 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 b0ff627c70e..b0644882bdc 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 1167efd6a66..95647ce0c40 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))
--
2.39.5