[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Feedback on getting rid of `term-suppress-hard-newline'
From: |
Stefan Monnier |
Subject: |
Re: Feedback on getting rid of `term-suppress-hard-newline' |
Date: |
Thu, 21 Feb 2019 09:55:20 -0500 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.0.50 (gnu/linux) |
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.
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).