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] Split the code of BpmnElementsRegistry and the code of its collaborators #2803

Closed
3 tasks done
tbouffard opened this issue Aug 22, 2023 · 0 comments · Fixed by #2832
Closed
3 tasks done
Assignees
Labels
refactoring Code refactoring
Milestone

Comments

@tbouffard
Copy link
Member

tbouffard commented Aug 22, 2023

Over time, BpmnElementsRegistry has become a veritable monster. It was originally intended to be responsible for the retrieval of model elements and their associated visual information.
But then we added methods to handle CSS classes, overlays and cell styling. It now has too many responsibilities, and we should consider splitting up its public API.

To prepare for this split, we first have to split the internal code, especially as some parts are very tightly coupled.
This will also prepare the code for tree-shaking and will introduce better separation of concerns.
The overlays part has already been managed in #2800

There are 2 parts in the code and here is how we can proceed (this has been followed in #2800):

  • the code in BpmnElementsRegistry can move to other "registry" files. It is generally glue code
  • the code in GraphCellUpdater, which generally does the real processing, that needs to be split into 2 different updater (for CSS classes and style)

In addition, extract dedicated interfaces for all new registries. This will:

  • ensure internal registry used the same method signatures as the public API
  • let plugins develop in addons have the same signature and ensure it includes all methods by following this interfaces
  • let share the JSDoc. By putting the JSDoc of the methods in the interface only, all implementers will provide the same information to users

Possible drawback in HTML API: the split into several interfaces may require more navigation for readers. This is already the case for the split of existing type like ShapeUpdate. There is nothing we can do to improve that.

Challenges

Notes:

The main issue for this change is the coupling which has been introduced in #2732 and that had already been noticed at that time:

StyleManager (mxGraph related) and CssRegistry (domain specific) know each other introducing a cyclic dependency. The domain code can know the mxGraph code but not the opposite.
This was introduced to ensure that the style update correctly works with the CSS classes management.

The two elements include caches that need to be cleared while loading a new model. This part is probably coupled and should be improved.

Proposed renaming and file organization

registry/css-registry.ts

  • rename the existing CssRegistry class into CssClassesCache
  • create a new CssClassesRegistry as entry point of CSS classes management

mxgraph/

  • style/StyleManager.ts: rename into 'style/style-updater.ts' and store a new StyleUpdater class with the style part of the existing GraphCellUpdater. Find a better name for StyleManager that should not been exported anymore as it should only be used by StyleUpdater
  • style/css-classes-updater.ts: store a new CssClassesUpdater class with the style part of the existing GraphCellUpdater
  • remove GraphCellUpdater

Note: in the existing code of GraphCellUpdater, this.graph.model.setStyle(...) should use the model constant directly instead of passing by the graph.

Proposal for new interfaces

in registry/types.ts, add the following interface

  • CssClassesRegistry
  • ElementsRegistry: for the methods of BpmnElementsRegistry that only managed the BPMN elements
  • OverlaysRegistry
  • StyleRegistry

Tasks

Preview Give feedback
@tbouffard tbouffard added the refactoring Code refactoring label Aug 22, 2023
@tbouffard tbouffard added this to the 0.38.1 milestone Aug 22, 2023
@tbouffard tbouffard self-assigned this Aug 22, 2023
@csouchet csouchet modified the milestones: 0.38.1, 0.38.2 Aug 22, 2023
@tbouffard tbouffard moved this to In Progress in Roadmap 2023 Aug 25, 2023
tbouffard added a commit that referenced this issue Aug 28, 2023
Previously, when applying the "CSS" API, the implementation ensured that
the cell's original style was cached (for any subsequent reset) without
any additional CSS classes. In fact, this was not necessary. Storing
styles in the cache with additional CSS classes information posed no
problem. When the style is reset, the value retrieved from the cache is
updated with the CSS classes currently applied to the cell. This works
just as well if there are no CSS classes as if there are. This
additional check has therefore been removed.

The tests verifying interactions between the "CSS" and "Style" APIs have
been improved. They confirm that the removal of this coupling is
legitimate:
  - Introduce a dedicated test file for these tests.
- Previously, they were stored in the file testing the style update at
template level.
- This change provides an explicit way of identifying API interaction
tests and a better separation of concerns.
- Add a dedicated test to check the impact of resetting the style after
a CSS and style update. This proves the uselessness of the coupling
described above.

covers #2803
closes #2743

---------

Co-authored-by: Souchet Céline <[email protected]>
@github-project-automation github-project-automation bot moved this from In Progress to Done in Roadmap 2023 Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Code refactoring
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants