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

Clone on copy #1707

Merged
merged 14 commits into from
Aug 16, 2022
Merged

Clone on copy #1707

merged 14 commits into from
Aug 16, 2022

Conversation

nikku
Copy link
Member

@nikku nikku commented Aug 11, 2022

This fixes a fundamental flaw in our copy mechanism.

Until today we would carry around a mutable copy of oldBusinessObject and oldDi that would change what has been copied if the copied element changes after the copy operation:

Copy <Task> with name <OLD>
Change <Task.name> property to <FOO>
Paste <Task'>
<Task'.name> is <FOO> despite it having name <OLD> at the time of copy

Unfortunately the PR got a little bigger than intended, fixing numerous small issues and addition additional test coverage to test copy + paste end-to-end.

Contributions

  • fixes the issue by cloning each element as we copy. As a result what is copied is frozen at copy time and not affected by future changes anymore.
  • fixes that paste behaviors would execute when pasting both labels and actual shapes (fac5f92, verified via 076bb35)
  • fix bogus meta-date being attached to shapes on paste (1f40b65)
  • adds a bunch of test cases to verify paste works in the way intended:
    • keeping IDs where possible, working with serialization (076bb35)
    • verify collapsed sub-process handling (63e6e2e)

Closes #798
Closes #1708

@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Aug 11, 2022
@nikku nikku force-pushed the clone-on-copy branch 4 times, most recently from b07db46 to e6f448d Compare August 11, 2022 22:38
Clone will copy elements in a side-effect free manner, i.e. not perform any wiring.
test/spec/ModelerSpec.js Outdated Show resolved Hide resolved
Ensures copy is unaffected by edit operations after copy.

Closes #798
@nikku
Copy link
Member Author

nikku commented Aug 15, 2022

Updated as discussed.

Using updateModdleProperties did not work as it gives me now way to wire parent child relationships at the moment. We'd need at least the ability to set $parent as a special prop.

@nikku
Copy link
Member Author

nikku commented Aug 15, 2022

To be released as a new major version.

@philippfromme
Copy link
Contributor

Using updateModdleProperties did not work as it gives me now way to wire parent child relationships at the moment. We'd need at least the ability to set $parent as a special prop.

You mean for adding root elements? Then we'll add this ability in the future.

Copy link
Contributor

@philippfromme philippfromme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should consider fixing this.

lib/features/copy-paste/BpmnCopyPaste.js Outdated Show resolved Hide resolved
@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Aug 16, 2022
@philippfromme philippfromme self-requested a review August 16, 2022 13:13
Copy link
Contributor

@philippfromme philippfromme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fake-join fake-join bot merged commit f0093cd into develop Aug 16, 2022
@fake-join fake-join bot deleted the clone-on-copy branch August 16, 2022 13:14
@bpmn-io-tasks bpmn-io-tasks bot removed the in progress Currently worked on label Aug 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clean up BPMN moddle copy and paste Copy & paste: I can mutate the copied business object after copying
2 participants