Scaling overrides

[tomgross]

• updated buildout to Plone 4.3
• adding custom scaling views for image-type, news-type and plone.namedfile
• changed api of save_cropped method to remove redundant parameter

supersedes #37, fixes #21, fixes #36 and probably even fixes #24

at a first glance this seems to work. @frisi, @petschki, @hvelarde can someone review this please?

[petschki]

can you fix the failing tests as mentioned by travis ci?

[hvelarde]

could it be missing dependencies on package distribution?

[frisi]

i reviewed the commits in this pull request and they look good to me.
(though i don't understand FHNW@362674f - but this is probably because i did not have to deal with namedfile yet)

thanks for your initiative @tomgross!

since we've been talking about no-monkey-patching or touching plone.app.imaging my solution to this problem would have been a IObjectModified listener that re-creates all scales we have cropping information in our annotation.
your approach looks much more elegant!

however, before merging this we should get rid of the test failures

[hvelarde]

I spent some time reviewing this and I have more comments:

• the list of package dependencies is far from complete
• the development configuration is confusing and the documentation on which combination of versions/packages (i.e. Plone 4.2/4.3, plone.app.contenttypes) is supported is missing; we need to be very careful with this and we should not include version pinning on the development/CI configurations if it is not documented
• I was able to remove the SupermodelParseError: Field type plone.namedfile.field.NamedBlobImage specified for field first_image is not supported by removing the plone.namedfile >= 2.0.1 requirement and adding plone.supermodel = 1.2.2, but then I got stuck with an AttributeError: _ElementInterface instance has no attribute 'iterchildren'; this seems to be related with plone.app.querystring and plone.app.registry but I was not able to identify the cause; I don't know neither is there should be a problem between Plone 4.2 and this version of plone.supermodel

I suggest to refactor the development/CI configurations first on master and then rebase this pull request to be able to better isolate the issues before merging.

can anyone help on documenting the combination of versions/packages supported first?

[hvelarde]

this try block is not needed; as per #36 we need to drop support for Plone 4.1

[hvelarde]

@tomgross IMO we should fix and merge #39 first and then fix this one or it could get worst; also I don't see the point on including a testing configuration for Plone 5.0; let Travis CI do the work.

but that's only IMO.

[tomgross]

The build is passing now for Plone 4.2 and 4.3 (Python 2.7) on Travis-CI. I tried hard but I did not find a way to support plone.namedfile < 2.0.1. I reviewed the changes there and did not see any backward incompatible changes. What I found was a complete restructure of the blob handling (which seems to be the key for us too) and an API change for the scale method. If we only support plone.namedfile >= 2.0.1 we only need to provide an override for one class.

[tomgross]

@hvelarde the Plone 5 testing configuration IS for Travis CI. Look at the .travis.yml. I restructured a little bit to have separate Travis configs for each Plone version to have better control over the versions and other configuration settings. Unfortunately the versions.cfg in http://dist.plone.org/release/5.0a1-pending/ is still empty :(

[hvelarde]

@tomgross this looks clear now but we still lack information on versions and options supported.

I don't see the need of having 3 different buildout configurations for testing this on Travis if we have a clear understanding of what we want to achieve; for instance: we are using this on production with Plone 4.2 and plone.namedfile 1.0.6; so we need the use cases anyway.

I propose:

• Plone 4.2 without plone.app.contenttypes
• Plone 4.3 without plone.app.contenttypes
• Plone 4.3 with plone.app.contenttypes

we can say on the documentation that those are the only supported configurations, that the package may work on a different one but we are not testing so we don't warranty it.

if we only depend on Dexterity on tests, then the only place we must check if Dexterity is installed is there and skip the test we don't need if Dexterity is not installed.

I'm going to close #39 now.

[hvelarde]

I still don't see the need of having this, as I mentioned before plone.namedfile is always installed on Plone.

[tomgross]

The need / advantage of having different buildouts for different scenarios is that you can maintain different egg versions / configurations.

[tomgross]

still any issues with this pull request #38 ?

[frisi]

limiting this override to ATImage and ATNewsItem requires people having custom archetypes based content types to add this page registration for their type, too.

from a usability point of view it should be sufficient to mark your type with plone.app.imagecropping.interfaces.IImageCropping to get cropping support for all image fields.

i'd suggest to register it for Archetypes BaseObject:

 <browser:page
      zcml:condition="installed Products.Archetypes"
      for="Products.Archetypes.BaseObject.BaseObject"
      name="images"
      class=".scaling.ImageScaling"
      allowed_interface="plone.app.imaging.interfaces.IImageScaling"
      permission="zope2.View" />

[frisi]

when trying this out locally i recognized, that uploading another image-file to the same image content does not invalidate the cropped scales (the old cropped image is still shown)

this is only the case when accessing the image via context/@@images/image/thumb
context/image_thumb shows the new image scaled and uncropped

see plone/plone.scale#3 (comment) for some explanation

we already tested both cases in test_accessing_images so i'd do the same in this test.

since i can't update your branch / this pull request i created a branch with your changes in this repository and modified the test there: a25b15d

@tomgross could you merge this test into your branch and make it pass?

[tomgross]

After some QA discussion and documentation encancements by @frisi this one is now good to merge I think.