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

Ruby: rename attribute env-idea… #198

Merged
merged 1 commit into from
Oct 18, 2021

Conversation

ahus1
Copy link
Contributor

@ahus1 ahus1 commented Jan 21, 2021

…to kroki-force-png as this attribute name is leaking an implementation details from an early version of this plugin.

In fact, this was only true for the JavaFX preview. The new JCEF preview supports SVG preview.

@ahus1
Copy link
Contributor Author

ahus1 commented Jan 21, 2021

didn't change JavaScript elements, therefore I plead "not guilty" for the failing JavaScript builds.

@ahus1 ahus1 changed the title rename attribute env-idea… Ruby: rename attribute env-idea… Jan 21, 2021
@ahus1
Copy link
Contributor Author

ahus1 commented Jan 23, 2021

rebased, checks are now green.

@ggrossetie
Copy link
Member

... as this attribute name is leaking an implementation details from an early version of this plugin.

Indeed, we should rename this attribute.
What about :kroki-force-format: png? Since we are already using :kroki-default-format: png, it would be consistent.
Could you please describe this attribute in https://github.com/Mogztter/asciidoctor-kroki#configuration?

@ahus1 ahus1 marked this pull request as draft January 25, 2021 07:29
@ahus1
Copy link
Contributor Author

ahus1 commented Jan 25, 2021

This would change the semantics, as now several formats would need to be supported, including the feature that the extension would need to know which format is support by what backend, see the following code comment:

# ... unless the diagram library does not support PNG as output format!
# Currently, mermaid, nomnoml, svgbob, wavedrom only support SVG as output format.

New proposal: I'd rather implement the kroki-default-format in the ruby variant, and drop the kroki-force-format altogether.

This will not be a perfect fit for the IntelliJ plugin; on the other hand the IntelliJ platform is switching to JCEF that support SVGs properly. The automatic switching to PNG caused confusion in some places before.

WDYT?

@ggrossetie
Copy link
Member

New proposal: I'd rather implement the kroki-default-format in the ruby variant, and drop the kroki-force-format altogether.

Sounds good, less is more 👍

This will not be a perfect fit for the IntelliJ plugin; on the other hand the IntelliJ platform is switching to JCEF that support SVGs properly. The automatic switching to PNG caused confusion in some places before.

So your plan is to use :kroki-default-format: png when the IntelliJ plugin is not using JCEF?

@ahus1 ahus1 marked this pull request as ready for review October 16, 2021 15:30
@ahus1
Copy link
Contributor Author

ahus1 commented Oct 16, 2021

Hi @Mogztter - I finally completed the rework for this. I'd be happy if you could have a look.

And yes, I plan to use :kroki-default-format: png when the IntelliJ plugin is not using JCEF.

@ggrossetie
Copy link
Member

Looks good.

The commit message should be something like: "Remove env-idea logic (use kroki-default-format instead)", right? I can update it when merging but I want to make sure that I fully understand the change.

The env-idea attribute leaked from an early version of this library that was used in an IntelliJ environment,and doesn't make sense in the more general usage of this library. kroki-default-format has already been implemented in the JavaScript version of this library.
@ahus1
Copy link
Contributor Author

ahus1 commented Oct 17, 2021

Yes, you're right. I updated the description, keeping the short "what" you suggested in the first line, and putting a longer "why" in the second part of the commit message.
Feel free to change as you see fit when merging.

@ggrossetie ggrossetie merged commit 86340a2 into asciidoctor:master Oct 18, 2021
@ahus1 ahus1 deleted the kroki-force-png branch October 18, 2021 20:30
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

2 participants