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.

Sign up for GitHub

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

PLANTUML-15 Allow setting output format for generated diagram #2

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

PLANTUML-15 Allow setting output format for generated diagram #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f54c30
PLANTUML-15 Allow setting output format for generated diagram
Dec 16, 2022
e9b6766
PLANTUML-15 Allow setting output format for generated diagram
Dec 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions macro-plantuml-macro/src/main/java/org/xwiki/contrib/plantuml/PlantUMLConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,11 @@ public interface PlantUMLConfiguration
* @return the (optional) PlantUML server URL (e.g. {@code http://www.plantuml.com/plantuml}) or null if not defined
*/
String getPlantUMLServerURL();

/**
* @return the default diagram output format (png)
* @Unstable
* @since 14.10
Copy link
Member

Choose a reason for hiding this comment

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

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.

*/
String getDefaultOutputFormat();
}
16 changes: 15 additions & 1 deletion macro-plantuml-macro/src/main/java/org/xwiki/contrib/plantuml/PlantUMLGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,19 @@
@Role
public interface PlantUMLGenerator
{
/**
* Generate the image in the passed output parameter, using PlantUML.
*
* @param input the textual definition input
* @param output the stream into which the generated image will be written to
* @param serverURL the optional plantUML server URL (e.g. {@code http://www.plantuml.com/plantuml}. If not an
* empty string or not null then the URL is called to get the generated image. Otherwise PlantUML works in
* embedded mode (requires installation of Graphviz locally for most diagram types, and the {@code
* 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;
Copy link
Member

Choose a reason for hiding this comment

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

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


/**
* Generate the image in the passed output parameter, using PlantUML.
*
Expand All @@ -42,7 +55,8 @@ public interface PlantUMLGenerator
* empty string or not null then the URL is called to get the generated image. Otherwise PlantUML works in
* embedded mode (requires installation of Graphviz locally for most diagram types, and the {@code
* GRAPHVIZ_DOT} environment variable must be set to point to the path of the GraphViz executable).
* @param format sets the output format from the PlantUML server
* @throws IOException when there's a generation or writing error
*/
void outputImage(String input, OutputStream output, String serverURL) throws IOException;
void outputImage(String input, OutputStream output, String serverURL, String format) throws IOException;
xumix marked this conversation as resolved.
Show resolved Hide resolved
}
22 changes: 22 additions & 0 deletions macro-plantuml-macro/src/main/java/org/xwiki/contrib/plantuml/PlantUMLMacroParameters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ public class PlantUMLMacroParameters
{
private String serverURL;

private String format;

/**
* @param serverURL see {@link #getServer()}
*/
Expand All @@ -42,6 +44,16 @@ public void setServer(String serverURL)
this.serverURL = serverURL;
}

/**
* @param format see {@link #getFormat()}. Valid values: svg, png, txt
*
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Also missing a @since (and ideally an @Unstable annotation too).

*/
@PropertyDescription("the PlantUML output format")
xumix marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Missing examples here. This is is important for users since this is what they'll see in the WYSIWYG macro UI

public void setFormat(String format)
{
this.format = format;
}

/**
* @return the (optional) PlantUML server URL (e.g. {@code http://www.plantuml.com/plantuml})
*/
Expand All @@ -50,6 +62,14 @@ public String getServer()
return this.serverURL;
}

/**
* @return the (optional) PlantUML output format, png by default. Valid values: svg, png, txt
Copy link
Member

Choose a reason for hiding this comment

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

Same as above. No need to dupicate. Better put a See {@link #getFormat()} above and document it here properly.

*/
public String getFormat()
{
return this.format;
}

@Override
public boolean equals(Object object)
{
Expand All @@ -65,6 +85,7 @@ public boolean equals(Object object)
PlantUMLMacroParameters rhs = (PlantUMLMacroParameters) object;
return new EqualsBuilder()
.append(getServer(), rhs.getServer())
.append(getFormat(), rhs.getFormat())
.isEquals();
}

Expand All @@ -73,6 +94,7 @@ public int hashCode()
{
return new HashCodeBuilder(5, 37)
.append(getServer())
.append(getFormat())
.toHashCode();
}
}
12 changes: 12 additions & 0 deletions ...macro/src/main/java/org/xwiki/contrib/plantuml/internal/DefaultPlantUMLConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,16 @@ public String getPlantUMLServerURL()
}
return serverURL;
}

@Override
public String getDefaultOutputFormat() {
Copy link
Member

Choose a reason for hiding this comment

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

formatting is not following our best practices, see above.

String format = this.plantUMLConfigurationSource.getProperty("format");
Copy link
Member

@vmassol vmassol Feb 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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?

// 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");
Copy link
Member

Choose a reason for hiding this comment

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

Note: my previous feedback was not applied. You can write:

return this.plantUMLConfigurationSource.getProperty("format", this.xwikiPropertiesConfigurationSource.getProperty("plantuml.format", "png");

}
xumix marked this conversation as resolved.
Show resolved Hide resolved

return format;
}
}
8 changes: 7 additions & 1 deletion ...uml-macro/src/main/java/org/xwiki/contrib/plantuml/internal/DefaultPlantUMLGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Copy link
Member

@vmassol vmassol Feb 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This looks like some unnecessary code duplication since we allow to pass null to default to the default configuration. See computeFormat()

}

@Override
public void outputImage(String input, OutputStream outputStream, String serverURL, String format) throws IOException
{
if (StringUtils.isEmpty(serverURL)) {
new SourceStringReader(input).outputImage(outputStream);
Expand All @@ -65,7 +71,7 @@ public void outputImage(String input, OutputStream outputStream, String serverUR
// https://plantuml.com/text-encoding
String compressedInput = this.asciiEncoder.encode(
this.compressor.compress(input.getBytes(StandardCharsets.UTF_8)));
String fullURL = String.format("%s/png/~1%s", StringUtils.removeEnd(serverURL, "/"), compressedInput);
String fullURL = String.format("%s/%s/~1%s", StringUtils.removeEnd(serverURL, "/"), format, compressedInput);
// Call the server and get the response
try (CloseableHttpClient httpclient = HttpClients.createDefault()) {
HttpGet httpGet = new HttpGet(fullURL);
Expand Down
11 changes: 10 additions & 1 deletion macro-plantuml-macro/src/main/java/org/xwiki/contrib/plantuml/internal/PlantUMLMacro.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ List<Block> executeSync(String content, PlantUMLMacroParameters parameters, bool
{
String imageId = getImageId(content);
try (OutputStream os = this.imageWriter.getOutputStream(imageId)) {
this.plantUMLGenerator.outputImage(content, os, computeServer(parameters));
this.plantUMLGenerator.outputImage(content, os, computeServer(parameters), computeFormat(parameters));
} catch (IOException e) {
throw new MacroExecutionException(
String.format("Failed to generate an image using PlantUML for content [%s]", content), e);
Expand All @@ -162,6 +162,15 @@ private String computeServer(PlantUMLMacroParameters parameters)
return serverURL;
}

private String computeFormat(PlantUMLMacroParameters parameters)
{
String format = parameters.getFormat();
if (format == null) {
format = this.configuration.getDefaultOutputFormat();
}
return format;
}

private String getImageId(String content)
{
return String.valueOf(content.hashCode());
Expand Down