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

feat: enrich links between edges and shapes in the internal model #2638

Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion src/component/parser/json/converter/ProcessConverter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,11 @@ export default class ProcessConverter {

deserialize(processes: string | TProcess | (string | TProcess)[]): void {
ensureIsArray(processes).forEach(process => this.parseProcess(process));

// Need to call this after all processes have been parsed, because to link a call activity to the elements of the called process, we have to parse all processes before.
ensureIsArray(processes).forEach(process => this.assignParentOfProcessElementsCalledByCallActivity(process.id));

this.assignIncomingAndOutgoingIdsFromFlows();
}

private assignParentOfProcessElementsCalledByCallActivity(processId: string): void {
Expand All @@ -87,6 +90,26 @@ export default class ProcessConverter {
}
}

private assignIncomingAndOutgoingIdsFromFlows(): void {
const fillShapeBpmnElementAttribute = (
Copy link
Member

Choose a reason for hiding this comment

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

question: why defining this function here and not as a private method of the class?
This is not consistent with the rest of the code (we always create private methods in this case).
It may be a good reason, in that case, I suggest to document it as this is unclear to me

Copy link
Member Author

Choose a reason for hiding this comment

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

As this is a technical method, only used within this context and I wasn't sure where to put it, I preferred to leave it here. But I am open to any ideas. ^^

Copy link
Member

Choose a reason for hiding this comment

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

OK fine. Let's keep it as it is.

shapeBpmnElementId: string,
shapeBpmnElementAttributeName: keyof Pick<ShapeBpmnElement, 'outgoingIds' | 'incomingIds'>,
Copy link
Member

Choose a reason for hiding this comment

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

thought: for simplicity, we may declare instead:

shapeBpmnElementAttributeName: 'outgoingIds' | 'incomingIds'

The 2 attributes won't be renamed (as they are part of the public API) so there is no risk of error.
Anyway, let's keep this as it is now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @tbouffard,

I understand your suggestion to simplify the code by declaring the property names directly. 
However, I personally prefer using keyof Pick to avoid potential naming errors in the future, especially using replace feature of IDE. This approach ensures that only the allowed property names are used, reducing the risk of introducing bugs or errors.

Thanks for sharing your thoughts though! 🙂

Copy link
Member

@tbouffard tbouffard May 3, 2023

Choose a reason for hiding this comment

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

Okay, that's fine.

valueToAdd: string,
): void => {
const shapeBpmnElement =
this.convertedElements.findFlowNode(shapeBpmnElementId) ?? this.convertedElements.findLane(shapeBpmnElementId) ?? this.convertedElements.findPoolById(shapeBpmnElementId);
if (shapeBpmnElement && !shapeBpmnElement[shapeBpmnElementAttributeName].includes(valueToAdd)) {
shapeBpmnElement[shapeBpmnElementAttributeName].push(valueToAdd);
}
};

const flows = [...this.convertedElements.getMessageFlows(), ...this.convertedElements.getSequenceFlows(), ...this.convertedElements.getAssociationFlows()];
Copy link
Member

Choose a reason for hiding this comment

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

question: I guess we didn't measure the impact of this new lookup on the parsing performance in particular on large diagram.
The lookup done with convertedElements.findXXX involves retrieving elements from a Map, so it should be fast.
But having real figures would be better.

We could modify the parseBpmn utils to make it display the processing time or create a dedicated new tool for this purpose.

Copy link
Member Author

Choose a reason for hiding this comment

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

As we decided with @tbouffard, we will manage the perfomance tests in another issue.

Copy link
Member

Choose a reason for hiding this comment

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

After discussions with @csouchet, we decided to create a dedicated issue for the "parser performance": #2665
We will quickly handle it.
We have never done specific parsing performance test, so we have to setup the whole infrastructure. Performance tests involved the whole diagram loading and/or navigation.

flows.forEach(flow => {
fillShapeBpmnElementAttribute(flow.sourceRefId, 'outgoingIds', flow.id);
fillShapeBpmnElementAttribute(flow.targetRefId, 'incomingIds', flow.id);
});
}

private parseProcess(process: TProcess): void {
const processRef = process.id;
const pool = this.convertedElements.findPoolByProcessRef(processRef);
Expand All @@ -106,7 +129,7 @@ export default class ProcessConverter {
ShapeUtil.flowNodeKinds()
.filter(kind => kind != ShapeBpmnElementKind.EVENT_BOUNDARY)
.forEach(kind => this.buildFlowNodeBpmnElements(process[kind], kind, parentId, process.id));
// process boundary events afterwards as we need its parent activity to be available when building it
// process boundary events afterward as we need its parent activity to be available when building it
this.buildFlowNodeBpmnElements(process.boundaryEvent, ShapeBpmnElementKind.EVENT_BOUNDARY, parentId, process.id);

// containers
Expand Down
9 changes: 9 additions & 0 deletions src/component/parser/json/converter/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ export class ConvertedElements {
findMessageFlow(id: string): MessageFlow {
return this.messageFlows.get(id);
}
getMessageFlows(): MessageFlow[] {
csouchet marked this conversation as resolved.
Show resolved Hide resolved
return Array.from(this.messageFlows.values());
}
registerMessageFlow(messageFlow: MessageFlow): void {
this.messageFlows.set(messageFlow.id, messageFlow);
}
Expand All @@ -69,6 +72,9 @@ export class ConvertedElements {
findSequenceFlow(id: string): SequenceFlow {
return this.sequenceFlows.get(id);
}
getSequenceFlows(): SequenceFlow[] {
return Array.from(this.sequenceFlows.values());
}
registerSequenceFlow(sequenceFlow: SequenceFlow): void {
this.sequenceFlows.set(sequenceFlow.id, sequenceFlow);
}
Expand All @@ -77,6 +83,9 @@ export class ConvertedElements {
findAssociationFlow(id: string): AssociationFlow {
return this.associationFlows.get(id);
}
getAssociationFlows(): AssociationFlow[] {
return Array.from(this.associationFlows.values());
}
registerAssociationFlow(associationFlow: AssociationFlow): void {
this.associationFlows.set(associationFlow.id, associationFlow);
}
Expand Down
Loading