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
Clip Text Overflow #268
Clip Text Overflow #268
Conversation
Hello @Nibba2018! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2020-07-19 19:53:26 UTC |
Codecov Report
@@ Coverage Diff @@
## master #268 +/- ##
==========================================
- Coverage 89.07% 89.04% -0.03%
==========================================
Files 19 19
Lines 4750 4812 +62
Branches 619 629 +10
==========================================
+ Hits 4231 4285 +54
- Misses 363 366 +3
- Partials 156 161 +5
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this @Nibba2018! Below, a first review but I need to test it. I will come back to you after some tests. Thank you again!
fury/utils.py
Outdated
return original_str | ||
|
||
else: | ||
while start_ptr < end_ptr: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder about the impact of this while
for performance. clip_overflow
seems to be called often. I need to test it. As a temporary solution, I will be fine but we will have to find a better approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The complexity of this is around log2(length)
. The problem is each character has a different height and width.
Also the width of two characters within a string isn't equal to the sum of their individual widths.
Ok, it works as expected, no issue. Thank you @Nibba2018! I will wait that you move the function and it will be ready for merge |
@skoudoro , codecov patch seems to fail. Any Idea how I can fix that? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for fixing this issue @Nibba2018! there is a small pep8 but we will fix it later.I will go ahead and merge your PR.
fury.ui
to maintain abstraction. Users can use this method to clip theirTextBlock2D
text for a given width.