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

Fix #1907: Support svg image in LaTeX output. #2166

Closed
wants to merge 1 commit into from

Conversation

xuhdev
Copy link
Contributor

@xuhdev xuhdev commented Dec 11, 2015

However this relies on an existing installation of inkscape. The tests of svg should be ignored if no inkscape installation is found.

@xuhdev
Copy link
Contributor Author

xuhdev commented Dec 11, 2015

This PR should solve #1907

@xuhdev xuhdev mentioned this pull request Dec 11, 2015
@xuhdev xuhdev force-pushed the latex-svg branch 2 times, most recently from 9becd94 to d3ebab0 Compare December 11, 2015 10:34
@xuhdev xuhdev changed the title Support svg image in LaTeX output. Fix #1907: Support svg image in LaTeX output. Dec 11, 2015
@@ -1182,7 +1184,11 @@ def visit_image(self, node):
options = ''
if include_graphics_options:
options = '[%s]' % ','.join(include_graphics_options)
self.body.append('\\includegraphics%s{%s}' % (options, uri))
if node['uri'].lower().endswith('svg') or \
Copy link
Member

Choose a reason for hiding this comment

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

I think endswith(('.svg', '.svgz')) is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I agree with you.

@tk0miya
Copy link
Member

tk0miya commented Dec 20, 2015

It seems this is a new feature. The master branch is more appropriate to apply this PR.
Could you resend it again?

@tk0miya
Copy link
Member

tk0miya commented Dec 20, 2015

In addition, it seems enabling --shell-escape is insecure operation.
It should be better that the user specify any settings consciously (conf.py or texmf.cnf).
What do you think about the option?

@tk0miya tk0miya added the type:enhancement enhance or introduce a new feature label Dec 21, 2015
@xuhdev
Copy link
Contributor Author

xuhdev commented Dec 22, 2015

@tk0miya Yes, I can add an option in conf.py. How about latex_shell_escaped = True | False(default)? I would also add an error message when svg files are included but this option is not turned on. And yes, I will resend the PR for master.

@xuhdev
Copy link
Contributor Author

xuhdev commented Dec 23, 2015

I think I have a better way to implement this without touching the --shell-escaped setting. We can convert the image format (pdf to svg for html output and svg to pdf for LaTeX output) using inkscape before generate the documents. I can reimplement these on master.

@tk0miya
Copy link
Member

tk0miya commented Dec 24, 2015

New idea looks good to me!

And, I think the feature should be implemented as sphinx extension (cf. sphinx.ext.latexsvg ).
This feature depends on inkscape. If we compose it as extension, it keeps sphinx-core simple and less dependencies.

@birkenfeld @shimizukawa What do you think about this?

@xuhdev
Copy link
Contributor Author

xuhdev commented Dec 24, 2015

@tk0miya It is more than LaTeX and svg --- it can also convert pdf/eps to svg for html output. Maybe we can call it compatible_images?

@tk0miya
Copy link
Member

tk0miya commented Jan 2, 2016

Oh, sorry. I did not noticed your comments...

It might the users can not guess what it does if it is named as compatible_images.
I think more direct words are better like 'convert'.

The naming of the extensions is very important, I think. but, at the same time, it is very difficult to get an agreements about it.
So, in this time, please name it as you like.

@xuhdev
Copy link
Contributor Author

xuhdev commented Jan 2, 2016

Thanks, I'll name it as auto_image_converter . I'll make another PR :)

@xuhdev xuhdev closed this Jan 2, 2016
@mgeier
Copy link
Contributor

mgeier commented Jan 2, 2016

In case you're not aware of it, the nbconvert package has a converter using Inkscape: https://github.com/jupyter/nbconvert/blob/master/nbconvert/preprocessors/svg2pdf.py

About the name: Since it depends on Inkscape, probably it should have inkscape in its name?
Something like sphinx.ext.inkscapeconverter perhaps?

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
builder:latex type:enhancement enhance or introduce a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants