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

[Stefan Monnier <monnier@iro.umontreal.ca>]

John Shahid <jvshahid@gmail.com> 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.

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


        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).

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

> +(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.

> +(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).

> @@ -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.

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").

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))))

> @@ -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))

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