feat: allow text annotations for message flows

[comlit]

added ability to add a text annotation to a message flow

closes #2281

[CLAassistant]

All committers have signed the CLA.

[barmac]

Thanks for the contribution. Could you please add tests for the solution? Best if they are next to the sequence flow + text annotation tests https://github.com/bpmn-io/bpmn-js/pull/1652/files#diff-e392ba8806c6b6cc243eb595499dc53b20b902520100055ffd344331d3be71a0

[comlit]

@barmac while creating the tests i found no way to actually see the models created by the tests to visually check them. Could you please explain or link to a guide on how to run a single test to be able to debug it?

[barmac]

You can use mocha's it.only (cf. https://mochajs.org/#exclusive-tests).
For test run in watch mode, do npm run dev.
Try out bpmn-js in a complete setup via npm start (remember to remove your it.only before running or there will be multiple tests running in the browser).

[sombrek]

@comlit Also have a look at

bpmn-js/test/spec/features/context-pad/ContextPadProviderSpec.js

Line 381 in 102f21d

it('should provide SequenceFlow entries', inject(function() {

I imagine a similar test is required for message flow entries.

[comlit]

@barmac thanks for the directions. I added the tests for the messageflow next to the ones for the sequenceflow in the latest commits. While adding the tests two questions came up.

1. When autoplacing elements, whether elements are placed horizontally or vertically (to my understanding), is decided by the verticality of the pool they are in. Since Messageflows exist outside of pools, how should that be handled?
2. Should there be an additional test for the LayoutConnectionBehaviour for Messageflows and if so should it be placed in LayoutConnectionBehaviourSpec.js or MessageflowBehaviourSpec.js?

[barmac]

When autoplacing elements, whether elements are placed horizontally or vertically (to my understanding), is decided by the verticality of the pool they are in. Since Messageflows exist outside of pools, how should that be handled?

I think we can safely assume that a text annotation is placed top-right from the anchor point (so the horizontal pool way). Users can always move it, and we avoid surprises.

Should there be an additional test for the LayoutConnectionBehaviour for Messageflows and if so should it be placed in LayoutConnectionBehaviourSpec.js or MessageflowBehaviourSpec.js?

Place it in LayoutConnectionBehaviorSpec as the behavior deals with associations attached to connections, so both sequence flows and message flows.

[comlit]

While creating the LayoutConnectionBehaviour test I noticed another problem. When importing a diagram, the associations between MessageFlows and their connected TextAnnotations are not rendered correctly (well technically not rendered at all). I attached the error Message and the file it occurs on below. It would assume that this has something to do with the order in which the elements are rendered, but found no approach on how to change that behaviour.

Error: element <bpmn:MessageFlow id="Flow_0guy1ye" /> referenced by <bpmn:Association id="Association_0yq399j" />#sourceRef not yet drawn
    at notYetDrawn (webpack-internal:///./lib/import/BpmnImporter.js:69:10)
    at BpmnImporter._getConnectedElement (webpack-internal:///./lib/import/BpmnImporter.js:325:11)
    at BpmnImporter._getTarget (webpack-internal:///./lib/import/BpmnImporter.js:338:15)
    at BpmnImporter.add (webpack-internal:///./lib/import/BpmnImporter.js:179:23)
    at Object.element (webpack-internal:///./lib/import/Importer.js:62:25)
    at visit (webpack-internal:///./lib/import/BpmnTreeWalker.js:87:20)
    at visitIfDi (webpack-internal:///./lib/import/BpmnTreeWalker.js:97:38)
    at handleArtifact (webpack-internal:///./lib/import/BpmnTreeWalker.js:295:5)
    at eval (webpack-internal:///./lib/import/BpmnTreeWalker.js:303:11)
    at handleDeferred (webpack-internal:///./lib/import/BpmnTreeWalker.js:243:7)

minimal_test_xml.zip

[barmac]

I can reproduce the error. This is indeed a problem with the order of drawing the elements, so the fix would be placed somewhere in the import directory. So this issue is not really a quick fix.
If you want to continue working on this issue, I'd suggest to start with a minimal test case to work towards the solution. It looks that the importer is handling associations before the message flows because the error is thrown when it tries to draw association while message flow is not there yet. We may want to order rendering so that message flows are drawn before the artifacts.

Please let me know what your decision is.

[comlit]

Yes I would like to try fixing that. If I need further assistance or get stuck I will write again.

[comlit]

I have implemented a fix for the problem that occured because of the rendering order. Key is - as you said - to handle messageflows before handling the associations. My solution does that. But I think it is kinda hacky and misplaced right now. I have pushed my solution as a proof of concept and would now like to ask you for some guidance as to how to implement it in a way that is not that scuffed.

[barmac]

Hi,

Great to see you back :)

Let's consider what the code is doing. We walk the BPMN tree in a specific order. We render most of the elements right away, but some of them are pushed to the deferred queue. Why is that? That's because they cannot be rendered until another element related to them is rendered (attachment after host, connection after connected shapes).

I think your solution proposal is heading into the right direction. Below there is a suggestion how we can further simplify it.

I noticed that in your solution sketch you filter out the associations to handle them separately. However, note that the already existing code defers associations rendering. So perhaps we can skip the rendering, and simply handle the artifacts after the message flows have been pushed to the end of the queue. Have you considered that?

Still, before you adjust the solution, I suggest you to add a test case verifying (1) that the bug exists before your changes, and (2) that your solution fixes the problem. You can use this importer integration test as an example. We need a test anyway to merge the PR; it's part of our definition of done (cf. contributing guide)).

[comlit]

yeah you are right. Just switching the two statements works. I remember trying that earlier and it failing, but that apparently was a different unrelated issue. I also added and fixed all other tests to the point that I think I got all of them. The only thing I am not quite sure about is the placement of the tests I added.

[barmac]

I was able to test the solution, and it works as supposed. It's sufficiently tested. I'll run the CI with this now, and after ✅, I will merge it. Thank you so much for your contribution.