-
Notifications
You must be signed in to change notification settings - Fork 932
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
Group text lines if they are centered (#382) #384
Group text lines if they are centered (#382) #384
Conversation
It is
Look at |
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.
Looks good!
69eda17
to
6b3c75c
Compare
@pietermarsman I've added 4 new commits, addressing your comments and then adding tests. I couldn't find any tests for the function I've changed, but think what I added tests it fairly well (even if it takes a while of figuring out bounding boxes to work out what it's doing....). Before my change, this test would fail because the Please could you take a look? |
4a99d77
to
d0d8866
Compare
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 love the new *_aligned_with
methods. They make find_neighbours()
so much easier to read. Also the tests are really good :)
I've left 3 small cosmetic suggestions.
d0d8866
to
69489c5
Compare
@pietermarsman PR updated with your comments |
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.
Looks good!
Description
Add a check when checking if lines should be merged to see if they are centered above each other.
Fixes #382
Note: it might also be worth having a read of #383, as there is perhaps a discussion there about how to deal with these checks for a negative line margin...
How Has This Been Tested?
Example PDF
Here is my test code (which simply loads the pdfs and prints the elements):
Before this change, you get:
After this change you get:
(The last two elements in each are just artefacts in the example pdf, ignore them).
Checklist
I will do these things once I get a bit of guidance on whether this is an acceptable change. I'll also need a pointer on how to add a relevant test.