Skip to content

Introduce Width::set_ambiguous_wide#806

Open
kojiishi wants to merge 2 commits into
console-rs:mainfrom
kojiishi:ea_ambiguous
Open

Introduce Width::set_ambiguous_wide#806
kojiishi wants to merge 2 commits into
console-rs:mainfrom
kojiishi:ea_ambiguous

Conversation

@kojiishi

Copy link
Copy Markdown
Contributor

Provides a solution for #497 by allowing callers to opt-in when they're in ambiguous-wide environment.

It doesn't cover grapheme cluster issues also discussed in #497.

@kojiishi
kojiishi marked this pull request as ready for review May 16, 2026 16:32
@kojiishi

Copy link
Copy Markdown
Contributor Author

Note, this PR is on top of #799, and thus if you go to the diff tab, you'll see both changes. You probably want either look at only the 2nd commit in the commits tab, or merge #799 and review the diff tab.

Comment thread src/width.rs Outdated
@djc

djc commented Jun 2, 2026

Copy link
Copy Markdown
Member

How would callers decide to set this value, anyway? Are there ways to query the font metrics, or does the caller basically just have to guess what environment it's running in?

@kojiishi

Copy link
Copy Markdown
Contributor Author

How would callers decide to set this value, anyway? Are there ways to query the font metrics, or does the caller basically just have to guess what environment it's running in?

It depends on the callers.

  • If they know the fonts, they can query the width of one of such characters.
  • If they're console programs (most likely for indicatif I suppose?), unfortunately console API doesn't provide font information. It should be a guess, or a user setting.

Either way, the bad news is that there's no very easy way to do it. If there's a concrete way, unicode-width might have already done it. They just delegated to callers, and we're delegating to our callers, until it reaches someone who knows the font or the end user environment.

That's where a global setting might win. We could check an env var to initialize the default, then users (not callers) can change the settings without modifying their programs.

A few new thoughts from the discussion:

  • It is the cached char_width that makes it complicated. Moving it to the state and compute it on-demand should be the cleanest way, though the change becomes a bit bigger.
  • Even if we make it non-global, it may be a good idea to check an env var to setup the default value, in case callers of indicatif didn't do anything, but users want to switch. Maybe a separate, additional enhancement.
  • Further, if we're doing this "delegate" chains, it may be a good idea for unicode-width itself to check an env var and switch to the cjk variant.
    If unicode-width likes the idea, then no changes are needed to indicatif, unless you like this refactoring (to gather all width logic to one place).

@kojiishi
kojiishi force-pushed the ea_ambiguous branch 2 times, most recently from 89ab0be to 7fb5cc9 Compare June 27, 2026 09:00
@kojiishi

Copy link
Copy Markdown
Contributor Author
  • Further, if we're doing this "delegate" chains, it may be a good idea for unicode-width itself to check an env var and switch to the cjk variant.

...it may not be a good idea, because unicode-width is so low-level that branching by a global var might regress the performance. Better to branch at higher-level.

Anyway, I updated the patch, with 2 commits:

  1. The 1st commit moves ProgressStyle.char_width cache to BarState. It's a pure-refactoring without behavior changes.
  2. The 2nd commit adds the new flag, to switch between width and width_cjk.

@djc

djc commented Jul 1, 2026

Copy link
Copy Markdown
Member

That's where a global setting might win. We could check an env var to initialize the default, then users (not callers) can change the settings without modifying their programs.

Okay, I think I now agree that this should be a process-global (maybe a static WIDTH_CJK: AtomicBool?). Sorry for flipflopping...

@kojiishi

kojiishi commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

That's where a global setting might win. We could check an env var to initialize the default, then users (not callers) can change the settings without modifying their programs.

Okay, I think I now agree that this should be a process-global (maybe a static WIDTH_CJK: AtomicBool?). Sorry for flipflopping...

Thanks for reviewing. I just rebased before I saw your reply. I'll revert to the global version.

@kojiishi

kojiishi commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

Reverted to the global version, and renamed to WIDTH_CJK.

Comment thread src/width.rs Outdated
Comment thread src/width.rs Outdated
Comment thread src/width.rs Outdated
Comment thread src/width.rs Outdated
Comment thread src/style.rs
@kojiishi
kojiishi force-pushed the ea_ambiguous branch 5 times, most recently from 87e0f68 to 35b7e38 Compare July 2, 2026 04:55
kojiishi added 2 commits July 11, 2026 21:15
Consolidate functions to measure widths into a new struct `Width`.

This patch has no behavior changes, except a fix where `WideElement::expand` calls the `unicode-width` version if the feature is on, instead of always calling `measure_width`.
Provides a solution for console-rs#497 by allowing callers to opt-in when they're in ambiguous-wide environment.

It doesn't cover grapheme cluster issues also discussed in console-rs#497.
@djc

djc commented Jul 13, 2026

Copy link
Copy Markdown
Member

You changed a bunch of stuff back from my push (merged the test file, method order/names). Any particular reason?

@kojiishi

kojiishi commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

You changed a bunch of stuff back from my push (merged the test file, method order/names). Any particular reason?

Not intentional, I think I merged your changes to my checkout manually to fix the test failures (not knowing how to download yours from github), but it sounds like I missed some, sorry about that. Please go ahead with your own.

@djc

djc commented Jul 14, 2026

Copy link
Copy Markdown
Member

You changed a bunch of stuff back from my push (merged the test file, method order/names). Any particular reason?

Not intentional, I think I merged your changes to my checkout manually to fix the test failures (not knowing how to download yours from github), but it sounds like I missed some, sorry about that. Please go ahead with your own.

Sorry, can you try that again? You should be able to download my commits from GitHub with git fetch git@github.com:console-rs/indicatif.git ea_ambiguous, then git reset --hard FETCH_HEAD while you have your own branch checked out.

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