X Tutup
The Wayback Machine - https://web.archive.org/web/20200905014531/https://github.com/bpython/bpython/pull/839
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

Add width awareness to fix wrapping of strings with full width chars #839

Open
wants to merge 1 commit into
base: master
from

Conversation

@rybarczykj
Copy link
Contributor

rybarczykj commented Aug 5, 2020

This is a redux of #731 which was put on hold until a need for width awareness was found. I'd say this is a decent justification for bringing it back

Wrapping strings with fullwidth chars
Here's the same string being typed before and after width awareness:
image

image

New method to fix cursor position:
When we put a 2-column character at the end of the line where there's only 1 column of room, curtsies adds a padding character (space) so we can kick it down to the line below. Therefore, in order to get the right cursor position, I had to add a method that determines how many padding chars there are (number_of_padding_chars_on_current_cursor_line).

Note on this method: Right now, to determine the amount of padding, it calls display_linize, doubling the amount of times we call that function. It's definitely possible to keep track of this as a variable in the repl class instead, or to do some changes other to ensure less redundant work is done. Feedback appreciated.

Runtime
One of the barriers stopping us from implementing this before was runtime . I did not find any noticeable difference when pasting, printing, or moving my cursor around large strings.

@rybarczykj rybarczykj marked this pull request as ready for review Aug 11, 2020
@sebastinas
Copy link
Contributor

sebastinas commented Aug 14, 2020

Could you please rebase this PR?

@rybarczykj rybarczykj force-pushed the rybarczykj:wrapping branch from 83d87fd to a7bb458 Aug 14, 2020
@rybarczykj
Copy link
Contributor Author

rybarczykj commented Aug 17, 2020

Just noticed a bug with this; maybe don't accept yet.

width_aware_splitlines throws an error and bpython crashes when the user printes format strings by importing curtsies. This is because wcswidth can't parse strings that look like this '\x1b[36m\x1b[42ma\x1b[49m\x1b[39m'. Maybe we should just try to use the old method if an error like this is thrown.

@rybarczykj rybarczykj force-pushed the rybarczykj:wrapping branch from a7bb458 to 90cdd1d Aug 18, 2020
@rybarczykj
Copy link
Contributor Author

rybarczykj commented Aug 18, 2020

That bug I mentioned ^^ is fixed now, as of that last push. Now, if wcswidth throws an error, we catch it and use the old display_linize.

I also added some relevant tests.

- update display_linize to use curtsies 3.0 width functionality
- correct cursor positioning on wrapped fullwidth chars by adding new method
- add test coverage
@rybarczykj rybarczykj force-pushed the rybarczykj:wrapping branch from 90cdd1d to 9da6783 Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.
X Tutup