Skip to content

Fix swap_if_landscape call in backend_ps #17509

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 1 commit into from
Aug 24, 2020
Merged

Fix swap_if_landscape call in backend_ps #17509

merged 1 commit into from
Aug 24, 2020

Conversation

Contextualist
Copy link
Contributor

@Contextualist Contextualist commented May 26, 2020 •
edited
Loading

orientation.swap_if_landscape takes one tuple/list, but here the parenthesis is dropped. Another similar call right above demonstrates the correct usage:

matplotlib/lib/matplotlib/backends/backend_ps.py

Lines 834 to 837 in 51a1940

papertype = _get_papertype(
*orientation.swap_if_landscape((width, height)))
paper_width, paper_height = orientation.swap_if_landscape(
papersize[papertype])

QuLogic
QuLogic approved these changes May 26, 2020
View reviewed changes
Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

LGTM, are you able to write a test?

@Contextualist
Copy link
Contributor Author

Now I look at this portion of code more closely, and I found that I do not understand the rationale behind.

matplotlib/lib/matplotlib/backends/backend_ps.py

Lines 832 to 846 in 69e651f

# find the appropriate papertype
width, height = self.figure.get_size_inches()
if papertype == 'auto':
papertype = _get_papertype(
*orientation.swap_if_landscape((width, height)))
paper_width, paper_height = orientation.swap_if_landscape(
papersize[papertype])
if mpl.rcParams['ps.usedistiller']:
# distillers improperly clip eps files if pagesize is too small
if width > paper_width or height > paper_height:
papertype = _get_papertype(
*orientation.swap_if_landscape(width, height))
paper_width, paper_height = orientation.swap_if_landscape(
papersize[papertype])

Both _get_papertype and orientation.swap_if_landscape are essentially two pure functions, being executed on a set of same parameters ((width, height)). The results are the same. How should the dimensions be adjusted when a distiller is used? Do I miss anything?

@QuLogic
Copy link
Member

QuLogic commented Jun 23, 2020

In the first call, _get_papertype is called if papertype is 'auto'. In the second case, it's called to override the paper size if it's too small. The conditions are different, and may or may not be called in either case, but they aren't called twice.

jklymak
jklymak approved these changes Aug 24, 2020
View reviewed changes
Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

Lets merge this as pretty clearly a typo. Its a shame there is no test, but a test seems a bit of a PITA here....

@jklymak jklymak merged commit e3c2b31 into matplotlib:master Aug 24, 2020
@QuLogic QuLogic added this to the v3.4.0 milestone Aug 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic approved these changes

@jklymak jklymak jklymak approved these changes

Assignees
No one assigned
Labels
backend: ps
Projects
None yet
Milestone
v3.4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@Contextualist @QuLogic @jklymak