Add support for PlantUML diagrams #887

alyip98 · 2019-06-10T08:41:34Z

What is the purpose of this pull request? (put "X" next to an item, remove the rest)
• [x] New feature

Resolves #877

What is the rationale for this request?

As MarkBind is to be optimized for software documentation, it may be useful to support PlantUML.

PlantUML is a tool written in Java that allows users to write diagrams in a simple and intuitive language. Being able to write (and track) UML diagrams in a text format compared to using tools such as Powerpoint or Photoshop allows better version control and ease of use.

What changes did you make? (Give an overview)
Add ability to include UML diagrams via a <puml> tag. The diagram itself is written in PlantUML syntax in a separate file. The page includes the diagram using the following syntax:
<puml src="diagrams/example.puml"/>

Is there anything you'd like reviewers to focus on?
Due to limitations of htmlparser2, including UML code inline will cause problems as UML syntax violates HTML specifications (e.g. presence of symbols such as <-- which are misinterpreted as HTML tags). Fixing this will require cheerio to use a forked version of htmlparser2, discussed in #582, which in itself is an open problem.

Testing instructions:
The user guide contains an entry

Proposed commit message: (wrap lines at 72 characters)
Markbind page authors can use diagrams in their sites by linking them
in a <pic> tag. This is tedious as it requires the author to generate
the diagrams using an external tool, and the binary files generated by
that tool (e.g. .pptx, .psd) are not version control friendly.

Let's support PlantUML, a tool that allows users to write diagrams in
a simple and intuitive language. Users can then write their diagrams
in a text editor, and Markbind will generate the diagrams using
PlantUML, eliminating the need of an external editor.

Chng-Zhi-Xuan · 2019-06-11T06:33:54Z

Will it be more advantageous to just use an existing module? node-plantuml is tested, documented, and provides more features. However, not all of its features are relevant to MarkBind (due to it also being a CLI application)

Does your current implementation require the user to already have Java installed? If so it might be problematic to impose installing a JRE / JDK to use this feature.

Having looked through node-plantuml, looks like its more suited for MarkBind.
Seems like we can avoid the "diagrams generated after MarkBind generates the website" problem as well by using a custom tag and cheerio to selectively parse the input into node-plantuml

<plantuml>
Bob->Alice : hello
</plantuml>

Page.prototype.generatePlantuml = function (content) {
  const $ = cheerio.load(content);
  const plantumlContent = $("plantuml");
  ...
  ...
}

Is the delay going to be a problem, especially on large sites with numerous diagrams?

Build times is not really a concern (unless it is a significant increase).

You should take note on how it affects previewing websites for authors. Manually recording some timings on a test page with different number of diagrams & other components can be a good gauge if it is problematic.

Other comments:

Looks like your code crashes if an out folder is not created. If a new folder is required, you should place it within _markbind as well as creating that folder on markbind init by modifying code in Site.js.
Name the folder storing the generated diagrams something better than out
A good enhancement would be to ensure minimal generation of duplicate diagrams (E.G 2 different pages uses the same plantuml markup). Not sure if this is already handled by the plugin/parser.

Although not fully tested, this seems like it is a great feature addition as I can place the plantuml diagrams within MarkBind components.

alyip98 · 2019-06-12T03:12:19Z

Does your current implementation require the user to already have Java installed? If so it might be problematic to impose installing a JRE / JDK to use this feature.

Yeah, PlantUML is supplied as a JAR, there's no way around it. node-plantuml is also a wrapper that runs the JAR which offers an abstracted API.

Having looked through node-plantuml, looks like its more suited for MarkBind.
Seems like we can avoid the "diagrams generated after MarkBind generates the website" problem as well by using a custom tag and cheerio to selectively parse the input into node-plantuml

Currently I'm using markdown-it to parse since it's more convenient. But I think I'll try using cheerio and see if its better.

My reservation for not using node-plantuml is because all the extra features are unnecessary, and it presents another potential source of errors.

But now I'm encountering a new problem where certain symbols within the PlantUML language are misinterpreted as HTML tags. For now I'll try using cheerio instead and see if it solves this problem

Chng-Zhi-Xuan

Other Comments

The .puml files do not end with an empty line. Might want to change it such that parsers will know that is the EOF? (and have Github stop complaining about it)
The graphviz not found bug is fixed after installing the stable version of graphviz 2.38.

Chng-Zhi-Xuan · 2019-06-24T04:52:47Z

src/plugins/default/markbind-plugin-plantuml.js

+  // Read diagram file
+  fs.readFile(src, (err, data) => {
+    if (err) {
+      throw err;


We should log the error with a meaningful message, and continue to generate the website.

Throwing the error would stop the parsing entirely, requiring the user to debug the verbose error on their own.

Since you used error below, you should use it here instead of err for consistency.

Likewise for all error catching code below, we should be able to fail gracefully with a meaningful message. Since we can just not generate this specific plant-uml diagram and continue with parsing.

Ok, good point

As discussed in person:

Unable to share the same error names as the same name was defined in a higher scope. So we decided to move the verbose error message to the logger file and print out a more meaningful message on the console.

Chng-Zhi-Xuan · 2019-06-24T04:59:57Z

src/plugins/default/markbind-plugin-plantuml.js

+  const diagramSrc = src.replace('.puml', '.png');
+  const rawDiagramPath = path.resolve(path.dirname(sourcePath), src);
+  const outputFilePath = path.resolve(rootPath, '_site', path.relative(rootPath, rawDiagramPath)).replace('.puml', '.png');
+  // const outputFilePath = path.resolve(diagramSrc);


Since this PR is still [WIP], remember to remove redundant comments + console logging code below.

Chng-Zhi-Xuan · 2019-06-24T05:00:37Z

src/plugins/default/markbind-plugin-plantuml.js

+    try {
+      fs.mkdirSync(outputDir, { recursive: true });
+      // eslint-disable-next-line no-empty
+    } catch (_e) {}


Is there a reason to catch this error but not throw it?

Chng-Zhi-Xuan · 2019-06-24T05:05:34Z

docs/_markbind/layouts/userGuide/navigation.md

@@ -7,6 +7,7 @@
  * [Adding Pages]({{baseUrl}}/userGuide/addingPages.html)
  * [MarkBind Syntax Overview]({{baseUrl}}/userGuide/markBindSyntaxOverview.html)
  * [Formatting Contents]({{baseUrl}}/userGuide/formattingContents.html)
+  * [UML]({{baseUrl}}/userGuide/puml.html)


Inserting PlantUML Diagrams?

Chng-Zhi-Xuan · 2019-06-24T05:12:46Z

docs/userGuide/puml.md

+
+<span id="overview" class="lead">
+
+**MarkBind offers integration with PlantUML in your pages,** allowing you design and write UML diagrams (and more) using PlantUML syntax, which is then compiled and served as images.


Add a newline after the bold text?

(and more) should naturally link to other examples

You should put a warning at the top of this page indicating that authors need to have a JRE/JDK installed, along with the graphviz installer to use this feature.

Chng-Zhi-Xuan · 2019-06-24T05:24:22Z

test/functional/test_site/testPlantUML.md

+## PlantUML Test
+
+### Sequence Diagram
+<puml src="diagrams/sequence.puml" />


When I generate this page, the src will be changed to test_site/diagrams/abc.png when the correct one should be without test_site.

Seems to be an issue with the <pic> component, will look into it

Chng-Zhi-Xuan

For the graceful error handling, there is room for some improvement:

All errors that result in the diagram not being generated should return immediately.
All errors should be logged in logger.debug, not just replaced with a generic error message.
We should only show 1 logger.error per diagram (to reduce clutter).

Example

if (criticalErr1) {
  logger.debug(criticalErr1);
  logger.error("Tell user something went wrong");
  return;
} 

if (nonCriticalError1) {
  logger.debug(nonCriticalError1);
}

if (nonCriticalError2) {
  logger.debug(nonCriticalError2);
}

if (criticalError2) {
  logger.debug(criticalError2);
  logger.error("Tell user smth went wrong);
  return;
}

This would ensure we only get 1 console error message per diagram, and all errors that occurred up until the critical error is logged properly.

damithc · 2019-06-25T04:06:55Z

Use Github's 'draft PR' feature instead of WIP prefix in the PR title.

alyip98 · 2019-06-25T04:41:29Z

@Chng-Zhi-Xuan

For the graceful error handling, there is room for some improvement:

All errors that result in the diagram not being generated should return immediately.

All errors should be logged in logger.debug, not just replaced with a generic error message.

We should only show 1 logger.error per diagram (to reduce clutter).

This would ensure we only get 1 console error message per diagram, and all errors that occurred up until the critical error is logged properly.

Currently it already does this. There are 2 main locations where errors occur - reading the .puml, and running the jar. If an error occurs during the read, the error is logged and the jar is not run. If an error occurs while running the jar, either the error or close event is triggered, but not both, so it will not be double logged.

alyip98 · 2019-06-25T04:42:28Z

@damithc there are some issues with Github's draft PR feature, namely not being able to revert back to it or retroactively mark an opened PR as draft

Chng-Zhi-Xuan

Other comments:

You have not initialised the diagrams folder within _markbind during markbind init. You also have to update the Site.test.js code since an additional folder is expected on initialisation. All sharable assets between pages (such as site-nav) should be in the _markbind folder.
Good that you tried to reduce duplicate processing for the same diagrams using Sets.

Chng-Zhi-Xuan · 2019-06-26T13:03:46Z

docs/userGuide/puml.md


-**[Graphviz](https://www.graphviz.org/download/) must be installed to generate diagrams**, with the exception of sequence and activity diagrams. (_tested with v2.38_)
+**[<tooltip content="Tested with v2.38">Graphviz</tooltip>](https://www.graphviz.org/download/) must be installed to generate <tooltip content="with the exception of sequence and activity diagrams">diagrams</tooltip>**


Will there be a case where activity and sequence diagrams don't work if they used a certain syntax for another component (without Graphviz) ?

Honestly, I don't know. What do you mean by a certain syntax for another component?

On a separate note, use tooltips only in cases where you want to supply a definition/explanation of a term. Otherwise, the reader will have to retrieve every tooltip to see if there is something relevant in the tooltip. Use dimmed text to provide unimportant details.
recent version is not precise enough. Say something like version 8 or later.

Chng-Zhi-Xuan · 2019-06-26T13:28:26Z

docs/userGuide/puml.md

@@ -15,11 +15,11 @@

 <box type="warning">

-**[Java](https://www.java.com/en/download/) must be installed to use this feature**
+**[Java](https://www.java.com/en/download/) must be installed to use this feature**. _(Any recent version should work)_


How about "Java must be installed to generate the UML diagrams via a JAR executable"?

To inform the user why is it needed.

Chng-Zhi-Xuan · 2019-06-26T13:29:48Z

docs/userGuide/puml.md

 </box>
 <box type="warning">

-**[Graphviz](https://www.graphviz.org/download/) must be installed to generate diagrams**, with the exception of sequence and activity diagrams
+**[Graphviz](https://www.graphviz.org/download/) must be installed to generate diagrams**, with the exception of sequence and activity diagrams. (_tested with v2.38_)


"... to generate all PlantUML diagrams"?

alyip98 · 2019-06-26T14:03:30Z

You have not initialised the diagrams folder within _markbind during markbind init. You also have to update the Site.test.js code since an additional folder is expected on initialisation.

The diagram files can be in any folder, not necessarily restricted to the diagrams folder. I used diagrams in this case as an example, but it could have been any folder.

alyip98 · 2019-06-27T03:08:29Z

@Chng-Zhi-Xuan @damithc I've rephrased the requirements section

Chng-Zhi-Xuan · 2019-06-27T09:38:17Z

You have not initialised the diagrams folder within _markbind during markbind init. You also have to update the Site.test.js code since an additional folder is expected on initialisation. All sharable assets between pages (such as site-nav) should be in the _markbind folder.

@alyip98 You have not addressed the request above.

In addition, since all the diagrams are in _markbind/diagrams, you can simplify the src specification with some minor tweaks to your code.

<!-- src will search starting from "_markbind/diagrams" -->
<puml src="abc.puml">

Chng-Zhi-Xuan · 2019-06-28T02:24:50Z

As discussed in person, since <puml> is eventually going to be replaced with a <img> tag, we should not enforce that diagrams be placed in _markbind/diagrams because we don't enforce that for images as well.

Markbind page authors can use diagrams in their sites by linking them in a `<pic>` tag. This is tedious as it requires the author to generate the diagrams using an external tool, and the binary files generated by that tool (e.g. .pptx, .psd) are not version control friendly. Let's support PlantUML, a tool that allows users to write diagrams in a simple and intuitive language. Users can then write their diagrams in a text editor, and Markbind will generate the diagrams using PlantUML, eliminating the need of an external editor.

alyip98 force-pushed the 877-plantuml branch 3 times, most recently from d1804f6 to 099093b Compare June 21, 2019 04:25

Chng-Zhi-Xuan reviewed Jun 24, 2019

View reviewed changes

alyip98 force-pushed the 877-plantuml branch from 0c551f1 to e6c1c3d Compare June 25, 2019 04:13

alyip98 changed the title ~~[WIP] Add PlantUML diagrams support~~ Add PlantUML diagrams support Jun 26, 2019

Chng-Zhi-Xuan requested changes Jun 26, 2019

View reviewed changes

alyip98 added 3 commits June 28, 2019 10:39

Docs: Document PlantUML feature

0a76639

Test: Add tests for PlantUML

a0b3c0c

alyip98 force-pushed the 877-plantuml branch from fe8ba8f to a0b3c0c Compare June 28, 2019 03:04

Chng-Zhi-Xuan approved these changes Jun 28, 2019

View reviewed changes

Chng-Zhi-Xuan changed the title ~~Add PlantUML diagrams support~~ Add support for PlantUML diagrams Jun 28, 2019

Chng-Zhi-Xuan merged commit 1912d4a into MarkBind:master Jun 28, 2019

Chng-Zhi-Xuan added this to the v2.5.2 milestone Jun 28, 2019

ang-zeyu mentioned this pull request Jan 29, 2021

Inquiry on PlantUML diagrams rendering with surge/netlify previews #1459

Closed

tlylt mentioned this pull request Apr 14, 2022

PUML not working on CI deployment (Teammates) due to lack of graphviz #1903

Closed


		<span id="overview" class="lead">

		MarkBind offers integration with PlantUML in your pages, allowing you design and write UML diagrams (and more) using PlantUML syntax, which is then compiled and served as images.


		[Graphviz](https://www.graphviz.org/download/) must be installed to generate diagrams, with the exception of sequence and activity diagrams. (_tested with v2.38_)
		[<tooltip content="Tested with v2.38">Graphviz</tooltip>](https://www.graphviz.org/download/) must be installed to generate <tooltip content="with the exception of sequence and activity diagrams">diagrams</tooltip>

Add support for PlantUML diagrams #887

Add support for PlantUML diagrams #887

Uh oh!

Conversation

alyip98 commented Jun 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Chng-Zhi-Xuan commented Jun 11, 2019

Uh oh!

alyip98 commented Jun 12, 2019

Uh oh!

Chng-Zhi-Xuan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chng-Zhi-Xuan Jun 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chng-Zhi-Xuan left a comment

Choose a reason for hiding this comment

Uh oh!

damithc commented Jun 25, 2019

Uh oh!

alyip98 commented Jun 25, 2019

Uh oh!

alyip98 commented Jun 25, 2019

Uh oh!

Chng-Zhi-Xuan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyip98 commented Jun 26, 2019

Uh oh!

alyip98 commented Jun 27, 2019

Uh oh!

Chng-Zhi-Xuan commented Jun 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Chng-Zhi-Xuan commented Jun 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

alyip98 commented Jun 10, 2019 •

edited

Loading

Chng-Zhi-Xuan Jun 24, 2019 •

edited

Loading

Chng-Zhi-Xuan commented Jun 27, 2019 •

edited

Loading

Chng-Zhi-Xuan commented Jun 28, 2019 •

edited

Loading