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

ui: Usability improvements #110

Merged
merged 3 commits into from
Jan 10, 2025
Merged

ui: Usability improvements #110

merged 3 commits into from
Jan 10, 2025

Conversation

danobi
Copy link
Owner

@danobi danobi commented Dec 28, 2024

Some usability improvements. See individual commits for more details.

The original need for stripping ansi escape sequences was so we could
apply our own styling when the terminal is attended and we're doing
custom terminal drawing. However, when the output is unattended, we
do not have a need for stripping ansi escape sequences.

Lots of CIs force coloring even though output is not attended. This is
so the logs are easier to read. vmtest should respect that and not strip
ansi escape sequences when output is unattended.
@danobi danobi changed the title ui: Do not strip styling for unattended output ui: Usability improvements Dec 28, 2024
@chantra
Copy link
Collaborator

chantra commented Jan 6, 2025

I think the command within the guest is going to run as attended because it is attached to a tty. Therefore commands may generate escape characters assuming a human is behind the command.
On the other hand, vmtest may or may not be attended, but this is disconnected from what the guest knows.

The confusing part too is that here we essentially want to "disable styling by stripping it" when attended (because we do our own) but leave it untouched when not attended, which is counter intuitive as usually, a command should not do any styling if unattended (so it is easier to grep ...).

Comment on lines +16 to +21
const HELP_ENV_VARS: &str = r#"Environment variables:
VMTEST_NO_UI Set to disable UI [default: unset]
"#;

#[derive(Parser, Debug)]
#[clap(version)]
#[clap(version, disable_colored_help=true, after_help=HELP_ENV_VARS)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

clap supports both parameter and env variable by setting env in the argument derive. https://stackoverflow.com/a/73531388

This means that the value of args.dry_run could be set either from the CLI with --dry-run or from the environment with DRY_RUN=1, not exactly what you are trying to do, but mentioning htis in case.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I was trying to avoid needing to thread the variable through a bunch of the code (which would require a larger refactor). Seemed ok enough to just bypass with env var :P

src/ui.rs Outdated Show resolved Hide resolved
@chantra
Copy link
Collaborator

chantra commented Jan 7, 2025

I think the command within the guest is going to run as attended because it is attached to a tty.

Actually... the opposite, it is always running in unattended mode because the command runs through the qemu guest agent, not the shell we spin during init.

danobi added 2 commits January 9, 2025 12:23
This disable the terminal drawing UI in vmtest. Previous workaround was
`vmtest ... | cat -`. But that's only obvious to people who are deeply
familiar with linux/unix. So add a more mundane environment variable and
document it.
String clipping is only useful if we're rendering the UI. If output is
unattended or user has requested UI disabled, no need to clip output.
@danobi
Copy link
Owner Author

danobi commented Jan 9, 2025

I think the command within the guest is going to run as attended because it is attached to a tty.

Actually... the opposite, it is always running in unattended mode because the command runs through the qemu guest agent, not the shell we spin during init.

Right, so by default the output from the guest is unstyled. And the UI will do a pre-emptive strip so window styling can reliably be applied. That's in case guest styling was forced by the user.

But in the case the UI is disabled, we should leave any forced styling in. B/c there's no need to strip in that case - user is explicitly requesting it and we're not drawing the window.

@danobi danobi merged commit c0f1f10 into master Jan 10, 2025
1 check passed
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