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

Fix print_status when using widechars in desc #410

Merged
merged 2 commits into from Oct 11, 2019

Conversation

redbug312
Copy link
Contributor

@redbug312 redbug312 commented Jul 14, 2017

I found that passing widechars into set_description() would cause the width of description field miscalculated, and break the progress bar.

These are the screen records of before and after this fix.

@casperdcl casperdcl self-assigned this Jul 15, 2017
@casperdcl casperdcl added the p3-enhancement 🔥 Much new such feature label Jul 15, 2017
@casperdcl
Copy link
Sponsor Member

casperdcl commented Jul 15, 2017

Thanks @redbug312 - happy to merge if you can fix the unit tests for this PR (#410)

@redbug312
Copy link
Contributor Author

I fixed Job #1475.2; But for Job #1475.10, this commit causes too much overhead in print_status(s), even 2.5 times slower than simple_progress() in the job

Maybe a more efficient solution or widechar-support flag is needed

@casperdcl casperdcl added p2-bug-warning ⚠ Visual output bad help wanted 🙏 We need you (discussion or implementation) to-fix ⌛ In progress labels Jul 22, 2017
@casperdcl casperdcl force-pushed the master branch 6 times, most recently from 6ec00f1 to 4b6476a Compare July 22, 2017 14:15
@codecov-io
Copy link

codecov-io commented Jul 26, 2017

Codecov Report

Merging #410 into master will increase coverage by 13.72%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           master     #410       +/-   ##
===========================================
+ Coverage   85.56%   99.29%   +13.72%     
===========================================
  Files          15        7        -8     
  Lines         977      568      -409     
  Branches      169      108       -61     
===========================================
- Hits          836      564      -272     
+ Misses        136        3      -133     
+ Partials        5        1        -4

@mia-0
Copy link

mia-0 commented Feb 1, 2018

This yields wrong results for combining character strings such as 'a\u0322' (returns 2 instead of 1) or strings containing non-printable characters, such as '\u200b\u200b\u200b' (returns 3 instead of 0).

What we’re interested in is not the string length but the width in terminal cells. The third-party wcwidth module does just that with its wcswidth function, which is a pure Python implementation of the libc equivalent.

@casperdcl
Copy link
Sponsor Member

thanks, like your suggestion @lachs0r

@casperdcl
Copy link
Sponsor Member

@FichteFoll thanks for bumping this. I don't have much time - will merge much quicker if someone fixes tests.

@casperdcl casperdcl changed the base branch from master to devel October 10, 2019 16:13
casperdcl added a commit that referenced this pull request Oct 10, 2019
- fixes #803
- closes #410
- closes #805
@casperdcl casperdcl mentioned this pull request Oct 10, 2019
1 task
@casperdcl casperdcl merged commit 825f7dc into tqdm:devel Oct 11, 2019
Casper automation moved this from Next Release to Done Oct 11, 2019
@casperdcl casperdcl mentioned this pull request Oct 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted 🙏 We need you (discussion or implementation) p2-bug-warning ⚠ Visual output bad p3-enhancement 🔥 Much new such feature to-fix ⌛ In progress
Projects
Casper
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants