Unwrap terminal buffer

[Cimbali]

This PR attempts to fix #2865 by removing newlines marked as « continuation » line feeds from the terminal output, when pushing these lines to the buffer.

This allows e.g. to open a file with gf when the file name is long and split across several lines. Also nice visually because the wrapping of broken lines is ugly

The good:

• it compiles
• it doesn’t segfault
• it works for me: long lines are now a single line in the buffer (whether they are added when scrollback is enabled or paused etc.), and the buffer wrapping mechanism takes care of the displaying them nicely. See screenshots below.

The bad:

• absolutely not sure this is how it should be implemented. In particular:
  • the changes inside add_scrollback_line_to_buffer(): Is ml_replace the correct approach ?
  • Is it OK to modify the sb_pushline callback ? It seems to be a minimal change and I’ve checked other occurrences of this callback.
• no test cases
• might have messed up code-style or code-quality wise, I’m unsure
• resizing the terminal window still causes loss of data. Not a lot that can be done about this until proper terminal reflowing gets implemented.

I'd like some feedback before continuing to work on this.

Finally, I’ve added the continuation info to the scrollback lines, which may not be 100% necessary (there could be a work-around) at this time − however I believe this will be useful to « reflow » lines, that is move the continuation line breaks on window resizes. See discussion in #2865.

---

Terminal mode: long line causing « continuation » line feeds (same for both vim version):

Switching to normal mode (current vim master), long output line is artifically split, causes problems to select as 1 word and messy display:

Switching to normal mode (vim built from this PR), long output line appears as a single line (see line numbers), line split is moved due to gutter size (now in 53 instead of after 54) single word split across displayed lines is really a single word:

[jamessan]

Cc @leonerd

[k-takata]

Appveyor build fails on this line ? Not sure why

Because MSVC 10 doesn't support C99 style variable declaration.
All the variables should be declared at the top of the block.

[codecov]

Codecov Report

Merging #8365 (f427c3e) into master (0dc2fd3) will increase coverage by 0.47%.
The diff coverage is 97.67%.

❗ Current head f427c3e differs from pull request most recent head 56f16ad. Consider uploading reports for the commit 56f16ad to get more accurate results

@@            Coverage Diff             @@
##           master    #8365      +/-   ##
==========================================
+ Coverage   81.80%   82.28%   +0.47%     
==========================================
  Files         162      154       -8     
  Lines      188049   171344   -16705     
  Branches    42753    39250    -3503     
==========================================
- Hits       153837   140990   -12847     
+ Misses      21696    17224    -4472     
- Partials    12516    13130     +614     

Flag	Coverage Δ
huge-clang-none	?
huge-gcc-none	82.24% <97.67%> (+27.70%)	⬆️
huge-gcc-testgui	?
huge-gcc-unittests	2.03% <0.00%> (+1.73%)	⬆️
linux	?
mingw-x64-HUGE-gui	?
windows	?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/libvterm/include/vterm.h	44.44% <ø> (+44.44%)	⬆️
src/terminal.c	82.93% <95.65%> (+5.29%)	⬆️
src/libvterm/src/screen.c	53.26% <100.00%> (+0.23%)	⬆️
src/libvterm/src/state.c	89.31% <100.00%> (+51.84%)	⬆️
src/libvterm/t/harness.c	89.45% <100.00%> (ø)
src/gui.c	58.81% <0.00%> (-13.99%)	⬇️
src/libvterm/src/unicode.c	70.83% <0.00%> (-13.17%)	⬇️
src/libvterm/src/mouse.c	36.06% <0.00%> (-9.10%)	⬇️
src/cmdexpand.c	84.38% <0.00%> (-7.59%)	⬇️
src/float.c	91.33% <0.00%> (-7.08%)	⬇️
... and 155 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[leonerd]

Just FYI: I was pinged on this PR but I'm not intending to look into it all that much. vim's forked copy of libvterm has continued to diverge from my upstream copy to the point that it isn't easy to synchronise these things in again. In any case this particular development has happened entirely without my thoughts or input on the matter, and is somewhat different to the still in-progress work I have in a similar direction upstream.

Overall if youfolks want to try to synchronise these things back together again we need to do a much better job of coördinating things; otherwise I suspect you're on your own at this point.

[Cimbali]

TBH @leonerd this PR modifies one callback from libvterm, the rest are just modifications to vim. There’s no real functional changes to libvterm: text reflowing is not implemented in libvterm in this PR − only in the vim buffer.

The sb_pushline callback is changed to send some info on the line being pushed (specifically the VTermLineInfo since we need to know whether the line has a continuation marker):

vim/src/libvterm/include/vterm.h

Line 492 in 7d013b6

int (*sb_pushline)(int cols, const VTermScreenCell *cells, const VTermLineInfo *newinfo, void *user);

If anyone suggests a better way I’m happy to go with it:

• The number of the row being pushed back by the sb_pushline (row from sb_pushline_from_row(VTermScreen *screen, int row)) does not seem trivial to find out without modifying the callback (at least to my novice eyes). However if someone can suggest how to do that I can leave the callback unchanged.
• If it’s better to only send the row number instead of the VTermLineInfo I can do that too: the lineinfo array is accessible through the cbdata in this case.
• If you think this modification can be useful in upstream libvterm or if there is added value to keeping the separate copies somewhat parallel, I can propose this patch upstream if/when this PR is accepted.

[brammool]

@leonerd You can easily subscribe to Vim changes and filter on "libvterm" to see what changes are made in the Vim copy of libvterm code.
Is there a way to subscribe to changes in the root libvterm? Not the copy that neovim has made (I believe it lags behind, I rather follow the original).

[Cimbali]

I did the missing case for Windows wide-chars, but this approach seems somwhat doomed I’m afraid due to unforeseen interactions with scroll.

1. When the terminal is scrolled, lineinfos are shifted before calling scrollrect, which in turn calls the sb_pushline callback − this makes it impossible to find out whether lines that were just pushed out of screen are continuations of the previous line.
2. scrollrect actually seems to require the lineinfo to be scrolled, as shifting after lineinfo after causes « DWL spill over », i.e. the last test of libvterm’s test 67 fails.

The ways this manifests in vim is that lines are incorrectly joined (joins are shift by 1) when the continuation lines are offscreen.

[GitMensch]

So we either have scrolling broken or the automatic resize?

Especially in TermDebug sessions with GDB often having to output longer content it would be very nice if making the gdb window wider would also show the content instead of multiple hard-broken lines and a lot of spaces.

[Cimbali]

At least that’s the issue with this attempt of implementing reflow @GitMensch. I’m sure there could be another way, but likely not without some input from people who have a better understanding of libvterm and/or vim’s src/terminal.c code.

[Cimbali]

After some more debugging I’ve managed to actually make this work as expected. Just to reiterate, this doesn’t handle resizing the terminal when it’s running, just displaying the terminal contents without undesired linebreaks when in normal buffer mode (and all that entails, see discussion above).

I’ve opened a discussion on the libvterm launchpad to see whether this would cause some additional divergence, or if we can keep that as low as possible.

[damnskippy]

Sounds like this will address #5769 as well if I'm not mistaken.
For me personally it's the wraps when using termdebug that's the hardest to deal with that @GitMensch is also alluding to.

[Cimbali]

I haven’t heard back from @leonerd yet, but I’ve been able to have a look at his work on reflowing in libvterm and the changes proposed there seem perfectly complementary to those in this PR. In the mean time, I’ve setup some mirrors so you can visualize the libvterm reflow changes etc.

[leonerd]

Hi @Cimbali what are you waiting for from me?

[Cimbali]

Just your input on the changes affecting libvterm really (of which I also provided a rebased version on the upstream codebase on launchpad).

There’s a callback change, so it’s a pretty significant change. However I believe it fits in well with your work in progress on reflowing. Roughly, it allows history to be reflowed, so in this instance the vim buffer. It will also allow to pull lines with proper length from history with sb_popline after resize (or during resize if adding more rows to the window).

If you think these changes are good and you’re likely to integrate them into your reflow work, then I suppose it’s low risk to merge them here. If you’d prefer another way of doing, I can look into it. And if you’d think you’d rather not do these changes in ilbvterm at all, it’s up to @brammool to arbitrate the risk of diverging codebases further versus the added functionality.

[leonerd]

I've added comment on the launchpad issue; copied here:

Overall this looks quite a lot of code change to have without any sort of unit tests. I'd like to see some tests.

Moreover, I'd like to have seen the tests first as a way to discuss what actual behaviour is required. Once we agree that's useful I can likely go about implementing it.

But please start first with the tests. Always.

[Cimbali]

Hi @leonerd, just a ping to notify you that I replied on launchpad last month and added a test. It’s also in this PR.

The changeset is really small, just adding a value to a callback. Please let us know if you plan on reviewing it.

[noscript]

@Cimbali I tried the patch and it mostly works great. However there is a problem with very long lines.

Try seq 1000 | paste -sd+ several times, go to normal mode and scroll the buffer. You will notice that at some point the line breaks disappear and colors from the shell prompt (if your prompt has colors) shift to other lines or completely disappear.

[Cimbali]

Thanks for testing @noscript, I'll look into it. Do you maybe have a screenshot?

[noscript]

vim -g --clean +'set lines=20 columns=22' +'term bash --noprofile --norc' +'only'

then

seq 100 | paste -sd+
tput setab 1; echo "this is red text"; tput sgr0
seq 100 | paste -sd+

then go to normal mode

UPDATED: it should be set lines=20 and not set lines=30

[GitMensch]

@Cimbali Any updates? Finishing this would be so helpful for debugging...

[Cimbali]

This PR has never been about reflow in an active terminal session, but about reflowing terminal contents once passed on to vim.

We’re still stuck at the same point @simlei:

1. either libvterm adds an info to its pushline callback (whether line breaks are “soft”, i.e. line wraps, or “hard”, i.e. line ends).
2. or vim’s copy of libvterm adds it without it existing upstream

There’s just no way to reflow without knowing which line breaks can be moved and which can’t.

So far (1) seemed the preferred option, with @leonerd stating in this thread he preferred implementing his own version of this requirement over the patches I sent, to which I replied I’ll happily rebase on that work when made available. That was 20 months ago.

I think the correct way forward is to set a deadline by which we fall back to option (2) if this current reminder (and any potential future ones) keep remaining unaddressed.

[leonerd]

I'm sure I could look into adding something in libvterm for that then, yes.

[leonerd]

Right - thinking out loud this is slightly nontrivial. If there were no API compat guaratnees at all, then I could simply add a VTermLineInfo * parameter to sb_pushline and all would be well. But I can't because that would break older code.

I'm in a dilemma of

1. Major version bump, up to v0.4, just for this one one change. If I'm going to bump that number I feel I should take the opportunity to change a lot more stuff while I'm there and that makes it take a lot longer.
2. Temporarily add an API to set a different callback that takes that extra argument, as a workaround for not changing it currently. You'll have to call a different function to set the pushline callback with a different signature.

It leads to a messier API but it's only temporary, so I'm going to go for option 2.

[simlei]

Let me know if I can help, however I have very limited knowledge of the vim codebase.

…

On Tue, 16 Jul 2024, 14:14 Paul Evans ***@***.***> wrote: Right - thinking out loud this is slightly nontrivial. If there were no API compat guaratnees at all, then I could simply add a VTermLineInfo * parameter to sb_pushline and all would be well. But I can't because that would break older code. I'm in a dilemma of 1. Major version bump, up to v0.4, just for this one one change. If I'm going to bump that number I feel I should take the opportunity to change a lot more stuff while I'm there and that makes it take a lot longer. 2. Temporarily add an API to set a different callback that takes that extra argument, as a workaround for not changing it currently. You'll have to call a different function to set the pushline callback with a different signature. It leads to a messier API but it's only temporary, so I'm going to go for option 2. — Reply to this email directly, view it on GitHub <#8365 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHP225FSSUTWI3PSE2ZTSTZMUFDBAVCNFSM46RA3CN2U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TEMRTGA3TGOJVG42Q> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[leonerd]

Let me know if I can help, however I have very limited knowledge of the vim codebase.

Ah this is all in libvterm, it's really not directly vim-related code at all.

[leonerd]

1. either libvterm adds an info to its pushline callback (whether line breaks are “soft”, i.e. line wraps, or “hard”, i.e. line ends).

Right then. Took me a while to remember to getting around to do it, but this is now done in latest source on bzr. What would be a good way to deliver this for your use? I'd like you to try it out a bit first before I call it the official 0.3.4 version tarball, but I could pack you a pre-release version, or make a .diff out of the relevant commits... or something else?

[Cimbali]

Thanks a lot @leonerd ! I’ve fetch the commits from bazaar and am playing around with it. It seems to work well as a drop-in replacement for what I had written, but I need to do some further testing.

[Cimbali]

I’ve confirmed this doesn’t work on Windows for some reason. Will have to get my hands on an appropriate setup to build where I can investigate properly why that is (appveyor rdp is a fallback but a bit too limited at this stage).

I can’t reproduce the linux uchar test issue/flakiness however.

[simlei]

I have a Windows PC and can help with testing. Can get any build to work in wsl

…

On Thu, 25 Jul 2024, 17:10 Cimbali ***@***.***> wrote: I’ve confirmed this doesn’t work on Windows for some reason. Will have to get my hands on an appropriate setup to build where I can investigate properly why that is (appveyor rdp is a fallback but a bit too limited at this stage). I can’t reproduce the linux uchar test issue/flakiness however. — Reply to this email directly, view it on GitHub <#8365 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHP222XHT3Y7DZGRUC4G4DZOEIP7AVCNFSM46RA3CN2U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TEMRVGA3DGMBQG4ZA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[simlei]

I'd like to again offer my support in debugging this issue. Have a windows PC. gdb noob though; instructions required.

[Cimbali]

Thanks @simlei. I think good first steps would be compiling and running the unit tests, to reproduce. Then figuring out why the failing unit test fails. Probably adding stderr printfs in handle_pushline and add_scrollback_line_to_buffer and redirecting that to a log with vim 2> log to understand what’s going on, whether continuation flags are correctly received, if so why pasting lines together doesn’t work, and so on.

[simlei]

Ok, will get a setup running on the weekend and ping you back when I get stumped ok?

…

On Tue, Oct 29, 2024 at 11:34 AM Cimbali ***@***.***> wrote: Thanks @simlei <https://github.com/simlei>. I think good first steps would be compiling and running the unit tests, to reproduce. Then figuring out why the failing unit test fails. Probably adding stderr printfs in handle_pushline and add_scrollback_line_to_buffer and redirecting that to a log with vim 2> log to understand what’s going on, whether continuation flags are correctly received, if so why pasting lines together doesn’t work, and so on. — Reply to this email directly, view it on GitHub <#8365 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHP223VGNSJZGFEGC6OUZDZ55QEHAVCNFSM46RA3CN2U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TENBUGM4DIMRVGIYQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[vE5li]

@Cimbali Can you give an update on the current state of this PR?

Also, is there any way to assist you? I would be happy to help :)

[simlei]

I have previously offered to have a look too but ultimately have not found the time yet. However during 10-15 March this year might be able to

…

On Tue, 11 Feb 2025, 14:41 Lucas Pütz ***@***.***> wrote: @Cimbali <https://github.com/Cimbali> Can you give an update on the current state of this PR? Also, is there any way to assist you? I would be happy to help :) — Reply to this email directly, view it on GitHub <#8365 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHP225SQDWBZWA62KNIIXL2PH4XJAVCNFSM46RA3CN2U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TENRVGA4DMMJUGU2A> . You are receiving this because you were mentioned.Message ID: ***@***.***>