Layer functionality, renewed edit mode #146
Conversation
…ct loading patch and UI bugs(ruler opacity, history clicks)
…history, added multiselection(on canvas and board)
…ded modification functionality of multipolygons in edit mode
…text annotation html for edit mode
|
Important Installation incomplete: to start using Gemini Code Assist, please ask the organization owner(s) to visit the Gemini Code Assist Admin Console and sign the Terms of Services. |
Aiosa
left a comment
Aiosa
left a comment
There was a problem hiding this comment.
Please, address the concerns in my comments, and also please go thgough the functionality after the merge and fix bugs. I tried to get the selection back as much as possible, but it is not polished. For example, object deselection from group misbehaves.
| if (!options.exportsObjects) return undefined; | ||
| let objs = context.toObject(exportAll, ...exportedProps).objects; | ||
|
|
||
| const ids = options?.filter?.ids; |
There was a problem hiding this comment.
Can you please fix the jsdoc for options to now cover all the options we might use? It was not properly documented, but since it is getting more complex (it had only flags up till now), I want a proper docs,,
| @@ -0,0 +1,2 @@ | |||
| /*! Sortable 1.15.6 - MIT | git://github.com/SortableJS/Sortable.git */ | |||
There was a problem hiding this comment.
Can we make this a module? Annotations would depend on the module, but other parts of the system could also use it. Later, we might even want to move the dependency away from annotations module and just have the UI logics in the annotations-gui.
modules/annotations/annotations.js
Outdated
| objects[i].lockScalingY = false; | ||
| objects[i].lockUniScaling = false; | ||
| } | ||
| if (this.cachedTargetCanvasSelection) { |
There was a problem hiding this comment.
cachedTargetCanvasSelection looks very much like a private prop, should start with _. If not, I am missing any documentation on usage...
There was a problem hiding this comment.
It says outdated but the code just moved, the issue still persists.
modules/annotations/annotations.js
Outdated
| this._layer.setActive(false); | ||
| this._layer = undefined; | ||
|
|
||
| this.raiseEvent('active-layer-changed', { |
modules/annotations/annotations.js
Outdated
| return; | ||
| } | ||
| const objs = ids | ||
| .map(id => this.findObjectOnCanvasByIncrementId(Number(id))) |
There was a problem hiding this comment.
very expensive call, prefer using array of objects as input argument
There was a problem hiding this comment.
It says outdated but the code just moved, the issue still persists.
modules/annotations/annotations.js
Outdated
| : (annotationIds !== undefined && annotationIds !== null ? String(annotationIds) : annotationIds); | ||
|
|
||
| // todo selection event collision | ||
| this.raiseEvent('annotation-selection-changed', { |
There was a problem hiding this comment.
Pass around arrays of objects, not ids, mapping object -> id is much cheaper. Also, prefer using selected and deselected, instead of 'isSelected' flag.
There was a problem hiding this comment.
It says outdated but the code just moved, the issue still persists.
modules/annotations/annotations.js
Outdated
|
|
||
| const handleDeselectionFromCanvas = (e) => { | ||
| const ids = (e?.deselected || []).map(obj => obj.incrementId); | ||
| if (ids.length) _this._emitAnnotationSelectionChanged(ids, false, true); |
There was a problem hiding this comment.
We should fiure out how to prevent re-insertion, right now mouse click internally fires deselection, and then our logics fires selection... a bit too expensive
There was a problem hiding this comment.
Also what might be problem:
- object is selected
- we click on different object
- first deselection event here is called because fabric swaps the selection
- then objectClicked is called, we change selection agan (potentially)
- again deselection event here is called (e.g. active selection created)
So it would be good to add console logs to inspect whether these events are not over-firing
There was a problem hiding this comment.
It says outdated but the code just moved, the issue still persists.
| if (withObjects) { | ||
| scope = this.exportOptions.scope === 'selected' ? 'selected' : 'all'; | ||
| if (scope === 'selected') { | ||
| const selectedAnns = (this.context.getSelectedAnnotations?.() || []); |
There was a problem hiding this comment.
This logics should be within the convertor. You should send only flags, options, but the logic itself should be part of convertor API.
There was a problem hiding this comment.
It says outdated but the code just moved, the issue still persists.
…pare for multi-viewport support)
…d doppelganger bugs
|
Okay, I fixed few more things, and added more changes that later simplify the transfer to multiviewport sessions. Some final remarks:
I don't also know why selection is driven by increment id instead of internal one - increment changes wildly, while internal stays consistent -> such selection is more stable (e.g. wrt. history undo/redo). Also, remove all the calls to |
Aiosa
left a comment
Aiosa
left a comment
There was a problem hiding this comment.
Some minor issues with deletion of large amount of objects.
| if (activeObject.type === 'activeSelection') { | ||
| activeObject.getObjects().forEach(obj => { | ||
| // todo this overloads history, needs to do in one step | ||
| this.deleteObject(obj); |
There was a problem hiding this comment.
Here the deletion will overload history with unit operations, we need single undo/redo. I propose that deleteObject supports active selection as well and deletes it as a whole within history steep.
| */ | ||
| deleteObject(o, _raise=true) { | ||
| // this._deletedObject = o; | ||
| if (this.isAnnotation(o)) return this.deleteAnnotation(o, _raise); |
There was a problem hiding this comment.
This must also use history, changed to non-private call. I would support active selection or array of objects, or even fabric canvas reference. It would delete anything that is thrown at it and ensure the deletion is a single histor step.
There was a problem hiding this comment.
Maybe even accept layer as argument.
…c of deleteObject, added docs
…ygon and multipolygon can move during edit mode
Introducing layer hierarchy, synced multi-selection, selection import/export, and renewed fixed board edit mode.