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

Use the wcwidth() implementation from the Fish shell instead of the one from libc #1143

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

creaktive
Copy link

https://github.com/ridiculousfish/widecharwidth tables are generated directly from the Unicode (and emoji) specifications, so it is guaranteed to be up-to-date. Moreover, this guarantees the parity between the client & the server.

@faho
Copy link

faho commented Mar 22, 2022

Speaking as a widecharwidth contributor: This PR doesn't currently work.

The reason is that widechar_width() doesn't simply return the width, it returns the width or one of a few special negative values that need handling in the application.

These include e.g. "widechar_widened_in_9", which explains that the codepoint was narrow according to Unicode 8, and is wide after (they made it so Emoji_Presentation makes a codepoint wide), or "widechar_ambiguous", which marks a codepoint as having ambiguous width - what the effective width is can usually be configured in the terminal (the default is usually 1).

So this needs to have a cover function that handles those according to mosh's needs. As it stands, it would misinterpret a lot of codepoints (most emoji).

(if nobody is is interested in this anymore this is of course moot)

@creaktive
Copy link
Author

@faho thanks for your review! Actually, it does work, because I edited the original widecharwidth.h file so that it returns the character width or -1. If there is enough interest in this PR, I can do it right and implement a wrapper that does the translation between the return values and the actual character width. Otherwise, it works well for me (been using mosh with this patch for almost a year and the sole purpose of this exercise is to correctly display emoji).

@faho
Copy link

faho commented Mar 22, 2022

I edited the original widecharwidth.h file so that it returns the character width or -1

Of course that would cause issues on upgrading, and you absolutely want to upgrade this regularly, to support newer Unicode versions. Case in point I'm pretty sure this does Unicode 12, current widecharwidth does 14.

There's a reason widecharwidth.h is generated and not hand-written.


I would be okay with accepting a patch to widecharwidth that implemented a "simple" mode with the recommended values - or maybe the recommended values or "ambiguous", since ambiguous codepoints really should be configurable (because they are configurable in terminals). Either with an optional "simple" argument (that defaults to false) or a separate "widecharwidth_simple" method.

jdrouhard added a commit to jdrouhard/mosh that referenced this pull request May 20, 2023
Use the wcwidth() implementation from the Fish shell instead of the one from libc
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