Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Image crops lost when editing image title #21

Closed
frisi opened this Issue · 10 comments

5 participants

@frisi
Collaborator

crops get invalidated when the image title/description or other metadata is changed.

this should only happen if the image file itself gets changed.

to reproduce:

  • upload image
  • define a crop for a scale (eg large) using @@croppingeditor
  • include the image in a page template using plone.scale
<img tal:define="scales image_object/@@images;
                         img python:scales.scale('image', scale='large')"
         tal:attributes="src img/url" />
  • template shows the right crop
  • edit the image and change description and/or title
  • view the template again and the image will be scaled instead of cropped

it's important to mention that this only happens for scales.scale and not for the plone.app.imaging traverser:

test1.jpg/@@images/8689307d-f3e7-42c5-9134-f77f6e704cfe.jpeg will show the scaled image

test1.jpg/image_frontpage will still show the cropped image

did not yet investigate if this bug belongs to imagecropping or plone.scale or plone.app.imaging

@petschki
Collaborator
@petschki
Collaborator

until we don't want to change plone core modules, the only solution is to make a method which restores/resaves the scale on ObjectModifiedEvent

@hvelarde
Owner

@jpgimenez this is going to bite us when we deprecate our image tile in favor of the standard tile (collective/collective.cover#81)

@saily
Owner

crops are also lost when image is copied.
i've done some experimental monkey-patching of plone.scales to avoid that, but needs cleanup and more testing. Feedback is welcome.

    <monkey:patch
        description="Patch plone.scales to create cropped images"
        class="plone.scale.storage.AnnotationStorage"
        original="scale"
        replacement=".storage.patched_scale"
        preserveOriginal="true"
        />

Several ugly things are happening down below. I'd love to see an adapter for plone.scales which queries for a factory how to create crops. What do you think about that?

from plone.app.imagecropping import PAI_STORAGE_KEY
from plone.app.imagecropping.interfaces import IImageCroppingUtils
from zope.annotation.interfaces import IAnnotations
from zope.component import getMultiAdapter


def patched_scale(self, factory=None, **parameters):
    """This is a really dirty hack to re-create blob files if a 
       cropping information was stored in object annotation.
    """
    info = self._old_scale(factory, **parameters)

    # XXX: That's very nasty
    if not factory.func_name == 'crop_factory' \
       and (parameters.get('width') != info.get('width') or
            parameters.get('height') != info.get('height')):

        cropped = IAnnotations(self.context).get(PAI_STORAGE_KEY)
        if not cropped:
            return info

        utils = IImageCroppingUtils(self.context)

        cropping_view = getMultiAdapter(
            (self.context, self.context.REQUEST), name='crop-image')

        for fieldname in utils.image_field_names():
            for image_name, box in cropped.items():
                scale = image_name.split('_').pop()
                cropping_view._crop(fieldname, scale, box)

    return info
@hvelarde
Owner

any news on this issue?

@frisi
Collaborator

just stumbled over a discussion on plone.scale (plone/plone.scale#3) which is related to this

@tomgross tomgross added the bug label
@frisi frisi closed this in #38
@frisi
Collaborator

@tomgross and i fixed this ticket in #38

for dexterity, this currently means to implement another interface.
(see FHNW@e2b510d)

@joka as dexterity pro: are you ok with that? personally i'd love to see that we can find a way to make dexterity content that implement IImageCropping also implement IImageCroppingScale.

if we can't find a way to make that happen, we should think about moving this interface out of the browser package to plone.app.imagecropping.interfaces and add some documentation to the readme

@frisi
Collaborator

@saily reading through the comments here i see you mentioned copying an image too. i did not test that yet. would be great to get your feedback if this fixes your issues too.

@frisi frisi reopened this
@saily
Owner

Let's write a test to check that, should be easy to implement. i'll try to find some time for that... anyway, thanks for fixing

@saily
Owner

Hi @frisi, unfortunately this does not fix the copying issue. I've opened a pull #40 which could be a nice base to fix that. i'm closing this right now, because the main problem seems to be fixed in #38

@saily saily closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.