search

mobile-shell / mosh

Mobile Shell
https://mosh.org
GNU General Public License v3.0
12.69k stars 741 forks source link

Use wcwidth implementation from fish-shell #1289

Open zhaofengli opened 1 year ago

zhaofengli commented 1 year ago

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 (https://github.com/mobile-shell/mosh/pull/1143#issuecomment-1075022777)

achernya commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.