Re: Feedback on getting rid of `term-suppress-hard-newline'

[John Shahid]

Stefan Monnier <address@hidden> writes:

> John Shahid <address@hidden> writes:
>> Are there more changes to the previously attached patch in order to
>> merge it into master ?
>
> Oh, sorry, I dropped the ball.
> Yes, it looks good, thank you very much.

No worries and thanks for taking the time to review this patch.

> Of course I couldn't resist making further comments, but feel free to
> push it,

Great, I made some changes to the patch, see below.

>         Stefan
>
>
>> +(make-obsolete-variable 'term-suppress-hard-newline nil
>> +                        "term removes newlines used for wrapping on resize 
>> and when text is copied"
>> +                        "27.1")
>
> Please remove the third argument above (it should be the version).

Done.  I can't remember what I was thinking at the time when I wrote
this, it was a long time ago.

>> +  (add-function :filter-return
>> +                (local 'filter-buffer-substring-function)
>> +                'term--filter-buffer-substring)
>                   ^
> I'd put a # there.

Done.

>> +(defun term--insert-fake-newline (&optional count)
>> +  (let ((old-point (point)))
>> +    (term-insert-char ?\n count)
>> +    (put-text-property old-point (point) 'term-line-wrap t)))
>
> `count` is always nil, AFAICT, so you can just drop this argument.

I got rid of this function.  Replaced it with calls to
`term-unwrap-line'.

>> +(defun term--remove-fake-newlines ()
>> +  (goto-char (point-min))
>> +  (let (fake-newline)
>> +    (while (setq fake-newline (next-single-property-change (point)
>> +                                                           'term-line-wrap))
>> +      (goto-char fake-newline)
>> +      (let ((inhibit-read-only t))
>> +        (delete-char 1)))))
>
> I'd add a test (or an assertion) that (eq ?\n (char-after))), in case
> the text-property ends up applied somewhere we didn't expect
> (e.g. because it was inherited via insert-and-inherit).

I added an assertion.  I don't see any uses of `insert-and-inherit' in
term.el and based on my limited testing this test passes, in other words
the assertion didn't fail.

>> @@ -1147,7 +1177,19 @@ term-reset-size
>>        (setq term-start-line-column nil)
>>        (setq term-current-row nil)
>>        (setq term-current-column nil)
>> -      (goto-char point))))
>> +      (goto-char point))
>> +    (save-excursion
>> +      ;; Add newlines to wrap long lines that are currently displayed
>> +      (forward-line (- (term-current-row)))
>> +      (beginning-of-line)
>> +      (while (not (eobp))
>> +        (let* ((eol (save-excursion (end-of-line) (current-column))))
>> +          (when (> eol width)
>> +            (move-to-column width)
>> +            (let ((inhibit-read-only t))
>> +              (term--insert-fake-newline)))
>> +          (unless (eobp)
>> +            (forward-char)))))))
>
> I'd move this to a separate function.

Done, introduced a new function `term--unwrap-visible-long-lines'.

> More importantly, I don't quite understand why (- (term-current-row)))
> should be the exactly correct number of lines to move back at the
> beginning, so please add a comment explaining it (or if it's not exactly
> right, the comment could explain why it's "right enough").

I replaced this call with using `term-home-marker'.  I believe the two
approaches are equivalent.

I'm assuming that this marker will correctly account for the home
position and I'm also assuming that terminal programs won't try to go
past that marker.  I think that the accounting could be off, specially
during resizing, but when I was testing, that assumption didn't cause
any noticeable issues.

BTW, I tried to look for documentation on how terminals handle resize
but couldn't find one.  Maybe I should pick a terminal emulator and
inspect the code.

> Also, I think you can simplify the loop by always calling
> `move-to-column` and never `current-column`, as in the 100% untested
> code below:
>
>     (while (not (eobp))
>       (move-to-column width)
>       (if (eolp)
>           (forward-char)
>         (let ((inhibit-read-only t))
>           (term--insert-fake-newline))))

Done.

[...]

> I think I'd leave the term-suppress-hard-newline test in for now (that's
> the point of marking is as obsolete).

I reverted this change.

Cheers,

JS

>From 966edc96b34b998b04d1d9de8529c6d01dae8192 Mon Sep 17 00:00:00 2001
From: John Shahid <address@hidden>
Date: Sun, 20 Jan 2019 19:08:17 -0500
Subject: [PATCH] Adjust line wrapping on window resize and killing text

* lisp/term.el (term-mode): Advice filter-buffer-substring-function to
remove line unwrapping from killed text.
(term-reset-size): Add or remove line unwrapping depending on the new
terminal width.
(term-suppress-hard-newline): Mark obsolete.
(term-unwrap-line): Use text properties to be able to find the
newlines later.
---
 lisp/term.el | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 54 insertions(+), 2 deletions(-)

diff --git a/lisp/term.el b/lisp/term.el
index f49777f94c..fdcc39de72 100644
--- a/lisp/term.el
+++ b/lisp/term.el
@@ -545,6 +545,9 @@ term-suppress-hard-newline
   :version "24.4"
   :type 'boolean
   :group 'term)
+(make-obsolete-variable 'term-suppress-hard-newline nil
+                        "27.1"
+                        'set)
 
 ;; Where gud-display-frame should put the debugging arrow.  This is
 ;; set by the marker-filter, which scans the debugger's output for
@@ -1116,6 +1119,9 @@ term-mode
 
   (set (make-local-variable 'font-lock-defaults) '(nil t))
 
+  (add-function :filter-return
+                (local 'filter-buffer-substring-function)
+                #'term--filter-buffer-substring)
   (add-function :filter-return
                 (local 'window-adjust-process-window-size-function)
                 (lambda (size)
@@ -1132,9 +1138,51 @@ term-mode
       (setq term-input-ring (make-ring term-input-ring-size)))
   (term-update-mode-line))
 
+(defun term--remove-fake-newlines ()
+  (goto-char (point-min))
+  (let (fake-newline)
+    (while (setq fake-newline (next-single-property-change (point)
+                                                           'term-line-wrap))
+      (goto-char fake-newline)
+      (assert (eq ?\n (char-after)))
+      (let ((inhibit-read-only t))
+        (delete-char 1)))))
+
+(defun term--filter-buffer-substring (content)
+  (with-temp-buffer
+    (insert content)
+    (term--remove-fake-newlines)
+    (buffer-string)))
+
+(defun term--unwrap-visible-long-lines (width)
+  ;; Unwrap lines longer than width using fake newlines.  Only do it
+  ;; for lines that are currently visible (i.e. following the home
+  ;; marker).  Invisible lines don't have to be unwrapped since they
+  ;; are unreachable using the cursor movement anyway.  Not having to
+  ;; unwrap the entire buffer means the runtime of this function is
+  ;; bounded by the size of the screen instead of the buffer size.
+
+  (save-excursion
+    ;; We will just assume that our accounting for the home marker is
+    ;; correct, i.e. programs will not try to reach any position
+    ;; earlier than this marker.
+    (goto-char term-home-marker)
+
+    (move-to-column width)
+    (while (not (eobp))
+      (if (eolp)
+          (forward-char)
+        (let ((inhibit-read-only t))
+          (term-unwrap-line)))
+      (move-to-column width))))
+
 (defun term-reset-size (height width)
   (when (or (/= height term-height)
             (/= width term-width))
+    ;; Delete all newlines used for wrapping
+    (when (/= width term-width)
+      (save-excursion
+        (term--remove-fake-newlines)))
     (let ((point (point)))
       (setq term-height height)
       (setq term-width width)
@@ -1147,7 +1195,8 @@ term-reset-size
       (setq term-start-line-column nil)
       (setq term-current-row nil)
       (setq term-current-column nil)
-      (goto-char point))))
+      (goto-char point))
+    (term--unwrap-visible-long-lines width)))
 
 ;; Recursive routine used to check if any string in term-kill-echo-list
 ;; matches part of the buffer before point.
@@ -3719,7 +3768,10 @@ term-down
 ;; if the line above point wraps around, add a ?\n to undo the wrapping.
 ;; FIXME:  Probably should be called more than it is.
 (defun term-unwrap-line ()
-  (when (not (bolp)) (insert-before-markers ?\n)))
+  (when (not (bolp))
+    (let ((old-point (point)))
+      (insert-before-markers ?\n)
+      (put-text-property old-point (point) 'term-line-wrap t))))
 
 (defun term-erase-in-line (kind)
   (when (= kind 1) ;; erase left of point
-- 
2.20.1