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

Remove option to use system wcwidth #5777

Merged
merged 1 commit into from
Apr 1, 2019

Conversation

faho
Copy link
Member

@faho faho commented Mar 30, 2019

As it turns out it didn't work much better, and it fell behind in
support when it comes to things that wcwidth traditionally can't
express like variation selectors and hangul combining characters, but
also simply $fish_*_width.

I've had to tell a few people now to rebuild with widecharwidth after
sending them on a fool's errand to set X variable.

So keeping this option is doing our users a disservice.

TODOs:

  • [N/A] Changes to fish usage are reflected in user documentation/manpages.
  • [N/A] Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.md

As it turns out it didn't work much better, and it fell behind in
support when it comes to things that wcwidth traditionally can't
express like variation selectors and hangul combining characters, but
also simply $fish_*_width.

I've had to tell a few people now to rebuild with widecharwidth after
sending them on a fool's errand to set X variable.

So keeping this option is doing our users a disservice.
@faho faho added this to the fish 3.1.0 milestone Mar 30, 2019
@floam
Copy link
Member

floam commented Mar 31, 2019

Do we know of any platforms where packages are distributed which use the system wcwidth? Are there any systems which are doing width calculations very well?

@faho
Copy link
Member Author

faho commented Mar 31, 2019

Do we know of any platforms where packages are distributed which use the system wcwidth?

Nix is the only one I know of.

Are there any systems which are doing width calculations very well?

The problem is even if they are doing it mostly, the system wcwidth path doesn't have any of the quirks - not the variation selectors, not the hangul stuff, not $fish_ambiguous_width.

My initial assumption was that there would be a happy path where the terminal just displays everything according to wcwidth, and that system wcwidth is reasonably correct or that both would be wrong in the same way so being wrong with them was preferable. That's basically not the case.


Let me put it this way: I have had to tell multiple people to switch away from system wcwidth, I haven't had to tell anyone to go the other way.

That means this is just objectively worse.

@zanchey
Copy link
Member

zanchey commented Mar 31, 2019

Let me put it this way: I have had to tell multiple people to switch away from system wcwidth, I haven't had to tell anyone to go the other way.

That's a good argument to me.

@zanchey
Copy link
Member

zanchey commented Mar 31, 2019

This will definitely need a CHANGELOG entry, as #4816 got one.

@mqudsi
Copy link
Contributor

mqudsi commented Mar 31, 2019

Note that the recent improvements to console mode support feature explicit use of the system wcwidth() (and are not affected/reverted by this change), which is correct and proper.

IMHO, speaking from the other (dark!) side of things, in my endeavor to get fish to display properly and handle encodings correctly at login (non-X11, non-SSH, text-only) console sessions, it's basically the only case where the system wcwidth() comes into play for TUIs and is otherwise only going to improve things by pure chance/at random while breaking things in other cases. So a big +1 from me.

@faho faho merged commit da1b32f into fish-shell:master Apr 1, 2019
@faho
Copy link
Member Author

faho commented Apr 1, 2019

Merged and changelogged.

@faho faho deleted the remove-system-wcwidth branch April 1, 2019 14:06
nyarly added a commit to nyarly/nixpkgs that referenced this pull request Jul 18, 2019
Fish 3.0 has an updated an more robust handling of unicode glyphs. Per
the original author of the INTERNAL_WCWIDTH flag, it was something of
misfeature, and they regret that NixOS came to rely on it.

Removes the flag from the Nix expression.

Flag was added originally to Nixpkgs in 68076b7

It is being removed entirely from upstream fish:
fish-shell/fish-shell#5777.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants