Ticket #10328 (closed Feature Request: wontfix)

Opened 4 years ago

Last modified 11 months ago

fields with blob support w/o schemaextender in plone.app.blob

Reported by: frisi Owned by: witsch
Priority: minor Milestone: 4.x
Component: General Version:
Keywords: blob Cc: dukebody

Description

currently plone.app.blob lets developers create image or filefields with blob support by using the  ExentsionBlobField provided in plone.app.blob.subtypes.image|file (see  the example content type)

as plone4 ships with plone.app.blob using schemaextender just adds useless complexity since there is no need to replace an existing imagefield with one that supports blobstorage just in case plone.app.blob is installed.

we should add BlobImageField and BlobFileField to plone.app.blob for immediate usage in custom content types.

something like this should do it (i'm sure there's place for improvements)


def imageSizes():
    """plone.app.imaging needs a callable to not use site-wide settings.
    """
    sizes = {'preview' : (400, 400),
             'portlet-right' : (270, 270*2),
             'listing' :  (80, 80),
             }
    return sizes

class BlobImageField(BlobField, ImageFieldMixin):
    """imagefield based on plone.app.blob.subtype.image.ExtensionBlobField
    that supports blobstorage in plone4-buildouts.
    (plone4 ships with plone.app.blob so there is no need to use schemaextender
    to replace the imagefield just in case plone.app.blob is available)


    don't set content id to the filename of the image
    and don't use plone.app.imaging's sizes but the field property

    did not subclass ExtensionField since we're not using schemaextender here so
    we have our accessors and mutators generated.
    IndexMethodFix is only needed to fixe some issue w/ schemaextender so we can skip that, too
    """
    implements(IBlobImageField)

    pil_resize_algo = zconf.pil_config.resize_algo
    pil_quality = zconf.pil_config.quality

    def set(self, instance, value, **kwargs):
        super(BlobImageField, self).set(instance, value, **kwargs)
        #self.fixAutoId(instance)
        if hasattr(aq_base(instance), blobScalesAttr):
            delattr(aq_base(instance), blobScalesAttr)

#    @property
#    def sizes(self):
#        return getAllowedSizes()

imageField = BlobImageField('image',
        required = False,
        storage = atapi.AnnotationStorage(),
        max_size = zconf.ATNewsItem.max_image_dimension,
        sizes = imageSizes,
        crop_scales = ['listing'],
        validators = (('isNonEmptyFile', V_REQUIRED),
                             ('checkNewsImageMaxSize', V_REQUIRED)),
        widget = ImageWidget(label=_(u"Image"),),
                             preview_scale='preview'),
        )

Change History

comment:1 Changed 4 years ago by witsch

  • Status changed from new to assigned
  • Owner set to witsch

comment:2 Changed 4 years ago by witsch

  • Status changed from assigned to closed
  • Resolution set to fixed

heh, always nice when the svn hooks don't work as expected... :) anyway & fyi, convenience base classes for file and image fields have been added in r35476, which should significantly simplify the above. in fact, the above example should be reducible like so:

from plone.app.blob.field import ImageField

def imageSizes():
    """ plone.app.imaging needs a callable """
    return { 'preview': (400, 400),
             'portlet-right': (270, 270*2),
             'listing': (80, 80) }

imageField = ImageField('image',
    sizes = imageSizes,
    widget = ImageWidget(label=_(u'Image'),), preview_scale='preview')

the PIL-related setting might require some adjustments — please feel free to test & let me know about further problems...

comment:3 follow-up: ↓ 4 Changed 4 years ago by davisagli

Using a callable to provide image sizes fails in at least one place in AT that expects a dict (yeah, it's a bug in AT, but some of us are using this with older versions of AT). Would you please merge Huub Bouma's plone.app.imaging branch which makes it possible to specify custom scale sizes as a dict again? The other issue of course is that requiring a callable like this requires additional, annoying changes to old code that is being updated to use blob-based image fields.

comment:4 in reply to: ↑ 3 ; follow-up: ↓ 5 Changed 4 years ago by witsch

Replying to davisagli:

Using a callable to provide image sizes fails in at least one place in AT that expects a dict (yeah, it's a bug in AT, but some of us are using this with older versions of AT).

just to clarify, keeping the callable here was just to stick with frisi's example, yet try to simplify it. it's not to indicate this should be considered best practice.

Would you please merge Huub Bouma's plone.app.imaging branch which makes it possible to specify custom scale sizes as a dict again?

it's on my list, but i won't merge it as it is. huub himself pointed out that his solution using the marker is "clumsy", so i'll need to find some time to try to come up with a better way. of course, i'd be happy about any suggestions... ;)

The other issue of course is that requiring a callable like this requires additional, annoying changes to old code that is being updated to use blob-based image fields.

again, it's merely meant as a condensed version of the original example, not a guide line or anything...

comment:5 in reply to: ↑ 4 ; follow-up: ↓ 7 Changed 4 years ago by davisagli

Replying to witsch:

Replying to davisagli:

Using a callable to provide image sizes fails in at least one place in AT that expects a dict (yeah, it's a bug in AT, but some of us are using this with older versions of AT).

just to clarify, keeping the callable here was just to stick with frisi's example, yet try to simplify it. it's not to indicate this should be considered best practice.

Yes, okay. It does point out the annoyance of what you currently have to do to use scales not controlled via the imaging_properties sheet, though.

Would you please merge Huub Bouma's plone.app.imaging branch which makes it possible to specify custom scale sizes as a dict again?

it's on my list, but i won't merge it as it is. huub himself pointed out that his solution using the marker is "clumsy", so i'll need to find some time to try to come up with a better way. of course, i'd be happy about any suggestions... ;)

His approach isn't ideal, but I don't think it's so bad that it shouldn't get merged. One alternative would be to compare the dict of scales against ATImageSchemaimage?.sizes and only fall through to the imaging_properties lookup if they are equal (doing proper dict comparison of course).

Is there a better place to discuss this than this unrelated ticket? ;) (My fault, I know...)

comment:6 Changed 4 years ago by frisi

  • Status changed from closed to reopened
  • Resolution fixed deleted

i tested  rev35476 with the example code in your comment2.

when we do not provide the pil parameters, scales do not get created and we get a 404 when accessing image_preview.

in addition, the sizes property of the base class takes precedence over the sizes method we provide in our field definition. as already discussed above this is part of another issue. however, having the sizes property on the baseclass makes it impossible to define custom scales at all at the moment.

proposed solution: add pil params to our base-class and remove the sizes method but add some hint for developers:

from Products.ATContentTypes.configuration import zconf

class ImageField(BlobField, ImageFieldMixin):
    """ base class for a blob-based image field.
    to use plone.app.imaging scales in your field import

       from plone.app.imaging.utils import getAllowedSizes
   
    and set ``sizes = getAllowedSizes`` in your field definition
    """
    implements(IBlobImageField)

    pil_resize_algo = zconf.pil_config.resize_algo
    pil_quality = zconf.pil_config.quality

comment:7 in reply to: ↑ 5 Changed 4 years ago by witsch

Replying to davisagli:

Yes, okay. It does point out the annoyance of what you currently have to do to use scales not controlled via the imaging_properties sheet, though.

it does, yes. but it's not like i wouldn't like to fix it...

His approach isn't ideal, but I don't think it's so bad that it shouldn't get merged. One alternative would be to compare the dict of scales against ATImageSchemaimage?.sizes and only fall through to the imaging_properties lookup if they are equal (doing proper dict comparison of course).

right. like i said, i need to find some time to review and test, and that's the problem. there is just too much other stuff going on, and this is "only" plone...

i'd be very happy to see an updated patch for this, though! ;)

Is there a better place to discuss this than this unrelated ticket? ;) (My fault, I know...)

heh, irc i guess? :)

comment:8 Changed 4 years ago by witsch

(In [35505]) merge (a slightly modified version of) huub's branch to restore the possibility to define per-field image scale sizes (refs #10159, #10328)

comment:9 follow-up: ↓ 14 Changed 4 years ago by frisi

the changes do not yet make the example in comment 2 above working. still plone.app.imaging sizes are used and not the sizes provided as method or dictionary(tried both) please post a comment here as soon as i should test again.

comment:10 Changed 4 years ago by witsch

(In [35509]) update base fields with (most) properties also set on the original archetypes fields (refs #10328)

comment:11 Changed 4 years ago by witsch

(In [35511]) avoid using a property for sizes, which apparently shadows any sizes method (or dictionary) defined for custom fields. setting a callable property should work around this... (refs #10328)

comment:12 Changed 4 years ago by witsch

(In [35512]) also avoid the sizes property in case of the ExtensionField (fixes #10159, refs #10328)

comment:13 Changed 4 years ago by witsch

(In [35514]) pin the  latest archetypes to get the  fix necessary to make r35511 & r35512 work (refs #10328, #10159)

comment:14 in reply to: ↑ 9 Changed 4 years ago by witsch

  • Status changed from reopened to closed
  • Resolution set to fixed

Replying to frisi:

the changes do not yet make the example in comment 2 above working.

nobody said i was done yet... ;)

please post a comment here as soon as i should test again.

now things should be good, so i'd appreciate you testing again. please note, though, that i've extended the fields like AT does, i.e. they're not using Products.ATContentTypes.configuration.zconf atm. perhaps that could be updated, but otoh ATCT so far also required to specify more than just the field itself...

in any case, i think/hope we're done. please reopen should there be more issues!

comment:15 Changed 4 years ago by frisi

  • Status changed from closed to reopened
  • Resolution fixed deleted

thanks for your work andi, simply using plone.app.blob.field.ImageField and spezify sizes as a dict/callable works like a charm!

imho we should use atct.zconf to support setting quality/algorithm/maxsize for all imagefields via the configuration file. (although i did not yet use this feature, i can image there are some integrators out there happily using it)

if you disagree completely feel free to close this issue forever ;-)

comment:16 Changed 4 years ago by frisi

after a bit more testing i noticed, that using plone.app.blob.field.ImageField does not invalidate scales when the image-file gets replaced by a new image. the full-size image gets updated however.

turned out that adding the custom set method to our ImageField fixes this:

    def set(self, instance, value, **kwargs):
        super(ImageField, self).set(instance, value, **kwargs)
        if hasattr(aq_base(instance), blobScalesAttr):
            delattr(aq_base(instance), blobScalesAttr)

comment:18 in reply to: ↑ 17 Changed 4 years ago by dukebody

comment:19 Changed 22 months ago by davisagli

  • Component changed from Infrastructure to General

comment:20 Changed 11 months ago by eleddy

  • Status changed from reopened to closed
  • Resolution set to wontfix

This ticket has not been modified in over 9 months. In another brazen attempt to clean this tracker up, this is closed. If you really, REALLY care about this ticket, please re-verify that it is still an issue on the current supported releases (4.2 or 4.3) and reopen. Better yet, submit a pull request to fix the bug and then close the bug properly. We <3 you and all of your effort, but we can't go on like this anymore. I hope you aren't too mad and we can still be friends. Hugs.

Note: See TracTickets for help on using tickets.