Bugfix some kinds of emojis with grapheme boundry support - #1100
Conversation
This requires integration with wcwidth>=0.3 with new iter_graphemes() function. It is true, though, that sometimes measuring things individually by a character can accidentally be correct, so some tests do succeed,
Uses ``wcwidth.iter_graphemes()`` and a dumb hack to go previous grapheme for now, I am working on a new wcwidth.grapheme_boundary_before() function, maybe in wcwidth>=0.4. WIP, still in testing
| def __del__(self) -> None: | ||
| self.file.close() | ||
| if self.file is not None: | ||
| self.file.close() |
There was a problem hiding this comment.
Whenever I left the editor with F8 key I had a Nonetype exception here, I'm sure it has to do with race condition of __del__ at any unexpected time depending on python version and platform, anyway I just added this to remove the exception
Pull Request Test Coverage Report for Build 21433031372Details
💛 - Coveralls |
Added some raw control characters to docs/examples/edit_text.txt, there is a strange effect of the cursor, it will not "move" into them, as each cell is measured as 0 -- but at least it doesn't crash! (For an editor to handle control characters, it needs to change the displayed presentation, eg \x00 -> "^A" and usually bolded by tradition, it also needs to allow input, eg "^V^A")
|
I changed the PR a bit to ignore control codes 30b4da6 and added a line of control codes to the sample txt file, per @penguinolog's suggestion and benchmarked per @ulidtko's request using pytest-benchmark, source and image attached. Up to 5-20% slower, probably depends on complexity of data. Test data is udhr_combined.txt Source of benchmark_text_measurement.py#!/usr/bin/env python
import os
from urwid.str_util import calc_width, calc_text_pos, move_prev_char, move_next_char
# using UDHR test file from 'wcwidth' project (mix of world languages)
UDHR_FILE = os.path.join(os.path.dirname(__file__), '..', 'wcwidth', 'tests', 'udhr_combined.txt')
with open(UDHR_FILE, encoding='utf-8') as f:
UDHR_TEXT = f.read()
UDHR_LINES = [line for line in UDHR_TEXT.splitlines() if line.strip()]
def test_calc_width(benchmark):
text = UDHR_TEXT
length = len(text)
def work():
for _ in range(8):
calc_width(text, 0, length)
benchmark(work)
def test_calc_text_pos(benchmark):
lines = UDHR_LINES[:200]
def work():
for _ in range(200):
for line in lines:
calc_text_pos(line, 0, len(line), 40)
benchmark(work)
def test_move_next_char(benchmark):
text = UDHR_TEXT[:20000]
end = len(text)
def work():
for _ in range(100):
pos = 0
while pos < end:
pos = move_next_char(text, pos, end)
benchmark(work)
def test_move_prev_char(benchmark):
text = UDHR_TEXT[:20000]
start = 0
def work():
for _ in range(100):
pos = len(text)
while pos > start:
pos = move_prev_char(text, start, pos)
benchmark(work)Note str_utils.py from master branch on left, this PR is on right. calc_width performancecalc_text_pos performancemove_next_char performancemove_prev_char performanceThis one does not make sense, I might have made an error or some local effect of my computer? |
|
|
||
| cols = 0 | ||
| for idx in range(start_offs, end_offs): | ||
| width = get_char_width(text[idx]) |
There was a problem hiding this comment.
should we remove get_char_width completely so that we're not calculating widths different ways in different parts of the code?
There was a problem hiding this comment.
I'd like to do it, but it may be used by dependent packages
There was a problem hiding this comment.
I'd like to do it, but it may be used by dependent packages
There was a problem hiding this comment.
In this PR get_char_width is still being used in text_layout.py and font.py, we could at least remove those and mark the function as deprecated
There was a problem hiding this comment.
All internal uses now replaced wcwidth.width(control_codes='ignore') and has .. deprecated:: 0.3.4
|
Thanks so much for the review, I didn't understand how the if-utf8-bytes branches are used in a practical way so I did not give it attention, I will study and address all feedback! |
c7a75b8 to
820efff
Compare
Checklist
masterbranchtoxsuccessfully in local environmentProblem: urwid processes each unicode point as wide or zero, but is missing support for grapheme clustering, several kinds of common unicode strings "in sequence", like hearts, flags, etc:
Reproduce: Easy to experience but hard to capture by video/screenshot. Copy docs/examples/edit_text.txt and edit using urwid:
The cursor moves into invisible areas between the emojis and makes them difficult to navigate and edit, if you know that they contain hidden sequences, you can edit around them with successful care, though!
Solution: Integrate with wcwidth>=0.4 which adds grapheme cluster support with
wcwidth.iter_graphemes()(in 0.3.0) and functionwcwidth.grapheme_boundary_before()(0.4.0, added for this PR).By careful integration with grapheme clustering, I am able to "navigate" the cursor directly from one emoji to the next, column by column, without any strange invisible/non-movement behaviors.