ljmotta commented on code in PR #3580:
URL:
https://github.com/apache/incubator-kie-tools/pull/3580#discussion_r3289201301
##########
packages/bpmn-editor/src/mutations/moveNodesInsideSubProcess.ts:
##########
@@ -51,37 +47,54 @@ export function moveNodesInsideSubProcess({
>[] = [];
const nodeIdsToMoveInside = new Set(__readonly_nodeIds);
- const subProcessNodes = new Set();
+ const subProcessNodes = new Set<string>();
subProcess.flowElement?.forEach((flowElement) => {
- if (flowElement.__$$element !== "sequenceFlow") {
+ if (flowElement.__$$element !== "sequenceFlow" && flowElement["@_id"]) {
subProcessNodes.add(flowElement["@_id"]);
}
});
- for (let i = 0; i < (process.flowElement ?? []).length; i++) {
- const flowElement = (process.flowElement ?? [])[i];
- if (
- nodeIdsToMoveInside.has(flowElement["@_id"]) ||
- (flowElement.__$$element === "boundaryEvent" &&
nodeIdsToMoveInside.has(flowElement["@_attachedToRef"]))
- ) {
- flowElementsToMove.push(...((process.flowElement?.splice(i, 1) ?? []) as
typeof flowElementsToMove));
- i--; // repeat one index because we just altered the array we're
iterating over.
- } else if (
+ const toMove: Unpacked<NonNullable<BPMN20__tProcess["flowElement"]>>[] = [];
+
+ function shouldMoveSequenceFlow(
+ flowElement: Unpacked<NonNullable<BPMN20__tProcess["flowElement"]>>,
+ nodeIds: Set<string>,
+ existingNodes: Set<string>
+ ): boolean {
+ return (
flowElement.__$$element === "sequenceFlow" &&
- ((nodeIdsToMoveInside.has(flowElement["@_sourceRef"]) &&
nodeIdsToMoveInside.has(flowElement["@_targetRef"])) ||
- (subProcessNodes.has(flowElement["@_sourceRef"]) &&
nodeIdsToMoveInside.has(flowElement["@_targetRef"])) ||
- (nodeIdsToMoveInside.has(flowElement["@_sourceRef"]) &&
subProcessNodes.has(flowElement["@_targetRef"])))
- ) {
- // If the source and target are both outside of the sub-process
- // or if the source and target is already in the sub process the
sequenceFlow must be copied
- flowElementsToMove.push(...((process.flowElement?.splice(i, 1) ?? []) as
typeof flowElementsToMove));
- i--; // repeat one index because we just altered the array we're
iterating over.
- }
+ !!flowElement["@_sourceRef"] &&
+ !!flowElement["@_targetRef"] &&
+ ((nodeIds.has(flowElement["@_sourceRef"]) &&
nodeIds.has(flowElement["@_targetRef"])) ||
+ (existingNodes.has(flowElement["@_sourceRef"]) &&
nodeIds.has(flowElement["@_targetRef"])) ||
+ (nodeIds.has(flowElement["@_sourceRef"]) &&
existingNodes.has(flowElement["@_targetRef"])))
+ );
}
+ const collectElements = (flowElements:
NonNullable<BPMN20__tProcess["flowElement"]>): void => {
+ for (let i = flowElements.length - 1; i >= 0; i--) {
+ const flowElement = flowElements[i];
+ if (
+ (flowElement["@_id"] && nodeIdsToMoveInside.has(flowElement["@_id"]))
||
Review Comment:
Same here.
##########
packages/bpmn-editor/src/mutations/moveNodesInsideSubProcess.ts:
##########
@@ -51,37 +47,54 @@ export function moveNodesInsideSubProcess({
>[] = [];
const nodeIdsToMoveInside = new Set(__readonly_nodeIds);
- const subProcessNodes = new Set();
+ const subProcessNodes = new Set<string>();
subProcess.flowElement?.forEach((flowElement) => {
- if (flowElement.__$$element !== "sequenceFlow") {
+ if (flowElement.__$$element !== "sequenceFlow" && flowElement["@_id"]) {
subProcessNodes.add(flowElement["@_id"]);
}
});
- for (let i = 0; i < (process.flowElement ?? []).length; i++) {
- const flowElement = (process.flowElement ?? [])[i];
- if (
- nodeIdsToMoveInside.has(flowElement["@_id"]) ||
- (flowElement.__$$element === "boundaryEvent" &&
nodeIdsToMoveInside.has(flowElement["@_attachedToRef"]))
- ) {
- flowElementsToMove.push(...((process.flowElement?.splice(i, 1) ?? []) as
typeof flowElementsToMove));
- i--; // repeat one index because we just altered the array we're
iterating over.
- } else if (
+ const toMove: Unpacked<NonNullable<BPMN20__tProcess["flowElement"]>>[] = [];
+
+ function shouldMoveSequenceFlow(
+ flowElement: Unpacked<NonNullable<BPMN20__tProcess["flowElement"]>>,
+ nodeIds: Set<string>,
+ existingNodes: Set<string>
+ ): boolean {
+ return (
flowElement.__$$element === "sequenceFlow" &&
- ((nodeIdsToMoveInside.has(flowElement["@_sourceRef"]) &&
nodeIdsToMoveInside.has(flowElement["@_targetRef"])) ||
- (subProcessNodes.has(flowElement["@_sourceRef"]) &&
nodeIdsToMoveInside.has(flowElement["@_targetRef"])) ||
- (nodeIdsToMoveInside.has(flowElement["@_sourceRef"]) &&
subProcessNodes.has(flowElement["@_targetRef"])))
- ) {
- // If the source and target are both outside of the sub-process
- // or if the source and target is already in the sub process the
sequenceFlow must be copied
- flowElementsToMove.push(...((process.flowElement?.splice(i, 1) ?? []) as
typeof flowElementsToMove));
- i--; // repeat one index because we just altered the array we're
iterating over.
- }
+ !!flowElement["@_sourceRef"] &&
+ !!flowElement["@_targetRef"] &&
+ ((nodeIds.has(flowElement["@_sourceRef"]) &&
nodeIds.has(flowElement["@_targetRef"])) ||
+ (existingNodes.has(flowElement["@_sourceRef"]) &&
nodeIds.has(flowElement["@_targetRef"])) ||
+ (nodeIds.has(flowElement["@_sourceRef"]) &&
existingNodes.has(flowElement["@_targetRef"])))
+ );
}
+ const collectElements = (flowElements:
NonNullable<BPMN20__tProcess["flowElement"]>): void => {
+ for (let i = flowElements.length - 1; i >= 0; i--) {
+ const flowElement = flowElements[i];
+ if (
+ (flowElement["@_id"] && nodeIdsToMoveInside.has(flowElement["@_id"]))
||
+ (flowElement.__$$element === "boundaryEvent" &&
+ flowElement["@_attachedToRef"] &&
+ nodeIdsToMoveInside.has(flowElement["@_attachedToRef"]))
+ ) {
+ toMove.push(...flowElements.splice(i, 1));
+ } else if (shouldMoveSequenceFlow(flowElement, nodeIdsToMoveInside,
subProcessNodes)) {
+ toMove.push(...flowElements.splice(i, 1));
+ } else if (isSubProcessElement(flowElement) && flowElement.flowElement) {
+ collectElements(flowElement.flowElement);
+ }
+ }
+ };
+
+ collectElements(process.flowElement ?? []);
+ flowElementsToMove.push(...(toMove as typeof flowElementsToMove));
Review Comment:
If you use the `Normalized<>` on the `toMove` you will not require to do a
casting.
##########
packages/bpmn-editor/src/mutations/moveNodesInsideSubProcess.ts:
##########
@@ -51,37 +47,54 @@ export function moveNodesInsideSubProcess({
>[] = [];
const nodeIdsToMoveInside = new Set(__readonly_nodeIds);
- const subProcessNodes = new Set();
+ const subProcessNodes = new Set<string>();
subProcess.flowElement?.forEach((flowElement) => {
- if (flowElement.__$$element !== "sequenceFlow") {
+ if (flowElement.__$$element !== "sequenceFlow" && flowElement["@_id"]) {
subProcessNodes.add(flowElement["@_id"]);
}
});
- for (let i = 0; i < (process.flowElement ?? []).length; i++) {
- const flowElement = (process.flowElement ?? [])[i];
- if (
- nodeIdsToMoveInside.has(flowElement["@_id"]) ||
- (flowElement.__$$element === "boundaryEvent" &&
nodeIdsToMoveInside.has(flowElement["@_attachedToRef"]))
- ) {
- flowElementsToMove.push(...((process.flowElement?.splice(i, 1) ?? []) as
typeof flowElementsToMove));
- i--; // repeat one index because we just altered the array we're
iterating over.
- } else if (
+ const toMove: Unpacked<NonNullable<BPMN20__tProcess["flowElement"]>>[] = [];
+
+ function shouldMoveSequenceFlow(
+ flowElement: Unpacked<NonNullable<BPMN20__tProcess["flowElement"]>>,
+ nodeIds: Set<string>,
+ existingNodes: Set<string>
+ ): boolean {
+ return (
flowElement.__$$element === "sequenceFlow" &&
- ((nodeIdsToMoveInside.has(flowElement["@_sourceRef"]) &&
nodeIdsToMoveInside.has(flowElement["@_targetRef"])) ||
- (subProcessNodes.has(flowElement["@_sourceRef"]) &&
nodeIdsToMoveInside.has(flowElement["@_targetRef"])) ||
- (nodeIdsToMoveInside.has(flowElement["@_sourceRef"]) &&
subProcessNodes.has(flowElement["@_targetRef"])))
- ) {
- // If the source and target are both outside of the sub-process
- // or if the source and target is already in the sub process the
sequenceFlow must be copied
- flowElementsToMove.push(...((process.flowElement?.splice(i, 1) ?? []) as
typeof flowElementsToMove));
- i--; // repeat one index because we just altered the array we're
iterating over.
- }
+ !!flowElement["@_sourceRef"] &&
+ !!flowElement["@_targetRef"] &&
+ ((nodeIds.has(flowElement["@_sourceRef"]) &&
nodeIds.has(flowElement["@_targetRef"])) ||
+ (existingNodes.has(flowElement["@_sourceRef"]) &&
nodeIds.has(flowElement["@_targetRef"])) ||
+ (nodeIds.has(flowElement["@_sourceRef"]) &&
existingNodes.has(flowElement["@_targetRef"])))
+ );
}
+ const collectElements = (flowElements:
NonNullable<BPMN20__tProcess["flowElement"]>): void => {
+ for (let i = flowElements.length - 1; i >= 0; i--) {
+ const flowElement = flowElements[i];
+ if (
+ (flowElement["@_id"] && nodeIdsToMoveInside.has(flowElement["@_id"]))
||
+ (flowElement.__$$element === "boundaryEvent" &&
+ flowElement["@_attachedToRef"] &&
+ nodeIdsToMoveInside.has(flowElement["@_attachedToRef"]))
+ ) {
+ toMove.push(...flowElements.splice(i, 1));
+ } else if (shouldMoveSequenceFlow(flowElement, nodeIdsToMoveInside,
subProcessNodes)) {
+ toMove.push(...flowElements.splice(i, 1));
+ } else if (isSubProcessElement(flowElement) && flowElement.flowElement) {
+ collectElements(flowElement.flowElement);
+ }
+ }
+ };
+
+ collectElements(process.flowElement ?? []);
+ flowElementsToMove.push(...(toMove as typeof flowElementsToMove));
+
for (let i = 0; i < (process.artifact ?? []).length; i++) {
const artifact = (process.artifact ?? [])[i];
- if (nodeIdsToMoveInside.has(artifact["@_id"])) {
+ if (artifact["@_id"] && nodeIdsToMoveInside.has(artifact["@_id"])) {
Review Comment:
Same here.
##########
packages/bpmn-editor/src/mutations/moveNodesOutOfSubProcess.ts:
##########
@@ -34,49 +89,56 @@ export function moveNodesOutOfSubProcess({
__readonly_nodeIds: string[];
}) {
const { process } = addOrGetProcessAndDiagramElements({ definitions });
- const subProcess = process.flowElement?.find((s) => s["@_id"] ===
__readonly_subProcessId);
- if (
- !(
- subProcess?.__$$element === "subProcess" ||
- subProcess?.__$$element === "adHocSubProcess" ||
- subProcess?.__$$element === "transaction"
- )
- ) {
- throw new Error(`Can't find subProcess with ID
${__readonly_subProcessId}`);
+
+ const subProcess = findSubProcessRecursively(process.flowElement ?? [],
__readonly_subProcessId ?? "");
+ if (!subProcess) {
+ throw new Error(`Cannot find subprocess with ID:
${__readonly_subProcessId}`);
}
+ const parentFlowElements =
+ findParentFlowElements(process.flowElement ?? [], __readonly_subProcessId
?? "") ?? process.flowElement ?? [];
+
const flowElementsToMove:
Normalized<Unpacked<NonNullable<BPMN20__tProcess["flowElement"]>>>[] = [];
const artifactsToMove: Normalized<
ElementExclusion<Unpacked<NonNullable<BPMN20__tProcess["artifact"]>>,
"association">
>[] = [];
const nodeIdsToMoveOut = new Set(__readonly_nodeIds);
- const subProcessNodes = new Set();
+ const subProcessNodes = new Set<string>();
subProcess.flowElement?.forEach((flowElement) => {
- if (flowElement.__$$element !== "sequenceFlow") {
+ if (flowElement.__$$element !== "sequenceFlow" && flowElement["@_id"]) {
Review Comment:
Same here.
##########
packages/bpmn-editor/src/mutations/moveNodesInsideSubProcess.ts:
##########
@@ -51,37 +47,54 @@ export function moveNodesInsideSubProcess({
>[] = [];
const nodeIdsToMoveInside = new Set(__readonly_nodeIds);
- const subProcessNodes = new Set();
+ const subProcessNodes = new Set<string>();
subProcess.flowElement?.forEach((flowElement) => {
- if (flowElement.__$$element !== "sequenceFlow") {
+ if (flowElement.__$$element !== "sequenceFlow" && flowElement["@_id"]) {
subProcessNodes.add(flowElement["@_id"]);
}
});
- for (let i = 0; i < (process.flowElement ?? []).length; i++) {
- const flowElement = (process.flowElement ?? [])[i];
- if (
- nodeIdsToMoveInside.has(flowElement["@_id"]) ||
- (flowElement.__$$element === "boundaryEvent" &&
nodeIdsToMoveInside.has(flowElement["@_attachedToRef"]))
- ) {
- flowElementsToMove.push(...((process.flowElement?.splice(i, 1) ?? []) as
typeof flowElementsToMove));
- i--; // repeat one index because we just altered the array we're
iterating over.
- } else if (
+ const toMove: Unpacked<NonNullable<BPMN20__tProcess["flowElement"]>>[] = [];
+
+ function shouldMoveSequenceFlow(
+ flowElement: Unpacked<NonNullable<BPMN20__tProcess["flowElement"]>>,
+ nodeIds: Set<string>,
+ existingNodes: Set<string>
+ ): boolean {
Review Comment:
Could you please rename `existingNodes` to `existingNodesIds`?
##########
packages/bpmn-editor/src/store/computeDiagramData.ts:
##########
@@ -102,46 +103,49 @@ export function computeDiagramData(
nodeBpmnElementsById.set(bpmnElement["@_id"], bpmnElement);
// sub-processes
- if (
- bpmnElement?.__$$element === "subProcess" ||
- bpmnElement?.__$$element === "adHocSubProcess" ||
- bpmnElement?.__$$element === "transaction"
- ) {
- for (const flowElement of bpmnElement.flowElement ?? []) {
- if (flowElement.__$$element === "boundaryEvent") {
- parentIdsById.set(flowElement["@_id"],
flowElement["@_attachedToRef"]);
- } else {
- parentIdsById.set(flowElement["@_id"], bpmnElement["@_id"]);
+ if (isSubProcessElement(bpmnElement)) {
+ const processSubProcessElements = (subProcess: typeof bpmnElement,
parentId: string): void => {
+ for (const flowElement of subProcess.flowElement ?? []) {
+ if (flowElement.__$$element === "boundaryEvent") {
+ parentIdsById.set(flowElement["@_id"],
flowElement["@_attachedToRef"]);
+ } else {
+ parentIdsById.set(flowElement["@_id"], parentId);
+ }
+
+ if (flowElement.__$$element !== "sequenceFlow") {
+ if (
+ flowElement.__$$element !== "callChoreography" &&
+ flowElement.__$$element !== "choreographyTask" &&
+ flowElement.__$$element !== "dataObjectReference" &&
+ flowElement.__$$element !== "dataStoreReference" &&
+ flowElement.__$$element !== "implicitThrowEvent" &&
+ flowElement.__$$element !== "manualTask" &&
+ flowElement.__$$element !== "receiveTask" &&
+ flowElement.__$$element !== "sendTask" &&
+ flowElement.__$$element !== "subChoreography"
+ ) {
+ nodeBpmnElementsById.set(flowElement["@_id"], flowElement);
+
+ if (isSubProcessElement(flowElement)) {
+ processSubProcessElements(flowElement,
flowElement["@_id"]);
+ }
+ }
Review Comment:
Please, add back the else clause with the comment
##########
packages/bpmn-editor/src/mutations/moveNodesOutOfSubProcess.ts:
##########
@@ -34,49 +89,56 @@ export function moveNodesOutOfSubProcess({
__readonly_nodeIds: string[];
}) {
const { process } = addOrGetProcessAndDiagramElements({ definitions });
- const subProcess = process.flowElement?.find((s) => s["@_id"] ===
__readonly_subProcessId);
- if (
- !(
- subProcess?.__$$element === "subProcess" ||
- subProcess?.__$$element === "adHocSubProcess" ||
- subProcess?.__$$element === "transaction"
- )
- ) {
- throw new Error(`Can't find subProcess with ID
${__readonly_subProcessId}`);
+
+ const subProcess = findSubProcessRecursively(process.flowElement ?? [],
__readonly_subProcessId ?? "");
+ if (!subProcess) {
+ throw new Error(`Cannot find subprocess with ID:
${__readonly_subProcessId}`);
}
+ const parentFlowElements =
+ findParentFlowElements(process.flowElement ?? [], __readonly_subProcessId
?? "") ?? process.flowElement ?? [];
+
const flowElementsToMove:
Normalized<Unpacked<NonNullable<BPMN20__tProcess["flowElement"]>>>[] = [];
const artifactsToMove: Normalized<
ElementExclusion<Unpacked<NonNullable<BPMN20__tProcess["artifact"]>>,
"association">
>[] = [];
const nodeIdsToMoveOut = new Set(__readonly_nodeIds);
- const subProcessNodes = new Set();
+ const subProcessNodes = new Set<string>();
subProcess.flowElement?.forEach((flowElement) => {
- if (flowElement.__$$element !== "sequenceFlow") {
+ if (flowElement.__$$element !== "sequenceFlow" && flowElement["@_id"]) {
subProcessNodes.add(flowElement["@_id"]);
}
});
- // Check if we're moving out of an Event Sub-Process
const isEventSubProcess = subProcess.__$$element === "subProcess" &&
(subProcess["@_triggeredByEvent"] ?? false);
+ function shouldMoveSequenceFlow(
+ flowElement: Unpacked<NonNullable<BPMN20__tProcess["flowElement"]>>,
+ nodeIds: Set<string>,
+ existingNodes: Set<string>
+ ): boolean {
+ return (
+ flowElement.__$$element === "sequenceFlow" &&
+ !!flowElement["@_sourceRef"] &&
+ !!flowElement["@_targetRef"] &&
+ ((nodeIds.has(flowElement["@_sourceRef"]) &&
nodeIds.has(flowElement["@_targetRef"])) ||
+ (existingNodes.has(flowElement["@_sourceRef"]) &&
nodeIds.has(flowElement["@_targetRef"])) ||
+ (nodeIds.has(flowElement["@_sourceRef"]) &&
existingNodes.has(flowElement["@_targetRef"])))
+ );
+ }
+
for (let i = 0; i < (subProcess.flowElement ?? []).length; i++) {
const flowElement = (subProcess.flowElement ?? [])[i];
if (
- nodeIdsToMoveOut.has(flowElement["@_id"]) ||
- (flowElement.__$$element === "boundaryEvent" &&
nodeIdsToMoveOut.has(flowElement["@_attachedToRef"]))
+ (flowElement["@_id"] && nodeIdsToMoveOut.has(flowElement["@_id"])) ||
+ (flowElement.__$$element === "boundaryEvent" &&
+ flowElement["@_attachedToRef"] &&
+ nodeIdsToMoveOut.has(flowElement["@_attachedToRef"]))
) {
flowElementsToMove.push(...((subProcess.flowElement?.splice(i, 1) ?? [])
as typeof flowElementsToMove));
i--; // repeat one index because we just altered the array we're
iterating over.
- } else if (
- flowElement.__$$element === "sequenceFlow" &&
- ((nodeIdsToMoveOut.has(flowElement["@_sourceRef"]) &&
nodeIdsToMoveOut.has(flowElement["@_targetRef"])) ||
- (subProcessNodes.has(flowElement["@_sourceRef"]) &&
nodeIdsToMoveOut.has(flowElement["@_targetRef"])) ||
- (nodeIdsToMoveOut.has(flowElement["@_sourceRef"]) &&
subProcessNodes.has(flowElement["@_targetRef"])))
- ) {
- // If the source and target are both outside of the sub-process
- // or if the source and target is already in the sub process the
sequenceFlow must be copied
+ } else if (shouldMoveSequenceFlow(flowElement, nodeIdsToMoveOut,
subProcessNodes)) {
Review Comment:
please add back the comment, adpat it if necessary.
##########
packages/bpmn-editor/src/mutations/moveNodesInsideSubProcess.ts:
##########
@@ -51,37 +47,54 @@ export function moveNodesInsideSubProcess({
>[] = [];
const nodeIdsToMoveInside = new Set(__readonly_nodeIds);
- const subProcessNodes = new Set();
+ const subProcessNodes = new Set<string>();
subProcess.flowElement?.forEach((flowElement) => {
- if (flowElement.__$$element !== "sequenceFlow") {
+ if (flowElement.__$$element !== "sequenceFlow" && flowElement["@_id"]) {
Review Comment:
We should use the `Normalized<>` type. This ensure that `@_id` is present on
the all elements.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
