[Emacs-diffs] master e44b56d: Fix setting and resetting of scroll-with-delete

[Stefan Monnier]

branch: master
commit e44b56d16ad0749e599700a73509c16391ed973e
Author: John Shahid <address@hidden>
Commit: Stefan Monnier <address@hidden>

    Fix setting and resetting of scroll-with-delete
    
    The start and end lines of the scroll region must to be in the range
    [0,term-height).  There are few placees that incorrectly set the end
    line of the scroll region to term-height which is outside the valid
    range.  Combined with another off-by-one error in
    term-set-scroll-region's clamping logic, this would cause
    term-scroll-with-delete to be unnecessarily turned on.
    
    * lisp/term.el (term-scroll-start,term-scroll-end): Use defvar-local
    to define the variables and document the valid range of values that
    the variables can take.
    (term--last-line): New function to calculate the 0-based index of the
    last line.
    (term--reset-scroll-region): New function to reset the scroll region
    to the full height of the terminal.
    (term-mode,term-reset-size,term-reset-terminal): Call
    term--reset-scroll-region to reset the scroll region.
    (term-set-scroll-region): Fix the off-by-one error in the clamping
    logic which allowed term-scroll-end to have values outside the valid
    range [0,term-height).
---
 lisp/term.el            |  43 +++++++++------
 test/lisp/term-tests.el | 136 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 163 insertions(+), 16 deletions(-)

diff --git a/lisp/term.el b/lisp/term.el
index 283e568..553c3a1 100644
--- a/lisp/term.el
+++ b/lisp/term.el
@@ -390,8 +390,16 @@ This emulates (more or less) the behavior of xterm.")
   "A queue of strings whose echo we want suppressed.")
 (defvar term-terminal-undecoded-bytes nil)
 (defvar term-current-face 'term)
-(defvar term-scroll-start 0 "Top-most line (inclusive) of scrolling region.")
-(defvar term-scroll-end) ; Number of line (zero-based) after scrolling region.
+(defvar-local term-scroll-start 0
+  "Top-most line (inclusive) of the scrolling region.
+`term-scroll-start' must be in the range [0,term-height).  In addition, its
+value has to be smaller than `term-scroll-end', i.e. one line scroll regions 
are
+not allowed.")
+(defvar-local term-scroll-end nil
+  "Bottom-most line (inclusive) of the scrolling region.
+`term-scroll-end' must be in the range [0,term-height).  In addition, its
+value has to be greater than `term-scroll-start', i.e. one line scroll regions 
are
+not allowed.")
 (defvar term-pager-count nil
   "Number of lines before we need to page; if nil, paging is disabled.")
 (defvar term-saved-cursor nil)
@@ -1075,9 +1083,6 @@ Entry to this mode runs the hooks on `term-mode-hook'."
   (make-local-variable 'term-current-column)
   (make-local-variable 'term-current-row)
   (make-local-variable 'term-log-buffer)
-  (make-local-variable 'term-scroll-start)
-  (set (make-local-variable 'term-scroll-end) term-height)
-  (make-local-variable 'term-scroll-with-delete)
   (make-local-variable 'term-pager-count)
   (make-local-variable 'term-pager-old-local-map)
   (make-local-variable 'term-old-mode-map)
@@ -1117,6 +1122,8 @@ Entry to this mode runs the hooks on `term-mode-hook'."
 
   (add-hook 'read-only-mode-hook #'term-line-mode-buffer-read-only-update nil 
t)
 
+  (term--reset-scroll-region)
+
   (easy-menu-add term-terminal-menu)
   (easy-menu-add term-signals-menu)
   (or term-input-ring
@@ -1133,6 +1140,9 @@ Entry to this mode runs the hooks on `term-mode-hook'."
       (let ((inhibit-read-only t))
         (delete-char 1)))))
 
+(defun term--last-line ()
+  (1- term-height))
+
 (defun term--filter-buffer-substring (content)
   (with-temp-buffer
     (insert content)
@@ -1174,7 +1184,7 @@ Entry to this mode runs the hooks on `term-mode-hook'."
       (setq term-start-line-column nil)
       (setq term-current-row nil)
       (setq term-current-column nil)
-      (term-set-scroll-region 0 height)
+      (term--reset-scroll-region)
       ;; `term-set-scroll-region' causes these to be set, we have to
       ;; clear them again since we're changing point (Bug#30544).
       (setq term-start-line-column nil)
@@ -3205,7 +3215,7 @@ option is enabled.  See `term-set-goto-process-mark'."
        (goto-char term-home-marker)
        (term-vertical-motion (1+ count))
        (set-marker term-home-marker (point))
-       (setq term-current-row (1- term-height))))))
+       (setq term-current-row (term--last-line))))))
 
 (defun term-reset-terminal ()
   "Reset the terminal, delete all the content and set the face to the default 
one."
@@ -3213,8 +3223,7 @@ option is enabled.  See `term-set-goto-process-mark'."
   (term-ansi-reset)
   (setq term-current-row 0)
   (setq term-current-column 1)
-  (setq term-scroll-start 0)
-  (setq term-scroll-end term-height)
+  (term--reset-scroll-region)
   (setq term-insert-mode nil)
   ;; FIXME: No idea why this is here, it looks wrong.  --Stef
   (setq term-ansi-face-already-done nil))
@@ -3423,6 +3432,10 @@ option is enabled.  See `term-set-goto-process-mark'."
      (1- (or (nth 1 params) 0))))
    (t)))
 
+(defun term--reset-scroll-region ()
+  "Sets the scroll region to the full height of the terminal."
+  (term-set-scroll-region 0 (term--last-line)))
+
 (defun term-set-scroll-region (top bottom)
   "Set scrolling region.
 TOP is the top-most line (inclusive) of the new scrolling region,
@@ -3433,13 +3446,13 @@ The top-most line is line 0."
            0
          top))
   (setq term-scroll-end
-       (if (or (<= bottom term-scroll-start) (> bottom term-height))
-           term-height
+       (if (or (<= bottom term-scroll-start) (> bottom (term--last-line)))
+           (term--last-line)
          bottom))
   (setq term-scroll-with-delete
        (or (term-using-alternate-sub-buffer)
            (not (and (= term-scroll-start 0)
-                     (= term-scroll-end term-height)))))
+                      (= term-scroll-end (term--last-line))))))
   (term-move-columns (- (term-current-column)))
   (term-goto 0 0))
 
@@ -3568,7 +3581,7 @@ The top-most line is line 0."
     (when (> moved lines)
       (backward-char))
     (cond ((<= deficit 0) ;; OK, had enough in the buffer for request.
-          (recenter (1- term-height)))
+          (recenter (term--last-line)))
          ((term-pager-continue deficit)))))
 
 (defun term-pager-page (arg)
@@ -3582,7 +3595,7 @@ The top-most line is line 0."
   (goto-char (point-min))
   (when (= (vertical-motion term-height) term-height)
     (backward-char))
-  (recenter (1- term-height)))
+  (recenter (term--last-line)))
 
 ;; Pager mode command to go to end of buffer.
 (defun term-pager-eob ()
@@ -3600,7 +3613,7 @@ The top-most line is line 0."
     ;; Move cursor to end of window.
     (vertical-motion term-height)
     (backward-char))
-  (recenter (1- term-height)))
+  (recenter (term--last-line)))
 
 (defun term-pager-back-page (arg)
   (interactive "p")
diff --git a/test/lisp/term-tests.el b/test/lisp/term-tests.el
index 9f5dcd5..6923096 100644
--- a/test/lisp/term-tests.el
+++ b/test/lisp/term-tests.el
@@ -119,7 +119,141 @@ line3\r
 line4\r
 line5\r
 line6\r
-"))))
+")))
+
+  ;; test reverse scrolling
+  (should (equal "line1
+line7
+line6
+line2
+line5"
+                 (term-test-screen-from-input 40 5
+                                              '("\e[0;0H"
+                                                "\e[J"
+                                                "line1\r
+line2\r
+line3\r
+line4\r
+line5"
+                                                "\e[2;4r"
+                                                "\e[2;0H"
+                                                "\e[2;0H"
+                                                "\eMline6"
+                                                "\e[2;0H"
+                                                "\eMline7"))))
+
+  ;; test scrolling down
+  (should (equal "line1
+line3
+line4
+line7
+line5"
+                 (term-test-screen-from-input 40 5
+                                              '("\e[0;0H"
+                                                "\e[J"
+                                                "line1\r
+line2\r
+line3\r
+line4\r
+line5"
+                                                "\e[2;4r"
+                                                "\e[2;0H"
+                                                "\e[4;5H"
+                                                "\n\rline7"))))
+
+  ;; setting the scroll region end beyond the max height should not
+  ;; turn on term-scroll-with-delete
+  (should (equal "line1
+line2
+line3
+line4
+line5
+line6
+line7"
+                 (term-test-screen-from-input 40 5
+                                                      '("\e[1;10r"
+                                                        "line1\r
+line2\r
+line3\r
+line4\r
+line5\r
+line6\r
+line7"))))
+
+
+  ;; resetting the terminal should set the scroll region end to (1- 
term-height).
+  (should (equal "
+line1
+line2
+line3
+line4
+"
+                 (term-test-screen-from-input 40 5
+                                                      '("\e[1;10r"
+                                                        "\ec" ;reset
+                                                        "line1\r
+line2\r
+line3\r
+line4\r
+line5"
+                                                        "\e[1;1H"
+                                                        "\e[L"))))
+
+  ;; scroll region should be limited to the (1- term-height).  Note,
+  ;; this fixes an off by one error when comparing the scroll region
+  ;; end with term-height.
+  (should (equal "
+line1
+line2
+line3
+line4
+"
+                 (term-test-screen-from-input 40 5
+                                              '("\e[1;6r"
+                                                "line1\r
+line2\r
+line3\r
+line4\r
+line5"
+                                                "\e[1;1H" ;go back to home
+                                                "\e[L"    ;insert a new line 
at the top
+                                                ))))
+
+  ;; setting the scroll region to the entire height should not turn on
+  ;; term-scroll-with-delete
+  (should (equal "line1
+line2
+line3
+line4
+line5
+line6"
+                 (term-test-screen-from-input 40 5
+                                                      '("\e[1;5r"
+                                                        "line1\r
+line2\r
+line3\r
+line4\r
+line5\r
+line6"))))
+
+  ;; reset should reset term-scroll-with-delete
+  (should (equal "line1
+line2
+line3
+line4
+line5
+line6
+line7"
+                 (term-test-screen-from-input 40 5
+                                              '("\e[2;5r" ;set the region
+                                                "\ec" ;reset
+                                                "line1\r
+line2\r
+line3\r
+line4\r
+line5\r
+line6\r
+line7")))))
 
 (ert-deftest term-set-directory ()
   (let ((term-ansi-at-user (user-real-login-name)))