feat: add ghost elements to subprocess view

[marstamm]

Closes #1484
Closes #1511

[marstamm]

Results from Sync

• make color configurable
• Try decoupeling flow rendering from renderer --> render flows before the bpmn renderer is called
• Make sure moving the element has intuitive behavior --> moved to Handle collapsed sub-processes #1443

[marstamm]

Created bpmn-io/diagram-js#579 and #1515 to implement the changes needed in the renderer.

[andreasgeier]

@marstamm The last time we synced over the sub-process's boundary and its "ghost" connectors, we weren't sure if this is the best approach or if we should just display the sub-process's content without boundary and context at all.

Seeing now the implementation, the visual complexity becomes clear, in particular with boundary events and multiple greyed connector lines.

But there are also arguments in favor of this solution. So, since you already implemented everything, let's push that out of the door and learn from the feedback.

Appproved.

[philippfromme]

✅ Approved from my side. Let's wait for final approval by @nikku if that's okay.

[marstamm]

Sure, sounds good to me. This is not blocking anything at the moment 👍

[marstamm]

I have to request changes myself, as I found a bug 🙈

When the Process has a legacy collapsed subprocess (no separate plane in the XML), we don't show any ghost elements. Because the breadcrumbs are attached to the expanded shape, the navigation breaks as well

[marstamm]

@philippfromme and I discussed some potential solutions for this problem, but could not come to a clear conclusion. All options have their own pros and cons.

@nikku, it would be great to hear your opinion on this. I sketched the solutions below:

Problem
We create the ghost elements during import. Because legacy Processes do not have their own di:plane, expanded shapes are not added.
In the SubprocessCompatibility.js, they are moved to their own di:plane, but the secondary shapes are not added

Option 1
Adjust SubprocessCompatibility.js to add the secondary shapes.

Pro	Con
Easy fix for edge cases	Bad Maintainability
encapsulated in SubprocessCompatibility.js	Potential Code Duplication

Option 2
Transform legacy processes on the moddle level before import

Pro	Con
Consistent import behavior	We have to parse the moddle twice (first to find potential legacy subprocesses, second during import in the BpmnTreeWalker)
Legacy and new Diagrams behave the same afterwards	We have to do it with every diagram, even if it does not contain legacy subprocesses
finds and solves the issues early
Could be done as a plugin, similar to signavio-compat

Option 3
We import diagrams as planes only and add the ghost elements in a module

Pro	Con
clean scoping of subprocess functionality outside of import	Complex fix, a lot of moving elements around
legacy behavior continues to work as originally implemented	Many layers in an import: (normal import -> moving legacy elements into new plane -> moving sub-process elements into new ghost element -> adding boundary events

[nikku]

I'd go for Option 2️⃣ and hook into the import.render.start event to perform the modifications.

Under the assumption of course that doing the adjustments is cheap. The only alternative I see is lazily expanding (and importing) collapsed sub-processes and doing the fixes there. That is the way to mitigate the impact without affecting all users.

[nikku]

To sum up our little chat earlier:

• I believe the implementation is sufficient for now, going for Option 2️⃣, essentially drastically simplifying the SubprocessCompatibility util would be my suggestion
• There is a couple of small things you can improve with regards to code style:
  • Early returns
  • (Basic) documentation
  • // TODO(marstamm): Do this some day, maybe if you want to leave your legacy in the code base
• Overall naming of the subprocess-navigation components: Is that thing actual subprocess navigation or drill down?
• What do individual components do? Specifically, what is the distinction between Subprocess Elements and Flows component? If both draw Fake or outline or Ghost elements, let us call the component SubprocessFakeBoundaries (and Flows) or the like.
• If we are not able to make everything modular (which is absolutely fine!) consider adding a comprehensive comment that establishes the relevant context (talking about BpmnImporter#add here, adding the subprocess boxes).

[philippfromme]

I'd go for Option 2️⃣ and hook into the import.render.start event to perform the modifications.

Same option I'd go for. Glad we're all on the same page.

[marstamm]

I added the discussed changes and followed the styling hints to the best of my ability. Ready to be reviewed again 🙂

[marstamm]

@andreasgeier, sorry, did not want to request review from you again. The button slipped under my mouse when the UI updated 😅

[marstamm]

As discussed in the Sync Today:

• We will not ship ghost elements for now
• The costs do not justify the benefits
• We will continue with the state on develop
• Fixes in this PR (import, documentation, ...) will be merged to develop with Drilldown documentation and fixes #1521