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

Moved from unicode-width to unicode-display-width for visual grapheme width estimation #210

Closed
wants to merge 2 commits into from

Conversation

TapGhoul
Copy link

@TapGhoul TapGhoul commented Apr 4, 2024

This change had to be made in both crates in order to be updated correctly.
The console-rs crate has been bumped an extra version as a result of a behavior change of char_width() with the unicode-display-width feature enabled - it now returns always 1 or 2, never 0 as it used to.
The indicatif crate may also need to be bumped a full version as a result of the dependency change, but I'm not sure if you'd consider this a breaking change to the same degree, as it's not an advertised feature.

Related issue: console-rs/indicatif#638
Related PR: console-rs/indicatif#639

@TapGhoul
Copy link
Author

TapGhoul commented Apr 4, 2024

Pulling in @djc as requested

{
use unicode_width::UnicodeWidthStr;
s.width()
unicode_display_width::width(s) as usize
Copy link
Author

Choose a reason for hiding this comment

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

Note: this usize cast is here to avoid changing APIs. It may not be what is wanted?

Copy link
Member

Choose a reason for hiding this comment

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

This seems okay to me. Would sure be nice to avoid breaking compatibility for this.

@@ -13,13 +13,13 @@ readme = "README.md"
rust-version = "1.56.0"

[features]
default = ["unicode-width", "ansi-parsing"]
default = ["unicode-display-width", "ansi-parsing"]
Copy link
Member

Choose a reason for hiding this comment

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

If we use a feature named unicode-width that then enables ["dep:unicode-display-width"] this might be backwards incompatible?

src/utils.rs Outdated
{
use unicode_width::UnicodeWidthChar;
c.width().unwrap_or(0)
match unicode_display_width::is_double_width(c) {
Copy link
Member

Choose a reason for hiding this comment

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

For compatibility, maybe we should replicate the logic from unicode-width here for control characters? Maybe like:

match c.is_control() {
     true if c == '\x00' => ?,
     true => 0,
     false => match unicode_display_width::is_double_width(c) {
         true => 2,
         false => 1,
     }
}

@TapGhoul
Copy link
Author

TapGhoul commented Apr 5, 2024

Worth an extra note: I'm not sure if truncate_str() needs to be adjusted - it'll depend on how ANSI is handled here, and how much we need to care about grapheme clusters. I'm not 100% sure if we need to change if unicode_display_width::is_double_width(c) { into if c == '\u{FEFF}' || unicode_display_width::is_double_width(c) { or not, as a result, as per how unicode_display_width::get_grapheme_width() operates. Unfortunately, I don't have a test case on hand to check this - do you have one that can be used to validate things?

use unicode_width::UnicodeWidthChar;
c.width().unwrap_or(0)
#[cfg(feature = "unicode-display-width")]
if c < '\u{7F}' {
Copy link
Member

Choose a reason for hiding this comment

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

Why did you go this route? I'd like to avoid referring to explicit code points as much as possible as it would be likely for these to fall out of sync for Unicode updates -- that's what we're trying to rely on other crates for.

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