Improve vttest

[AutumnMeowMeow]

This is for #1650 , plus a bit more, and rounds out the "'terminal reports" tests:

1. Test 6.4: Report terminal Primary Device Attributes as VT220 with sixel rather than VT400 family with sixel. This fixes a hang when launching vttest as it is waiting for a response to DECRQSS.

2. Test 6.2: Support NewLine mode (CR --> CRLF).

3. Test 6.3: Fix DSR cursor position report to honor scrolling region.

4. Test 6.7: Parse and respond to DECREQTPARM (Request Terminal Parameters - CSI x). This is a VT100 sequence that xterm used to respond to always, but more recently only responds to when explicitly set to VT100 level.

Note that this is AFAIK my first PR for zellij, so may have missed some things and/or have non-optimal/non-idiomatic Rust. Happy to update as per suggestions.

I have run fmt and tests. Tests are failing:

Snapshot: terminal_reports
Source: zellij-server/src/panes/unit/grid_tests.rs:723
-old snapshot
+new results
────────────────────────────────────────────────────────────────────────────────
format!("{:?}", grid.pending_messages_to_pty)
────────────┬───────────────────────────────────────────────────────────────────
    0       │-[[27, 91, 63, 54, **52**, 59, 52, 99], [27, 91, 56, 59, 53, 49, 59, 57, 55, 116], [27, 91, 63, 54, 99], [27, 91, 48, 110], [27, 91, 49, 59, 49, 82]]
          0 │+[[27, 91, 63, 54, **50**, 59, 52, 99], [27, 91, 56, 59, 53, 49, 59, 57, 55, 116], [27, 91, 63, 54, 99], [27, 91, 48, 110], [27, 91, 49, 59, 49, 82]]
────────────┴───────────────────────────────────────────────────────────────────
To update snapshots run `cargo insta review`

I believe this is due to the DA response being different. How can I update the test?

[tlinford]

Hey, can't really say much on the changes here (other than it's great to get such competent help!), but the test is failing because a insta snapshot has changed.
The file in question should be zellij-server/src/panes/unit/snapshots/zellij_server__panes__grid__grid_tests__terminal_reports.snap, and you should find a .snap.new file next to it. You can either replace the .snap file with the .snap.new one, or run cargo insta review (after cargo install cargo-insta) to get an interactive ui to compare the changes and accept them.

[imsnif]

Hey @AutumnMeowMeow - I'm happy to see your work on this!

All the changes seem great to me. In addition to @tlinford 's comment about the tests, I also saw clippy is failing. In case you don't know it, it's essentially a bunch of extra lint rules. You can see its output in the CI, but in case you want to run it locally, you can do so with cargo clippy --all-targets --all-features -- --deny warnings.

Otherwise, would you be comfortable adding some unit tests for this change? I think some good places would be here and/or here. Do note that this is unfortunately a very under-tested part of the code-base, so it's definitely no biggie if we leave things as is.

[AutumnMeowMeow]

@imsnif Thank you for the pointer on clippy. Obviously I'm still newish to Rust and just barely aware of zellij's internal design, so will have to graciously thank someone else if unit tests magically appear. :)

For completeness, here are the remaining vttest gaps at the VT102 level for zellij:

• Reverse video (DECSCNM). This would be implemented as a screen-level bool (default false), such that if true the foreground and background colors would be reversed at output. In most terminals, this is screen level, meaning that even scrolling up would show reversed lines while reverse video is set, and normal line when reverse video is disabled. I have seen this used well as a visual bell indicator: briefly flash the terminal.
• Double-height / double-width support (DECDHL/DECDWL/DECSWL). Normally, a terminal multiplexer would either deliberately ignore these sequences because it can't render double-width / double-height, or fudge it by rendering it as "[char] [space]" like this; or be very careful and send these sequences to the host terminal when possible like this. BUT now that zellij has sixel support, if you really want to have fun you could render them via custom bitmap fonts like this ; if you desire that path, there is some pain ahead to work on sixel performance. ;-)
• ENQ answerback. This would be a config option to respond to ENQ (0x05). No one really uses it, and amusingly it can create a loop at the shell.
• VT52 sub-mode. This one is most difficult, see here. On nice new Rusty codebases with a state machine it can be hard to weave in.
• UK character set. The only difference is that '#' maps to '£' on output. This might be nice to include actually, since you already have the StandardCharset enum and updating StandardCharset::map is like 5 lines.

See also wez/wezterm#904 and wez/wezterm#133 (comment) for more in-depth discussion.

My vote would be:

• I implement UK StandardCharset in this PR. DONE
• I open a new issue for reverse video, since it could also lead to a useful feature for accessibility (flash terminal visual bell) and should have broader discussion on configuration and/or accessibility.
• Leave all the other features deliberately unimplemented.

Thoughts?

[imsnif]

Hey @AutumnMeowMeow - this looks great. I totally understand about the tests, I guess with these sorts of features it also won't be super trivial to make a proper test. Everything you did is comprehensible to me, so I'm happy to merge this as is and add tests when I go over this section and add test for the rest of the uncovered parts.

* ~I implement UK StandardCharset in this PR.~ DONE

👍

* I open a new issue for reverse video, since it could also lead to a useful feature for accessibility (flash terminal visual bell) and should have broader discussion on configuration and/or accessibility.

Sounds great.

* Leave all the other features deliberately unimplemented.

I very much agree. I think it's great to be as compatible as possible, but with rarely used features that hardly affect our users (and would take a lot of effort to implement) I'm happy to settle. We can look into this again if we have a specific request or run into a specific issue.

[imsnif]

Thank you so much for your work on this! I hope to see more contributions from you.