Use isolated functions for each behavior of InteractiveImage
The InteractiveImage has a lot of specific logic, due to it having multiple "tools". For example, a "mouse down" event starts CSS translations to allow moving the image around when no tool is selected, but it can cause a polygon to start being drawn with the polygon tool is selected.
Currently, this is handled using various if-else if
clauses such as these:
if (action === 'zoom') {
this.zoom(mouseCoords, event)
} else if (!this.imageEdit) {
if (this.mouseDown) this.translate(mouseCoords, action)
} else if (this.editionMode === 'polygon') {
this.editPolygon(mouseCoords, action)
} else if (this.editionMode === 'rectangle') {
this.editRectangle(mouseCoords, action)
} else if (this.editionMode === 'select') {
this.editSelected(mouseCoords, action)
}
By themselves, they are not really an issue; although a switch
statement could maybe be used here instead. The main problem is the code before and after those statements. A missing return
statement inside one of the if
blocks caused duplicate updates fixed by !654 (merged), which was very hard to troubleshoot. The many possible paths for the code make it very complex to understand what is going on.
I suggest having instead a series of 'atomic' methods such as polygonMouseUp
, rectangleMouseDown
, mouseWheel
, etc., which do nothing but handle one specific type of event. The existing mouseAction
method then does nothing other than process a MouseEvent
and select the right method to call. Here is a simple example:
mouseEvent (event) {
const allHandlers = {
// No tool
default: {
// Zoom on the image
wheel: this.mouseWheel
},
// Polygon drawing tool to create new elements
polygon: {
// Add a point; if the polygon is closed, open the creation modal
mouseup: this.polygonMouseUp
// Automatically close the polygon and open the modal
dblclick: this.polygonDblClick,
...
},
...
}
const handler = allHandlers[this.editionMode || 'default'][event.type]
handler()
}
This makes documenting the point of each method easier as everything is in one place; if we have to try to understand how the component behaves, we can read through the allHandlers
object to get a 'table of contents'. This also prevents weird side effects where some code designed for another mouse event triggers for something else and would prevent !654 (merged) from happening again.