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

Visually wide character support #74

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

Conversation

frizinak
Copy link

@frizinak frizinak commented Nov 5, 2016

It is currently assumed a rune is exactly 1 column wide. Drawing japanese hiragana for example results in only half of the runes in View.lines to be copied to View.viewLines (visually) because the x offset of each character, as passed to v.setRune, is incremented by 1 instead of 2.
Same goes for wrapping and most methods in editor.go.

What does the nul byte in editor.go's writeRune do? It breaks unicode code points that contain that byte, which results in every hiragana character to be (visually) separated with a space after a call to View.Buffer. This should be fixed in View.Buffer and View.ViewBuffer but since I'm not sure of its use, you should double check my 'fix'.

I think most changes should not break existing code that uses gocui except for MoveCursor which now moves entire runes instead of columns. It would be trivial to rename this though and expose the original moveCursor again.

I have only done some really basic testing with demo.go and my own cli-app that uses gocui.

ps: I'm no unicode nor mattn/go-runewidth expert by any means so forgive me if I did something stupid ;)

And thank for your great work on gocui btw.

@frizinak
Copy link
Author

frizinak commented Nov 5, 2016

forgot to mention moveCursor still needs work wherever v.ox is referenced, if line wrapping is disabled and the origin is shifted, it does not take wide characters into account and overflows by runeWidth - 1

@jroimartin
Copy link
Owner

First of all, thanks for your contribution! :)

I'm in the process of rewriting the edition mode (issue #60) and I'd prefer to finish it before considering to merge PRs that can be incompatible with this refactoring. Also, this rewrite should fix several issues like the null byte one that you mentioned.

Of course, I'll take into account your PR because I'd definitely like to support variable width runes in the new version.

@jroimartin jroimartin self-assigned this Nov 5, 2016
y-yagi added a commit to y-yagi/egose that referenced this pull request Feb 8, 2017
But need to wait to merge jroimartin/gocui#74
@xh-78
Copy link

xh-78 commented Jan 19, 2018

I encountered the same problem, I'm using mattn/go-runewidth as a solution for now. hope this problem can get properly solved.

@y-yagi
Copy link

y-yagi commented Jun 15, 2018

@jroimartin
It seems #60 was already closed. How do you feel about this PR?

@itchyny
Copy link

itchyny commented Aug 8, 2018

@jroimartin Will reconsider including this please? Thanks for your library and it works almost well but missing wide characters support is very critical issue. For example, jesseduffield/lazygit depends on this library and commit messages including wide characters mess up. Thanks.

@jesseduffield
Copy link

@itchyny I may as well say this here :)

I'm going to merge this PR into my gocui fork because it seems to do the trick. If it ends up being merged into the main gocui repo down the line I'll revisit this and try to stay at parity with the main repo.

@jroimartin Thankyou for all your effort so far with gocui it's been a massive help to me :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants