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

Master mysterious egg 2 nby #4234

Open
wants to merge 2 commits into
base: master-mysterious-egg
Choose a base branch
from

Conversation

Goaman
Copy link

@Goaman Goaman commented Mar 20, 2025

No description provided.

@robodoo
Copy link

robodoo commented Mar 20, 2025

This PR targets the un-managed branch odoo-dev/odoo:master-mysterious-egg, it needs to be retargeted before it can be merged.

@Goaman Goaman force-pushed the master-mysterious-egg-2-nby branch from 6edce45 to 05b6911 Compare March 24, 2025 09:22
Comment on lines +36 to +37
process_image_warmup_handlers: this.processImageWarmup.bind(this),
process_image_post_handlers: this.processImagePost.bind(this),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who is using this ressources ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shared method processImage.
Without the plugin shape, processImage will look at all the dataset that it understand and process the image accordingly.
With the plugin shape, if an image has a shape, it will need to be processed sligthly differently and be wrapped in a svg.
process_image_warmup_handlers allow to change how the image will be processed.
process_image_post_handlers will wrap the image in a svg and return the base64 of that svg.

Comment on lines +74 to +75
apply: ({ loadResult: updateImageAttributes }) => {
updateImageAttributes();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loadResult is a function ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method processImage need to change the dataset of the img. In order to make the mutations outside the processImage (for history purposes), processImage returns a method that apply the attributes.

Comment on lines +217 to +238
const { svg, svgAspectRatio, svgWidth } = processContext;
if (!svg) {
return;
}
svg.querySelectorAll("image").forEach((image) => {
image.setAttribute("xlink:href", b64url);
});
// Force natural width & height (note: loading the original image is
// needed for Safari where natural width & height of SVG does not return
// the correct values).
const loadedImage = await loadImage(b64url);
// If the svg forces the size of the shape we still want to have the resized
// width
if (!svg.dataset.forcedSize) {
svg.setAttribute("width", loadedImage.naturalWidth);
svg.setAttribute("height", loadedImage.naturalHeight);
} else {
const imageWidth = Math.trunc(svgWidth);
const newHeight = imageWidth / svgAspectRatio;
svg.setAttribute("width", imageWidth);
svg.setAttribute("height", newHeight);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

async mutation ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean? Are you talking about the svg.setAttribute ? The svg is not in the dom.

Comment on lines +45 to +55
onSave: async (newDataset) => {
// todo: should use the mutex if there is one?
const updateImageAttributes =
await this.dependencies.imagePostProcess.processImage(
selectedImg,
newDataset
);
updateImageAttributes();
this.dependencies.history.addStep();
},
document: this.document,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mutex ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I adapted the previous code that used the method applyModification as the equivalent now is processImage.
While adapting the code, I noticed that the mutation made by updateImageAttributes will not be mutexified with the OperationPlugin and will corrupt the history in some circumstance.
In practice, I don't think this method will be called in the html_builder so we're safe for now.

const cropper = await activateCropper(originalImg, aspectRatio, data);
const croppedCanvas = cropper.getCroppedCanvas(width, height);
cropper.destroy();
const processedCanvas = (await postProcessCroppedCanvas?.(croppedCanvas)) || croppedCanvas;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why about what?

  1. We re-use the logic of the cropper to crop an image.
    If an image has some crop attributes (data-x/y/height/width/aspectRatio), the crop will be performed.
    You have to understand that for any modification of an image, all the transformation are re-applied from the original src.

  2. postProcessCroppedCanvas will be used by the svg plugin because some svg shape require to crop the cropped image.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants