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

abstract models with ImageRatioField #57

Closed
DmitryFillo opened this issue Nov 18, 2014 · 3 comments
Closed

abstract models with ImageRatioField #57

DmitryFillo opened this issue Nov 18, 2014 · 3 comments
Assignees
Labels

Comments

@DmitryFillo
Copy link

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
Copy link
Collaborator

escaped commented Mar 11, 2015

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
Copy link
Member

anrie commented Mar 30, 2015

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

@DmitryFillo
Copy link
Author

@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. :)

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

No branches or pull requests

3 participants