[FEAT] Add overlays on Edge #1207

aibcmars · 2021-03-29T14:20:25Z

Closes #1166
Closes #1168

github-actions · 2021-03-29T14:20:45Z

♻️ PR Preview 7814873 has been successfully destroyed since this PR has been closed.

_{🤖 By surge-preview}

github-actions · 2021-03-29T14:20:46Z

♻️ PR Preview 7814873 has been successfully destroyed since this PR has been closed.

_{🤖 By surge-preview}

…mentation Don't check verticalAlign as we only need 3 positions. This reduces the method complexity.

…n shape

in integration and e2e tests

…unds

…hold

Use a dedicated msg flow diagram Based on the existing one with complex path but without labels an with envelope icons.

Use array of ids and positions

tbouffard · 2021-03-30T15:21:14Z

Important note about overlays on message flows

This may also apply to other Edges.

Some visualization test snapshots show overlays put on the middle of message flows rendered on the source (aka start point) of the message flows. The coordinates are correctly computed.
Depending on the zoom factor, the overlay is displayed at the right place or not.
This may be related to other issues we have with message flows, in particular, the Message/Sequence Flow start and end points are sometimes not at the right position. See #994

aibcmars · 2021-03-30T15:31:13Z

in addition to what @tbouffard wrote above I found that:

The path itself is changed during the scaling action.

The overlay is well centered while the path attribute value is:
M 464.4 369.51 L 250.07 369.51

and positioned badly(starting point) while path attribute value is:
M 526.1 377 L 526.55 377 Q 527 377 525.58 377 L 387.75 377

Q part that appears in the path is to draw quadratic Bezier curve
(check more about Q here: https://css-tricks.com/svg-path-syntax-illustrated-guide)

Further investigation must be done to find out what happens under the hood of mxgraph.

…clarity

csouchet · 2021-04-01T08:12:59Z

src/component/mxgraph/overlay/custom-overlay.ts

+    const pts = state.absolutePoints;
+    // 1st point for start position
+    if (this.align == mxgraph.mxConstants.ALIGN_LEFT) {
+      return pts[0];


I won't position the overlay on the first point of the edge, because it looks like the overlay is on the shape.
But rather 5% from the start, like I did in my POC: https://github.com/process-analytics/bpmn-visualization-js/pull/1118/files#diff-45741ef0472aa3227232cd7aa01f962ea65e42f186107a274fb2ad868a7d707dR58

Indeed, this will be better but this won't cover all use cases. Depending on the size of the text put in the overlay, the fixed size 5% may not be enough.
I suggest we think about it quietly (not in a rush right now) and provide a dedicated feature for this, and why not letting integration pass an offset values or a percentage

To do in #1169

csouchet · 2021-04-01T08:14:46Z

src/component/mxgraph/overlay/custom-overlay.ts

+    }
+    // last point for end position
+    else {
+      return pts[pts.length - 1];


Same comment.

I won't position the overlay on the last point of the edge (and after the arrow), because it looks like the overlay is on the shape.
But rather 5% from the end (before the arrow), like I did in my POC.

csouchet · 2021-04-01T08:18:23Z

test/e2e/diagram.overlays.rendering.test.ts

+  const bpmnDiagramNameAssociations = 'overlays.edges.associations.complex.paths';
+  const bpmnDiagramNameMessageFlows = 'overlays.edges.message.flows.complex.paths';
+  const bpmnDiagramNameSequenceFlows = 'overlays.edges.sequence.flows.complex.paths';
+  function getEdgeKindAndIds(bpmnDiagramName: string): [string, string[]] {


I would rather have made a map with the different values instead.

csouchet · 2021-04-01T08:22:03Z

test/e2e/diagram.overlays.rendering.test.ts

+    }
+    return [edgeKind, ids];
+  }
+  describe.each([bpmnDiagramNameSequenceFlows, bpmnDiagramNameAssociations, bpmnDiagramNameMessageFlows])('diagram %s', (bpmnDiagramName: string) => {


Or directly give the diagram file + the edge kind + the edges id as an array of array. It's more readable to know what we test.

ok I will check this solution too.

I choose this solution, it is clearer. See 91d206b

csouchet · 2021-04-01T08:26:52Z

test/helpers/overlays.ts

+
+export const overlayEdgePositionValues = <Array<OverlayEdgePosition>>['start', 'middle', 'end'];
+
+export const overlayShapePositionValues = <Array<OverlayShapePosition>>[


I think to avoid to have this method, and to find all the value manually for the HTML page, it's better to have an enum like FitType.

With enum: https://github.com/process-analytics/bpmn-visualization-js/blob/master/src/static/js/rendering-diagram.js#L155-L157

Without enum: https://github.com/process-analytics/bpmn-visualization-js/blob/master/src/static/js/overlays.js#L135-L145

This function is only used to avoid duplication in tests. Integration won't probably need to put an overlay on all positions for real use cases. So to me this is not an issue.
We already discussed about the drawbacks of using enums. I don't remember exactly why we decided to put a string type union here. For pros/cons, we can check https://2ality.com/2020/02/enum-alternatives-typescript.html#type-unions-vs.-enums
Anyway, if you think this change is valuable, we can think about it but not in this PR IMHO: it is already quite big and this change would break the API as well.

… instead)

sonarqubecloud · 2021-04-01T10:04:57Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

aibcmars added 2 commits March 29, 2021 16:12

add positioning for edges

5413e4f

uncomment tests: positioning for edges

aee248e

aibcmars added BPMN diagram usability Something about the way we can interact with BPMN diagrams mvp Formerly used for bpmn-visualization MVP issues/PRs labels Mar 29, 2021

aibcmars added the WIP 🚧 Pull request in progress and/or not ready for review. Used in addition to marking it as draft label Mar 29, 2021

aibcmars and others added 23 commits March 29, 2021 16:39

tests on association ?

c75db7f

tests on association ?

75f5b81

tests on association ?

b5a0d5d

Custom overlay: add integration tests + simplify getBounds edge imple…

964c4af

…mentation Don't check verticalAlign as we only need 3 positions. This reduces the method complexity.

shape integration tests: ensure no issue with position for overlays o…

5b0c7db

…n shape

edge integration tests: also test flow with waypoints

8cb11dc

refactor tests: simplify overlays position values retrieval

d691759

in integration and e2e tests

Rename e2e tests and snapshot images

a1fd72b

Add linux e2e diff image threshold

f5eb12a

refactor: reduce 'Cognitive Complexity' in MxGraphCustomOverlay.getBo…

43dbdcf

…unds

tests on association more paths

ac6d62f

tests on association more paths

b50989d

tests on association more paths

7a58f62

update overlays.edges.associations.complex.paths linux chromium thres…

4b0d14a

…hold

Visu test: add overlays on message flows

c791337

Use a dedicated msg flow diagram Based on the existing one with complex path but without labels an with envelope icons.

Visu test: shape and association - add missing 'customDiffDir' config

46e6d85

Visu test: shape and association - add overlays by passing arrays

f80b37d

Use array of ids and positions

Visu test - refactor: rename 'addOverlay' into 'addOverlays'

739727f

Visu test: remove overlay of message flow

16c4b6b

tests on sequence

7255a4f

tests on sequence

9065d9f

tests on message flow mutualization

d24a81f

tests on message flow mutualization

5dca003

tests segregation

fa7c2b0

tbouffard and others added 3 commits March 30, 2021 18:02

Set visu test thresholds for linux (chromium and firefox)

bda1d7f

refactor: rename the sequence flows bpmn diagram for consistency and …

2697243

…clarity

change output snapshots files' names

aa8fa0d

csouchet mentioned this pull request Mar 31, 2021

[EXAMPLE] Add example to add & remove all overlays on Edges on BPMN Diagram process-analytics/bpmn-visualization-examples#158

Merged

aibcmars added 8 commits March 31, 2021 11:09

thresholds firefox

a5986c2

thresholds chromium

d00f9b4

thresholds

4d2de7b

thresholds

af7d420

thresholds

cbbe0d4

thresholds

7b43d9f

thresholds

245a811

thresholds

5fe82a1

aibcmars marked this pull request as ready for review March 31, 2021 14:35

aibcmars added 2 commits March 31, 2021 16:36

Merge branch 'master' into 1166-overlays_on_edge

2428717

import fix

c745eca

tbouffard removed the WIP 🚧 Pull request in progress and/or not ready for review. Used in addition to marking it as draft label Mar 31, 2021

csouchet reviewed Apr 1, 2021

View reviewed changes

refactor test: simplify edges input data (no function, use each table…

91d206b

… instead)

csouchet approved these changes Apr 1, 2021

View reviewed changes

Merge branch 'master' into 1166-overlays_on_edge

7814873

tbouffard merged commit 99e7360 into master Apr 1, 2021

tbouffard deleted the 1166-overlays_on_edge branch April 1, 2021 11:50

tbouffard mentioned this pull request Apr 1, 2021

[FEAT] Improve the overlay positions on edge #1169

Open

tbouffard mentioned this pull request Apr 15, 2021

[BUG] The 'middle' position for the overlay is not the same after zoom than after fit #1236

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEAT] Add overlays on Edge #1207

[FEAT] Add overlays on Edge #1207

aibcmars commented Mar 29, 2021 •

edited by csouchet

Loading

github-actions bot commented Mar 29, 2021 •

edited

Loading

github-actions bot commented Mar 29, 2021 •

edited

Loading

tbouffard commented Mar 30, 2021

aibcmars commented Mar 30, 2021

csouchet Apr 1, 2021

tbouffard Apr 1, 2021

csouchet Apr 1, 2021

csouchet Apr 1, 2021

csouchet Apr 1, 2021

tbouffard Apr 1, 2021

csouchet Apr 1, 2021

tbouffard Apr 1, 2021

tbouffard Apr 1, 2021

csouchet Apr 1, 2021

tbouffard Apr 1, 2021

sonarqubecloud bot commented Apr 1, 2021


		export const overlayEdgePositionValues = <Array<OverlayEdgePosition>>['start', 'middle', 'end'];

		export const overlayShapePositionValues = <Array<OverlayShapePosition>>[

[FEAT] Add overlays on Edge #1207

[FEAT] Add overlays on Edge #1207

Conversation

aibcmars commented Mar 29, 2021 • edited by csouchet Loading

github-actions bot commented Mar 29, 2021 • edited Loading

github-actions bot commented Mar 29, 2021 • edited Loading

tbouffard commented Mar 30, 2021

Important note about overlays on message flows

aibcmars commented Mar 30, 2021

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

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

sonarqubecloud bot commented Apr 1, 2021

aibcmars commented Mar 29, 2021 •

edited by csouchet

Loading

github-actions bot commented Mar 29, 2021 •

edited

Loading

github-actions bot commented Mar 29, 2021 •

edited

Loading