jonasundderwolf / django-image-cropping

Django helper application to easily and non-destructively crop arbitrarily large images in admin and frontend.
Other
552 stars 131 forks source link

abstract models with ImageRatioField #57

Closed DmitryFillo closed 9 years ago

DmitryFillo commented 9 years ago

I have several ratio fields in the model class. Many models can contain this and so I implemented this with abstract class.

For example:

class AbstractImagePreviewSlider(models.Model):
    cropping_slider = ImageRatioField('image_original', '188x120', size_warning=True, verbose_name='Slider ratio')

    class Meta:
        abstract = True

class AbstractImagePreviewAnotherSlider(models.Model):
    cropping_another_slider = ImageRatioField('image_original', '500x500', size_warning=True, verbose_name='Another slider ratio')

    class Meta:
        abstract = True

class ModelOne(AbstractImagePreviewSlider):
    # smth
    pass

class ModelAnother(AbstractImagePreviewAnotherSlider):
    # smth
    pass

But ImageRatioField adds ratio_fields list to the class for the needs of widgets. (https://github.com/jonasundderwolf/django-image-cropping/blob/master/image_cropping/fields.py#L84)

And so ModelAnother will contain cropping_slider in the ratio_fields list and ModelOne - cropping_another_slider. This will lead to an error in the method initial_cropping for example.

I think that occurs due to ratio_fields isn't unique for child classes.

Dirty workaround:

class ImageRatioFieldFixed(ImageRatioField):
    def contribute_to_class(self, cls, name):
        super(ImageRatioFieldFixed, self).contribute_to_class(cls, name)

        # fix
        if cls._meta.abstract:
            delattr(cls, 'ratio_fields')

Any thoughs? Maybe I miss something?

escaped commented 9 years ago

Unfortunately i could not reproduce your problem. But you are correct about the abstract models. There is no need to append either crop_fields or ratio_fields to that class. This change may solves your problem. Could you please try the next branch. If this does not help, please provide a complete "working" example (as git repo).

anrie commented 9 years ago

@DmitryFillo Could you check the latest 1.0.1 release and check if your issue is resolved?

DmitryFillo commented 9 years ago

@anrie I have been testing new release and it's OK. No errors.

@escaped What was the error: it's very tricky. It's related to MRO. I will try to explain this, my first example wasn't correct enough.

class AbstractImagePreviewSlider(models.Model):
    cropping_slider = ImageRatioField('image_original', '188x120', size_warning=True, verbose_name='Slider ratio')

    class Meta:
        abstract = True

class AbstractImagePreviewAnotherSlider(models.Model):
    cropping_another_slider = ImageRatioField('image_original', '500x500', size_warning=True, verbose_name='Another slider ratio')

    class Meta:
        abstract = True

class ModelOne(AbstractImagePreviewSlider, AbstractImagePreviewAnotherSlider):
    # smth
    pass

class ModelAnother(AbstractImagePreviewSlider):
    # smth
    pass

ModelOne initialization: ImageRatioField will be attached to the ModelOne via add_to_class, which calls contribute_to_class. contribute_to_class of ImageRatioField do hasattr (crop_fields/ratio_fields) on ModelOne, but MRO and so we will find crop_fields/ratio_fields in the AbstractImagePreviewSlider and will append name of cropping property. ratio_fields for ModelOne: ['cropping_slider', 'cropping_slider', 'cropping_another_slider']. Warning! This ratio_fields stored in the AbstractImagePreviewSlider! It's smells bad, because ModelAnother will inherit this lists and ratio_fields will be: ['cropping_slider', 'cropping_slider', 'cropping_another_slider', 'cropping_slider']. And Django Admin will not find cropping_another_slider for ModelAnother.

Hope I didn't make mistakes. I love MRO. :)