-
Notifications
You must be signed in to change notification settings - Fork 250
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
feat :: add soft-hyphen for multi-cell #308
Conversation
- refactor existing codebase for readability - introduce automatic line break at soft-hyphen location
At this point it is in |
- able to render 20+ lines of lorem ipsum with refactored multi-line - tests are failing
basic test is working now. able to print 20+ pages of lorem ipsum using refactored approach. |
That looks promising, very cool!
Tests breaking on a code change are often caused by non-semantic differences in the PostScript code of the PDF (sometimes simply changing insignificant whitespace). The previous code uses |
Thank you for your contribution @oleksii-shyman! I second @gmischler answer. |
- only 6 testcases fail
I was able to fix most of the cases, only 1 pdf updated, visually validated that it has no differences with the original one. Currently, only 2 markdown related cases are failing:
I was able to investigate the root cause, and understand why does it fail.
However, this approach doesn't work on markdown strings, because markdown special characters ( I want to discuss how to properly solve this issue. One easy way to do it - to give up on the idea of 2 stages, just issue a call to
Based on the problems with the existing approach i propose another way to do it:
The problem here is that
@gmischler @Lucas-C What do you think? |
Examples (ttf \ no ttf) of basic test case with soft hyphen
As you can see - there are 3 soft hyphens in the same word, but only one is present. |
Your latest demo-screenshot looks very promising @oleksii-shyman ! I introduced @all-contributors please add @oleksii-shyman for code, design, ideas |
I've put up a pull request to add @oleksii-shyman! 🎉 |
Yeah, I expected that to be a non-trivial task, but you're getting there!
Now allow me to make things even more complicated... 😁 You may have noticed that When chosing your approach for I haven't looked at it very closely, but I expect that both Given that you need to do the markdown splitting at a very early stage, wrapping As an organisational detail, if you create new classes to do the formatting, then it might be a good idea to place those in a seperate file (assuming they don't use nested scoping). |
Sounds reasonable, I think it is realistic to do it this way |
- basic test cases are added - breaking works with variable length (for compatibily with fpdf.write method) - testcases are passing
The list of things I changed:
I think this patch is ready for high level review. Please let me know if you have any objections or concerns, and I will add more tests and documentation. |
Codecov Report
@@ Coverage Diff @@
## master #308 +/- ##
==========================================
+ Coverage 89.99% 90.43% +0.44%
==========================================
Files 19 20 +1
Lines 5815 5803 -12
Branches 1167 1148 -19
==========================================
+ Hits 5233 5248 +15
+ Misses 365 339 -26
+ Partials 217 216 -1
Continue to review full report at Codecov.
|
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.
You really did an excellent job here!
The code will be much more cleaner after merging this.
The tests are also great 👍
I made a few comments, but overall this is very good.
Could you add a mention of this change in CHANGELOG.md
as part of this PR please?
Thank you for your feedback, will do my changes shortly |
When I submitted #291, I was prepared to wait quite a bit, as I realized it required some substantial refactoring to get it to work. This will serve as a solid basis for someone later adapting |
I have no bandwidth to handle another change of scope. Do you think that this basis is solid enough for someone later adapting |
Yes, I think it's better to get this merged first, than anyone who's willing to improve things could submit other PRs. |
I didn't mean to imply that any changes to |
@oleksii-shyman please ping me if I can ever help 😊 |
- add more tests - address review comments
- updated one markdown test pdf file - removed unused line
Thank you! I was able to added much more tests, and update my code (according to review comments) |
This looks perfect, merging the PR now! |
I think we should add a mention of soft-hyphen support in the docs, but I'm not sure where... |
Close #291
Checklist:
The GitHub pipeline is OK (green),
meaning that both
pylint
(static code analyzer) andblack
(code formatter) are happy with the changes of this PR.A unit test is covering the code added / modified by this PR
This PR is ready to be merged
In case of a new feature, docstrings have been added, with also some documentation in the
docs/
folderA mention of the change is present in
CHANGELOG.md
The PR description or comment contains a picture of a cute animal (not mandatory but encouraged 😉)