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

Make wcwidth configurable #4816

Merged
merged 3 commits into from Mar 25, 2018

Conversation

Projects
None yet
5 participants
@faho
Copy link
Member

faho commented Mar 13, 2018

This adds the cmake option "BROKEN_WCWIDTH" "ENABLEINTERNAL_WCWIDTH" (to be set to "ON" or
"OFF") and the configure option --[en,dis]able-internal-wcwidth.

Both default to enabling our fallback, but can be set to use the system's wcwidth again.

Sequel to #4554.
See #4571, #4539, #4609.

On my system, this would fix #4306.

Ideally, this would default to using the system wcwidth instead - as noted in #4571, it doesn't really matter whether we get the width "right". What matters is that we get the same width that the terminal gets, and that's much more likely with a shared implementation. I have gotten vte, urxvt, st and xterm to disagree with my right-prompt, which causes an interesting stair-case effect that makes fish nigh-unusable.

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 previously noted, I suck at build systems, so someone checking this would be much appreciated.

Make wcwidth configurable
This adds the cmake option "BROKEN_WCWIDTH" (to be set to "ON" or
"OFF") and the configure option --[en,dis]able-wcwidth.

Both default to enabling our fallback, but can be set to use the system's wcwidth again.

Sequel to #4554.
See #4571, #4539, #4609.

On my system, this would fix #4306.

@faho faho added the enhancement label Mar 13, 2018

@faho faho added this to the fish-3.0 milestone Mar 13, 2018

@faho faho referenced this pull request Mar 13, 2018

Closed

Disable HAVE_BROKEN_WCWIDTH #4554

0 of 1 task complete
@ariasuni

This comment has been minimized.

Copy link

ariasuni commented Mar 13, 2018

The cmake and configure options name are unclear; I used the wrong one at first. I suggest using: -DENABLE_INTERNAL_WCWIDTH and --{en,dis}able-internal-wcwidth (or something like that).

It works as expected when I use ./configure --disable-wcwidth, but I didn’t manage to build this branch with cmake:

[  2%] Built target tinyexpr
[  2%] Built target pofiles_2
[  2%] Built target CHECK-FISH-BUILD-VERSION-FILE
[  3%] Generating lexicon_filter
/home/ariasuni/projets/perso/fish-shell/build_tools/build_lexicon_filter.sh: line 25: command_list_toc.txt: No such file or directory
make[2]: *** [CMakeFiles/BUILD_MANUALS.dir/build.make:914: lexicon_filter] Error 1
make[2]: *** Deleting file 'lexicon_filter'
make[1]: *** [CMakeFiles/Makefile2:207: CMakeFiles/BUILD_MANUALS.dir/all] Error 2
make: *** [Makefile:130: all] Error 2

Note that I have no problem building master fish with cmake.

Rename options
These are now "--{en,dis}able-internal-wcwidth" and "ENABLE_INTERNAL_WCWIDTH".
@faho

This comment has been minimized.

Copy link
Member

faho commented Mar 13, 2018

The cmake and configure options name are unclear; I used the wrong one at first. I suggest using: -DENABLE_INTERNAL_WCWIDTH and --{en,dis}able-internal-wcwidth (or something like that).

Makes sense, yes.

/home/ariasuni/projets/perso/fish-shell/build_tools/build_lexicon_filter.sh: line 25: command_list_toc.txt: No such file or directory

I don't think this branch should change anything about that. Did you build with cmake previously?

@ariasuni

This comment has been minimized.

Copy link

ariasuni commented Mar 13, 2018

Well, I guess compiling with autotools then with cmake doesn’t work very well, because it works with a clean clone. Well, I guess because when I build fish with cmake, master or your branch, it’s so broken I can’t test emojis: when I launch ./fish, I get:

~/.config/fish/user.fish (line 5): 
alias src="source ~/.config/fish/config.fish"
^
from sourcing file ~/.config/fish/user.fish
        called on line 36 of file ~/.config/fish/config.fish

from sourcing file ~/.config/fish/config.fish
        called during startup

~/.config/fish/user.fish (line 6): 
alias c="cd"
^
from sourcing file ~/.config/fish/user.fish
        called on line 36 of file ~/.config/fish/config.fish

from sourcing file ~/.config/fish/config.fish
        called during startup
[…]

Maybe worth another issue though.

@faho

This comment has been minimized.

Copy link
Member

faho commented Mar 13, 2018

Yeah, the cmake build doesn't set up fish_function_path so it can be used without installing.

It's a different issue, yes.

@ariasuni

This comment has been minimized.

Copy link

ariasuni commented Mar 13, 2018

OK, I can confirm that’s working just fine when building with cmake then installing.

@faho

This comment has been minimized.

Copy link
Member

faho commented Mar 20, 2018

I'll merge this on saturday if there are no objections.

@amosbird

This comment has been minimized.

Copy link
Contributor

amosbird commented Mar 20, 2018

thanks!

@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Mar 22, 2018

What's a system where this helps? I don't object to merging but I want to understand where our Unicode support is lacking.

@@ -90,6 +90,13 @@ SET_SOURCE_FILES_PROPERTIES(src/fish_version.cpp
PROPERTIES OBJECT_DEPENDS
${CMAKE_CURRENT_BINARY_DIR}/${FBVF})

OPTION(ENABLE_INTERNAL_WCWIDTH "use fallback wcwidth" ON)

This comment has been minimized.

@zanchey

zanchey Mar 22, 2018

Member

Can I suggest this is just "INTERNAL_WCWIDTH" in line with other options?

This comment has been minimized.

@faho

faho Mar 23, 2018

Member

Done.

@ariasuni

This comment has been minimized.

Copy link

ariasuni commented Mar 22, 2018

I was wondering if we shouldn’t make the workaround opt-in: I think it makes more sense that the default is using the system wcwidth, and that the few (I guess?) broken systems/configs enable the flag. But I wasn’t here when the workaround was written so maybe I’m missing something.

@faho

This comment has been minimized.

Copy link
Member

faho commented Mar 23, 2018

What's a system where this helps?

Mine, for instance. Archlinux with glibc 2.26. If I use either

  • anything based on vte (like gnome-terminal)

  • xterm

  • st

  • rxvt

with my right-prompt, I'll get a nice little staircase-effect like this:

screenshot_20180323_195901

(That's after I've typed one "e" in each of the terminals - though note that in this case the suggestion is also triggering it)

One of the offenders is \U26A1 ("HIGH VOLTAGE SIGN").

But the point is, and I've brought this up before, that even if we squash that one, others may pop up.

And, as I've said in #4571 (where the terminal rendered a zero-width-space with a width that was not zero), sometimes it's better for us to be wrong in the same way as the terminal.

And while it's certainly not perfect to rely on the system wcwidth (they frequently have bugs), it's the best chance we have to get the same value as the terminal. And if the terminal uses its own implementation, there's nothing we can do that won't break fish for all other terminals.

I was wondering if we shouldn’t make the workaround opt-in

So I've come to believe that we really should use the system wcwidth. This PR is a compromise so that people on old systems or weird terminals that we can't test before the next release can still use fish.

Long term, I'd love it if we switched the default to system wcwidth and let people on RHEL 6 and the like build it with the internal one (because of stuff like ssh - where you have a terminal on a new system, with a new wcwidth, but a shell on an old one). But there's no real harm in being a little conservative.

@faho faho merged commit ad5bbeb into fish-shell:master Mar 25, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

amosbird added a commit to amosbird/fish-shell that referenced this pull request Mar 26, 2018

Make wcwidth configurable (fish-shell#4816)
* Make wcwidth configurable

This adds the cmake option "INTERNAL_WCWIDTH" (to be set to "ON" or
"OFF") and the configure option --[en,dis]able-internal-wcwidth.

Both default to enabling our fallback, but can be set to use the system's wcwidth again.

Sequel to fish-shell#4554.
See fish-shell#4571, fish-shell#4539, fish-shell#4609.

On my system, this would fix fish-shell#4306.

@nitely nitely referenced this pull request May 8, 2018

Closed

east_asian_width ? #3

gebner added a commit to gebner/nixpkgs that referenced this pull request Dec 30, 2018

fish: do not use internal wcwidth
This is important when typing characters such as ⚡(U+26A1 HIGH VOLTAGE
SIGN), otherwise fish computes a different character width than the
terminal.  See fish-shell/fish-shell#4816

@gebner gebner referenced this pull request Dec 30, 2018

Merged

fish: do not use internal wcwidth #53069

4 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment