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

vttest #133

Closed
erf opened this issue Jan 26, 2020 · 28 comments
Closed

vttest #133

erf opened this issue Jan 26, 2020 · 28 comments
Labels
bug Something isn't working

Comments

@erf
Copy link
Contributor

erf commented Jan 26, 2020

Describe the bug

Are you using vttest ? It is a great way to test your terminal features. Some of them fails now.

Also maybe this comment might help.

Environment (please complete the following information):

  • macOS 10.15.2 (19C57)
  • Frontend: not sure
  • 20200113-222147-724ad3a

To Reproduce

Download vttest from: https://invisible-island.net/vttest/

run vttest from terminal

press 1 to run the first test. press Enter several times to skip to next test and show errors.

@erf erf added the bug Something isn't working label Jan 26, 2020
@wez
Copy link
Owner

wez commented Jan 26, 2020

I ran it in the past; the problem I had with it was that it requires a human to look at it and know whether the output is correct; there is no scalable way to automate its conformance tests. I agree that we should have correct behavior, but I'm more motivated to correct behavior that manifests for real applications before behaviors that no one is actually hitting.

With that in mind, if you're experiencing something broken in an application, please file a separate issue about that with repro steps and that will get priorized ahead of vttest conformance!

wez added a commit that referenced this issue Jan 26, 2020
This makes us pass the cursor positioning tests
(with the exception of the 132 column mode)

refs: #133
wez added a commit that referenced this issue Jan 26, 2020
@wez
Copy link
Owner

wez commented Jan 26, 2020

I fixed up the cursor positioning conformance. There are a couple of screens that switch to 132 column mode that look messed up simply because we don't (and lkely won't) support 132 mode at all.
I also ran through the screen tests, and they look reasonable; some don't make a lot of sense to me (scroll positioning test doesn't have any information on what correct should look like), and we're not handling some of the character set shifts, but in a UTF-8 world I don't think that is a super high priority.

@erf
Copy link
Contributor Author

erf commented Jan 26, 2020

Thanks, i opened up a new issue, regarding the vis editor.

Maybe you could make a set of unit test files which tries to run spesific vt100 commands and read back the result?

Feel free to close this.

@wez
Copy link
Owner

wez commented Jan 26, 2020

Thanks, i opened up a new issue, regarding the vis editor.

Thanks!

Maybe you could make a set of unit test files which tries to run spesific vt100 commands and read back the result?

Yep, that's the approach used by our unit tests already!

@erf
Copy link
Contributor Author

erf commented Jan 26, 2020

I noticed the background colour is not set in the vttest 2, screen 3.

Btw. i notice a lot of executables that works in other terminals, does not in this. E.g. brew and vttest i have to enter the full path. Is there a configuration to fix this?

@wez
Copy link
Owner

wez commented Jan 26, 2020

@erf regarding the path: that may be because your shell configuration doesn't set up your PATH in all cases; by default your $SHELL is spawned and the PATH is inherited from your desktop environment.

You can force wezterm to spawn a login shell on macOS by default placing this in your ~/.wezterm.toml; this will tell your shell to process login related startup items and will likely make your PATH more consistent:

# on macOS, ask the system to start the shell in login mode.
# This makes new tabs and windows slightly slower to appear.
default_prog = ["login", "-pf"]

@erf
Copy link
Contributor Author

erf commented Jan 26, 2020

If i add that to config file, im prompted for login each time i start the terminal, which is cumbersome, but the paths work then. I dont have to do this on the other emulators like terminal, term2 and alacritty. Wonder if there is another way to solve this. Im using the zsh btw.

@wez
Copy link
Owner

wez commented Jan 26, 2020

I may have used the wrong arguments for login (was writing that from memory as I'm not at a mac machine right now), but if you are using zsh then you can use:

default_prog = ["zsh", "-l"]

to run it as a login shell with no auth prompts.

@wez
Copy link
Owner

wez commented Jan 26, 2020

btw: the reason this isn't automatic is because the unix way to spawn a login shell for any shell is to set its argv[0] to start with a - character, and Rust's command spawner doesn't current expose a way to do this.

I'm looking at adding an option to speculatively try passing -l as an option to deal with this, which happens to work for zsh, bash and fish, but may not work for other shells (so we need a way to disable it)

@erf
Copy link
Contributor Author

erf commented Jan 26, 2020

Thanks, that works!

btw resize the window from vttest 2 does not work, but it does not work in Iterm2 or Alacritty either, only in the macOS Terminal app. Its a xterm controll sequence as mentioned here, not sure if it should be added or not.

wez added a commit that referenced this issue Jan 26, 2020
@wez
Copy link
Owner

wez commented Jan 26, 2020

I pushed a commit that should make the login shell automatic.

regarding resizing: that's often seen as annoying; the user picked the size and placement and usually doesn't want an application to change it. There are some potential security/abuse concerns about a number of the escape sequences that manipulate things outside of the terminal display which is why they are often disabled by default. I don't see a compelling use case for that feature, and don't have plans to implement it.

@erf
Copy link
Contributor Author

erf commented Jan 27, 2020

vis opens now after the 38b19cb fix ! will close #134

one weird thing is that the terminal now opens in the root folder, not in the HOME folder, on startup

@wez
Copy link
Owner

wez commented Jan 27, 2020

Ah, I just commented on that other issue: I think you were running the wrong vis due to the problems with your path.

Re: starting in the root folder, do you still have default_prog set? Remove that from your config file and I think you'll pick up the correct home dir.

@erf
Copy link
Contributor Author

erf commented Jan 27, 2020

I removed the default_prog and that worked. Thanks for your help - good to be up and running:) Will have to look into the configuration now. Seem like a promising new terminal, great work so far!

Maybe i should close this issue now, and i could rather report in a new issue to avoid too much mess;)

@erf erf closed this as completed Jan 27, 2020
@erf
Copy link
Contributor Author

erf commented Jan 27, 2020

For reference, when i removed default_prog it seem i can't run executables anymore, even though i first though i could. Not sure what happened, but could you supply the link to the latest build again with the fix?

Unrelated but there seem to be an issue with utf-8 symbols, not sure if this is related to my system setup, or not supported yet.

@wez
Copy link
Owner

wez commented Jan 27, 2020

Please use the latest nightly build from the download page, and open new issue(s) for things that you're seeing!

@ghost
Copy link

ghost commented Jun 20, 2021

The test of tab stops can be passed easily if the TabulationClear::ClearCharacterTabStopsAtActiveLine case is ignored in TabStop::clear() , e.g.:

    fn clear(&mut self, to_clear: TabulationClear, col: usize) {
        match to_clear {
            TabulationClear::ClearCharacterTabStopAtActivePosition => {
                if let Some(t) = self.tabs.get_mut(col) {
                    *t = false;
                }
            }
            TabulationClear::ClearAllCharacterTabStops
            // | TabulationClear::ClearCharacterTabStopsAtActiveLine
            => {
                for t in &mut self.tabs {
                    *t = false;
                }
            }
            _ => log::warn!("unhandled TabulationClear {:?}", to_clear),
        }
    }

This would be consistent with real xterm, which only supports:

CSI Ps g  Tab Clear (TBC).
            Ps = 0  -> Clear Current Column (default).
            Ps = 3  -> Clear All.

It appears ECMA-48 defined more options, which if one honors them you get a failure of the test. I'm wondering if that was deliberate for vttest, since it was trying to check conformity with real DEC hardware which deviated from ECMA-48.

@ghost
Copy link

ghost commented Jun 20, 2021

It appears that wezterm is quite close to passing vttest at the VT102 level if the following features were completed:

I am interested in completing vttest compliance. It would enable wezterm to feasibly replace real hardware for some use cases, and be in the rather rare company of cross-platform open-source terminals that are close in behavior to a real VT102.

(Also sidebar on VT52 mode tests: the actual VT52 (DECscope) had a different graphics character set ("Graphics Mode") than the DEC Special Graphics of VT100+. There is a test in vttest for the graphics mode of VT52 submode. I verified via MAME emulation of VT102 ROMs that when the VT100 is in VT52 submode, it will use the VT100 DEC Special Graphics, not the VT52 Graphics Mode.)

@wez wez reopened this Jun 20, 2021
@wez
Copy link
Owner

wez commented Jun 20, 2021

I am interested in completing vttest compliance. It would enable wezterm to feasibly replace real hardware for some use cases, and be in the rather rare company of cross-platform open-source terminals that are close in behavior to a real VT102.

I'd welcome this! Many thanks in advance!

@ghost
Copy link

ghost commented Jun 23, 2021

Hi @wez, a quick checkpoint. With the changes so far, this branch has all of the VT102 cursor movements, screen features, and terminal reports tests completed, with the following limitations:

  • No blinking text on the screen features test. I want to see this one finished, since blinking text is used by games to good effect.
  • No ENQ AnswerBack message. This should just be a config value to emit.
  • The DECREQTPARM code feels "wrong", like not proper idiomatic Rust: https://github.com/klamonte/wezterm/commit/952ba0b3d4442b89e0361d263cfe7714447a6834 . I do not know how to get the first parameter of the sequence passed around so that the response can do math on it, or map 0 => 2 and 1 => 3 as I have done. I could use a pointer.

The remaining features are much more invasive. Here are my thoughts:

Double-width / Double-height

I actually think this is a very cool feature, but it can easily step on things like accessibility, line wrapping, and fullwidth chars (what is a double-width, top-half only grapheme supposed to look like?). I recommend explicitly parsing these sequences, setting (new) line-level flags, but not implementing the render side. Hence, it would be available for downstream projects, keep the wezterm log clean, but not be something to have to work around for any other features.

VT52 Mode

Due to the following text in the VT100 manual, implementing VT52 is not as simple as cutting to a different parser that operates on the same screen:

You use ANSI mode to select most terminal features; the terminal uses the same features when it switches to VT52 mode. You cannot, however, change most of these features in VT52 mode.

VT52 mode has to drive the same TerminalState as VT100 mode, so support must be woven into the same state machine. I notice your parser matches the Paul Flo Williams' machine (https://vt100.net/emu/dec_ansi_parser). Mine does too, but I had to add one state for the VT52 Direct Cursor Addressing sequence (ESC Y {line} {col} - and it has no terminator).

This is all subjective, but generally I perceive a terminal that has implemented VT52 as having put a bit more care into its VT100. A bit like seeing a nice dovetail joint on wood furniture: a sign that the big stuff is probably going to be good too. Now I am obviously an odd duck and have my own tunnel vision, caveat emptor etc etc.

Getting VT52 in would impact the following:

  • The arrow keys and number pad keys would have three modes: VT100, ANSI, and VT52.
  • The PF1-PF4 function keys would have two modes: VT100, and VT52.
  • VT52 is 7-bit only, so S8CIT (C1 controls) would not be recognized but only while in VT52 mode.
  • DECID would have a different response.
  • Quite a few ESC {char} sequences would be recognized, but only for VT52.
  • Most ESC sequences would not do anything, and there would be no transitions to other states besides the Direct Cursor Addressing state.

Most of these aren't a big deal, but the last one in particular is what I don't know how to do. It was straightforward for me writing in C because I had the state machine unrolled into a massive switch rather than a table. My State::Escape / TerminalState::esc_dispatch equivalent spot could check the vt52 mode flag for the overlapping sequences, bypass the sequences VT52 won't recognize, and also select the next transition based on the mode flag.

I don't know the best / most transparent way to do these things in a table-based state machine. If you have a pointer on how I could get TerminalState::esc_dispatch to go to a new State::VT52DirectCursorAddress, then I can get the rest of it in.

But I wanted to outline the full effect of VT52 submode, so that you could also make a fair decision. This may not be something you really want for wezterm, even if it was ready to merge in nice idiomatic Rust.

@wez
Copy link
Owner

wez commented Jun 24, 2021

@klamonte Sounds great!

blinking

This should already be in the model, and it "just" needs hooking up in the renderer. I'd suggest looking at how animated gif frames are scheduled as part of this. eg: we keep track of whether blinking text is in the viewport and schedule a repaint at the appropriate blink interval. wezterm models both slow and fast blinks so it would be great to respect those.

For the blink itself, I'd be inclined to set a boolean uniform in the shader that indicates whether we're on the hide portion of a blink and then override or skip rendering the glyph

https://github.com/wez/wezterm/blob/main/wezterm-gui/src/termwindow/render.rs#L492
https://github.com/wez/wezterm/blob/main/wezterm-gui/src/glyph-frag.glsl#L7

The DECREQTPARM code feels "wrong"

I commented on that commit!

Double width/height

Adding support to the model sounds good to me and it would be the first step to one day rendering it if we want to do that.

VT52 Mode

I haven't really looked in this at all. Is this mode switchable via an escape sequence? Per your last point about efficiently NOPing the escape sequences: I'm sure we can come up with something reasonable.

As to whether it should be in wezterm: my stance is fairly pragmatic: if there are applications that use it and need it, I don't have any objections. If it is a bit more theoretical and it is a PITA to build and maintain then I'm less inclined to go for it.

@ghost
Copy link

ghost commented Jun 26, 2021

@wez

VT52 Mode

I haven't really looked in this at all. Is this mode switchable via an escape sequence? Per your last point about efficiently NOPing the escape sequences: I'm sure we can come up with something reasonable.

Yes, it's enabled by CSI ? 2 l (DECANM).

As to whether it should be in wezterm: my stance is fairly pragmatic: if there are applications that use it and need it, I don't have any objections. If it is a bit more theoretical and it is a PITA to build and maintain then I'm less inclined to go for it.

At this point it's more PITA, but not hard to maintain once built. I have not seen an application besides vttest itself use it, even when running on a real VMS system.

@wez
Copy link
Owner

wez commented Jun 26, 2021

At this point it's more PITA, but not hard to maintain once built. I have not seen an application besides vttest itself use it, even when running on a real VMS system.

I don't have an objection to it going in if maintenance isn't going to be a burden and you're OK doing the work to get it in :-)

@ghost
Copy link

ghost commented Jun 27, 2021

I don't have an objection to it going in if maintenance isn't going to be a burden and you're OK doing the work to get it in :-)

It would be a nice feather in the cap to get it in. :-) I will aim to get more comfortable with Rust and see if I can figure it out. I'm liking Rust quite a bit, actually. It reminds me a lot of Lisp, but with good lessons from Java, D1, and Go. It helps to have a clean codebase to work with too. :)

Just got the normal/fast blinking in, and it works which is cool. A bit ugly with the two different scans: I would rather have a single scan that returns either zero (no blinking), one, or two new intervals plus set the two last_text_blink times. But given so few uses for animations/blinking on the text side, a more general solution is probably YAGNI.

Also got the ENQ answerback in (default nothing). For giggles, if you set it like:

enq_answerback = "echo \x16\x05\r",

...then you get into a loop of enquire/response/enquire/response. But you can type anything to interrupt the shell command and stop it. I've been wondering if that were possible, and now I know. ;-)

Anyway, I am just about ready to make a rebase'd PR against head, so all the changes can be reviewed as a whole.

@ghost ghost mentioned this issue Jun 27, 2021
wez pushed a commit that referenced this issue Jul 25, 2021
wez pushed a commit that referenced this issue Jul 25, 2021
wez pushed a commit that referenced this issue Jul 25, 2021
wez pushed a commit that referenced this issue Jul 25, 2021
wez pushed a commit that referenced this issue Jul 25, 2021
wez pushed a commit that referenced this issue Jul 25, 2021
wez pushed a commit that referenced this issue Jul 25, 2021
wez pushed a commit that referenced this issue Jul 25, 2021
wez pushed a commit that referenced this issue Jul 25, 2021
wez added a commit that referenced this issue Jul 25, 2021
Also fixes an issue where only the first frame schedule would
take effect!  Surprised this didn't bubble up as a bug with
animated gifs already.

refs: #133
wez added a commit that referenced this issue Jul 25, 2021
In the case where the cells vec is shorter than the line width,
we need to ensure that we render the inverse video background
color if that mode is in effect.

refs: #133
wez added a commit that referenced this issue Jul 25, 2021
@wez
Copy link
Owner

wez commented Aug 18, 2021

@klamonte do you think we're in a state to close this issue now that your changes are merged?

@ghost
Copy link

ghost commented Aug 18, 2021

@wez I would say so. The only outstanding feature is VT52 submode, and if I ever figure out a clean way to do that you will see it in a fresh clean PR. :)

@wez
Copy link
Owner

wez commented Aug 18, 2021

Alright, let's close this!
Thanks for your work on this!

@github-actions
Copy link

github-actions bot commented Feb 4, 2023

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants