PLANTUML-15 Allow setting output format for generated diagram #2
Conversation
macro-plantuml-macro/src/main/java/org/xwiki/contrib/plantuml/PlantUMLConfiguration.java
Outdated
Show resolved
Hide resolved
macro-plantuml-macro/src/main/java/org/xwiki/contrib/plantuml/PlantUMLGenerator.java
Show resolved
Hide resolved
macro-plantuml-macro/src/main/java/org/xwiki/contrib/plantuml/PlantUMLMacroParameters.java
Show resolved
Hide resolved
macro-plantuml-macro/src/main/java/org/xwiki/contrib/plantuml/PlantUMLMacroParameters.java
Outdated
Show resolved
Hide resolved
...ml-macro/src/main/java/org/xwiki/contrib/plantuml/internal/DefaultPlantUMLConfiguration.java
Show resolved
Hide resolved
|
@xumix let me know if you need help with my review comments. Wishing you happy end of the year time. |
Hi! Had no time to fix the issues, now resolved them, thanks for review. |
| /** | ||
| * @return the default diagram output format (png) | ||
| * @Unstable | ||
| * @since 14.10 |
There was a problem hiding this comment.
The since doesn't seem correct. This is the version of this extension when this changes will be in. Right now the pom says 2.1.2-SNAPSHOT, so I'd say it should be 2.2 since you're making non bug-fixing changes, hence we'll increase the minor version by 1.
| * GRAPHVIZ_DOT} environment variable must be set to point to the path of the GraphViz executable). | ||
| * @throws IOException when there's a generation or writing error | ||
| */ | ||
| void outputImage(String input, OutputStream output, String serverURL) throws IOException; |
There was a problem hiding this comment.
This will break backward compatibility. Revapi should fail. Also note that the javadoc formatting is not following our best practices but that should be easy to fix. See https://dev.xwiki.org/xwiki/bin/view/Community/CodeStyle/JavaCodeStyle/
To make this not break backward compatibiilty, just make it a default method. BTW the javadoc is not correct as it doesn't explain what's the difference with void outputImage(String input, OutputStream output, String serverURL, String format) throws IOException; . What format will be used? (this is what needs to be documented).
Are you sure we need this when we can already specify the format? Why is it needed? Thx
| @@ -42,6 +44,16 @@ public void setServer(String serverURL) | |||
| this.serverURL = serverURL; | |||
| } | |||
|
|
|||
| /** | |||
| * @param format see {@link #getFormat()}. Valid values: svg, png, txt | |||
| * | |||
There was a problem hiding this comment.
This can be improved. Best is to use: Valid values are: {@code svg}, {@code png} and {@code txt}, or link to some plantuml doc explaining the valid values.
There was a problem hiding this comment.
Also missing a @since (and ideally an @Unstable annotation too).
| * @param format see {@link #getFormat()}. Valid values: svg, png, txt | ||
| * | ||
| */ | ||
| @PropertyDescription("the PlantUML output format") |
There was a problem hiding this comment.
Missing examples here. This is is important for users since this is what they'll see in the WYSIWYG macro UI
| @@ -50,6 +62,14 @@ public String getServer() | |||
| return this.serverURL; | |||
| } | |||
|
|
|||
| /** | |||
| * @return the (optional) PlantUML output format, png by default. Valid values: svg, png, txt | |||
There was a problem hiding this comment.
Same as above. No need to dupicate. Better put a See {@link #getFormat()} above and document it here properly.
| // The returned value can be null if no xobject has been defined on the wiki config page. | ||
| if (format == null) { | ||
| // Fallback to xwiki.properties | ||
| format = this.xwikiPropertiesConfigurationSource.getProperty("plantuml.format", "png"); |
There was a problem hiding this comment.
Note: my previous feedback was not applied. You can write:
return this.plantUMLConfigurationSource.getProperty("format", this.xwikiPropertiesConfigurationSource.getProperty("plantuml.format", "png");
| @@ -56,4 +56,16 @@ public String getPlantUMLServerURL() | |||
| } | |||
| return serverURL; | |||
| } | |||
|
|
|||
| @Override | |||
| public String getDefaultOutputFormat() { | |||
There was a problem hiding this comment.
formatting is not following our best practices, see above.
| @@ -57,6 +57,12 @@ public class DefaultPlantUMLGenerator implements PlantUMLGenerator | |||
|
|
|||
| @Override | |||
| public void outputImage(String input, OutputStream outputStream, String serverURL) throws IOException | |||
| { | |||
| outputImage(input, outputStream, serverURL, "png"); | |||
There was a problem hiding this comment.
This looks like some unnecessary code duplication since we allow to pass null to default to the default configuration. See computeFormat()
|
|
||
| @Override | ||
| public String getDefaultOutputFormat() { | ||
| String format = this.plantUMLConfigurationSource.getProperty("format"); |
There was a problem hiding this comment.
I don't see a format xproperty in the XClass at https://github.com/xwiki-contrib/macro-plantuml/blob/master/macro-plantuml-ui/src/main/resources/PlantUML/PlantUMLConfigClass.xml. Is it missing?
No description provided.