Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Simplify and fix dpi handling in tight_bbox#2621
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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. |
👍 for this change. It doesn't merge cleanly (probably the api_changes.rst file) so will need a rebase before merging. @leejjoon, thoughts? |
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 |
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 |
👍 to a privacy MEP if anyone wants to write one. (I'm not volunteering...) |
@mdboom, so, thumbs up for this PR too ;) ? |
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. |
Simplify and fix dpi handling in tight_bbox
Being based on PR#2588, this PR is now able to (again) fix
tight_bbox.adjust_bboxhandling 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(). Whenadjust_bbox()is called fromFigureCanvas.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 theirprint_()methods. The solution was to predict this setting from the file format and use the 72dpi implementation calledadjust_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.pyto the backends. The code duplication inadjust_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.