[FEAT] Render Collapsed Embedded Sub-Process #378

csouchet · 2020-07-01T15:35:21Z

Closes #305

Example with sub-process without label bounds

With BPMN.io:

Example with B.1.0 from migw-test-suite

tbouffard

The stroke of the cross is not large enough which make it render as gray when it is small. I will try to do some tests with largest values later
Some pending questions

src/component/mxgraph/shape/IconPainter.ts

tbouffard · 2020-07-02T12:45:56Z

src/component/mxgraph/extension/MxScaleFactorCanvas.ts

+
+  public static translateIconToShapeBottomCenter(canvas: mxgraph.mxXmlCanvas2D, shape: ShapeConfiguration, iconSize: Size): void {
+    const insetW = (shape.w - iconSize.width) / 2;
+    const insetH = shape.h - iconSize.height - 7;


❓ where does the -7 comes from? Is this something you found empirically when testing with various BPMN diagrams?

Yes, I just test with different values.

perhaps we can put it in some constant (TASK_SHAPE_BOTTOM_MARGIN)
I do not know If we would have other Flow elements with this plus icon?

I have also noticed that in BPMN2.0 spec icon is sticked to the bottom

The spec states

The Sub-Process marker MUST be a small square with a plus sign (+) inside. The square MUST be positioned at the bottom center of the shape.

A lot of BPMN vendor don't stick it to the bottom, so this seems valid

@aibcmars the other flow using this marker will be the call activity and we aim to reuse the current marker implementation

tbouffard · 2020-07-02T12:46:46Z

src/component/mxgraph/shape/activity-shapes.ts

+    if (StyleUtils.getBpmnIsExpanded(this.style) === 'false') {
+      const subProcessKind = StyleUtils.getBpmnSubProcessKind(this.style);
+      if (subProcessKind === ShapeBpmnSubProcessKind.EMBEDDED) {
+        const paintParameter = IconPainter.buildPaintParameter((c as unknown) as mxgraph.mxXmlCanvas2D, x, y, w, h, (this as unknown) as mxgraph.mxShape, 0.17, false, 1.5);


❓ same question about the 0.17 and 1.5 magic numbers

Yes, I just test with different values.

magic numbers for ratios are OK I think - it is more problem of the conversion from SVG -> mxGraph
(however as I remember the ratio params were somewhere in the XML file after generation)
perhaps just adding a comment from where they come can be sufficient

I calculate myself these numbers, so there is no comment to add.

Co-authored-by: Thomas Bouffard <27200110+tbouffard@users.noreply.github.com>

tbouffard · 2020-07-02T15:42:57Z

@csouchet I give a try with a stroke of 3 on commit bcab1e2 and here are the comparison with a stroke of 2

all_activity_types.bpmn

overview

zoom on the sub-process

B.1.0

csouchet · 2020-07-02T15:55:27Z

@csouchet I give a try with a stroke of 3 on commit bcab1e and here are the comparison with a stroke of 2

I think 3 is too big. Maybe, a value between 2 and 3 is better.

tbouffard · 2020-07-02T16:10:50Z

@csouchet new comparison with B.1.0, stroke 2, 2.5 and 3

tbouffard · 2020-07-03T09:59:42Z

With stroke of 2 (what we have in bcab1e2) and not rounded. I guess this this the rounded part which adds some fuzziness.
I prefer this configuration: rounded for the outer rectangle, cross not rounded

NathalieC · 2020-07-03T13:27:25Z

ok for 2 with "not rounded" option

aibcmars · 2020-07-02T15:33:30Z

src/component/mxgraph/extension/MxScaleFactorCanvas.ts

+
+  public static translateIconToShapeBottomCenter(canvas: mxgraph.mxXmlCanvas2D, shape: ShapeConfiguration, iconSize: Size): void {
+    const insetW = (shape.w - iconSize.width) / 2;
+    const insetH = shape.h - iconSize.height - 7;


perhaps we can put it in some constant (TASK_SHAPE_BOTTOM_MARGIN)
I do not know If we would have other Flow elements with this plus icon?

aibcmars · 2020-07-02T15:34:00Z

src/component/mxgraph/extension/MxScaleFactorCanvas.ts

+
+  public static translateIconToShapeBottomCenter(canvas: mxgraph.mxXmlCanvas2D, shape: ShapeConfiguration, iconSize: Size): void {
+    const insetW = (shape.w - iconSize.width) / 2;
+    const insetH = shape.h - iconSize.height - 7;


I have also noticed that in BPMN2.0 spec icon is sticked to the bottom

aibcmars · 2020-07-02T15:38:16Z

src/component/mxgraph/shape/activity-shapes.ts

+    if (StyleUtils.getBpmnIsExpanded(this.style) === 'false') {
+      const subProcessKind = StyleUtils.getBpmnSubProcessKind(this.style);
+      if (subProcessKind === ShapeBpmnSubProcessKind.EMBEDDED) {
+        const paintParameter = IconPainter.buildPaintParameter((c as unknown) as mxgraph.mxXmlCanvas2D, x, y, w, h, (this as unknown) as mxgraph.mxShape, 0.17, false, 1.5);


magic numbers for ratios are OK I think - it is more problem of the conversion from SVG -> mxGraph
(however as I remember the ratio params were somewhere in the XML file after generation)
perhaps just adding a comment from where they come can be sufficient

csouchet added dependencies Pull requests that update a dependency (dev or runtime) depends on another PR ⚠️ Pull request depending on another one. The depending must be merged first BPMN rendering Something about the way the lib is rendering BPMN elements labels Jul 1, 2020

csouchet changed the title ~~305 render collapsed embedded sub process~~ [FEAT] Render Collapsed Embedded Sub-Process Jul 1, 2020

csouchet marked this pull request as ready for review July 1, 2020 15:49

csouchet requested review from tbouffard and aibcmars and removed request for tbouffard July 1, 2020 15:49

csouchet force-pushed the 305-Render_Collapsed_Embedded_Sub-Process branch 2 times, most recently from a241bf9 to a6954b5 Compare July 1, 2020 16:23

csouchet mentioned this pull request Jul 1, 2020

[FEAT] Render Collapsed Event Sub-Process #379

Merged

csouchet added enhancement New feature or request and removed dependencies Pull requests that update a dependency (dev or runtime) labels Jul 2, 2020

csouchet force-pushed the 305-Render_Collapsed_Embedded_Sub-Process branch from a6954b5 to 5aa8371 Compare July 2, 2020 08:31

tbouffard added the rebase needed 💥 Pull request that must be rebased on the latest master commit prior being reviewed or merged label Jul 2, 2020

tbouffard self-assigned this Jul 2, 2020

csouchet added 2 commits July 2, 2020 14:38

Render Collapsed Embedded sub-process

4b1726d

Fix lint-check

0c767e5

tbouffard force-pushed the 305-Render_Collapsed_Embedded_Sub-Process branch from 5aa8371 to 0c767e5 Compare July 2, 2020 12:42

tbouffard removed depends on another PR ⚠️ Pull request depending on another one. The depending must be merged first rebase needed 💥 Pull request that must be rebased on the latest master commit prior being reviewed or merged labels Jul 2, 2020

tbouffard requested changes Jul 2, 2020

View reviewed changes

csouchet and others added 3 commits July 2, 2020 15:02

Update src/component/mxgraph/shape/IconPainter.ts

1137225

Co-authored-by: Thomas Bouffard <27200110+tbouffard@users.noreply.github.com>

Merge branch 'master' into 305-Render_Collapsed_Embedded_Sub-Process

8ed6050

Merge branch 'master' into 305-Render_Collapsed_Embedded_Sub-Process

bcab1e2

With not rounded cross

7fcf4e0

csouchet requested a review from tbouffard July 6, 2020 08:03

aibcmars approved these changes Jul 6, 2020

View reviewed changes

tbouffard approved these changes Jul 6, 2020

View reviewed changes

Add constant for Shape margin

03ce42b

csouchet merged commit 1275615 into master Jul 6, 2020

csouchet deleted the 305-Render_Collapsed_Embedded_Sub-Process branch July 6, 2020 08:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEAT] Render Collapsed Embedded Sub-Process #378

[FEAT] Render Collapsed Embedded Sub-Process #378

csouchet commented Jul 1, 2020 •

edited by tbouffard

Loading

tbouffard left a comment

tbouffard Jul 2, 2020

csouchet Jul 2, 2020

aibcmars Jul 2, 2020

aibcmars Jul 2, 2020

tbouffard Jul 6, 2020 •

edited

Loading

tbouffard Jul 6, 2020

tbouffard Jul 2, 2020

csouchet Jul 2, 2020

aibcmars Jul 2, 2020

csouchet Jul 6, 2020

tbouffard commented Jul 2, 2020 •

edited

Loading

csouchet commented Jul 2, 2020 •

edited by tbouffard

Loading

tbouffard commented Jul 2, 2020

tbouffard commented Jul 3, 2020

NathalieC commented Jul 3, 2020

aibcmars Jul 2, 2020

aibcmars Jul 2, 2020

aibcmars Jul 2, 2020

[FEAT] Render Collapsed Embedded Sub-Process #378

[FEAT] Render Collapsed Embedded Sub-Process #378

Conversation

csouchet commented Jul 1, 2020 • edited by tbouffard Loading

Example with sub-process without label bounds

Example with B.1.0 from migw-test-suite

tbouffard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbouffard Jul 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbouffard commented Jul 2, 2020 • edited Loading

B.1.0

csouchet commented Jul 2, 2020 • edited by tbouffard Loading

tbouffard commented Jul 2, 2020

tbouffard commented Jul 3, 2020

NathalieC commented Jul 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csouchet commented Jul 1, 2020 •

edited by tbouffard

Loading

tbouffard Jul 6, 2020 •

edited

Loading

tbouffard commented Jul 2, 2020 •

edited

Loading

csouchet commented Jul 2, 2020 •

edited by tbouffard

Loading