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

Fix how wide runes are displayed #109

Merged
merged 3 commits into from
Nov 29, 2021

Conversation

dankox
Copy link

@dankox dankox commented Nov 27, 2021

This PR should fix problems with wide characters displaying and editing and also SetWritePos function not working correctly.

Internal cell buffer represent each rune which should be displayed as a single cell.
This cell buffer is translated into tcell representation in View.draw(). For wide runes, it will be set as one cell with the rune and following cell would be empty (until the width of the rune).

This was used before, but internal representation was using rune width in some places making it mixed and therefore creating some problems with the wide runes.

@aynakeya can you please check and test this PR for your problem? I think it should work with the wide characters now (as far as I tested). But I'm not sure what other problems you might have, so it would be helpful if you can try.

@coloursofnoise can you check if this one helps with the SetWritePos if you used it elsewhere? In tables.go it should work fine.

Internal cell buffer represent each rune which should be displayed.
This cell buffer is translated into tcell representation in `draw`.
If wide rune is presented, it will be represented in `tcell` by one cell
with the rune and following with "empty" cells.

Internal representation is fixed in multiple places so it's not mixed
with interpreting rune width.

This fixes also problem with `SetWritePos` function.
@dankox dankox requested a review from mjarkk November 27, 2021 15:50
@dankox dankox mentioned this pull request Nov 27, 2021
@dankox
Copy link
Author

dankox commented Nov 27, 2021

@mjarkk There is one more fix in this, where ViewBuffer and ViewBufferLines is updated to use viewLines cells instead of lines cells.
I find out this, that it was using lines which is the same as Buffer and BufferLines. So I've changed it to look more like the original implementation. However, I'm not sure if it's ok.
If not, I can revert that change

@mjarkk
Copy link
Member

mjarkk commented Nov 28, 2021

Awesome work, compared to master i can now properly edit lines with multi with terminal characters.

compared to master this now seems to be fixed: pasting in the demo.go example and then removing them would not correctly remove the characters

I still see 1 issue that's also in master, when i paste multiple in the demo.go while with screen with is not even and a line gets wrapped the visual cursor on screen gets moved one to the right while when edits still happens on the correct place.

@coloursofnoise
Copy link

@dankox this seems to fix the issue, I'll rebase my tables.go tweak onto it.

@dankox
Copy link
Author

dankox commented Nov 29, 2021

@mjarkk thanks... actually I was testing this scenario and saw it fixed. And now as I looked at it, it's not 🤦‍♀️
So I guess I had there something more which I accidentaly removed. Thanks for looking into it. Will fix that too.

Wide character will not be visible if the line is wrapped on it and
"half" of it is on the first line and second "half" on the second line.
@dankox
Copy link
Author

dankox commented Nov 29, 2021

@mjarkk it should be fixed. Hopefully I cover your case as I'm not sure I understand it completly (maybe I did:).
What the last commit fixes is that when you have a line which wraps and there is wide rune at the "wrapping point" it would disappear from the screen together with cursor (or maybe cursor not).

Now it should move correctly and display it correctly too.

Copy link
Member

@mjarkk mjarkk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks that fixes the problem

@mjarkk mjarkk merged commit f8e8c10 into awesome-gocui:master Nov 29, 2021
@dankox dankox deleted the fix/wide-rune-handling branch November 29, 2021 20:53
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.

3 participants