Skip to content

Implement measure_text_width with no allocation #246

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

Merged
merged 1 commit into from
Mar 14, 2025

Conversation

remi-dupre
Copy link
Contributor

@remi-dupre remi-dupre commented Mar 11, 2025

I implemented that while working on optimizing something I don't remember, it might still be useful.

Also, I noticed that the test for this function was wrong when only using the unicode-width feature (probably due to some update in the corresponding?).

# Running on origin/main
$ cargo test --no-default-features -F unicode-width 
running 6 tests
test utils::test_pad_str ... ok
test utils::test_truncate_str_no_ansi ... ok
test utils::test_pad_str_with ... ok
test utils::test_text_width ... FAILED
test utils::test_attributes_many ... ok
test utils::test_attributes_single ... ok

failures:

---- utils::test_text_width stdout ----
thread 'utils::test_text_width' panicked at 'assertion failed: `(left == right)`
  left: `21`,
 right: `17`', src/utils.rs:940:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I've completed the Makefile so that all possible sets of feature are tested, as the project is not too big I don't think it's too heavy.

@remi-dupre remi-dupre force-pushed the measure-text-width-noalloc branch 4 times, most recently from 3d9295a to 7329c4b Compare March 11, 2025 22:09
@remi-dupre remi-dupre force-pushed the measure-text-width-noalloc branch from 7329c4b to c47485b Compare March 12, 2025 09:07
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

Thanks, this mostly looks good!

src/utils.rs Outdated
.filter(|(_, is_ansi)| !is_ansi)
.map(|(sub, _)| str_width(sub))
Copy link
Member

Choose a reason for hiding this comment

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

Nit: suggest spelling it like this:

.filter_map(|(s, is_ansi)| (!is_ansi).then(|| str_width(s)))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find it less readable, but that's your project 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. In that case, I'd like it to be written like this (assuming clippy is okay with it):

.filter_map(|(s, is_ansi)| match is_ansi {
    false => Some(str_width(s)),
    true => None,
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clippy is okay with that 👍

@remi-dupre remi-dupre force-pushed the measure-text-width-noalloc branch from c47485b to 0b56d9c Compare March 12, 2025 09:30
@remi-dupre
Copy link
Contributor Author

I'll investigate on why the tests fails later, I start to realy suspects some behavior change in unicode_width (I know this sounds cliché but it worked on my computer 😢)

I implemented that while working on optimizing something I don't
remember, it might still be useful.
@remi-dupre remi-dupre force-pushed the measure-text-width-noalloc branch from 83970ac to 7a4ad64 Compare March 14, 2025 21:36
@remi-dupre
Copy link
Contributor Author

Tests are fixed.

This was just a rooky mistake of forgetting to put .force_styling(true) in the new test case, it took me way too much time to figure that out 😅

@djc djc merged commit e6882ab into console-rs:main Mar 14, 2025
15 checks passed
@djc
Copy link
Member

djc commented Mar 14, 2025

Nice, thanks!

@remi-dupre remi-dupre deleted the measure-text-width-noalloc branch March 14, 2025 22:21
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