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

[John Shahid <jvshahid@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2097 bytes --]


Sorry it took me a while to get back to you.  I fixed most of the issues
you mentioned but I had a few questions (see below).

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> ping
>
> FWIW, I think this makes a lot of sense, so please go ahead.
> A few comments below,
>
>
>         Stefan
>
>
>>> @@ -1106,6 +1106,7 @@ term-mode
>>>    (make-local-variable 'term-scroll-show-maximum-output)
>>>    (make-local-variable 'term-ptyp)
>>>    (make-local-variable 'term-exec-hook)
>>> +  (setq-local filter-buffer-substring-function 'term-filter-buffer-substring)
>
> Please use `add-function` to modify filter-buffer-substring-function.

Done.

>
>>> +    (put-text-property old-point (point) 'term-newline t)))
>
> I'd recommend you use a more explicit name which doesn't just state the
> obvious "this is a newline in term mode".  E.g. something like
> `term-wrap-newline`.

I changed the property name to 'term-line-wrap'.

>
>>> +    (let (buffer-read-only)
>>> +      (delete-char 1))))
>
> Never let-bind `buffer-read-only`: let-bind `inhibit-read-only`
> instead.

Done.  I am curious to know why I shouldn't let-bind 'buffer-read-only' ?

>
>>> +    ;; delete all fake newlines
>
> Please capitalize and punctuate your comments.

Done.

>
>>> +          (when (> len width)
>>> +            (goto-char (+ bol width))
>
> Not sure how well term deals with wide chars (e.g. TAB or chinese
> chars), but this code of yours clearly won't help.  I think you
> want to test current-column rather than `len` and then you want
> move-to-column rather than advancing by N chars.

I made the change in the attached patch.  I don't have any fonts locally
that are suitable for displaying Chinese characters so I couldn't test
those.  I tested with Arabic characters (e.g. 'C-x 8 RET ARABIC LIGATURE
AKBAR ISOLATED FORM') and I don't think 'current-column' is reporting
the right column number.  In this case, it reports column count of 1
which isn't right.  It does work though with tab characters.

One more question, should I deprecate 'term-suppress-hard-newline' as
part of this changeset ?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Adjust-line-wrapping-on-window-resize-and-killing-te.patch --]
[-- Type: text/x-patch, Size: 3939 bytes --]

From 8efc90d9a3a406dc795842bd86174da8e3944874 Mon Sep 17 00:00:00 2001
From: John Shahid <jvshahid@gmail.com>
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 wrapping from killed text.
(term-reset-size): Add or remove line wrapping depending on the new terminal
width.
(term-emulate-terminal):
(term-unwrap-line):
---
 lisp/term.el | 46 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 2 deletions(-)

diff --git a/lisp/term.el b/lisp/term.el
index f49777f94c..1406b80fd5 100644
--- a/lisp/term.el
+++ b/lisp/term.el
@@ -1116,6 +1116,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 +1135,32 @@ term-mode
       (setq term-input-ring (make-ring term-input-ring-size)))
   (term-update-mode-line))
 \f
+(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)))
+
+(defun term--remove-fake-newlines ()
+  (goto-char (point-min))
+  (while (setq fake-newline (next-single-property-change (point)
+                                                         'term-line-wrap))
+    (goto-char fake-newline)
+    (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-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 +1173,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)))))))
 
 ;; Recursive routine used to check if any string in term-kill-echo-list
 ;; matches part of the buffer before point.
@@ -2906,6 +2944,7 @@ 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)
                       (setq decoded-substring
                             (substring decoded-substring (- term-width old-column)))
                       (setq old-column 0)))
@@ -3719,7 +3758,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