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

[REFACTOR] Store part of the BpmnModel into memory for searches #1115

Merged
merged 10 commits into from
Mar 3, 2021

Conversation

tbouffard
Copy link
Member

This is a first step to avoid depending on the mxGraph model for elements
search.

It introduces a new dedicated registry to manage the BpmnModel and related
objects. They are now used to search element by ids. This first implementation
holds a structure to make search faster: this is probably not optimal and will
be rework later.
The previously called 'DisplayModel' has been extracted out of mxgraph code as
it is not tight to it. It has been renamed into 'RenderedModel' for consistency
with the wording we use everywhere else.

covers #953

@tbouffard tbouffard added depends on another PR ⚠️ Pull request depending on another one. The depending must be merged first refactoring Code refactoring labels Feb 16, 2021
@tbouffard tbouffard added the rebase needed 💥 Pull request that must be rebased on the latest master commit prior being reviewed or merged label Feb 17, 2021
This is a first step to avoid depending on the mxGraph model for elements
search.

It introduces a new dedicated registry to manage the BpmnModel and related
objects. They are now used to search element by ids. This first implementation
holds a structure to make search faster: this is probably not optimal and will
be rework later.
The previously called 'DisplayModel' has been extracted out of mxgraph code as
it is not tight to it. It has been renamed into 'RenderedModel' for consistency
with the wording we use everywhere else.
@tbouffard tbouffard force-pushed the refactor/592-extract_display_model branch from 3867169 to 7b3d41d Compare February 20, 2021 09:45
@tbouffard tbouffard removed depends on another PR ⚠️ Pull request depending on another one. The depending must be merged first rebase needed 💥 Pull request that must be rebased on the latest master commit prior being reviewed or merged labels Feb 20, 2021
@tbouffard tbouffard marked this pull request as ready for review February 21, 2021 13:56
@github-actions
Copy link

github-actions bot commented Feb 22, 2021

♻️ PR Preview 6724ec7 has been successfully destroyed since this PR has been closed.

🤖 By surge-preview

export class BpmnModelRegistry {
private searchableModel: SearchableModel;

computeRenderedModel(bpmnModel: BpmnModel): RenderedModel {
Copy link
Member

Choose a reason for hiding this comment

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

👍🏼

@@ -2,7 +2,7 @@
<html lang="en">
<head>
<meta charset="UTF-8">
<title>BPMN Visualization Non Regression</title>
<title>BPMN Visualization - Elements Identification</title>
Copy link
Member

Choose a reason for hiding this comment

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

👍🏼

@tbouffard
Copy link
Member Author

Putting back to draft to integrate review feedbacks.
As this PR may introduce performance changes, I am also waiting for #1127 to be merged first prior making this PR back to 'Ready for Review'

@tbouffard tbouffard marked this pull request as draft March 1, 2021 07:57
src/component/registry/bpmn-model-registry.ts Outdated Show resolved Hide resolved
}
}

function toRenderedModel(bpmnModel: BpmnModel): RenderedModel {
Copy link
Member Author

Choose a reason for hiding this comment

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

⚠️ Sonar suggests that this function is not fully covered by unit and integration tests
We should at least model tests for all use cases.

image

Copy link
Member Author

Choose a reason for hiding this comment

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

This is managed by the removal of the 'safe navigation operator' ?. in 7dd96bf

csouchet and others added 5 commits March 1, 2021 14:46
It must never be null or undefined (if so this is a bug elsewhere), so drop it
for simplicity.
It must never be null or undefined (if so this is a bug elsewhere), so drop it
for simplicity.
@tbouffard tbouffard marked this pull request as ready for review March 2, 2021 11:54
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 3, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

97.0% 97.0% Coverage
0.0% 0.0% Duplication

@tbouffard tbouffard merged commit 4b6a720 into master Mar 3, 2021
@tbouffard tbouffard deleted the refactor/592-extract_display_model branch March 3, 2021 13:59
@@ -32,19 +33,22 @@ export default class BpmnVisualization {
* @experimental subject to change, feedback welcome
*/
readonly bpmnElementsRegistry: BpmnElementsRegistry;
private readonly _bpmnModelRegistry: BpmnModelRegistry;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big fan of underscore for private fields. IMHO it is enough to have linting and modifiers.

If we decide to go with that convention, we need to perhaps do the refactoring to convert other private fields to follow this approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, as decided with @csouchet, I will prepare a PR to make everything consistent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Code refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants