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

std.Progress: fix inaccurate line truncation and use optimal max terminal width #12079

Merged
merged 14 commits into from
Oct 13, 2022

Conversation

wooster0
Copy link
Contributor

@wooster0 wooster0 commented Jul 11, 2022

I really had a lot of attempts at this and I think this might be the best one so far. I think there's still some things to improve but this is definitely a start.

Output now fits nicely on pretty much any terminal width (well, not if it's >256 though, but that limit can easily be bumped):
image

Here's another common issue that I think Andrew described in #12024 (comment):

image

That issue seems to be fixed now; here's a compilation on a terminal with a very small width:

2022-07-11.20-05-03.mp4

It succeeds with nothing left on the terminal.

Closes #12024

@wooster0
Copy link
Contributor Author

wooster0 commented Jul 11, 2022

samu: job failed: cd /usr/home/build/zig && /usr/home/build/zig/build/zig0 src/stage1.zig -target x86_64-freebsd-gnu -mcpu=baseline --name zig1 --zig-lib-dir /usr/home/build/zig/lib -femit-bin=/usr/home/build/zig/build/zig1.o -fcompiler-rt -OReleaseFast --strip -lc --pkg-begin build_options /usr/home/build/zig/build/config.zig --pkg-end --pkg-begin compiler_rt /usr/home/build/zig/lib/compiler_rt.zig --pkg-end
./lib/std/os.zig:181:27: error: container 'std.c' has no member called 'termios'
pub const termios = system.termios;
                          ^

So FreeBSD doesn't have termios which I use to get the terminal width...

Or maybe FreeBSD does have it but we just don't have the things exposed in https://github.com/ziglang/zig/blob/master/lib/std/c/freebsd.zig?

I've limited it to Linux (and Windows) for now. Good enough for a start.

Currently, the code assumes a terminal width of 100.

If we look at what's printed for the last test:
```
Test [1/1] test "basic functionality"... [101/100] this is a really long name designed to activate the truncation code. let's fi...
```
No, it does not really work because the relevant part here is `"[101/100] this is a really long name designed to activate the truncation code. let's fi... "`,
which is 90 characters, but we expect 100 because that's the width that is assumed.
The reason is that it also measures **unprintable characters** (escape sequences) at least non-Windows systems.
With this commit the output is now:
```
Test [1/1] test "basic functionality"... [101/100] this is a really long name designed to activate the truncation code. let's find out if...
```
Of which `"[101/100] this is a really long name designed to activate the truncation code. let's find out if... "`
is the actual output of *our* `std.Progress` (remember that `zig test` has an `std.Progress` and our test itself does).
The length of that string is 100. Now the length is consistent with Windows where we don't use escape sequences. This issue was only present on non-Windows systems.
This is done by 1. getting the current terminal width and 2. subtracting that by the current cursor column. This accounts for previous output from someone else.
They make it easier to see how the progress line is printed in different cases.
It also expands an acronym used as a variable name. It confused me.
@andrewrk andrewrk merged commit cd3d8f3 into ziglang:master Oct 13, 2022
@andrewrk
Copy link
Member

Thanks for the deep dive into this problem!

@andrewrk
Copy link
Member

This did not solve the problem of resizing the terminal window while progress is being printed:

image

@wooster0
Copy link
Contributor Author

wooster0 commented Oct 13, 2022

Yes, this behavior is as expected.

The main problem I focused on to solve here was the one you originally described where if the line goes beyond the end of the terminal then the output becomes malformed. Resizing the terminal while progress is being printed and then the output becoming malformed is a different, probably more difficult problem.

It's because getTerminalWidth, a function we would definitely need if we wanted to fix this somehow, is only called once at the start:

const terminal_width = self.getTerminalWidth(terminal.handle) catch 100;

So after that we don't recalculate any values based on the terminal width. Well, it's only really max_width that we have to calculate based on terminal width and cursor position.

It seems I talked a bit about it in #12024 (comment) as well.

There are two ways I can think of that may be useful here for attempting to fix this:

  1. Detect terminal width changes based on some callback. The only option I can think of at the moment is if we listen to the SIGWINCH ("signal window change") signal on POSIX systems and then call getTerminalWidth if we catch it. Next question would be how to do this on Windows.
  2. Periodically call getTerminalWidth. When would we call it? Before every refresh in refreshWithHeldLock? Well here's a scenario: we printed some progress, then we sleep for say 2 seconds for whatever reason and then during those 2 seconds the user resizes their terminal: the user will immediately notice that characters are overflowing off the right of the terminal down to the next line, and from there on it's kind of hard to recover the previous state. What could we do? Clear the line below to clear the characters that overflowed? I think there are a number of ways we could kind of fix this, but it would need some more thought.

I did try to put a self.calculateMaxWidth(); right here:


but I couldn't really notice a difference. It certainly didn't fix that issue you're describing.

Maybe we can come up with a way to fix this someday. Another thing we could do of course is enabling the terminal's alternate screen buffer which would give us full control and then we could just clear the screen if the terminal is resized.
I guess for now what std.Progress expects is that once some process of work is started, std.Progress does its progress output and expects the user to lean back and wait and not adjust the terminal size any further until std.Progress is done.

andrewrk added a commit that referenced this pull request Oct 19, 2022
I have noticed this causing my terminal to stop accepting input
sometimes. The previous implementation with all of its flaws was better
in the sense that it never caused this to happen.

This commit has multiple reverts in it:

Revert "Merge pull request #13148 from r00ster91/progressfollowup"

This reverts commit cb257d5, reversing
changes made to f5f28e0.

Revert "`std.Progress`: fix inaccurate line truncation and use optimal
max terminal width (#12079)"

This reverts commit cd3d8f3.
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.

std.Progress: possible simplifications and ideas
3 participants