Improve VT102 compliance #904

ghost · 2021-06-27T10:50:19Z

The following features are implemented by these changes, closing most of the items in #133:

Render blinking text, both normal (SGR 5) and rapid (SGR 6)
Render reverse video (DECSCNM)
Respond to DECREQTPARM
Parse sequences for double-height / double-width awareness. These are not rendered currently, but are available for future use and/or downstream projects.
ENQ answerback config option
AutomaticNewline mode (ANM)

wez

It's great to see this, thanks!

I read the bitflags manual and found some nice helper methods that make the bit manipulation a bit easier to grok, and peppered some suggestions throughout!

Some of those suggestions may be redundant if we can reasonably defer setting LineBits::REVERSE based on the mode at render time; I think some of these additions may not be needed if that is the case.

The main thing I'm "worried" about is the blink stuff: at the moment it seems like it will be expensive in the common case of no blinking text, but I have a couple of suggestions for that.

wez · 2021-06-27T15:47:07Z

term/src/terminalstate.rs

+            // If we want to exactly match VT100/xterm behavior, then
+            // we cannot honor ClearCharacterTabStopsAtActiveLine.
+            TabulationClear::ClearAllCharacterTabStops => {
+                // | TabulationClear::ClearCharacterTabStopsAtActiveLine => {


Suggested change

// | TabulationClear::ClearCharacterTabStopsAtActiveLine => {

// | TabulationClear::ClearCharacterTabStopsAtActiveLine

I sometimes use brace matching in vim to navigate, so it would be good to avoid this unbalanced curly!

wez · 2021-06-27T15:52:03Z

termwiz/src/surface/line.rs

+    /// Force the reverse bit set.  This also implicitly sets dirty.
+    #[inline]
+    pub fn set_reverse(&mut self, reverse: bool) {
+        if reverse {


bitflags has some nice helpers:
https://docs.rs/bitflags/1.2.1/bitflags/example_generated/struct.Flags.html#method.set

self.bits.set(LineBits::REVERSE, reverse); self.bits.insert(LineBits::DIRTY);

wez · 2021-06-27T15:52:39Z

termwiz/src/surface/line.rs

+    /// colors reversed.
+    #[inline]
+    pub fn is_reverse(&self) -> bool {
+        (self.bits & LineBits::REVERSE) == LineBits::REVERSE


Suggested change

(self.bits & LineBits::REVERSE) == LineBits::REVERSE

self.bits.contains(LineBits::REVERSE)

wez · 2021-06-27T15:54:33Z

termwiz/src/surface/line.rs

+    /// double-height-(top/bottom) and dirty.
+    #[inline]
+    pub fn set_single_width(&mut self) {
+        self.bits &= !LineBits::DOUBLE_WIDTH_HEIGHT_MASK;


Suggested change

self.bits &= !LineBits::DOUBLE_WIDTH_HEIGHT_MASK;

self.bits.remove(LineBits::DOUBLE_WIDTH_HEIGHT_MASK);

wez · 2021-06-27T15:56:17Z

termwiz/src/surface/line.rs

+    /// double-height-(top/bottom) and dirty.
+    #[inline]
+    pub fn set_double_width(&mut self) {
+        self.bits |= LineBits::DOUBLE_WIDTH;


self.bits.remove(LineBits::DOUBLE_HEIGHT_TOP | LineBits::DOUBLE_HEIGHT_BOTTOM); self.bits.insert(LineBits::DOUBLE_WIDTH | LineBits::DIRTY);

wez · 2021-06-27T16:18:57Z

termwiz/src/surface/line.rs

+        let mut cells = Vec::with_capacity(width);
+        cells.resize(width, Cell::default());
+        let mut bits = LineBits::DIRTY;
+        if reverse {


You can save a couple of lines:

bits.set(LineBits::REVERSE, reverse);

wez · 2021-06-27T16:27:21Z

term/src/terminalstate.rs

+                // Turn on reverse video for all of the lines on the
+                // display.
+                self.screen.reverse_video_mode = true;
+                for y in 0..self.screen.physical_rows as i64 {


In rust, array dimension accesses are bounds-checked by default which means that there is some (small) overhead for C style iteration. It's preferable to iterate containers directly so that the bounds check happens just once and the individual steps are essentially equivalent to bumping a pointer with no other overhead:

for line in &mut self.screen.lines { line.set_reverse(true); }

This is O(scrollback_size) whenever the reverse video mode is changed, and some people love having really huge scrollback. Is reverse video really a property of the line? Does it make sense to expose TerminalState::get_reverse_video() as an accessor to avoid this update pass?

For rendering purposes, we could still keep LineBits::REVERSE but teach terminal_get_lines to set it when retrieving the lines that need to be rendered:

https://github.com/wez/wezterm/blob/main/mux/src/renderable.rs#L72-L93

/// Implements Pane::get_lines for Terminal pub fn terminal_get_lines( term: &mut Terminal, lines: Range<StableRowIndex>, ) -> (StableRowIndex, Vec<Line>) { let screen = term.screen_mut(); let reverse = term.get_reverse_video(); // added let phys_range = screen.stable_range(&lines); ( screen.phys_to_stable_row_index(phys_range.start), screen .lines .iter_mut() .skip(phys_range.start) .take(phys_range.end - phys_range.start) .map(|line| { let cloned = line.clone(); line.clear_dirty(); cloned.set_reverse(reverse); // added cloned }) .collect(), ) }

If something like that makes sense, then terminalstate.rs would only need to track whether the mode is on or not, and not need to set it on the lines themselves, or deal with that when rewrapping.

A few thoughts:

Setting/clearing reverse ought to only impact lines on the visible screen, not every line in the scrollback. If the entire scrollback is begin affected (or scanned), then I need to fix that.

I have the line-level flag there so that reversed lines that enter the scrollback retain the reverse flag. But that's really just an opinion: the real VT100 had no scrollback, and reverse was only ever a whole-screen setting. I should see what xterm does: whether it keeps the reverse on for lines that go into scrollback or not.

The flag on the screen is only so that new lines are also set to reverse. But if we decide that reverse goes away for lines that enter scrollback, then we can eliminate the line-level flag entirely and just set a reverse flag at physical screen render time.

A small bit of exploring shows that for xterm, konsole, and VTE, reverse video is a property of the screen only, and furthermore when set all lines rendered, including those in scrollback, are reversed.

I will remove the line-level attribute and leave it on the ~~screen~~ terminalstate only.

OK, reverse is nicer, thank you! However, there is one gap remaining: on lines that do not stretch to the full physical pixels, we do not see the reversed background. Example:

I need to add a bit after render.rs:933, so that the selection and cursor are rendered, and then if there is more physical width remaining fill it with bg_color. Any suggestions?

wez · 2021-06-27T16:29:35Z

term/src/terminalstate.rs

+                // Turn off reverse video for all of the lines on the
+                // display.
+                self.screen.reverse_video_mode = false;
+                for y in 0..self.screen.physical_rows as i64 {


for line in &mut self.screen.lines { line.set_reverse(false); }

wez · 2021-06-27T16:41:40Z

term/src/terminalstate.rs

@@ -1977,6 +2007,13 @@ impl TerminalState {
                self.insert = false;
            }

+            Mode::SetMode(TerminalMode::Code(TerminalModeCode::AutomaticNewline)) => {


would you mind adding a unit test for this?
It can be similar to the one from your previous PR!

wez · 2021-06-27T16:46:46Z

term/src/terminalstate.rs

@@ -3318,6 +3368,23 @@ impl<'a> Performer<'a> {
            Esc::Code(EscCode::DecSaveCursorPosition) => self.dec_save_cursor(),
            Esc::Code(EscCode::DecRestoreCursorPosition) => self.dec_restore_cursor(),

+            Esc::Code(EscCode::DecDoubleHeightTopHalfLine) => {
+                let idx = self.screen.phys_row(self.cursor.y);
+                self.screen.line_mut(idx).set_double_height_top();


it would be great to add a some test coverage for these attributes, especially because we're not rendering them :-)

I don't think we have any existing tests or conveniences for inspecting LineBits, but you should still be able to use the same term.print stuff and then use assert_eq! to verify that the right things are set on the lines in the TestTerm.

ghost · 2021-07-09T23:21:24Z

Unit tests added, cleanup around bit sets and simplified logic.

We are now at the following unresolved items:

Fix conflict with wezterm-gui/src/termwindow/render.rs
Fix reverse video on lines that do not extend to the full width of the display.
Pick a path forward on blinking: 1) keep as is now, with the scans, possibly disabled by default; 2) change to line-level flags similar to hyperlinks to make scans go faster; 3) change to something like has_animation to only schedule an update if a blinking cell is visible; 4) other.

wez · 2021-07-25T06:48:04Z

I'm conscious that I've been making conflicting changes, and that it's a PITA to wrangle merge conflicts.
With that in mind, I cherry-picked the commits from this pr into the vttest branch in my repo: https://github.com/wez/wezterm/tree/vttest

A casualty of this is the code that manages timing for text blinking; I'd already made changes that broke your implementation when fixing up a general render performance problem. I'm happy to take a stab at making that work tomorrow.
I can also take a look at extending the reverse video to the full line width.

wez · 2021-07-25T15:24:21Z

I've pushed this to main; many thanks!
I went with option 3 for blinking and 7192fe7 was the missing piece for reverse video!

refs: #904

wez · 2022-01-20T15:57:46Z

@klamonte: re: double height as a line attribute, I'm considering reorganizing how I work with Line and more formally tracking the logical pre-wrapped line as it makes some things easier wrt. future bidi support but also cheaper because selection and search operations need to synthesize the logical line and that happens fairly frequently. The question on my mind is: what should the semantics be around lines that are double height wrt. re-wrapping them during a resize?

I'm inclined towards tracking the double-height-ness on the original logical line, but I'm not sure if that is totally cool.
What do you think?

ghost · 2022-01-20T18:05:37Z

That sounds fine to me.

Double-height will be a per-line attribute on both lines (top and bottom), and the top-line version will (according to someone with a hardware terminal when I asked around circa 2012) not render the underline attribute. One could even take top-halves and bottom-halves of different glyphs and make wholly new glyphs out of them -- though they would all be double-width. So even in hardware terminals it was a "trick" for the screen/printer rather than a semantic equivalence.

Autumn Lamonte added 17 commits June 19, 2021 16:53

Flesh out DEC Special Graphics character set

3c86470

Use black diamond U+25C6, not black diamond suit U+2666

25b72fa

#891 Fix format for CI

83155fe

#891 VT100 character set support

c64c812

#891 Fix tests

4b2f2ae

#133 Tab stops, reverse video.

cf024f9

#133 DECREQTPARM - needs cleanup

952ba0b

#133 Screen.reverse_video_mode + fmt fixes

309139a

#133 AutomaticNewline mode

558eebd

#133 clean up DECREQTPARM

eb88843

#133 parse out double-width/double-height, but do not render

2a33f63

#133 blinking text 50%

1a54d1b

#133 cleanup, rapid blinking text

e284d97

#133 typos

4983dc2

#133 ENQ response

6420222

Merge branch 'main' of https://github.com/wez/wezterm into wez-main

a8ef67a

Merge branch 'wez-main' into vttest

c4f4d4b

wez reviewed Jun 27, 2021

View reviewed changes

wez mentioned this pull request Jun 27, 2021

[terminfo] add smxx/rmxx, purge blink #905

Merged

Autumn Lamonte added 2 commits July 9, 2021 07:57

#133 cleanup + terminalstate-level reverse video mode

99c355a

#133 unit tests

957140f

wez closed this in bd7a2a6 Jul 25, 2021

wez added a commit that referenced this pull request Jul 25, 2021

docs: text_blink_rate

50c3e17

refs: #904

ghost mentioned this pull request Sep 21, 2021

Do something about the Bell #3

Closed

AutumnMeowMeow mentioned this pull request Aug 18, 2022

Improve vttest zellij-org/zellij#1671

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve VT102 compliance #904

Improve VT102 compliance #904

ghost commented Jun 27, 2021

wez left a comment

wez Jun 27, 2021

wez Jun 27, 2021

wez Jun 27, 2021

wez Jun 27, 2021

wez Jun 27, 2021

wez Jun 27, 2021

wez Jun 27, 2021

wez Jun 27, 2021

ghost Jun 27, 2021

ghost Jul 9, 2021 •

edited by ghost

ghost Jul 9, 2021

wez Jun 27, 2021

wez Jun 27, 2021

wez Jun 27, 2021

ghost commented Jul 9, 2021

wez commented Jul 25, 2021

wez commented Jul 25, 2021 •

edited

wez commented Jan 20, 2022 •

edited

ghost commented Jan 20, 2022

	// \| TabulationClear::ClearCharacterTabStopsAtActiveLine => {
	// \| TabulationClear::ClearCharacterTabStopsAtActiveLine

	(self.bits & LineBits::REVERSE) == LineBits::REVERSE
	self.bits.contains(LineBits::REVERSE)

	self.bits &= !LineBits::DOUBLE_WIDTH_HEIGHT_MASK;
	self.bits.remove(LineBits::DOUBLE_WIDTH_HEIGHT_MASK);

Improve VT102 compliance #904

Improve VT102 compliance #904

Conversation

ghost commented Jun 27, 2021

wez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jul 9, 2021 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 9, 2021

wez commented Jul 25, 2021

wez commented Jul 25, 2021 • edited

wez commented Jan 20, 2022 • edited

ghost commented Jan 20, 2022

ghost Jul 9, 2021 •

edited by ghost

wez commented Jul 25, 2021 •

edited

wez commented Jan 20, 2022 •

edited