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

AsciiDoctor Diagram support added #66

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

Conversation

JanWesterkamp-iJUG
Copy link
Contributor

This PR adds AsciiDoctor Diagram support and updates AsciiDoc related versions.

Also the source highligter is fixed and configured to PDF documets with the intended to be AsciiDoctor new default "rouge" implementation.
The source highlighter could be configured to HTML output manually, but this would generate dependencies to external static ressources that allow for potential user tracking.

Defaut postfix (.asciidoc) and source loaction (src/main/asciidoc) are unchanged for now (see #63).

@Emily-Jiang
Copy link
Member

Can you explain why and what you are trying to add this plugin in this PR?

@JanWesterkamp-iJUG
Copy link
Contributor Author

AsciiDoctor Diagram support.

Adding i.e. PlantUML, ditaa or BPMN diagrams to the component spec documents (or others).

It also updates some related versions and fixes some issues like usage of the source highlighter.

@Emily-Jiang
Copy link
Member

Which spec needs to use this one? Is it possible to add to that spec to try it out before making this to parent pom?

@JanWesterkamp-iJUG
Copy link
Contributor Author

JanWesterkamp-iJUG commented Oct 6, 2022

Which spec needs to use this one? Is it possible to add to that spec to try it out before making this to parent pom?

I would like to use it within MP Telemetry and it is a configuration I derived from my company's current platform project documentation and the official AsciiDoctor Maven examples. But this configuration (not as an user of the microprofile-parent) would be helpful on the MPSP project too, to render the BPMN drawing directly from the source there instead of generating a SVG to be included manually.

It is possible to overwrite the asciidoctor-maven-plugin and it's dependency inside a component specs plugin management section, but I would have to figure out this will work with the microprofile-parent asciidoc profile then the intendet way...
May be a local Maven build and (cache) install and configuring that artifact in a component spec for test proposal can make sure this does not break anything too.

I also can offer to show you my current use - but that project itself is not public yet (but will be to some extent in the future).

Another improvement for testing microprofile-parent changes (i.e. for major release plugin update too) could be an test component spec or template for it - but that approach would take some time.

Originally the AsciiDoctor Diagram support was discussed with @kwsutter and @radcortez about more than a year ago for using it in the Jakarta Security documentation to replace existing low quality images.
However, it took some time to improve and stabilise it to be ready for use here...

@Emily-Jiang
Copy link
Member

Which spec needs to use this one? Is it possible to add to that spec to try it out before making this to parent pom?

I would like to use it within MP Telemetry and it is a configuration I derived from my company's current platform project documentation and the official AsciiDoctor Maven examples. But this configuration (not as an user of the microprofile-parent) would be helpful on the MPSP project too, to render the BPMN drawing directly from the source there instead of generating a SVG to be included manually.

* [AsciiDoctor Diagram project](https://docs.asciidoctor.org/diagram-extension/latest/)

* [AsciiDoctor Diagram Maven example](https://github.com/JanWesterkamp-iJUG/asciidoctor-maven-examples/tree/main/asciidoctor-diagram-example)

It is possible to overwrite the asciidoctor-maven-plugin and it's dependency inside a component specs plugin management section, but I would have to figure out this will work with the microprofile-parent asciidoc profile then the intendet way... May be a local Maven build and (cache) install and configuring that artifact in a component spec for test proposal can make sure this does not break anything too.

I also can offer to show you my current use - but that project itself is not public yet (but will be to some extent in the future).

Another improvement for testing microprofile-parent changes (i.e. for major release plugin update too) could be an test component spec or template for it - but that approach would take some time.

Originally the AsciiDoctor Diagram support was discussed with @kwsutter and @radcortez about more than a year ago for using it in the Jakarta Security documentation to replace existing low quality images. However, it took some time to improve and stabilise it to be ready for use here...

Thank you @JanWesterkamp-iJUG for the detailed explanation! Do you think this can be done after MP 6.0, hopefully happening soon? We can then spend more time to experiment the plugin.

@JanWesterkamp-iJUG
Copy link
Contributor Author

Which spec needs to use this one? Is it possible to add to that spec to try it out before making this to parent pom?

I would like to use it within MP Telemetry and it is a configuration I derived from my company's current platform project documentation and the official AsciiDoctor Maven examples. But this configuration (not as an user of the microprofile-parent) would be helpful on the MPSP project too, to render the BPMN drawing directly from the source there instead of generating a SVG to be included manually.

* [AsciiDoctor Diagram project](https://docs.asciidoctor.org/diagram-extension/latest/)

* [AsciiDoctor Diagram Maven example](https://github.com/JanWesterkamp-iJUG/asciidoctor-maven-examples/tree/main/asciidoctor-diagram-example)

It is possible to overwrite the asciidoctor-maven-plugin and it's dependency inside a component specs plugin management section, but I would have to figure out this will work with the microprofile-parent asciidoc profile then the intendet way... May be a local Maven build and (cache) install and configuring that artifact in a component spec for test proposal can make sure this does not break anything too.
I also can offer to show you my current use - but that project itself is not public yet (but will be to some extent in the future).
Another improvement for testing microprofile-parent changes (i.e. for major release plugin update too) could be an test component spec or template for it - but that approach would take some time.
Originally the AsciiDoctor Diagram support was discussed with @kwsutter and @radcortez about more than a year ago for using it in the Jakarta Security documentation to replace existing low quality images. However, it took some time to improve and stabilise it to be ready for use here...

Thank you @JanWesterkamp-iJUG for the detailed explanation! Do you think this can be done after MP 6.0, hopefully happening soon? We can then spend more time to experiment the plugin.

@Emily-Jiang now MP 6.0 is released and we already fixed some of the (security) issues there in the same way I want to do it in MP Parent here (eclipse/microprofile#300).
Can we now add this PR for a upcoming MP Parent 3.1 release?

I can then proceed to create an new PR for the remaining issue in both projects.
I already created an upstream PR for the Asciidoctor Maven Plugin, so when there is a new release, we can remove some of the workarounds and simplify things then.

Copy link
Contributor

@radcortez radcortez left a comment

Choose a reason for hiding this comment

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

I've noticed a few style differences in the PDF version. We may want to confirm if we are ok with this.

The annotations color in the new version is gray (in the HTML version, they are blue, and the previous PDF showed light purple). I don't want to discuss colors, but I feel the annotations in the PDF version are hardly noticeable due to the similar color to the rest of the text.

@JanWesterkamp-iJUG
Copy link
Contributor Author

I've noticed a few style differences in the PDF version. We may want to confirm if we are ok with this.

The annotations color in the new version is gray (in the HTML version, they are blue, and the previous PDF showed light purple). I don't want to discuss colors, but I feel the annotations in the PDF version are hardly noticeable due to the similar color to the rest of the text.

@radcortez, thanks for your approval so far.
Can you explain what you meant exacly with annotation in this context, please:

Just to make sure not misunderstood you. ;-)
When you meant the source highlighting:

The existing configuration was broken already, as the attribute sourceHighlighter got replaced by source-highlighter in AsciiDoctor version 2.x.
So I repaced it with the new attribute name and Rouge (as the new default, instead of CodeRay) on the PDF generation only, because in HTML the highlighter references external ressources that allow user tracking. This need to be fixed in the upstream project to prevent complex workarounds of couse. I started discussing this already there (with your colleague @ahus1 at EclipseCon 2022), but it is not solved yet.
So here, I tried to be as conversative as possible and repaired it only, where it´s safe to use.

By the way, this is one of the differences for the MP 6.0 Spec change, where CodeRay was already active for both output documents. Many MP component specs fix this issue by setting the correct attribute in the docs as a workaround.
The 2nd difference is the newer asciidoctorj.version (2.5.7 instead of 2.5.6 here) - but I would like to update all the versions in a separate PR, as there are other updates too (for both projects).

@radcortez
Copy link
Contributor

I was referring to the Java annotations styling in the code. Here is an example from the PDF export:

Current:
Screenshot 2023-02-10 at 12 59 41

With these changes:
Screenshot 2023-02-10 at 13 00 27

@radcortez
Copy link
Contributor

My intent here is not to discuss colors. I'm terrible at that. I just wanted to make sure that we are aware of this.

@Emily-Jiang
Copy link
Member

My intent here is not to discuss colors. I'm terrible at that. I just wanted to make sure that we are aware of this.

I am not a big fun with the new color scheme.

@JanWesterkamp-iJUG
Copy link
Contributor Author

@radcortez and @Emily-Jiang, it looks like the style sheets deviate a little bit between the PDF and the HTML output and more reasonable between CodeRay and Rouge, where both try to style like GitHub. Rouge uses GitHub as the default style sheet, but there is the option to configure one from here.

So we will get the old behaviour, when CodeRay is configured and it looks like that inlines the CSS (now?), which solves the tracking issue in this respect (there are also 3rd party fonts included, but that´s a different story).

Another option would be finding the right theme for Rouge.

Can we solve this in the next PR or do I need to do it here?

@radcortez
Copy link
Contributor

I'm fine if this is resolved on a separate PR.

@JanWesterkamp-iJUG
Copy link
Contributor Author

@Emily-Jiang can I address you concerns by changing the source highlighter back to CodeRay? This would fix source highlighting for PDFs with the old design.
As it was broken for both document versions and is currently only working because of overwrites in the componen specs, this would then break nothing.

Then we can handle the final solution in a separate issue/PR.

I really would like to merge this, as I would like to fix the security issues with it and then do it for the remaining ones, as similar for the MP umbrella spec too:

@Emily-Jiang
Copy link
Member

@JanWesterkamp-iJUG as agreed on last week's call, can you split this PR to 2 PRs, one of which is to address plugin upgrades to address CVEs and the other one on diagram plugin? thanks

@JanWesterkamp-iJUG
Copy link
Contributor Author

@Emily-Jiang, as requested the seprate PR for the security fix is created here: #73

When this is merged, I will create an update for this one.

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

Successfully merging this pull request may close these issues.

None yet

3 participants