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

fix: set loop characteristics in a command #1970

Merged
merged 1 commit into from
Oct 12, 2023

Conversation

barmac
Copy link
Member

@barmac barmac commented Oct 10, 2023

Closes #1960

@barmac barmac requested review from a team, philippfromme and marstamm and removed request for a team October 10, 2023 16:38
@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Oct 10, 2023
@barmac barmac force-pushed the 1960-fix-undo-redo-on-multiinstance branch from d61ecae to 1f6a25e Compare October 10, 2023 16:51
if (entry.active) {
newLoopCharacteristics = undefined;
Copy link
Member

Choose a reason for hiding this comment

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

🙃

}

// update `isSequential` property
newLoopCharacteristics.set('isSequential', entry.options.isSequential);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if we need to copy the full loop characteristics here. As long as we properly undo or redo. What about using Modeling#updateModdleProperty to simply change isSequential to the target value?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could if there was only a single object type assigned to loopCharacteristics. However, this can be one of two:

  • bpmn:MultiInstanceLoopCharacteristics with property isSequential, or
  • bpmn:StandardLoopCharacteristics without properties.

Both of them can have Camunda properties which need to be copied if we change the type of the object.

Copy link
Member Author

Choose a reason for hiding this comment

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

Related to #1679

Copy link
Member Author

Choose a reason for hiding this comment

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

Or rather #1581

@barmac barmac force-pushed the 1960-fix-undo-redo-on-multiinstance branch from 1f6a25e to 32dc7a6 Compare October 11, 2023 11:52
Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

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

👍

@nikku nikku merged commit 1585775 into master Oct 12, 2023
12 checks passed
@nikku nikku deleted the 1960-fix-undo-redo-on-multiinstance branch October 12, 2023 09:24
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Oct 12, 2023
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.

2 participants