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

Fix wide element styling to work with 'improved_unicode' feature as well #357

Merged
merged 1 commit into from Jan 28, 2022

Conversation

chris-laplante
Copy link
Collaborator

@chris-laplante chris-laplante commented Jan 28, 2022

From style.rs, line 385:

impl<'a> WideElement<'a> {
    fn expand(
        self,
        cur: String,
        style: &ProgressStyle,
        state: &ProgressState,
        buf: &mut String,
    ) -> String {
        let left = state.width().saturating_sub(measure_text_width(&cur));
        let left = left + 1; // Add 1 to cover the whole terminal width

When the improved_unicode feature is off, measure_text_width just counts characters. Consider the case of rendering the default progress bar style. cur will be something like \x00 0/10. In this case measure_text_width returns the width as 6 since it is counting the null byte. This bug is what led to the addition of line 394 to offset it (in #350):

let left = left + 1; // Add 1 to cover the whole terminal width

When improved_unicode is on, measure_text_width correctly returns 5 because it ignores the control character. However, the aforementioned line adds a 1 which causes the progress bar to spill a character onto the next line of the terminal.

To reproduce, simply run cargo test --features=improved_unicode on current main branch. You will get several test failures caused by 'attempt to subtract with overflow', src/draw_target.rs:402:44

None of the automated tests caught this because they run in an environment without a tty, so indicatif considers the bars "hidden" and thus nothing is rendered. I spotted this as part of developing the in-memory terminal (#354).

@djc djc merged commit 8dc48d9 into console-rs:main Jan 28, 2022
@djc
Copy link
Collaborator

djc commented Jan 28, 2022

Awesome that your testing work is already finding bugs, thanks for your efforts!

@sigmaSd
Copy link
Contributor

sigmaSd commented Jan 28, 2022

Nice!

Is there a way to making testing all features the default, so cargo test tests everyrhing

I know github ci can be detected with github envioroment variable we can make those test return if theyre set

@sigmaSd
Copy link
Contributor

sigmaSd commented Jan 28, 2022

I ask this because I didnt even notice these tests so I feel like they are not discoverable

@mostthingsweb
Copy link

Nice!

Is there a way to making testing all features the default, so cargo test tests everyrhing

I know github ci can be detected with github envioroment variable we can make those test return if theyre set

I don't think there is a way to change the default behavior of cargo test, but you could try installing this: https://crates.io/crates/cargo-all-features

The GitHub workflow is already configured to test with --all-features. I think we should also have it test with just the default features enabled too (which currently is the empty set) though. I'll submit a PR to do that.

@mostthingsweb mostthingsweb deleted the cpl/unicode_fix branch January 29, 2022 23:37
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.

None yet

4 participants