Skip to content
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

Simplify and fix dpi handling in tight_bbox #2621

Merged
merged 2 commits into from
Jan 30, 2014
Merged

Simplify and fix dpi handling in tight_bbox #2621

merged 2 commits into from
Jan 30, 2014

Conversation

pwuertz
Copy link
Contributor

@pwuertz pwuertz commented Nov 28, 2013

Being based on PR #2588, this PR is now able to (again) fix tight_bbox.adjust_bbox handling of PGF figures.

The problem with that function is its dependence on a figure's dpi, which it naturally takes from the figure instance. This behaviour was implemented in adjust_bbox_png(). When adjust_bbox() is called from FigureCanvas.print_figure() this dpi value might be incorrect, since backends like svg and pdf use a fixed value of 72dpi and override the figure dpi later when calling their print_() methods. The solution was to predict this setting from the file format and use the 72dpi implementation called adjust_bbox_pdf(). This fails because the mapping from file types to backends is ambiguous.

I fixed this by moving all per-backend information from tight_bbox.py to the backends. The code duplication in adjust_bbox() is gone. The function instead provides a parameter for setting a custom dpi, which a backend/FigureCanvas announces if necessary.

The second commit adds a missing clip rectangle in backend_pgf (reported in #2586) and an appropriate test.

@pwuertz
Copy link
Contributor Author

pwuertz commented Dec 18, 2013

It seems that @leejjoon contributed most of the code related to the bbox feature. May I ask your opinion on this? @mdboom probably as well.

Unfortunately the master already advanced to a point where the PR isn't mergable anymore. I would like to wait for your consent for these changes before bringing them up to date again.

@pelson
Copy link
Member

pelson commented Dec 18, 2013

👍 for this change. It doesn't merge cleanly (probably the api_changes.rst file) so will need a rebase before merging.

@leejjoon, thoughts?

@tacaswell
Copy link
Member

Do we know if these functions are used by anyone outside the library?

If they are purely private functions, should they be renamed with leading _?

@pwuertz
Copy link
Contributor Author

pwuertz commented Jan 13, 2014

You could argue that these functions are "private enough" since they reside in a module a user normally wouldn't import, but we don't really know if someone might be using the tight_bbox module directly. I hence tried to keep the new interface at least similar to the old one.
Btw, is there a common set of rules regarding the privacy of matplotlib functions? Might be a MEP candidate..

@mdboom
Copy link
Member

mdboom commented Jan 13, 2014

👍 to a privacy MEP if anyone wants to write one. (I'm not volunteering...)

@pwuertz
Copy link
Contributor Author

pwuertz commented Jan 13, 2014

@mdboom, so, thumbs up for this PR too ;) ?

@mdboom
Copy link
Member

mdboom commented Jan 13, 2014

Looks fine to me. @leejjoon probably understands the potential issues, better, though. If he is unavailable for comment before 1.4.0rc1, I say we merge this anyway.

tacaswell added a commit that referenced this pull request Jan 30, 2014
Simplify and fix dpi handling in tight_bbox
@tacaswell tacaswell merged commit 80bba8e into matplotlib:master Jan 30, 2014
@pwuertz pwuertz deleted the fix_2586 branch January 30, 2014 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants