Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pasting Multiple Shapes Onto Connection Results In Unexpected Connections #1440

Closed
philippfromme opened this issue Apr 30, 2021 · 5 comments · Fixed by #1442
Closed

Pasting Multiple Shapes Onto Connection Results In Unexpected Connections #1440

philippfromme opened this issue Apr 30, 2021 · 5 comments · Fixed by #1442
Assignees
Labels
bug Something isn't working

Comments

@philippfromme
Copy link
Contributor

Describe the Bug

J8DRVppIr9

Steps to Reproduce

  1. Copy two tasks
  2. Paste the tasks onto a sequence flow

Expected Behavior

The behavior doesn't kick in for multiple shapes or it connects them in a way that makes sense.

Environment

  • Library version: 8.3.1
@marstamm
Copy link
Contributor

marstamm commented Apr 30, 2021

Root-Cause:

The elements are added in order from left to right, but on the same Sequenz flow. Because of that, the second element comes first, see the gif below:
recording

Solution Ideas
I see two options:

  1. Add the Elements back-to-front.
  2. Determine the target-Flow again after each element is created

However, both have Problems:

  1. I have made a draft implementation in the linked diagram-js PR. However, reversing the order in which the elements are added in the CreateElementsHandler makes assumptions about the structure of the arguments hand feels like a hack.
    EDIT: This will actually break the Nested elements, such as Participants/Sub-Processes. We would probably have to change the Tree-destructuring from https://github.com/bpmn-io/diagram-js/blob/master/lib/features/copy-paste/CopyPaste.js#L296
  2. The mouse event targets only one flow, so we do not have references to the next target flow. I'm not 100% sure if this approach can be achieve this without a bigger refactoring.

@philippfromme , I'd be interested in your opinion on this. Would changing the order when copying the shapes (opposed to when creating the new ones) make this less hacky?

@philippfromme
Copy link
Contributor Author

philippfromme commented Apr 30, 2021

Honestly, I think we should disable this behavior when more than one shape is created. We simply cannot know what connections to create if we are creating multiple shapes at the same time. The behavior can also easily blow up completely:

yX9DFnWuCq

The behavior was created at a time when only single elements could be created.

@philippfromme
Copy link
Contributor Author

Moving the elements is rejected so pasting should be rejected, too:

O0Lj6CJnIq

@marstamm
Copy link
Contributor

That sounds reasonable, I'll see if we can reuse logic from movement for pasting as well

marstamm added a commit that referenced this issue May 3, 2021
@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label May 3, 2021
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels May 3, 2021
marstamm added a commit that referenced this issue May 4, 2021
marstamm added a commit that referenced this issue May 4, 2021
fake-join bot pushed a commit that referenced this issue May 4, 2021
@pinussilvestrus
Copy link
Contributor

Closed via b56604d

@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging a pull request may close this issue.

3 participants