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

Truncate output on narrow terminals #11

Merged
merged 15 commits into from
Jan 24, 2024
Merged

Conversation

triarius
Copy link
Contributor

@triarius triarius commented Dec 29, 2023

When the terminal is narrower than the width of the output, currently, the default mode experiences line wrapping, which does not look great:
2023-12-27-15-54-52

In contrast, neofetch truncates each line to the width of the terminal:
2023-12-29-19-42-54

This PR causes zeitfetch to have the same behaviour:
2023-12-29-19-44-00

To do this, I had to implement a truncate function that ignores but preserves ANSI SGR escape codes. I couldn't find one that behaves precisely the way I wanted, in particular, my implementation will preserve all ANSI SGRs in the input, even if they are past the truncation point.

This lead me to notice that some of the logos don't reset the SGR \x1b[0m on each line, which I think is important because now the logo lines and the sysinfo lines are combined, so if the logo does not reset the graphics, the sysinfo line could be affected. So I've terminated each logo with \x1b[0m where necessary.

Note: the width detection may not work on all terminals. I've noticed kitty on hyprland has this issue: kovidgoyal/kitty#6530 which I've seen affecting the code in this PR. However, I still think this PR is worth merging because:

  1. Not all terminals have this issue on hyprland, I've tested alacritty and wezterm, and they seem to work fine.
  2. The solution for kitty (add a signal handler for SIGWINCH and wait for at least one before displaying the output) seems overly complex for an application like zeitfetch, is likely to lengthen the runtime, and there's no guarantee a SIGWINCH will be sent or caught in time.

I can have a go at implementing with some heuristic on how long to wait for a SIGWINCH if there is sufficient demand, but without it, it seems we only loose truncation for kitty on some window managers. The output will still be displayed without truncation, so nothing has been taken away from kitty users.

@nidnogg nidnogg self-requested a review December 29, 2023 17:14
@nidnogg
Copy link
Owner

nidnogg commented Dec 29, 2023

Hi @triarius! Happy New Year's and thanks for chipping in with another great PR! I might test these changes out later today, but if I don't merge it in right away, I'll possibly check them in after New Year's as I'm away from home at the moment.

I currently use zetifetch on iTerm and Powershell, and I know a few users who have it on macOS's default terminal too. So I think as long as those work OK these changes should be fine.

Regarding this change:

This lead me to notice that some of the logo don't reset the SGR \x1b[0m on each line, which I think is important because now the logo lines and the sysinfo lines are combined, so if the logo does not reset the graphics, the sysinfo line could be affected. So I've terminated each logo with \x1b[0m where necessary.

I want to test out how logos look without those SGR \x1b[0m completions a bit, as I can see myself or other contributors forgetting to add those later down the road, but they might not be too much of an extra step after all.

@triarius
Copy link
Contributor Author

@nidnogg Happy New Year to you too!

No rush to merge, I may be able to test it on a macOS environment soon, I'll post some screenshots if I do.

As for future devs forgetting the SGR \x1b[0m, how about a test that fails if they aren't present? I think that's a good idea anyway. I'll do that now.

This is a better test as it will allow line that never set any SGR to
not require resetting it.
Copy link
Contributor Author

@triarius triarius Jan 8, 2024

Choose a reason for hiding this comment

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

The logos are now in a static map, with the help of the phf dependency. I made the implicit map (i.e. a match statement) explicit, so I could write a test that checks the logos terminate their SGR codes. Ideally, I would create a map that takes enum keys and &'static str values, but I could not get phf to work like this. Instead, I created an enum and added a test that the variants of Logo enum are present in the static LOGOS map. I used the sturm and strum_derive crates for this.

While there is a small amount of duplication needed to add a new logo, it will be checked by the test_logos_keys_match_enum test.

@triarius
Copy link
Contributor Author

triarius commented Jan 8, 2024

@nidnogg I'm back from travels, and I've got screenshots showing the truncation working on macOS:

Terminal.app

Screenshot 2023-12-31 at 2 26 51 pm

iTerm 2

Screenshot 2023-12-31 at 2 25 57 pm

@nidnogg
Copy link
Owner

nidnogg commented Jan 23, 2024

@nidnogg I'm back from travels, and I've got screenshots showing the truncation working on macOS:

Terminal.app

Screenshot 2023-12-31 at 2 26 51 pm ## iTerm 2 Screenshot 2023-12-31 at 2 25 57 pm

Heya again @triarius! I'm finally back from travels and I've reviewed the PR and it should be good to merge soon. I'll do some final testing and wrap it in the next release.

Thanks again!

This is an issue for terminals that report their width asynchronously.
If zeitfetch is started before the terminal has reported its width, we
should not truncate instead of falling back to a default size.
@triarius
Copy link
Contributor Author

Thanks, @nidnogg. I realised I had tweaks to make, so I push a few more commits. Apologies.

The main change is b2a9999 which falls back to not truncating if the terminal width is not detected. It should make this PR safer to merge.

@nidnogg nidnogg merged commit 2d0048f into nidnogg:main Jan 24, 2024
4 checks passed
@nidnogg
Copy link
Owner

nidnogg commented Jan 24, 2024

Merged - should be live in latest release soon. Thanks for another great contribution!

@triarius triarius deleted the dont-wrap branch January 24, 2024 22:53
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.

2 participants