Closed sneridagh closed 5 months ago
@sneridagh Which version of Plone? This has been something of a known issue, but requires some careful work to fix. It has to do with the timing of when the scale hashes are generated, and which modification time is used in the hash. To complicate things, there was a recent change to which modification time is used (https://github.com/plone/plone.namedfile/pull/152), which was trying to solve the problem.
I'm willing to dig into investigating the details of this, but probably will not have time until after ploneconf.
@davisagli 6.0.6 and 6.0.7 at least, I tested with latest p.restapi.
@sneridagh I found the cause. It is not related to the modification time but because restapi uses the deprecated direction parameter when creating the scale, which ends up in the hash. We can fix it in plone.scale to normalize the direction. I have limited battery now, so will wait to fix it later.
It is not related to the modification time but because restapi uses the deprecated direction parameter when creating the scale, which ends up in the hash.
@davisagli the only direction parameter I found in restapi was this:
Is that the one you're talking about? Can you elaborate further on the fix you are considering?
I investigated this issue and came to the following conclusions:
direction
is actually deprecated and needs to be changed to mode
.direction
parameter will not solve the problem.scale
parameter and passes the width
and height
parameters.width
and height
parameters and pass the scale
parameter.scale
parameter is passed, width
and height
are calculated.scale
parameter in the "Document View"? It would be the name
parameter here:
https://github.com/plone/plone.namedfile/blob/d845513e1392696aabfe0a39536874c4280705c2/plone/namedfile/adapters.py#L85@davisagli @sneridagh opinions?
FWIW, documentation of Image handling was recently updated.
@wesleybl I was thinking that the hash_key method in plone.scale should be improved so that it normalizes the parameters before computing the hash. i.e. it should calculate width and height from a scale name and use get_scale_mode to get the mode parameter.
it should calculate width and height from a scale name and use get_scale_mode to get the mode parameter.
The problem is that there are situations where the scale is not passed. See:
And even in situations where it is passed, this calculation has already been done before. It would be a "waste of performance". The methods would only need to pass the necessary parameters. Maybe make the scale parameter obrigatory?
Maybe the correct thing to do is to pass the scale parameter in the "Document View"?
I tried passing the scale
parameter here. Then we would have the scale, width and height parameters. Then I got the warning:
A scale name and width/height are given. Those are mutually exclusive: solved by ignoring width/height and taking name
In other words, it seems that it is not a good idea to pass these three parameters. This also caused some plone.namedfile
tests to fail.
So perhaps the solution could be to not pass scale in plone.restapi
, so that it is the same as plone.namedfile
. In other words, it would be not making this call:
and doing this directly:
@wesleybl I still think it makes more sense to make plone.scale recognize parameters that are equivalent, rather than trying to find and update every bit of code that creates scales (which would also include addons, project code...)
The problem is that there are situations where the scale is not passed. See: https://github.com/plone/plone.namedfile/blob/d845513e1392696aabfe0a39536874c4280705c2/plone/namedfile/adapters.py#L96-L102
Why is that a problem? If only width and height are passed, use them. If they are not passed, look them up from the scale name.
And even in situations where it is passed, this calculation has already been done before. It would be a "waste of performance".
It's just a lookup; I don't think it'll have a big impact. And perhaps the code can be reorganized so that it's calculated once and used twice.
If only width and height are passed, use them.
@davisagli today there is a way to find out width and height from the scale. But I didn't see a way to get scale from width and height. Do you know if this exists? Would it have to be implemented?
@wesleybl Not needed; my point is that we should always use width and height to calculate the hash. So we need to convert a scale to width and height but not the other way.
@davisagli In other words, should we remove the scale parameter from hash?
Yes, otherwise it would not be the same hash as when there is no scale.
BUG/PROBLEM REPORT (OR OTHER COMMON ISSUE)
While debugging https://github.com/plone/plone.scale/issues/83
I've found something unsettling, the serialized
image_scales
of an image content type differ from the ones present in the catalog.I made a test to show it: https://github.com/plone/plone.restapi/pull/1716
/cc @davisagli @tisto @ericof @mauritsvanrees @fredvd
What I did:
Create a page, create an image block and add an image. Save. Inspect the name of the scales in the img tag. Go to the image content Inspect the name of the scales in the img tag.
They do both differ.
What I expect to happen:
They use the same scales, since we expect to have only a current valid set of them, right?