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 caps width far below terminal width #13440

Open
nektro opened this issue Nov 4, 2022 · 11 comments
Open

std.Progress caps width far below terminal width #13440

nektro opened this issue Nov 4, 2022 · 11 comments
Labels
contributor friendly This issue is limited in scope and/or knowledge of Zig internals. enhancement Solving this issue will likely involve adding new logic or components to the codebase. standard library This issue involves writing Zig code for the standard library.
Milestone

Comments

@nektro
Copy link
Contributor

nektro commented Nov 4, 2022

Zig Version

0.11.0-dev.52+42755a194

Steps to Reproduce and Observed Behavior

./build/zig2 build test-cases -Dskip-non-native -Dskip-release -Dskip-single-threaded

image

Expected Behavior

using the full terminal width

@nektro nektro added the bug Observed behavior contradicts documented or intended behavior label Nov 4, 2022
@andrewrk andrewrk added enhancement Solving this issue will likely involve adding new logic or components to the codebase. contributor friendly This issue is limited in scope and/or knowledge of Zig internals. standard library This issue involves writing Zig code for the standard library. labels Nov 4, 2022
@andrewrk andrewrk added this to the 0.12.0 milestone Nov 4, 2022
@andrewrk andrewrk removed the bug Observed behavior contradicts documented or intended behavior label Nov 4, 2022
@andrewrk
Copy link
Member

andrewrk commented Nov 4, 2022

This is a known limitation of the way std.Progress works right now. Would be nice if there were a way to solve this.

@ghost
Copy link

ghost commented Nov 4, 2022

It looks like the problem comes from output_buffer having fixed length. For it to work with the terminal width, it would have to be dynamically allocated. (Or maybe just pick a large upperbound, like 500?)

@mitchellh
Copy link
Contributor

mitchellh commented Nov 4, 2022

As a guy who has been learning too much about escape codes recently 😉 I took a look and would suggest perhaps a different approach. I just Iike really quickly looked at how Progress was implemented, I didn’t see how Progress was being used so this whole response might be dumb.

Right now, std.Progress uses ESC[nD to move the cursor back n so it needs to keep track of columns. That’s fine, totally workable. But another approach would be to call ESC[6n at the start of a refresh to query cursor position, which will return the <row, col>. Then just output the whole progress no matter the length and let the terminal wrap it if it has to (auto wrap). Then for the next call use ESC[H to set the cursor position back to an absolute row, col that was queried earlier.

You can have a call to throw away the prev position if you want to preserve it (i.e. you outputted test failure information you want to preserve and not overwrite since the last progress call).

Another approach is to query the terminal size, but this requires making a ioctl call which you may or may not want to deal with in the std.Progress implementation. Another hack that totally works is to use ESC[H with an absurdly large number (say, 10000, 10000) which sets the cursor position. The terminal emulator must bound the cursor position by the terminal size so if you do that then call ESC[6n it’ll report the size in grid cells.

Each of these might require small public API changes to Progress though, but I’m very sure this problem can be solved without heap allocations.

@nektro
Copy link
Contributor Author

nektro commented Nov 4, 2022

how feasible would a loop like below work for std.Progress?

  • *print line*
  • \n
  • ESC[1A
  • ESC[0K
  • repeat

@mitchellh
Copy link
Contributor

That approach would require you keep track of terminal width. All the line-oriented escape codes treat a line as a visible line (or physical line). It doesn’t unwrap soft-wrapped stuff. So if a terminal soft wraps, that’s 2 lines and you’ll start getting artifacts unless you’re careful to never wrap.

@wooster0
Copy link
Contributor

wooster0 commented Nov 5, 2022

I've already gone through this in my PRs:
#12079 #13085 #13148 #13148 (comment) 1952dd6
The status is that there's some weird issue going on that caused my main PR to be reverted

@wooster0
Copy link
Contributor

wooster0 commented Nov 5, 2022

That "weird issue" is very likely coming from this function https://github.com/ziglang/zig/pull/12079/files#diff-f05e9ed61d2716c50e09b00c1881f44cdb2a85107d1b5c95231f80e489ea265eR225 which uses ESC[6n that @mitchellh mentioned. It messes with termios which would be great if we could avoid it but there just doesn't seem to be a way to avoid it.

Querying the terminal window size is pretty easy and much less troublesome: https://github.com/ziglang/zig/pull/12079/files#diff-f05e9ed61d2716c50e09b00c1881f44cdb2a85107d1b5c95231f80e489ea265eR205

@wooster0
Copy link
Contributor

wooster0 commented Nov 5, 2022

@nektro doesn't work if you have multiple std.Progress instances from different processes (they can't communicate with each other) so if you do ESC[0K you might clear the progress of some other process behind it (see #13148 (comment))

@wooster0
Copy link
Contributor

wooster0 commented Nov 5, 2022

Here's how my PR determines the optimal width to use for the terminal when a std.Progress is created: https://github.com/ziglang/zig/pull/12079/files#diff-f05e9ed61d2716c50e09b00c1881f44cdb2a85107d1b5c95231f80e489ea265eR179-R181
It gets the terminal width and then the position of the cursor in the terminal. Then we just subtract the position from the terminal width so that we account for text printed before us.

@nektro
Copy link
Contributor Author

nektro commented Nov 5, 2022

@r00ster91 then you could make it so they have a reference to a parent that they call to print before printing their own status

@wooster0
Copy link
Contributor

wooster0 commented Nov 5, 2022

That would involve communication between entirely separate processes right? For example if you have multiple separate zig invocations. I think in that case the solution of getting the terminal cursor's x-axis is pretty simple, but maybe there is a simpler or more reliable solution? If we could communicate with std.Progresss of other system processes, we can solve it that way too. Not sure how we could obtain such a reference to a parent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor friendly This issue is limited in scope and/or knowledge of Zig internals. enhancement Solving this issue will likely involve adding new logic or components to the codebase. standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants