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

Proof of concept - plantuml support #272

Open
alexfargus opened this issue Jun 30, 2021 · 6 comments
Open

Proof of concept - plantuml support #272

alexfargus opened this issue Jun 30, 2021 · 6 comments

Comments

@alexfargus
Copy link
Contributor

I need plantuml support for my documents so I picked up the trail from #155.

I have had some some success using the png output from plantuml. (See plantuml.pdf) The problem encountered by @vtexier is due to an off py one error in purepng.py that causes an assertion to fail rather than skipping compressed metadata as intended.

@brechtm I'll head upstream to see if this issue has been fixed elsewhere. However since purepng.py is embedded into rinohtype will you accept a PR to fix the issue here?

The changes from my proof of concept branch can be found here

With regards to the Tansformer used to support the plantuml node, I don't think that it fits in rinohtype since that would introduce a dependency on sphinxcontrib-plantuml so I will find another home for that.

@brechtm
Copy link
Owner

brechtm commented Jul 1, 2021

I have had some some success using the png output from plantuml. (See plantuml.pdf) The problem encountered by @vtexier is due to an off py one error in purepng.py that causes an assertion to fail rather than skipping compressed metadata as intended.

@brechtm I'll head upstream to see if this issue has been fixed elsewhere. However since purepng.py is embedded into rinohtype will you accept a PR to fix the issue here?

Thanks for debugging! Yes, please submit this fix upstream. I see they have a test suite, so that should catch regressions due to the change. There doesn't seem to be much activity over at purepng's GitHub page though, so if your fix passes the test suite, I'm happy to include it in rinohtype.

The changes from my proof of concept branch can be found here

With regards to the Tansformer used to support the plantuml node, I don't think that it fits in rinohtype since that would introduce a dependency on sphinxcontrib-plantuml so I will find another home for that.

Yup. Except for the purepng fix, all code should become part of the plantuml codebase. You'll have to making sure it still works if rinoh can't be imported, for example by importing rinoh within by a try/except.

I don't think you need a transformer though. Did you see the example code from #118 (comment)? That is rinohtype's equivalent of the visit/depart functions of other builders.

PS. I haven't yet found the time to review #271. Will do that ASAP.

@alexfargus
Copy link
Contributor Author

I don't think you need a transformer though. Did you see the example code from #118 (comment)? That is rinohtype's equivalent of the visit/depart functions of other builders.

Yes, I saw that. However in my opinion the advantage of the transformer approach is that it should work for any builder, and not need to import anything specific to rinohtype (my implementation only uses imports from sphinx and plantuml).

@brechtm
Copy link
Owner

brechtm commented Jul 2, 2021

However in my opinion the advantage of the transformer approach is that it should work for any builder, and not need to import anything specific to rinohtype (my implementation only uses imports from sphinx and plantuml).

That's a good point.

For completeness, in many cases you can simply return a tree of standard docutils nodes from the SphinxDirective's subclass's run() method (no need for visit/depart methods). You could even do things specific to the builder in that method. See #201 (comment) and #201 (comment) for more info, and the DrawIOImage and DrawIOFigure classes for an example.

@brechtm
Copy link
Owner

brechtm commented Jul 2, 2021

I noticed that PyPNG, where purepng was forked from, is again being updated. It might be good to submit the fix to PyPNG (if it still has the same bug) and see whether we should replace purepng with it.

@alexfargus
Copy link
Contributor Author

alexfargus commented Jul 3, 2021

PyPng does not have this bug. This is due to the fact that it does not try to parse the iTXt information that is mishandled in purepng.

See https://github.com/drj11/pypng/blob/45e8e314ca0bf1cd0c390ac95d353c9b31b17723/man/chunk.rst#itxt

@basejumpa
Copy link

👍 for that feature. I also need it!

I'd love if this enhancement would support both png and svg formats.

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

No branches or pull requests

3 participants