* Few enhancements to ansi-term
@ 2019-04-30 17:21 John Shahid
2019-04-30 17:23 ` John Shahid
0 siblings, 1 reply; 4+ messages in thread
From: John Shahid @ 2019-04-30 17:21 UTC (permalink / raw)
To: emacs-devel
This is a follow up to this thread [1] where I proposed some changes to
ansi-term to get rid of `term-suppress-hard-newline'.
In that thread Stefan proposed adding an assertion to ensure the code
only removes line-wrap newlines. This assertion I think is useful but
turned out to be problematic for me. I would accidentally insert a
character in the term buffer. For example, I would sometimes type `p'
instead of `C-p' to go up a line, which ends up adding a `p' to the
buffer. The inserted character would inherit the text property and
generate assertion error when I resize the terminal window. I attached
a patch to make the line-wrap newlines have the `rear-nonsticky' to
prevent it from extending to nearby characters.
The other issue I ran into is having `term-scroll-with-delete' randomly
getting set to `t'. This causes the terminal to truncate text that grow
past of the top of the terminal, as opposed to letting it accumulate in
the buffer. For example assuming the terminal is 3 lines high and I cat
a 5 lines file, normally the terminal buffer will keep all 5 lines while
showing the last 3. When `term-scroll-with-delete' gets turned on, the
terminal buffer will be limited to the last 3 lines and the first 2
lines will get deleted, as in gone from the history.
[1]: https://lists.gnu.org/archive/html/emacs-devel/2019-02/msg00766.html
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Few enhancements to ansi-term
2019-04-30 17:21 Few enhancements to ansi-term John Shahid
@ 2019-04-30 17:23 ` John Shahid
2019-04-30 18:34 ` Stefan Monnier
0 siblings, 1 reply; 4+ messages in thread
From: John Shahid @ 2019-04-30 17:23 UTC (permalink / raw)
To: emacs-devel
[-- Attachment #1: Type: text/plain, Size: 84 bytes --]
Apologies, I forgot to attach the patches I am using locally to fix
those issues.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-setting-and-unsetting-scroll-with-delete.patch --]
[-- Type: text/x-patch, Size: 4337 bytes --]
From 174673f0d98b1042ef10a3307bc3956c17b41100 Mon Sep 17 00:00:00 2001
From: John Shahid <jvshahid@gmail.com>
Date: Mon, 29 Apr 2019 13:53:38 -0400
Subject: [PATCH] Fix setting and unsetting scroll-with-delete
* lisp/term.el (term-set-scroll-region): Do not set
term-scroll-with-delete when the region is set to the height of the
terminal.
* lisp/term.el (term-reset-terminal): Reset term-scroll-with-delete.
* test/lisp/term-tests.el (term-scrolling-region): Add more tests to
term mode handle scroll region properly.
---
lisp/term.el | 5 +--
test/lisp/term-tests.el | 78 ++++++++++++++++++++++++++++++++++++++++-
2 files changed, 80 insertions(+), 3 deletions(-)
diff --git a/lisp/term.el b/lisp/term.el
index d69ab94b7a..1fe27da181 100644
--- a/lisp/term.el
+++ b/lisp/term.el
@@ -1174,7 +1174,7 @@ term-reset-size
(setq term-start-line-column nil)
(setq term-current-row nil)
(setq term-current-column nil)
- (term-set-scroll-region 0 height)
+ (term-set-scroll-region 0 (1- height))
;; `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)
@@ -3215,6 +3215,7 @@ term-reset-terminal
(setq term-current-column 1)
(setq term-scroll-start 0)
(setq term-scroll-end term-height)
+ (setq term-scroll-with-delete nil)
(setq term-insert-mode nil)
;; FIXME: No idea why this is here, it looks wrong. --Stef
(setq term-ansi-face-already-done nil))
@@ -3439,7 +3440,7 @@ term-set-scroll-region
(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 (1- term-height))))))
(term-move-columns (- (term-current-column)))
(term-goto 0 0))
diff --git a/test/lisp/term-tests.el b/test/lisp/term-tests.el
index 9f5dcd559e..b0b9a11974 100644
--- a/test/lisp/term-tests.el
+++ b/test/lisp/term-tests.el
@@ -119,7 +119,83 @@ term-test-screen-from-input
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 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)))
--
2.21.0
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-Prevent-accidental-edits-in-the-ansi-term-buffer-fro.patch --]
[-- Type: text/x-patch, Size: 1592 bytes --]
From 6b490f9419e59adb7287b175c2bfca16acc8eb77 Mon Sep 17 00:00:00 2001
From: John Shahid <jvshahid@gmail.com>
Date: Sun, 28 Apr 2019 15:59:50 -0400
Subject: [PATCH] Prevent accidental edits in the ansi-term buffer from
breaking resizing
* lisp/term.el (term-unwrap-line, term-emulate-terminal): Add
rear-nonsticky text property to the newlines used for line wrapping.
---
lisp/term.el | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/lisp/term.el b/lisp/term.el
index b5949c4600..d69ab94b7a 100644
--- a/lisp/term.el
+++ b/lisp/term.el
@@ -2935,7 +2935,8 @@ term-emulate-terminal
(delete-region (point) (line-end-position))
(term-down 1 t)
(term-move-columns (- (term-current-column)))
- (put-text-property (1- (point)) (point) 'term-line-wrap t)
+ (add-text-properties (1- (point)) (point)
+ '(term-line-wrap t rear-nonsticky t))
(setq decoded-substring
(substring decoded-substring (- term-width old-column)))
(setq old-column 0)))
@@ -3754,7 +3755,8 @@ term-unwrap-line
(when (not (bolp))
(let ((old-point (point)))
(insert-before-markers ?\n)
- (put-text-property old-point (point) 'term-line-wrap t))))
+ (add-text-properties old-point (point)
+ '(term-line-wrap t rear-nonsticky t)))))
(defun term-erase-in-line (kind)
(when (= kind 1) ;; erase left of point
--
2.21.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: Few enhancements to ansi-term
2019-04-30 17:23 ` John Shahid
@ 2019-04-30 18:34 ` Stefan Monnier
2019-04-30 21:50 ` John Shahid
0 siblings, 1 reply; 4+ messages in thread
From: Stefan Monnier @ 2019-04-30 18:34 UTC (permalink / raw)
To: emacs-devel
> * lisp/term.el (term-set-scroll-region): Do not set
> term-scroll-with-delete when the region is set to the height of the
> terminal.
> * lisp/term.el (term-reset-terminal): Reset term-scroll-with-delete.
Better write these as:
> * lisp/term.el (term-set-scroll-region): Do not set
> term-scroll-with-delete when the region is set to the height of the
> terminal.
> (term-reset-terminal): Reset term-scroll-with-delete.
[ I read the first entry and thought the next was about another file so
I didn't even look at it. ]
The patch also changes term-reset-size but the ChangeLog doesn't explain
what this change does.
> @@ -3439,7 +3440,7 @@ term-set-scroll-region
> (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 (1- term-height))))))
The code just above does:
(setq term-scroll-end
(if (or (<= bottom term-scroll-start) (> bottom term-height))
term-height
bottom))
Should this also be changed to (1- term-height)?
And/or should the test use <= as in:
(not (and (<= term-scroll-start 0)
(>= term-scroll-end (1- term-height))))))
?
> * lisp/term.el (term-unwrap-line, term-emulate-terminal): Add
> rear-nonsticky text property to the newlines used for line wrapping.
The diff basically say that already, so I'd rather write it as "Prevent
the `term-line-wrap` property of newlines from spreading accidentally
when inserting text next to it" or something like that.
As for the code, it looks good to me.
Stefan
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Few enhancements to ansi-term
2019-04-30 18:34 ` Stefan Monnier
@ 2019-04-30 21:50 ` John Shahid
0 siblings, 0 replies; 4+ messages in thread
From: John Shahid @ 2019-04-30 21:50 UTC (permalink / raw)
To: emacs-devel
[-- Attachment #1: Type: text/plain, Size: 2047 bytes --]
Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> * lisp/term.el (term-set-scroll-region): Do not set
>> term-scroll-with-delete when the region is set to the height of the
>> terminal.
>> * lisp/term.el (term-reset-terminal): Reset term-scroll-with-delete.
>
> Better write these as:
>
>> * lisp/term.el (term-set-scroll-region): Do not set
>> term-scroll-with-delete when the region is set to the height of the
>> terminal.
>> (term-reset-terminal): Reset term-scroll-with-delete.
Fixed in the attached patch. I ended up changing more functions. I
hope that the commit message still follow the guidelines.
>
> [ I read the first entry and thought the next was about another file so
> I didn't even look at it. ]
>
> The patch also changes term-reset-size but the ChangeLog doesn't explain
> what this change does.
>
>> @@ -3439,7 +3440,7 @@ term-set-scroll-region
>> (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 (1- term-height))))))
>
> The code just above does:
>
> (setq term-scroll-end
> (if (or (<= bottom term-scroll-start) (> bottom term-height))
> term-height
> bottom))
>
> Should this also be changed to (1- term-height)?
> And/or should the test use <= as in:
>
> (not (and (<= term-scroll-start 0)
> (>= term-scroll-end (1- term-height))))))
>
> ?
Good catch. I fixed it in the latest patch and added more tests.
>
>> * lisp/term.el (term-unwrap-line, term-emulate-terminal): Add
>> rear-nonsticky text property to the newlines used for line wrapping.
>
> The diff basically say that already, so I'd rather write it as "Prevent
> the `term-line-wrap` property of newlines from spreading accidentally
> when inserting text next to it" or something like that.
I stole your above wording, thanks!
> As for the code, it looks good to me.
Thanks for reviewing the changes.
JS
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-setting-and-resetting-of-scroll-with-delete.patch --]
[-- Type: text/x-patch, Size: 6830 bytes --]
From 9564645a5b9e9abb5487094596a4d9a3b9279108 Mon Sep 17 00:00:00 2001
From: John Shahid <jvshahid@gmail.com>
Date: Mon, 29 Apr 2019 13:53:38 -0400
Subject: [PATCH] Fix setting and resetting of scroll-with-delete
* lisp/term.el (term-mode):
(term-reset-size):
term-height.
(term-reset-terminal): Set term-scroll-end to (1- term-height) instead
of term-height.
(term-set-scroll-region): Do not set term-scroll-with-delete when the
region is set to the height of the terminal. Fix off by one error
when checking if scroll-end exceeds the terminal height.
* test/lisp/term-tests.el (term-scrolling-region): Add more tests to
term mode handle scroll region properly.
---
lisp/term.el | 13 ++--
test/lisp/term-tests.el | 136 +++++++++++++++++++++++++++++++++++++++-
2 files changed, 141 insertions(+), 8 deletions(-)
diff --git a/lisp/term.el b/lisp/term.el
index f051e63415..f849b38417 100644
--- a/lisp/term.el
+++ b/lisp/term.el
@@ -1076,7 +1076,7 @@ term-mode
(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)
+ (set (make-local-variable 'term-scroll-end) (1- term-height))
(make-local-variable 'term-scroll-with-delete)
(make-local-variable 'term-pager-count)
(make-local-variable 'term-pager-old-local-map)
@@ -1174,7 +1174,7 @@ term-reset-size
(setq term-start-line-column nil)
(setq term-current-row nil)
(setq term-current-column nil)
- (term-set-scroll-region 0 height)
+ (term-set-scroll-region 0 (1- height))
;; `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)
@@ -3213,8 +3213,7 @@ term-reset-terminal
(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-set-scroll-region 0 (1- term-height))
(setq term-insert-mode nil)
;; FIXME: No idea why this is here, it looks wrong. --Stef
(setq term-ansi-face-already-done nil))
@@ -3433,13 +3432,13 @@ term-set-scroll-region
0
top))
(setq term-scroll-end
- (if (or (<= bottom term-scroll-start) (> bottom term-height))
- term-height
+ (if (or (<= bottom term-scroll-start) (> bottom (1- term-height)))
+ (1- term-height)
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 (1- term-height))))))
(term-move-columns (- (term-current-column)))
(term-goto 0 0))
diff --git a/test/lisp/term-tests.el b/test/lisp/term-tests.el
index 9f5dcd559e..6923096d22 100644
--- a/test/lisp/term-tests.el
+++ b/test/lisp/term-tests.el
@@ -119,7 +119,141 @@ term-test-screen-from-input
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)))
--
2.21.0
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-Prevent-accidental-edits-in-the-ansi-term-buffer-fro.patch --]
[-- Type: text/x-patch, Size: 1631 bytes --]
From 04c5cd7cf5539fddd3ca04c9730b1629e323211d Mon Sep 17 00:00:00 2001
From: John Shahid <jvshahid@gmail.com>
Date: Sun, 28 Apr 2019 15:59:50 -0400
Subject: [PATCH] Prevent accidental edits in the ansi-term buffer from
breaking resizing
* lisp/term.el (term-unwrap-line, term-emulate-terminal): Prevent the
`term-line-wrap` property of newlines from spreading accidentally
when inserting text next to it.
---
lisp/term.el | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/lisp/term.el b/lisp/term.el
index 8a28e6df28..f051e63415 100644
--- a/lisp/term.el
+++ b/lisp/term.el
@@ -2935,7 +2935,8 @@ term-emulate-terminal
(delete-region (point) (line-end-position))
(term-down 1 t)
(term-move-columns (- (term-current-column)))
- (put-text-property (1- (point)) (point) 'term-line-wrap t)
+ (add-text-properties (1- (point)) (point)
+ '(term-line-wrap t rear-nonsticky t))
(setq decoded-substring
(substring decoded-substring (- term-width old-column)))
(setq old-column 0)))
@@ -3754,7 +3755,8 @@ term-unwrap-line
(when (not (bolp))
(let ((old-point (point)))
(insert-before-markers ?\n)
- (put-text-property old-point (point) 'term-line-wrap t))))
+ (add-text-properties old-point (point)
+ '(term-line-wrap t rear-nonsticky t)))))
(defun term-erase-in-line (kind)
(when (= kind 1) ;; erase left of point
--
2.21.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-04-30 21:50 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-04-30 17:21 Few enhancements to ansi-term John Shahid
2019-04-30 17:23 ` John Shahid
2019-04-30 18:34 ` Stefan Monnier
2019-04-30 21:50 ` John Shahid
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).