Move multi selection outline into separate feature

[misiekhardcore]

Proposed Changes

This PR moves multi selection outline logic from `selection` to `outline`. As a result selection is a non-UI component, and outline builds (optionally) on top of it. Closes #944 ### Checklist To ensure you provided everything we need to look at your PR: * [x] **Brief textual description** of the changes present * [ ] **Visual demo** attached * [ ] **Steps to try out** present, i.e. [using the `@bpmn-io/sr` tool](https://github.com/bpmn-io/sr) * [x] Related issue linked via `Closes {LINK_TO_ISSUE}` or `Related to {LINK_TO_ISSUE}`

[nikku]

Please add an additional housekeeping PR to bump puppeteer, otherwise CI will continue to fail with this.

[nikku]

@misiekhardcore Please clearly elaborate on the rational (the what and why, along these lines) here, as this does not link to a diagram-js issue.

I.e. clearly separate UI and non-UI concerns. Viewer is not featuring outline, consistently.

[nikku]

What we could consider (not sure if valuable) is to make selection an optional dependency:

function MultiSelectionOutline(injector) {

  const selection = injector.get('selection', false);

  if (!selection) {
    // i'm a no-op
  }
}

(As a user rolling your own diagram-js powered editor you want to include both features anyway).

[misiekhardcore]

Regarding making the selection module optional, I think it will be a bit misleading - the MultiSelectionOutline, even from its name, suggests it is closely related to the selection. I think its better to require user to use both. WDYT?

Am I thinking correctly that most of the tests from SelectionVisualSpec should now be moved to MultiSelectionOutlineSpec? I think about these

[nikku]

Am I thinking correctly that most of the tests from SelectionVisualSpec should now be moved to MultiSelectionOutlineSpec?

Yes, all of these should be moved. You can verify that the implementation is correct:

• Run these specs (with only selection module) and they'll fail.
• Run these specs (describe.only) with the outline module and they shall succeeed.

I think its better to require user to use both. WDYT?

Fine to keep it a hard dependency!

[nikku]

To be done: Add CHANGELOG entries; consider how to market this, is it a breaking change or a bug fix?

[misiekhardcore]

I think this is a breaking change as some users might rely on Selection feature showing visual outline around elements which won't work anymore with this change. Should we add this kind of info to the bpmn-js changelog as well (https://github.com/bpmn-io/bpmn-js/pull/2253)

[nikku]

I think this is a breaking change as some users might rely on Selection feature showing visual outline around elements which won't work anymore with this change.

Agreed! Let's batch the change with https://github.com/bpmn-io/diagram-js/pull/662 then. A good time to change things up.

Should we add this kind of info to the bpmn-js changelog as well (https://github.com/bpmn-io/bpmn-js/pull/2253)

That depends if this affects user land (bpmn-js users). Do you think that is the case? In this case, bpmn-js should also be major-bumped. We do already have breaking changes in bpmn-js in the pipeline, so again, a good time to incorporate that additional breaking change, too.

---

Generally breaks should not accidentally destroy our users flows; we try to prevent breaking changes, and where they have wide ranging effects warn users (have migration hints built-in, cf. https://github.com/bpmn-io/diagram-js/pull/662). In the case of this change you can argue it was always broken from the start (not clear separation of concerns) and users are very unlikely to just use outline without selection.

[nikku]

Let's not mark this as refactor but feat (a feature), and we're good to go (to merge this).

[misiekhardcore]

It really depends what "Breaking" mean. In this case I would consider the change done in https://github.com/bpmn-io/bpmn-js/pull/2253 as non breaking, but as a fix for the reasons you mentioned, so user is really unlikely to use outline without selection, and if they use selection without outline, they simply wont have visual indication of the selected element, which is not breaking anything in the flow. I will go ahead and add a changelog entry mentioning https://github.com/bpmn-io/bpmn-js/pull/2253 as a fix