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

Upgraded Features #7

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Upgraded Features #7

wants to merge 19 commits into from

Conversation

frantiekm
Copy link

Added some features missing from base version:

  • now you can hide code tags
  • all of the attribute fields should be filled
  • added support for some more object types

@1azyman 1azyman self-requested a review October 22, 2024 17:52
HtmlExporter htmlExporter = new HtmlExporter();
htmlExporter.export(adocFile, tempHtmlFile);

String princeCommand = "prince " + tempHtmlFile.getAbsolutePath() + " -o " + outputPdf.getAbsolutePath();
Copy link
Member

Choose a reason for hiding this comment

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

Why did you switch to external command? Asciidoctor supports export to PDF, this creates dependency on another binary that might not be available. Native PDF export via asciidoctor is preferred, in case prince is needed then I'd expected user will use html export and then prince command separately

Choose a reason for hiding this comment

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

The main problem with the asciidoctor PDF export was formatting problem. Most of the generated documentation was unusable. The problem was that it did not recognize when the generated expression was ending therefore if the expression was longer and appeared on the next page it was overlapped with other sections. Use of the prince fixed that issue.

Copy link
Member

Choose a reason for hiding this comment

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

@matejglombik I understand but can't accept 3rd party dependency this way. If I understand it correctly prince can be still used with midscribe --output-format html --output document.html; and then prince document.html document.pdf . We can rather try to fix pdf generator later to get rid of prince altogether.

Copy link
Member

Choose a reason for hiding this comment

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

@matejglombik I've created some simple test with resource object that contains long groovy script (in terms of line count and line length), midscribe rendered pdf where script just truncated and didn't continue to next page, lines breaks were created correctly. Could you provide (send) xml that creates incorrect output please?

@@ -34,7 +35,7 @@ public void testDescribeCapability() throws Exception {
variables.put("capabilities", capabilities);

Properties props = new Properties();
props.setProperty(GeneratorProperties.VELOCITY_START_TEMPLATE, "/template/capabilities.vm");
props.setProperty(GeneratorProperties.VELOCITY_START_TEMPLATE, "/template/capabilities-native.vm");
Copy link
Member

Choose a reason for hiding this comment

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

why only native?

Choose a reason for hiding this comment

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

The reason for that is that i broke the 1 template into 2 and after that i did not had enough time to recreate tests. Thats why only native template.

@@ -0,0 +1,552 @@
/*! Asciidoctor default stylesheet | MIT License | https://asciidoctor.org */
Copy link
Member

Choose a reason for hiding this comment

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

is default css needed in this project?

Choose a reason for hiding this comment

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

No it is not needed, the default one from asciidoctor will be used. I removed it from there.

Copy link
Member

@1azyman 1azyman left a comment

Choose a reason for hiding this comment

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

Please have a look at few comments, I still haven't have the time to check velocity templates

@1azyman
Copy link
Member

1azyman commented Nov 18, 2024

@matejglombik just to make sure - I can accept this PR only when prince dependency (pdf exporter) is reverted and style.css is removed as well. Another thing I don't understand - why dependency org.apache.xmlgraphics:fop was added?
Velocity templates look OK probably, no tests yet I see. Maybe localization support (via ProcessorUtils.translator) could be somehow improved.

@1azyman 1azyman requested a review from matejglombik November 18, 2024 15:43
# Conflicts:
#	midscribe-core/src/main/java/com/evolveum/midscribe/generator/TemplateUtils.java
#	midscribe-core/src/main/java/com/evolveum/midscribe/generator/export/HtmlExporter.java
#	midscribe-core/src/main/java/com/evolveum/midscribe/generator/export/PdfExporter.java
#	midscribe-core/src/main/resources/midscribe.properties
#	midscribe-core/src/test/java/com/evolveum/midscribe/TemplateUtilsTest.java
#	pom.xml
@matejglombik
Copy link

matejglombik commented Dec 5, 2024

@1azyman I removed whole prince engine from the midscribe + style.css as well. I merged your changes from master to ours and used your changes. Dependency org.apache.xmlgraphics:fop was removed as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants