feat(dia.Graph, dia.Paper): cell layers #3025
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a cell layers feature to the JointJS diagramming library that allows organizing cells into separate layers within the graph. The key changes include replacing the paper layer system with a more structured layer view architecture and adding cell layer management to the graph model.
- Introduces
CellLayerandCellLayerViewclasses to manage and visualize cell organization - Replaces the
PaperLayerclass with a genericLayerViewclass for better extensibility - Adds a
CellLayersControllerto theGraphclass for managing cell layers and their relationships
Reviewed Changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/joint-core/src/dia/Graph.mjs | Adds cell layers controller and refactors cell collection management |
| packages/joint-core/src/dia/Paper.mjs | Refactors layer management from paper layers to layer views with better organization |
| packages/joint-core/src/dia/Cell.mjs | Adds layer() method for cell layer assignment and management |
| packages/joint-core/src/dia/layers/LayerView.mjs | Replaces PaperLayer with generic LayerView class |
| packages/joint-core/src/dia/layers/CellLayerView.mjs | New specialized layer view for managing cell collections |
| packages/joint-core/src/dia/groups/CellLayer.mjs | New cell layer model for organizing cells |
| packages/joint-core/src/dia/controllers/CellLayersController.mjs | New controller for managing cell layers in the graph |
| packages/joint-core/test/jointjs/layers/basic.js | Test suite for new cell layers functionality |
| }, | ||
|
|
||
| getCells: function() { | ||
|
|
||
| return this.get('cells').toArray(); | ||
| // Preserve old order without layers |
There was a problem hiding this comment.
What does old order mean? The reader would not know.
There was a problem hiding this comment.
Did comment it properly
| getFirstCell: function(layerId) { | ||
| let cells; | ||
| if (!layerId) { | ||
| cells = this.getCells(); |
There was a problem hiding this comment.
Can we do this more efficiently?
this.cellLayersController.getCellLayer(firstLayerId).cells;| import { GridLayer } from './layers/GridLayer.mjs'; | ||
| import { GridLayerView } from './layers/GridLayerView.mjs'; | ||
|
|
||
| export const LAYERS = { |
There was a problem hiding this comment.
Do we name enums with all upper-case letter anywhere?
| @@ -437,7 +451,7 @@ export const Paper = View.extend({ | |||
| }; | |||
|
|
|||
| // Render existing cells in the graph | |||
| this.resetViews(model.attributes.cells.models); | |||
| this.resetViews(model.cellCollection.models); | |||
There was a problem hiding this comment.
Is this a breaking change? cellCollection.models is not sorted, right?
There was a problem hiding this comment.
Yes, indeed. Changed to model.getCells(), this one returns ordered models
| } else { | ||
| layerView = this.getLayerView(cellLayer.id); | ||
| } | ||
| this.insertLayerView(layerView, LAYERS.LABELS); |
There was a problem hiding this comment.
Why Layers.LABELS? Can you add a comment to the code here.
There was a problem hiding this comment.
Added the comment. I am putting cell layers between back and labels layers, where cells layer is currently located
| }, | ||
|
|
||
| updateCellLayers: function(cellLayers) { | ||
| const removedCellLayerViewIds = this._cellLayers.filter(cellLayerView => !cellLayers.some(l => l.id === cellLayerView.id)).map(cellLayerView => cellLayerView.id); |
There was a problem hiding this comment.
This approach to removing leftovers isn’t the most optimal, but given the expected number of layers, it should be sufficient. Please add a comment to the code to indicate that while this can be improved, it’s acceptable for our current use case.
| getLayers() { | ||
| // Returns a sorted array of layer views. | ||
| return this.getLayerNames().map(name => this.getLayerView(name)); | ||
| getOrderedLayerViews() { |
There was a problem hiding this comment.
Do we have getLayerViews() method that is unordered?
There was a problem hiding this comment.
Added getLayerViews() and getCellLayerViews methods.
| if (!insertBefore) { | ||
| this._registerLayer(layerName, layerView, null); | ||
| // remove from order in case of a move command |
There was a problem hiding this comment.
Can you explain what is the move command?
It will be better to refer to it as in case we are changing position of an existing layer
| }, | ||
|
|
||
| resetLayers: function() { | ||
| resetLayerViews: function() { | ||
| const { _layers: { viewsMap }} = this; | ||
| Object.values(viewsMap).forEach(layerView => layerView.removePivots()); |
There was a problem hiding this comment.
I think it would be nice to have reset() method on LayerView (which will call removePivots() internally)
| @@ -1791,7 +1854,16 @@ export const Paper = View.extend({ | |||
| return this.model.getBBox() || new Rect(); | |||
| } | |||
|
|
|||
| return V(this.cells).getBBox(); | |||
| const cellLayerViews = Object.values(this._cellLayerViews); | |||
There was a problem hiding this comment.
Can we use Rect.fromRectUnion() for simplicity here?
Also, please add a comment what is being done here.
| var zB = cellB.attributes.z || 0; | ||
| return (zA === zB) ? 0 : (zA < zB) ? -1 : 1; | ||
| }); | ||
| Object.values(viewsMap).filter(view => view instanceof CellLayerView).forEach(view => view.sortExact()); |
There was a problem hiding this comment.
Don't we have a higher-level API to get all CellLayerViews?
There was a problem hiding this comment.
Added getCellLayerViews() method
| } | ||
| layerView.insertCellView(view); | ||
|
|
||
| this.trigger('cell:inserted', view, isInitialInsert); |
There was a problem hiding this comment.
Perhaps we should call it 'cell:insert' (we don't use added, changed either).
Or better 'cell:mount'. Analogous to the onMount() method.
There was a problem hiding this comment.
Or remove it completely, if it is not used in this PR.
There was a problem hiding this comment.
Removed in this PR
| }); | ||
| } | ||
|
|
||
| groupSet(key, val, opt) { |
There was a problem hiding this comment.
Please remove the method if it is not used. Let's not commit to an API we are not sure about yet.
There was a problem hiding this comment.
This method is used when I need to change layer attribute for all cells, when the default layer has changed
| this.cells.remove(cell, opt); | ||
| } | ||
|
|
||
| reset() { |
There was a problem hiding this comment.
The method removes cell by cell and does not allow replacing cells with a new set.
A better name could be clear(). Similarly to graph.clear().
There was a problem hiding this comment.
There is a clear method on mvc.Model, so I resorted to this naming
|
|
||
| _onModelEvent(event, model, collection, options) { | ||
| if (model) { | ||
| if (event === 'changeId') { |
There was a problem hiding this comment.
You can refactor Collection.mjs if you need to partially preserved its behavious.
Why do we omit this line?
if ((event === 'add' || event === 'remove') && collection !== this) return;There was a problem hiding this comment.
Added a comment. It is done, so the CellGroup can emit add and remove events, because the collection is normally graph.cellCollection origin collection
|
|
||
| _prepareModel(attrs, _options) { | ||
| if (this._isModel(attrs)) { | ||
| // do not change collection attribute of a cell model |
There was a problem hiding this comment.
Why did you omit this line?
if (!attrs.collection) attrs.collection = this;These are low-level features and changing them must be properly explained in the code.
| defaults() { | ||
| return { | ||
| type: 'CellLayer', | ||
| collectionConstructor: CellLayerCollection, |
There was a problem hiding this comment.
Do we need to store a JS class inside the model attributes (CellGroup is a model)?
There was a problem hiding this comment.
I removed this attribute and refactored cellLayers attribute in a way that it stores all layer attributes similar to cells.
| init() { | ||
| LayerView.prototype.init.apply(this, arguments); | ||
|
|
||
| // applying it here instead of using property |
There was a problem hiding this comment.
Can we update the snapshot instead?
| } | ||
| }, | ||
|
|
||
| _requestViewUpdate(cell, opt) { |
There was a problem hiding this comment.
This is not just an update request. This is a request for inserting / re-inserting.
This should be called _requestCellViewInsertion().
No description provided.