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

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

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