Skip to content

Add two fonts based on Unicode 13 2x3 TRS-80/Teletext mosaic characters #434

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

Merged
merged 6 commits into from
Apr 18, 2023

Conversation

rbanffy
Copy link
Contributor

@rbanffy rbanffy commented Sep 17, 2020

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)
Description:

Add two fonts, one with small digits with a 3x5 on 4x6 matrix, and another with a 5x7 on 6x9 matrix characters to the font.py file. These characters use recently added Unicode 13 symbols.

The PR has two small interim fixes for character width calculation that allow it to run, but that should be removed before merging (it can't be merged before #225 is fixed).

@@ -77,6 +77,7 @@ static const int widths[] = {
65500, 1,
65510, 2,
120831, 1,
130047, 1,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interim fix

@@ -72,6 +72,7 @@
(65500, 1),
(65510, 2),
(120831, 1),
(130047, 1),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interim fix

@coveralls
Copy link

coveralls commented Sep 17, 2020

Coverage Status

Coverage increased (+0.02%) to 77.884% when pulling c5b3a7a on rbanffy:master into a5f27ba on urwid:master.

@rbanffy
Copy link
Contributor Author

rbanffy commented Aug 9, 2021

If a proper fix to #225 takes too long, can we merge this as is?

@rbanffy
Copy link
Contributor Author

rbanffy commented Jul 2, 2022

Hi @urwid folks. This has been sitting here for quite some time. The characters used are supported directly by terminals based on VTE (Gnome Terminal and others), QtTermWidget (Konsole, etc). Can we merge this?

@rbanffy
Copy link
Contributor Author

rbanffy commented Aug 29, 2022

Hi, @ulidtko, do you think this is mergeable?

@penguinolog penguinolog added the Feature Feature request/implementation label Mar 28, 2023
Copy link
Collaborator

@penguinolog penguinolog left a comment

Choose a reason for hiding this comment

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

Validated, fonts rendered correctly.
Interim changes is required in current moment.

@penguinolog penguinolog merged commit 147be1c into urwid:master Apr 18, 2023
@rbanffy
Copy link
Contributor Author

rbanffy commented Apr 18, 2023

Validated, fonts rendered correctly. Interim changes is required in current moment.

I've bad news. I just did a fresh install and the examples/bigtext.py program that showcases the new fonts is failing with a

Traceback (most recent call last):
  File "/tmp/urwid/examples/bigtext.py", line 163, in <module>
    main()
...
  File "/tmp/urwid/urwid/canvas.py", line 402, in __init__
    raise CanvasError(f"Attribute extends beyond text \n{text[i]!r}\n{attr[i]!r}" )
urwid.canvas.CanvasError: Attribute extends beyond text 
b'      '
[(None, 16)]

I'll see if I can add a fix and a test to prevent this regression.

@penguinolog
Copy link
Collaborator

This issue is not produced by changes in this PR (happens on almost all fonts)

@penguinolog
Copy link
Collaborator

Real issue is caused by encoding to bytes, when equal length Unicode strings became not equal length byte strings and TextOverlay validation fails on lines with empty elements (space placeholders using 1 byte)

@penguinolog
Copy link
Collaborator

@rbanffy Issue, which you hit is: #496

@andresmrm
Copy link

Are you sure it's related to that issue?
I created this #554 to help others getting this error.
For me it only happens with BigText and even in cases that don't use ellipsis.

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

Successfully merging this pull request may close these issues.

4 participants