[FIX] web_editor: create attachment on _reapplyCurrentShape() call

[loco-odoo]

Steps to reproduce the bug:

• Add an image on the website.
• Add a shape on the image.
• Save and edit.
• Click on the cropping option but then outside of the cropper so you do not actually crop the image (the image does not have the o_we_image_cropped class).
• Save.

-> The image has raw data as src and no attachment has been created for the image.

The problem comes from the _reapplyCurrentShape() call at the image_cropper_destroyed event interception. Indeed, the current shape of the image is being re applied on the image (so the image src is transformed to raw data) but the o_modifed_image_to_save class is not added on the image. Due to it, no attachment linked to the "modified" image is created at the editor save. To solve the problem, the o_modifed_image_to_save class is added on the image at the _reapplyCurrentShape() call.

task-3916313

[robodoo]

.

[loco-odoo]

Note: Adapt the version 17.0 and more specifically this commit

[qsm-odoo]

@robodoo delegate=xO-Tx
@xO-Tx

[xO-Tx]

@loco-odoo The diff LGTM 👍, But I think there is a way to optimize this fix even more:

Actually, the use of .o_modified_image_to_save will force the creation of a new attachment for technically the same image every time we cancel the crop.

The idea here is to simply keep a copy of the image src "before the crop", and add it back when the crop widget is destroyed (if the image is not cropped)… Something like (To test 👍):

async crop() {
    ...
    const srcBeforeCrop = this.$target[0].getAttribute("src");
    new weWidgets.ImageCropWidget(this, this.$target[0]).appendTo(this.options.wysiwyg.odooEditor.document.body);

    await new Promise(resolve => {
        this.$target.one('image_cropper_destroyed', async () => {
            await this._reapplyCurrentShape();
            if (!this._isCropped()) {
                this.$target[0].src = srcBeforeCrop;
            }
            resolve();
        });
    });
    ...
},

Also, the second .add("o_modified_image_to_save") of _reapplyCurrentShape() won't be needed on resetCrop() since it was already handled by the ImageCropWidget > reset() > _save().

What do you think ?

[loco-odoo]

Actually, the use of .o_modified_image_to_save will force the creation of a new attachment for technically the same image every time we cancel the crop.

@xO-Tx Indeed, we could try to not create an attachment in this case 👍 However, I think that the this._isCropped() condition is not enough. Indeed, with the solution you proposed, if you:

• Add an image on the website.
• Add a shape on the image and crop it.
• Save and edit.
• Click on the cropping option but then outside of the cropper so you do not actually re-crop the image (but the image still has the o_we_image_cropped class).
• Save

-> The image will have raw data as src because _reapplyCurrentShape() is called but the src of the image is not replaced by srcBeforeCrop as it will not enter the if (!this._isCropped()) code.

In this case, I think that what we want is to know if the image has been cropped by the widget that triggered the image_cropper_destroyed event. There is currently no such indication but we could add a class (for example image_cropped_saved) in the save method of the ImageCropWidget widget and do something like

async crop() {
    ...
    const srcBeforeCrop = this.$target[0].getAttribute("src");
    new weWidgets.ImageCropWidget(this, this.$target[0]).appendTo(this.options.wysiwyg.odooEditor.document.body);

    await new Promise(resolve => {
            this.$target.one('image_cropper_destroyed', async () => {
                if (!this.$target[0].classList.contains("image_cropped_saved")) {
                    // If the image has not been modified by the cropper, reset
                    // the target to its src before a cropping has been applied.
                    this.$target[0].src = srcBeforeCrop;
                } else {
                    this.$target[0].classList.remove("image_cropped_saved");
                    await this._reapplyCurrentShape();
                }
                resolve();
            });
        });
    ...
},

What do you think?

Also, the second .add("o_modified_image_to_save") of _reapplyCurrentShape() won't be needed on resetCrop() since it was already handled by the ImageCropWidget > reset() > _save().

I agree with you, it is not needed in this case, however, I still think that the o_modified_image_to_save class is missing somewhere when calling _applyShapeAndColors() in _reapplyCurrentShape(). Indeed, this method calls _writeShape() that does loadImage(dataURL, img) which means that the src of the image in the DOM is replaced by raw data. Because the o_modified_image_to_save class is not added, if a customization or future code (for example the the hover effect option since 17.0) use _reapplyCurrentShape(), developers will have to think to add this class on the image themselves, otherwise, the image would still have raw data after the save. (Actually such a bug occurred in 17.0 but was fixed in this commit that I would like to adapt in the fw-port of this PR).
While looking in the code, I saw that the o_modified_image_to_save was added each time _applyShapeAndColors() is called. That it the reason why I did the same in the _reapplyCurrentShape() method. However, IMO, a refactoring should probably be done in master to add this o_modified_image_to_save each time loadImage(dataURL, img) is called (in other words, each time the src of an image in the DOM is replaced by raw data).
What do you think is the best for this version? (by adding the o_modified_image_to_save class in _reapplyCurrentShape() , we would not solve any bug but it will be more correct IMO).