8000 Correct URL area with rotated texts in PDFs by eindH · Pull Request #23288 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Correct URL area with rotated texts in PDFs #23288

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 27 commits into from
Jun 23, 2022
Merged

Correct URL area with rotated texts in PDFs #23288

merged 27 commits into from
Jun 23, 2022

Conversation

eindH
Copy link
Contributor
@eindH eindH commented Jun 16, 2022 •
edited
Loading

PR Summary

This ticket closes #23205

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

github-actions[bot]
github-actions bot reviewed Jun 16, 2022
View reviewed changes
Copy link
@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@eindH
Fixed lint issues
d5dda49 
Fixed errors thrown by flake8
@oscargus
Copy link
Member

I believe that there are a few issues here, although it is on the right track:

  1. The angle is typically given in degrees in Matplotlib, but the Python functions use radians. Use math.radians(angle) to convert.
  2. It would be great to use the QuadPoints box as outlined in [Bug]: URL-area not rotated in PDFs #23205
  3. To get the new coordinates for Rect, one should probably get all the four corners for the QuadPoints and take the min/max (now, the left x coordinate does not extend left for a counter clockwise rotation)
  4. I am not sure that the trigonometry is correct. Especially, the output of cos and sin are in the range 0 to 1, while there should be some relation to the width and height.

I suggest explicitly computing the additional locations a to f as in #23205 and use those to determine the rest.

(Also, there should ideally be some sort of test although I can honestly not come up with anything reasonable...)

eindH added 2 commits June 16, 2022 13:54
@eindH
Calculates vertices
7620ed8 
Created a new function that calculates the vertices of the rectangle rotated around x,y by angle. Changed spelling of  _get_coordinated_from_angle
@eindH
Vertices of outlining rectangle
13b48f7 
A rectangle that surrounds the URL text is calculated
@eindH
Copy link
Contributor Author
eindH commented Jun 16, 2022

I believe I've fixed points 1 & 4 and the last point about computing a-f. Not sure if I've implemented the QuadPoints correctly.

@eindH
Correct trigonometry
8ef3740 
Realised a mistake in my equations
oscargus
oscargus reviewed Jun 16, 2022
View reviewed changes
oscargus
oscargus reviewed Jun 16, 2022
View reviewed changes
oscargus
oscargus reviewed Jun 16, 2022
View reviewed changes
oscargus
oscargus reviewed Jun 16, 2022
View reviewed changes
eindH and others added 5 commits June 16, 2022 16:02
@eindH @oscargus
Update lib/matplotlib/backends/backend_pdf.py
ed8b1ca 
Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com>
@eindH @oscargus
Update lib/matplotlib/backends/backend_pdf.py
c47e089 
Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com>
@eindH @oscargus
Update lib/matplotlib/backends/backend_pdf.py
31f9b3b 
Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com>
@eindH @oscargus
Update lib/matplotlib/backends/backend_pdf.py
6da81cd 
Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com>
@eindH
Changed variable name
9eae986 
Corrected variable name
oscargus
oscargus reviewed Jun 16, 2022
View reviewed changes
oscargus
oscargus reviewed Jun 16, 2022
View reviewed changes
@oscargus
Copy link
Member

I am not sure how one can test this easily, but it would be great if you could manually run it and see if the clickable area ends up where it is expected (and that the PDF viewer does not complain etc).

The example code in the original issue should be a simple example.

@eindH
Copy link
Contributor Author
eindH commented Jun 16, 2022 •
edited
Loading

Would you mind telling how to import the module I've made? Tried copying the backend-pdf file into my current matplotlib installation but I just get errors that things can't be found. Specifically ImportError: cannot import name '_c_internal_utils' from partially initialized module 'matplotlib'

eindH and others added 2 commits June 16, 2022 17:01
@eindH @oscargus
Update lib/matplotlib/backends/backend_pdf.py
0ce7e87 
Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com>
@eindH
Spellcheck error
760267b 
Changed spelling in comment
@oscargus
Copy link
Member

It depends on how you did install Matplotlib. If you installed from source in edit mode (pip install -e . sort of), then it should work.

If not, it probably still should work in general, but errors like this may happen. (I edit in Spyder and there I need to start two new shells before I get the update, but the error you get is maybe not like that.)

Sometimes it may be easier just to copy the relevant parts into an existing install since there may be other things that have changed in the file that are not compatible with your installed version.

I can try to fetch it and try it out, probably tomorrow evening, if you do not succeed.

@oscargus
Copy link
Member
oscargus commented Jun 17, 2022 •
edited
Loading

I realized that there are some tests for embedding URL already:

matplotlib/lib/matplotlib/tests/test_backend_pdf.py

Lines 224 to 272 in 047254d

def test_text_urls():
pikepdf = pytest.importorskip('pikepdf')
test_url = 'https://test_text_urls.matplotlib.org/'
fig = plt.figure(figsize=(2, 1))
fig.text(0.1, 0.1, 'test plain 123', url=f'{test_url}plain')
fig.text(0.1, 0.4, 'test mathtext $123$', url=f'{test_url}mathtext')
with io.BytesIO() as fd:
fig.savefig(fd, format='pdf')
with pikepdf.Pdf.open(fd) as pdf:
annots = pdf.pages[0].Annots
# Iteration over Annots must occur within the context manager,
# otherwise it may fail depending on the pdf structure.
for y, fragment in [('0.1', 'plain'), ('0.4', 'mathtext')]:
annot = next(
(a for a in annots if a.A.URI == f'{test_url}{fragment}'),
None)
assert annot is not None
# Positions in points (72 per inch.)
assert annot.Rect[1] == decimal.Decimal(y) * 72
@needs_usetex
def test_text_urls_tex():
pikepdf = pytest.importorskip('pikepdf')
test_url = 'https://test_text_urls.matplotlib.org/'
fig = plt.figure(figsize=(2, 1))
fig.text(0.1, 0.7, 'test tex $123$', usetex=True, url=f'{test_url}tex')
with io.BytesIO() as fd:
fig.savefig(fd, format='pdf')
with pikepdf.Pdf.open(fd) as pdf:
annots = pdf.pages[0].Annots
# Iteration over Annots must occur within the context manager,
# otherwise it may fail depending on the pdf structure.
annot = next(
(a for a in annots if a.A.URI == f'{test_url}tex'),
None)
assert annot is not None
# Positions in points (72 per inch.)
assert annot.Rect[1] == decimal.Decimal('0.7') * 72

Basically one could just copy those, but rotate the text. Then one can list the expected rect and quadpoints and test against those.

@oscargus oscargus changed the title Url area not rotated in PDFs #23205 Correct URL area with rotated texts PDFs Jun 17, 2022
@oscargus
Copy link
Member

I took the liberty to change the title and replace the word "solves" with "closes" in the description. In that way #23205 will automatically be closed when this is merged. (You can see on the right hand side under Development that it is now linked.)

@oscargus oscargus changed the title Correct URL area with rotated texts PDFs Correct URL area with rotated texts in PDFs Jun 17, 2022
@eindH eindH force-pushed the URL-area-not-rotated-in-PDFs-#23205 branch from baebd65 to a99c3ec Compare June 17, 2022 10:34
@oscargus
Copy link
Member

I tested it manually and it didn't really work. Hence, I submitted a PR to your branch, see eindH#1

There is an inconsistency in the PDF specification and every viewer I have tested. The specs says that QuadPoints should be ignored if any point lies outside of Rect. However, if any point in QuadPoints is on the border of Rect it will be ignored.

Here are two files. link_nogap.pdf has the boundaries of the QuadPoints as Rect, link.pdf extends the boundaries 0.00001 (as we print with five decimal digits), so 0.00072 points wider than theoretical...

link.pdf
link_nogap.pdf

With the new change, this only happens when the rectangle is rotated something other than an integer multiple of 90.

oscargus
oscargus reviewed Jun 18, 2022
View reviewed changes
oscargus
oscargus reviewed Jun 18, 2022
View reviewed changes
oscargus
oscargus reviewed Jun 18, 2022
View reviewed changes
oscargus
oscargus reviewed Jun 18, 2022
View reviewed changes
@eindH @oscargus
Update lib/matplotlib/backends/backend_pdf.py
8c97b61 
Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com>
oscargus
oscargus reviewed Jun 18, 2022
View reviewed changes
oscargus
oscargus reviewed Jun 18, 2022
View reviewed changes
eindH and others added 5 commits June 18, 2022 12:10
@eindH @oscargus
Update lib/matplotlib/backends/backend_pdf.py
ff03152 
Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com>
@eindH @oscargus
Update lib/matplotlib/backends/backend_pdf.py
d9cbfbc 
Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com>
@eindH @oscargus
Update lib/matplotlib/backends/backend_pdf.py
e2b8ca0 
Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com>
@eindH @oscargus
Update lib/matplotlib/tests/test_backend_pdf.py
c9b4f26 
Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com>
@eindH
Lint
7600521
@eindH
Copy link
Contributor Author
eindH commented Jun 18, 2022

Thanks for those changes. Is there anything that needs doing to raise it for review?

@oscargus
Copy link
Member

I think this is good to go now.

Just a friendly reminder to whoever merges this to squash the commits in to a single one.

(Maybe we should also ask for a third approval since I feel sort of directly involved in the code...)

@jklymak jklymak merged commit 50aaa7c into matplotlib:main Jun 23, 2022
@jklymak
Copy link
Member
jklymak commented Jun 23, 2022

I squash merged this in case anyone was keeping track of individual commits...

@QuLogic QuLogic added this to the v3.6.0 milestone Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@jklymak jklymak jklymak approved these changes

@oscargus oscargus oscargus approved these changes

Assignees
No one assigned
Labels
backend: pdf
Projects
None yet
Milestone
v3.6.0
Development

Successfully merging this pull request may close these issues.

[Bug]: URL-area not rotated in PDFs
4 participants
@eindH @oscargus @jklymak @QuLogic
0