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

CairoMakie: Allow restricting PDF version #3845

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

barucden
Copy link

@barucden barucden commented May 9, 2024

Description

This PR allows users to restrict the version of an output PDF like this:

fig = Figure()
ax = Axis(fig[1, 1])
x = range(0, 10, length=100)
y = sin.(x)
save("version_1-4.pdf", fig, pdf_version="1.4")
save("version_1-7.pdf", fig, pdf_version="1.7")

shell> file version_1-4.pdf
version_1-4.pdf: PDF document, version 1.4, 1 page(s)

shell> file version_1-7.pdf
version_1-7.pdf: PDF document, version 1.7

This feature was discussed on Discourse (and also earlier)

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist

  • Added an entry in CHANGELOG.md (for new features and breaking changes)
  • Added or changed relevant sections in the documentation
  • Added unit tests for new algorithms, conversion methods, etc.

I need guidance regarding documentation (where to best put this) and testing (I am not sure how to check a PDF's version in Julia).

Also I was thinking that the CAIRO_PDF_VERSION_* constants together with restrict_pdf_version! should perhaps go to Cairo.jl. What do you think?

@barucden barucden force-pushed the restrict-pdf-version branch 2 times, most recently from 21ec306 to 7a71fc5 Compare May 9, 2024 12:42
CairoMakie/src/screen.jl Outdated Show resolved Hide resolved
CairoMakie/src/cairo-extension.jl Outdated Show resolved Hide resolved
src/theming.jl Outdated Show resolved Hide resolved
@t-bltg t-bltg added enhancement Feature requests and enhancements CairoMakie This relates to CairoMakie, the vector backend for Makie based on Cairo. labels May 11, 2024
@barucden barucden force-pushed the restrict-pdf-version branch 2 times, most recently from 08fee5d to e1f5a49 Compare May 13, 2024 09:33
@barucden
Copy link
Author

I have added an entry to changelog and documented the new config option.

Copy link
Member

@asinghvi17 asinghvi17 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@barucden
Copy link
Author

Rebased to resolve new conflicts

@barucden
Copy link
Author

The error is not related imho:

GLMakie/Normals of a Cat.png: Test Failed at /home/runner/work/Makie.jl/Makie.jl/ReferenceTests/src/runtests.jl:117
  Expression: score <= threshold
   Evaluated: 0.5715945363044739 <= 0.05

@barucden
Copy link
Author

CI's green ✅ Thank you, Simon.

@barucden
Copy link
Author

Rebased to fix conflicts.

@@ -74,3 +74,10 @@ function get_render_type(surface::Cairo.CairoSurface)
typ == Cairo.CAIRO_SURFACE_TYPE_IMAGE && return IMAGE
return IMAGE # By default assume that the render type is IMAGE
end

function restrict_pdf_version!(surface::Cairo.CairoSurface, v::Integer)
Copy link
Collaborator

Choose a reason for hiding this comment

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

would be simpler to type the v with the enum here, then you don't need yet another check. I'd guess you don't even have to manually convert it to Int32 in the ccall

Copy link
Author

Choose a reason for hiding this comment

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

I would like to move restrict_pdf_version! to Cairo.jl eventually, where they stick to numerical constants instead of enums, so I'd prefer to keep the argument as Integer.

You are right that the manual conversion in the ccall is not needed. I removed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CairoMakie This relates to CairoMakie, the vector backend for Makie based on Cairo. enhancement Feature requests and enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants