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

bugfix infinite loop in Terminal.wrap #275

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Conversation

jquast
Copy link
Owner

@jquast jquast commented Jun 26, 2024

Closes #273 by @grayjk,

The following code enters an infinite loop:

import blessed
blessed.Terminal().wrap('\u5973', 1)

This fixes by explicit check: when

  • the given individual sequence is of length '2'
  • and the wrap width is '1'
  • and the cur_len is '0'

we cannot break down this "Wide" character any further -- so it is allowed to flow outside the given cell.

  • faulthandler_timeout = 30 is added to [pytest] in tox.ini
  • Tests for East-Asian, Emoji, and ZWJ are added
  • Further noting that blessed gets ZWJ wrong

jquast added 2 commits June 26, 2024 11:59
From #273,

> The following code enters an infinite loop:

    import blessed
    blessed.Terminal().wrap('\u5973', 1)

This fixes by explicit test: when the given individual sequence is
of length '2', and the width is '1', and the cur_len is '0', we cannot
break down this "Wide" character any further -- so it is allowed to
flow outside the given cell.

- 'faulthandler_timeout = 30' is added to [pytest] in tox.ini,
- Tests for East-Asian, Emoji, and ZWJ are added
- Further noting that blessed gets ZWJ wrong
Copy link

codecov bot commented Jun 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.33%. Comparing base (167c34e) to head (2526384).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #275      +/-   ##
==========================================
+ Coverage   95.31%   95.33%   +0.01%     
==========================================
  Files           9        9              
  Lines        1025     1028       +3     
  Branches      216      217       +1     
==========================================
+ Hits          977      980       +3     
  Misses         44       44              
  Partials        4        4              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jquast jquast changed the title jq/bugfix issue 273 bugfix infinite loop in Terminal.wrap Jun 26, 2024
@@ -219,8 +227,15 @@ def _handle_long_word(self, reversed_chunks, cur_line, cur_len, width):
idx = nxt = 0
for text, _ in iter_parse(term, chunk):
nxt += len(text)
if Sequence(chunk[:nxt], term).length() > space_left:
break
seq_length = Sequence(chunk[:nxt], term).length()
Copy link

Choose a reason for hiding this comment

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

when seq_length is equal to space_left, the for loop does the next chunk even though there is no space remaining

is there another small optimization possible to call length fewer times? something like:

if seq_length == space_left:
    idx = nxt
    break
elif seq_length > space_left:
    ...

Copy link
Owner Author

Choose a reason for hiding this comment

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

There are many unicode characters that consume 0 cells, https://wcwidth.readthedocs.io/en/latest/specs.html#width-of-0, there are examples of this in the ZWJ test added, though it gets it wrong in that specific case, we would prefer to cojoin with 1,800+ possible Combining Marks that might follow, like in cafe\u0301 we would not wish to break immediately after the letter 'e', because the next character \u0301 is desired

Copy link
Owner Author

Choose a reason for hiding this comment

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

Added test in 6beb3c6 hope that makes it more clear

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.

infinite loop in Terminal.wrap
2 participants