-
Notifications
You must be signed in to change notification settings - Fork 14
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
Conversation
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.
I think the command within the guest is going to run as 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 ...). |
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)] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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. |
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.
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. |
Some usability improvements. See individual commits for more details.