Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve VT102 compliance #904

Closed
wants to merge 19 commits into from
Closed

Improve VT102 compliance #904

wants to merge 19 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jun 27, 2021

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)

Copy link
Owner

@wez wez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can save a couple of lines:

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

// 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 {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@ghost ghost Jul 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

reverse_incomplete

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?

// 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 {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Mode::SetMode(TerminalMode::Code(TerminalModeCode::AutomaticNewline)) => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

ghost commented Jul 9, 2021

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
Copy link
Owner

wez commented Jul 25, 2021

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
Copy link
Owner

wez commented Jul 25, 2021

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

@wez wez closed this in bd7a2a6 Jul 25, 2021
wez added a commit that referenced this pull request Jul 25, 2021
@ghost ghost mentioned this pull request Sep 21, 2021
@wez
Copy link
Owner

wez commented Jan 20, 2022

@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
Copy link
Author

ghost commented Jan 20, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant