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 wcwidth implementation from fish-shell #1289

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

Conversation

zhaofengli
Copy link

This PR makes mosh use up-to-date wcwidth tables from fish-shell, so certain Unicode characters can be displayed correctly on platforms like macOS.

It revives #1143 with a simple wrapper. We now vendor an unmodified copy of widechar_width.h from https://github.com/ridiculousfish/widecharwidth.

Fixes #234.

cc @faho (#1143 (comment))

Co-authored-by: Stanislaw Pusep <creaktive@gmail.com>
@achernya
Copy link
Collaborator

I don't think we want to take this PR, as we have not thought about the issues around mosh-client and mosh-server having out of sync wcwidth implementations. That's already possible today, but it's only going to get worse if we vendor wcwidth as this PR suggests.

I'm in general super not into vendoring source.

@zhaofengli
Copy link
Author

Understood, thanks for the review. I'll apply the patch locally in the meantime.

[...] issues around mosh-client and mosh-server having out of sync wcwidth implementations

I agree that this is problematic. Currently it seems that the state diff is generated by the server as an terminal sequence (Display::new_frame). The client parses and applies the sequence, applies prediction overlays, then generates a new diff to be streamed to the actual terminal. There are two points where character width issues can occur:

  1. mosh-client applying the over-the-wire diff from mosh-server (uses libc wcwidth)
  2. The actual terminal rendering the diff generated from the client (we can't control what it does)

If my understanding is correct, would it make sense to change the over-the-wire diff representation to be in terms of Row/Cell changes? This way the server controls how characters are laid out in Cells, avoiding (1). Without the escape sequences, the diff can also be more compact.

I'm in general super not into vendoring source.

That's reasonable. We can require users and downstream packaging to provide widecharwidth.

@faho
Copy link

faho commented Aug 18, 2023

That's reasonable. We can require users and downstream packaging to provide widecharwidth.

With my widecharwidth hat on, since I was CC'd:

widecharwidth is meant to be used as a single header that you generate and include in your source. It is not set up to be built as a shared library, so it's not amenable to downstream packaging.

I don't believe we want to support a shared library either, because that means caring about downstream packages.

And the generated header is a few long data tables together with a couple functions to read them. Overall not a lot of code to vendor.

the issues around mosh-client and mosh-server having out of sync wcwidth implementations

I can tell you that we haven't had many reports of problems with being out-of-sync, at least not once you get over the big 8->9 hump. Tho that is shell (remote or local) and terminal being out of sync, y'all would have an additional step.


Anyway, it's of course your decision. I'll unsubscribe from this issue now so if anyone wants to summon me feel free to @ me.

jdrouhard added a commit to jdrouhard/mosh that referenced this pull request Aug 20, 2023
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.

Display errors with certain characters
3 participants