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

Use own storage factory and drop Python 2.7 support #50

Merged
merged 9 commits into from
Mar 4, 2022

Conversation

mauritsvanrees
Copy link
Member

We get rid of some code then, using the upstream code from plone.namedfile and plone.scale, including their improvements from the last few years.
We require the latest alpha release of plone.namedfile. I have updated our own version to 4.0.0a1.

A bit more work needed/possible though, so I keep it as draft PR for now.

We could refactor much more code after dropping python 2.7, but I wanted to keep the changes small.

Plone 5.2 and 6.0 are supported, on Python 3.
See #49
We require `plone.namedfile >= 6.0.0a2` for this.
Fixes #48
On GHA on 5.2 Py 3.7 I get:
ImportError: type object 'Distribution' has no attribute '_finalize_feature_opts'
zc.buildout 3 does not yet work with pip 22.
mauritsvanrees added a commit to plone/plone.namedfile that referenced this pull request Feb 28, 2022
Mostly, this makes the view easier to customize.
I want to use this in plone/plone.app.tiles#50.
`plone.app.tiles` would no longer need to override the `publishTraverse` method then.
mister-roboto pushed a commit to plone/buildout.coredev that referenced this pull request Feb 28, 2022
Branch: refs/heads/master
Date: 2022-02-28T10:16:08+01:00
Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org>
Commit: plone/plone.namedfile@9c4574f

ImageScaling view: use guarded_orig_image to get field from a url.

Mostly, this makes the view easier to customize.
I want to use this in plone/plone.app.tiles#50.
`plone.app.tiles` would no longer need to override the `publishTraverse` method then.

Files changed:
A news/104.bugfix
M plone/namedfile/scaling.py
Repository: plone.namedfile

Branch: refs/heads/master
Date: 2022-02-28T22:51:56+01:00
Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org>
Commit: plone/plone.namedfile@02d18a6

Merge pull request #110 from plone/maurits-image-scaling-use-guarded-orig-image

ImageScaling view: use guarded_orig_image to get field from a url.

Files changed:
A news/104.bugfix
M plone/namedfile/scaling.py
Remove our publishTraverse copy, because it is not needed anymore in this case.
This fixes scaling images on tiles when the tile does not have the workaround of defining a property for the image field.
@mauritsvanrees mauritsvanrees marked this pull request as ready for review March 3, 2022 02:33
@mauritsvanrees
Copy link
Member Author

mauritsvanrees commented Mar 3, 2022

This is ready for review. Let me explain what is happening and why, because the changes here and in plone.namedfile and plone.scale can be hard to follow.

A few words on versions:

  • For this PR to work, we rely on the work I did in plone.namedfile 6.0.0a1, a2 and a3. We require at least a3 in setup.py. This means we drop Python 2 and Plone 5.1 support. Plone 5.2 on Py 3 still works, with the updated version pin.
  • This should be released as plone.app.tiles 4.0.0a1, and a branch 3.x created (I have just created this from master).
  • I will not update Plone coredev 5.2 to use the new versions when on Python 3. Coredev 5.2 should keep pinning 3.x.

First: why does plone.app.tiles have any image scaling code at all?

  1. plone.app.tiles has its own version of the AnnotationStorage of plone.scale. The standard storage stores scales in an annotation on the context. For tiles this does not work, because the context is a tile, which is basically a view. The data of the tile is accessed with an ITileDataManager to store scales in annotations on the real content item.
  2. plone.app.tiles has its own version of the ImageScale class from plone.namedfile. This is needed because the standard version does not work when the context is a tile: it cannot find the data. (A workaround is to add a @property on the tile for each image field, and let this get the image value from the tile data.)

These two reasons remain valid with this PR, but the hookup is different.

Note that the code seems only active and registered for persistent tiles. I guess a transient image tile just works without any special code, but I have not checked how it works for those.

Situation before this PR, using the standard plone.namedfile and plone.scale from Plone 5.2:

  • plone.app.tiles register its own @@images view for tiles. This is done with an own version of the ImageScaling view from plone.namedfile. An own version is needed so we can use our own AnnotationStorage and ImageScale in two methods: publishTraverse and scale.
  • publishTraverse needs our own storage when getting a previously generated unique image scale via url.
  • When a scale needs to be created, the scale method needs to get our own storage instead of the default one.
  • scale calls storage.scale. The standard storage, and also our own one, adapts the context to IImageScaleFactory, which finds the DefaultImageScalingFactory from plone.namedfile. It uses this to create the scale. This factory again does not find the correct data when the context is a tile. So in our version of the ImageScaling.scale method, we pass an extra argument factory=self.create. When this is passed, no adapter lookup is done. But this way of passing a factory was deprecated in plone.scale version 2 and should already have been removed in version 3.0 (Plone 5.1). We are now at version 3.1.2.
  • So we have an extra method ImageScaling.create that is basically our version of the DefaultImageScalingFactory class.
  • Both publishTraverse and scale need our own ImageScale when getting a previously generated unique image scale via url, or getting a scale of the original image (@@images/image), or generating a new unique scale.

That was the situation until now. Problems:

  1. This is very complex.
  2. plone.app.tiles has old copies of code from plone.namedfile and plone.scale. The upstream code has seen fixes that we miss. For example we miss some support for SVG images, and the scales are not marked as stable image scales for caching. And instead of a PersistentDict, plone.scale uses a ScalesDict, which has improved database conflict handling (this PR does nothing with that, but it would become simpler).

In the plone.namedfile 6.0.0 alpha versions, I have created more hooks and adapter lookups to make it possible to really override only the small parts where plone.app.tiles needs to do something specific. This PR makes full use of it.

New situation with this PR and latest alpha of plone.namedfile on Python 3:

  • Register our AnnotationStorage as IImageScaleStorage factory.
  • Register our own IImageScaleFactory factory for persistent tiles. This overrides two small methods so we get the image from the tile data. This replaces the ImageScaling.create method and means we no longer need to pass this as factory to storage.scale.
  • Result is that our customizations are now only about getting and storing data from the tile instead of a content item. The most difficult part of the code has been removed and we use the improved upstream code.
  • We no longer use a deprecated code path from plone.scale, so plone.scale can be cleaned up.

Copy link
Member

@jensens jensens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jensens jensens merged commit 1e5f939 into master Mar 4, 2022
@jensens jensens deleted the maurits-own-storage-factory-no-py27 branch March 4, 2022 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants