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

[John Shahid]

Are there more changes to the previously attached patch in order to
merge it into master ?

JS

John Shahid <address@hidden> writes:

> Stefan Monnier <address@hidden> writes:
>
>>> I changed the property name to 'term-line-wrap'.
>>
>> Thanks,
>>
>>>>>> +    (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' ?
>>
>> Various minor reasons:
>> - it also allows modifying text with `read-only` text-property.
>> - It allows the wrapped code to change read-only-mode if it wants/needs to,
>>   whereas let-binding buffer-read-only means that if the wrapped code
>>   changes read-only-mode, we'll silently undo this change at the end.
>> - `buffer-read-only` is a variable to *set* or *unset* rather than
>>   to let-bind temporarily.  Using inhibit-read-only clarifies that you
>>   just want to temporarily override the read-only-ness rather than to
>>   change the read-only-mode.
>> Depending on the situation, different things matter more.
>> Here it likely doesn't make much of a difference in practice, but the
>> normal style is to bind inhibit-read-only: that's what it's for.
>
> That makes sense.  Thanks for the detailed answer.
>
>>
>>> One more question, should I deprecate 'term-suppress-hard-newline' as
>>> part of this changeset ?
>>
>> I think it should mark it as obsolete, yes.
>
> Awesome.  I made the change and attached the latest patch.  FYI, I also
> went removed all usages of that variable.
>
> JS
>
> From f4a9e0f4968fa253da40eaf7f6fd84fa52c650b5 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 wrapping from killed text.
> (term-reset-size): Add or remove line wrapping depending on the new terminal
> width.
> (term-suppress-hard-newline): Mark obsolete.
> (term-emulate-terminal): Remove usage of 'term-suppress-hard-newline'
> (term-unwrap-line):
> ---
>  lisp/term.el | 77 +++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 61 insertions(+), 16 deletions(-)
>
> diff --git a/lisp/term.el b/lisp/term.el
> index f49777f94c..e9ed4a688f 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
> +                        "term removes newlines used for wrapping on resize 
> and when text is copied"
> +                        "27.1")
>  
>  ;; 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,33 @@ term-mode
>        (setq term-input-ring (make-ring term-input-ring-size)))
>    (term-update-mode-line))
>  
> +(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))
> +  (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)))))
> +
> +(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 +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)))))))
>  
>  ;; Recursive routine used to check if any string in term-kill-echo-list
>  ;; matches part of the buffer before point.
> @@ -2895,20 +2937,20 @@ term-emulate-terminal
>                  (let ((old-column (term-horizontal-column))
>                        (old-point (point))
>                        columns)
> -                  (unless term-suppress-hard-newline
> -                    (while (> (+ (length decoded-substring) old-column)
> -                              term-width)
> -                      (insert (substring decoded-substring 0
> -                                         (- term-width old-column)))
> -                      ;; Since we've enough text to fill the whole line,
> -                      ;; delete previous text regardless of
> -                      ;; `term-insert-mode's value.
> -                      (delete-region (point) (line-end-position))
> -                      (term-down 1 t)
> -                      (term-move-columns (- (term-current-column)))
> -                      (setq decoded-substring
> -                            (substring decoded-substring (- term-width 
> old-column)))
> -                      (setq old-column 0)))
> +                  (while (> (+ (length decoded-substring) old-column)
> +                            term-width)
> +                    (insert (substring decoded-substring 0
> +                                       (- term-width old-column)))
> +                    ;; Since we've enough text to fill the whole line,
> +                    ;; delete previous text regardless of
> +                    ;; `term-insert-mode's value.
> +                    (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))
>                    (insert decoded-substring)
>                    (setq term-current-column (current-column)
>                          columns (- term-current-column old-column))
> @@ -3719,7 +3761,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