Update unicode table to the version 15.1.0 by penguinolog · Pull Request #744 · urwid/urwid

penguinolog · 2024-01-12T21:44:51Z

Partially fix render of emoji

Checklist

I've ensured that similar functionality has not already been implemented
I've ensured that similar functionality has not earlier been proposed and declined
I've branched off the master or python-dual-support branch
I've merged fresh upstream into my branch recently
I've ran tox successfully in local environment
I've included docstrings and/or documentation and/or examples for my code (if this is a new feature)

Partial #225

coveralls · 2024-01-12T21:58:37Z

Pull Request Test Coverage Report for Build 7796909910

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 70.938%

Totals
Change from base Build 7796558930:	0.0%
Covered Lines:	8769
Relevant Lines:	12451

💛 - Coveralls

wardi · 2024-01-12T22:03:00Z

 #define ENC_NARROW 3

-static int widths_len = 2*38;
+static int widths_len = 2*820;


Doing a linear search over 820 items might slow. Should we think of using a more efficient data structure or am I overthinking this?

I'm even thinking about depreciation of C code and migration on `wcwidth.
It became global standard, handle multiple unicode versions and since author allow to copy-paste unicode version detect logic - we can better handle different terminals.
Re-implementation of unicode tables in C or migration on different language (like rust) at this moment is too painful

detecting things about the terminal is tricky because the text calculations are separated from the screen instance. If someone is connecting to an urwid app over ssh like the twisted ssh example how do we know which unicode version detection to use? (there can be many users attached each with their own screen)

but, using an existing library for the width calculation sounds like a great idea (assuming it doesn't make calculations significantly slower)

And simple copy-paste of logic and content (even if separate generated) looks strange.
We can use binary search, it is faster.

I checked: for current release we can use binary search and later we can migrate C part to rust and use upstream libraries for char width both in python and binary.

Re: use of wcwidth - good idea to do so, but I suggest you have a look at a recent issue posted by the author on his github, where he discusses the limits of UNICODE_VERSION jquast/wcwidth#104

the tl dr is that it's challenging to fix unicode version compatibility issues at the app layer, and the way forward is to get terminal emulator authors to stay up to date with the latest unicode version.

Re: use of wcwidth - good idea to do so, but I suggest you have a look at a recent issue posted by the author on his github, where he discusses the limits of UNICODE_VERSION jquast/wcwidth#104

I fully understand that issue, but it also has limitations: in case of widget decorations, incorrect symbols length of decorated widget will produce incorrect decorations length.
Technically we can play "stupid" and go forward with the last unicode version tables, since only few terminals at this moment fully stuck at unicode 12.

the tl dr is that it's challenging to fix unicode version compatibility issues at the app layer, and the way forward is to get terminal emulator authors to stay up to date with the latest unicode version.

Understood. Makes sense to update the tables. I am curious, why update to v.12 and not the latest v.15.1?

Originally went to the most supported.
After rethinking on weekends went to the 15.1
Technically we have only 1 fully working solution: ask terminal for cursor position after each double width character and add space if it rendered single width, but this solution is extremely expensive and works only with RawDisplay.

danschwarz

LGTM

ulidtko

A few questions.

Why are we duplicating the logic of Python's built-in stdlib? https://docs.python.org/3/library/unicodedata.html#unicodedata.east_asian_width
Can we get rid of the C impl?.. It has caused build issues which'd not exist in a pure-python package.
Can we compress the table by assuming width 1 for absent entries?

penguinolog · 2024-01-15T19:45:55Z

A few questions.

Why are we duplicating the logic of Python's built-in stdlib? https://docs.python.org/3/library/unicodedata.html#unicodedata.east_asian_width

Can we get rid of the C impl?.. It has caused build issues which'd not exist in a pure-python package.

Can we compress the table by assuming width 1 for absent entries?

Stdlib unicodedata maybe (in most cases) outdated on the end user machine:

>>> import sys; print('Python %s on %s' % (sys.version, sys.platform))
Python 3.11.4 (main, Dec  7 2023, 15:43:41) [GCC 12.3.0]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.15.0 -- An enhanced Interactive Python. Type '?' for help.
PyDev console: using IPython 8.15.0
Python 3.11.4 (main, Dec  7 2023, 15:43:41) [GCC 12.3.0] on linux
>>> import unicodedata
>>> unicodedata.unidata_version
'14.0.0'

Yes, we can. But it will not fix the main issue (unicode data in python, urwid and terminal are different). And now we do not have build issues as using standard toolchain for release.
As discussed, we'll migrate to wcwidth, which became standard de-facto. It's already using 2 tables instead of 1. It requires also to adopt to "-1" width (urwid do not expect such length).

penguinolog · 2024-01-16T15:41:11Z

Small extra details about unicodedata in python core:

Parsing of it again is expensive
In python 3.12.0 official docker image it's 15.0 - outdated already.
In python 3.7 it's already 11.0 - seriously outdated

jquast · 2024-01-18T15:53:43Z

https://docs.python.org/3/library/unicodedata.html#unicodedata.east_asian_width and other functions in unicodedata library are not sufficient for measuring zero-width, combining, VS-16 emoji, or ZWJ emoji sequences. There is a lot of work done in wcwidth library (update-tables.py) to transfer unicode.org data files into efficient lookup tables and make corrections in their categorization that the unicodedata library does not perform.

(I too would like to use the standard python library to measure characters for a terminal, I do hope to work on that this year jquast/wcwidth#94)

A basic example,

>>> l='💪🏿'
>>> [unicodedata.name(z) for z in l]
['FLEXED BICEPS', 'EMOJI MODIFIER FITZPATRICK TYPE-6']
>>> [unicodedata.combining(z) for z in l]
[0, 0]
>>> [unicodedata.east_asian_width(z) for z in l]
['W', 'W']

In this case, there is insufficient data in the std unicodedata library measure the width of this emoji,

while,

>>> import wcwidth
>>> wcwidth.wcswidth(l)
2

penguinolog · 2024-01-19T10:47:55Z

Switched python implementation to the wcwidth.
C implementation is untouched.

penguinolog · 2024-01-19T10:48:37Z

-    if o in {0xE, 0xF}:
+    width = wcwidth.wcwidth(chr(o), UNICODE_VERSION)
+    if width < 0:
        return 0


urwid using only positive width

Partially fix render of emoji * Use 2-dimensional array in C code and get rid of len constant * Adjust C implementation: all codes < 32 are not printable: It's not ASCII chars * Partially apply clang-format to the helper source * Use `wcwidth` for python character width calculation

penguinolog · 2024-02-12T14:58:00Z

Looks like Unicode problem is raising. I have to merge this PR and later work on proper Unicode handling in the internals.

penguinolog added the Unicode Issues related to Unicode <-> bytes conversion label Jan 12, 2024

penguinolog requested review from tonycpsu, ulidtko and wardi January 12, 2024 21:44

wardi reviewed Jan 12, 2024

View reviewed changes

penguinolog requested review from danschwarz and wardi January 15, 2024 08:28

penguinolog changed the title ~~Update unicode table to the version 12.0.0~~ Update unicode table to the version 151.0 Jan 15, 2024

penguinolog changed the title ~~Update unicode table to the version 151.0~~ Update unicode table to the version 15.1.0 Jan 15, 2024

danschwarz approved these changes Jan 15, 2024

View reviewed changes

ulidtko requested changes Jan 15, 2024

View reviewed changes

penguinolog requested a review from ulidtko January 15, 2024 20:48

penguinolog force-pushed the unicode_table branch from c40fc53 to 0750bee Compare January 19, 2024 10:46

penguinolog commented Jan 19, 2024

View reviewed changes

penguinolog force-pushed the unicode_table branch from 0f585c1 to 5424506 Compare February 6, 2024 08:42

penguinolog merged commit 6795bb8 into urwid:master Feb 12, 2024

penguinolog deleted the unicode_table branch February 12, 2024 14:58

AnonymouX47 mentioned this pull request Feb 15, 2024

[BUG] Urwid 2.5.3 causes the Urwidgets TextEmbed widget to crash #802

Closed

2 of 3 tasks

Conversation

penguinolog commented Jan 12, 2024

Checklist

Uh oh!

coveralls commented Jan 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 7796909910

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wardi Jan 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danschwarz left a comment

Choose a reason for hiding this comment

Uh oh!

ulidtko left a comment

Choose a reason for hiding this comment

Uh oh!

penguinolog commented Jan 15, 2024

Uh oh!

penguinolog commented Jan 16, 2024

Uh oh!

jquast commented Jan 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

penguinolog commented Jan 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

penguinolog commented Feb 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

coveralls commented Jan 12, 2024 •

edited

Loading

wardi Jan 12, 2024 •

edited

Loading

jquast commented Jan 18, 2024 •

edited

Loading