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

Allow configuring the width of East Asian ambiguous width characters #47

Merged
merged 2 commits into from
Jan 27, 2024

Conversation

junegunn
Copy link
Contributor

Would you be willing to accept a patch that allows changing the expected width of East Asian ambiguous width characters?

Similarly to RUNEWIDTH_EASTASIAN=1 of go-runewidth or set ambiwidth=double of Vim.

@rivo
Copy link
Owner

rivo commented Jan 21, 2024

Please explain why (and where) this is needed.

@junegunn
Copy link
Contributor Author

I'm replacing mattn/go-runewidth with rivo/uniseg in fzf. One difference I noticed is that it's not possible with uniseg to configure how ambiguous east asian characters should be treated. It always reports their width as 1.

But in some CJK (East Asian) fonts, those ambiguous characters take up 2 columns, so if the library reports the width as 1, users using those fonts will have rendering problems like the one shown here.

So go-runewidth provides RUNEWIDTH_EASTASIAN=1 flag so that the library reports the width as 2. Vim does the same thing via set ambiwidth=double.

I personally don't have this problem, it only seems to be an issue of CJK users on Windows environment.

@rivo
Copy link
Owner

rivo commented Jan 24, 2024

As far as I remember, the reason I set them to a width of 1 was this statement in Unicode Standard Annex #11:

If [ambiguous width characters] are not used in context of the specific legacy encoding they belong to, their width resolves to narrow.

I think for now, the crucial point is this:

I personally don't have this problem, it only seems to be an issue of CJK users on Windows environment.

The way I handle these things is that I wait for someone to actually have this problem with uniseg. But before that happens, I will avoid adding additional parameters and keep the complexity as low as possible.

That's not to say I won't come back to this at some point in the future. But for now, it seems to me that this is a rare edge case which the users of this package haven't encountered yet.

@junegunn
Copy link
Contributor Author

Fair enough. Thank you for your consideration. The patch is simple and I have no problem applying it to my fork, so feel free to close this.

FWIW, go-runewidth automatically detects the "wideness" by looking at the locale setting of the terminal,

but I suspect this has caused more problems than it has solved. Some users had to set RUNEWIDTH_EASTASIAN=0 to reverse it.

Granted, we don't know how many users actually benefit from the detection because they don't come and say they are well and fine.

@rivo
Copy link
Owner

rivo commented Jan 25, 2024

I have no problem applying it to my fork

So you do need this feature? I thought you mentioned that you don't have this problem.

Granted, we don't know how many users actually benefit from the detection because they don't come and say they are well and fine.

That's true. In my experience, however, they will let you know when it doesn't work.

@junegunn
Copy link
Contributor Author

So you do need this feature? I thought you mentioned that you don't have this problem.

Sorry, I wasn't clear enough. For my personal use, no. But as the maintainer of fzf, which is a fairly popular program, I need to provide a way to configure the behavior in case there are users on such terminal environments. But I figured you would be reluctant to merge this, which is completely understandable, so I decided to build the program using the fork for the time being. The patch is simple, so even if you decide not to merge this, I can still maintain my fork. So no pressure.


It looks like the author of go-runewidth needed this behavior himself, so he came up with the logic.

mattn/go-runewidth#14 (comment)

@rivo
Copy link
Owner

rivo commented Jan 26, 2024

So I assume your users have run into this issue before then? Or do you want to preemptively add this feature?

I will leave a comment/request for this PR and then I might actually merge it. It sounds like it is important enough to include it.

width.go Show resolved Hide resolved
@junegunn
Copy link
Contributor Author

junegunn commented Jan 27, 2024

So I assume your users have run into this issue before then? Or do you want to preemptively add this feature?

Replacing go-runewidth with uniseg means that fzf is removing the auto-detection of the setting. So, the users who have benefited from it (don't know how many there are) will notice that the rendering is broken when they upgrade fzf. And I didn't want to tell them that they're out-of-luck, just because the new library fzf uses doesn't support the configuration. Instead, "So, yeah, the auto-detection is gone, but you can use fzf --ambidouble."

Having said that, no one has complained so far after a few days since the release. Maybe there were only a few users who benefited from the auto-detection, so no one ever ran into the problem, or maybe they figured it out themselves from the release note. I don't know.

Added the comment as you requested.

@junegunn
Copy link
Contributor Author

Putting fzf aside, I think it makes sense for this library to provide the option, because we know there are terminals that behave differently.

@rivo rivo merged commit 97691fc into rivo:master Jan 27, 2024
@rivo
Copy link
Owner

rivo commented Jan 27, 2024

Makes sense. I merged it.

@junegunn
Copy link
Contributor Author

Thank you!

junegunn added a commit to junegunn/fzf that referenced this pull request Jan 27, 2024
@junegunn junegunn deleted the eastasian-ambiguous branch January 27, 2024 13:19
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

2 participants