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

Add to generate Plone image scales on save (opt-in) #43

Merged
merged 7 commits into from
Jul 7, 2021

Conversation

datakurre
Copy link
Member

when environment variable PLONE_SCALE_GENERATE_ON_SAVE=1 is set

@mister-roboto
Copy link

@datakurre thanks for creating this Pull Request and help improve Plone!

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass.

Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:

@jenkins-plone-org please run jobs

With this simple comment all the jobs will be started automatically.

Happy hacking!

@datakurre
Copy link
Member Author

@jenkins-plone-org please run jobs

@datakurre
Copy link
Member Author

@jenkins-plone-org please run jobs

@jensens
Copy link
Member

jensens commented Mar 9, 2021

@jenkins-plone-org please run jobs

@jensens jensens force-pushed the datakurre-generate-scales-on-save branch from a853452 to 3224846 Compare March 9, 2021 11:12
@tisto
Copy link
Member

tisto commented Jun 14, 2021

@jenkins-plone-org please run jobs

@tisto
Copy link
Member

tisto commented Jun 14, 2021

@plone/framework-team we would like to get feedback on this PR if possible. It is non-intrusive, non-breaking and the feature is purely optional. IMHO there is no risk in merging it and no need for an FWT vote IMHO. Though, a quick feedback/discussion would be nice.

@datakurre
Copy link
Member Author

datakurre commented Jun 15, 2021

Motivation for this in brief:

• for the current web, more and bigger scales are needed
• creating scales on request can make terrible user experience on image heavy landing pages
• especially true for Volto, because Plone REST API would the create all scales on single request
• replacing Plone scales with other solution would require reinventing cache invalidation for scales, so optimising plone scales might still be the easiest solution

(Also more fixes for scaling is possible. I recall that scale ids are now random, not deterministic, which may lead to broken images on strongly cached html. I have local patch for that, but no proper pull request. And also scaling itself could be made faster by making it async and parallel [there’s an experimental branch on plone.namedfile].)

@frapell
Copy link
Member

frapell commented Jun 15, 2021

+1 from me on this.
I don't like the idea of not voting it in the FWT however under current times, I understand the reasoning and agree that it is a safe change, so I am fine if there is no voting.

@tisto
Copy link
Member

tisto commented Jun 15, 2021

@frapell if this is something that the FWT should vote on, we should vote (or wait for the vote). I don't mind either way. :)

@ale-rt
Copy link
Member

ale-rt commented Jun 15, 2021

As a reminder the @plone/framework-team has received an invitation for the next meeting on Tue 29 Jun 2021 15:00 – 16:00 (CEST). Let's make it happen that day

@ale-rt
Copy link
Member

ale-rt commented Jun 15, 2021

BTW this functionality can easily be incorporated in an addon.

Copy link
Member

@ale-rt ale-rt left a comment

Choose a reason for hiding this comment

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

This looks good to me, I just added some remarks to avoid having too many functions in the datamanager module.

@ale-rt
Copy link
Member

ale-rt commented Jul 5, 2021

On the last meeting, the @plone/framework-team talked about this PR.
We decided there is no need for a PLIP.

@datakurre
Copy link
Member Author

Alessandro's review has good ideas for making this pull better before merge.

datakurre added a commit that referenced this pull request Jul 6, 2021
@datakurre
Copy link
Member Author

@jenkins-plone-org please run jobs

datakurre added a commit that referenced this pull request Jul 6, 2021
@datakurre datakurre force-pushed the datakurre-generate-scales-on-save branch from 8b6b440 to aff00ee Compare July 6, 2021 13:34
@datakurre
Copy link
Member Author

@jenkins-plone-org please run jobs

1 similar comment
@datakurre
Copy link
Member Author

@jenkins-plone-org please run jobs

@datakurre
Copy link
Member Author

I wonder what this

Failed doctest test for widget.rst
  File "/home/jenkins/workspace/pull-request-6.0-3.7@3/src/plone.formwidget.namedfile/plone/formwidget/namedfile/widget.rst", line 0

----------------------------------------------------------------------
File "/home/jenkins/workspace/pull-request-6.0-3.7@3/src/plone.formwidget.namedfile/plone/formwidget/namedfile/widget.rst", line 755, in widget.rst
Failed example:
    PIL.Image.open(six.BytesIO(content.image_field))
Expected:
    Traceback (most recent call last):
    ...
    OSError: cannot identify image file...
Got:
    Traceback (most recent call last):
      File "/srv/python3.7/lib/python3.7/doctest.py", line 1337, in __run
        compileflags, 1), test.globs)
      File "<doctest widget.rst[264]>", line 1, in <module>
        PIL.Image.open(six.BytesIO(content.image_field))
      File "/home/jenkins/.buildout/eggs/cp37m/Pillow-8.2.0-py3.7-linux-x86_64.egg/PIL/Image.py", line 2968, in open
        "cannot identify image file %r" % (filename if filename else fp)
    PIL.UnidentifiedImageError: cannot identify image file <_io.BytesIO object at 0x7f83122711d0>

is. I think I saw that already before the latest changes.

@jensens
Copy link
Member

jensens commented Jul 6, 2021

PIL 8+ now raises more specific errors. This is fixed on master, rebase this branch.

@datakurre datakurre force-pushed the datakurre-generate-scales-on-save branch from 4a85bc9 to b7676cb Compare July 7, 2021 05:00
@datakurre
Copy link
Member Author

@jenkins-plone-org please run jobs

@datakurre
Copy link
Member Author

@jenkins-plone-org please run jobs

@datakurre
Copy link
Member Author

datakurre commented Jul 7, 2021

I noticed that master targets >= 3.0.0 with Plone 4 no longer being supported, so I removed the compatibility code from this pull.

@ale-rt ale-rt merged commit 7ee1f45 into master Jul 7, 2021
@ale-rt ale-rt deleted the datakurre-generate-scales-on-save branch July 7, 2021 06:59
mister-roboto pushed a commit to plone/buildout.coredev that referenced this pull request Jul 7, 2021
Branch: refs/heads/master
Date: 2021-07-07T08:00:26+03:00
Author: Asko Soukka (datakurre) <asko.soukka@iki.fi>
Commit: plone/plone.formwidget.namedfile@83fc204

Add to generate Plone image scales on save when environment variable PLONE_SCALE_GENERATE_ON_SAVE=1 is set

Files changed:
A plone/formwidget/namedfile/datamanager.py
M plone/formwidget/namedfile/configure.zcml
M plone/formwidget/namedfile/interfaces.py
M plone/formwidget/namedfile/tests.py
Repository: plone.formwidget.namedfile

Branch: refs/heads/master
Date: 2021-07-07T08:00:28+03:00
Author: Asko Soukka (datakurre) <asko.soukka@iki.fi>
Commit: plone/plone.formwidget.namedfile@380e12e

Add changelog entry

Files changed:
A news/43.feature
Repository: plone.formwidget.namedfile

Branch: refs/heads/master
Date: 2021-07-07T08:00:29+03:00
Author: Asko Soukka (datakurre) <asko.soukka@iki.fi>
Commit: plone/plone.formwidget.namedfile@47023e8

Update changelog item

Files changed:
M news/43.feature
Repository: plone.formwidget.namedfile

Branch: refs/heads/master
Date: 2021-07-07T08:00:29+03:00
Author: Asko Soukka (datakurre) <asko.soukka@iki.fi>
Commit: plone/plone.formwidget.namedfile@29c6172

Apply comments from review for plone/plone.formwidget.namedfile#43

Files changed:
M plone/formwidget/namedfile/datamanager.py
M plone/formwidget/namedfile/utils.py
Repository: plone.formwidget.namedfile

Branch: refs/heads/master
Date: 2021-07-07T08:00:30+03:00
Author: Asko Soukka (datakurre) <asko.soukka@iki.fi>
Commit: plone/plone.formwidget.namedfile@b7676cb

Change get_scales_info to not depend on _IMREALLYPLONE5-hint

Files changed:
M plone/formwidget/namedfile/utils.py
Repository: plone.formwidget.namedfile

Branch: refs/heads/master
Date: 2021-07-07T09:13:25+03:00
Author: Asko Soukka (datakurre) <asko.soukka@iki.fi>
Commit: plone/plone.formwidget.namedfile@3714ae0

Change to set the feature flag attribute on class definition

Files changed:
M plone/formwidget/namedfile/datamanager.py
Repository: plone.formwidget.namedfile

Branch: refs/heads/master
Date: 2021-07-07T09:19:21+03:00
Author: Asko Soukka (datakurre) <asko.soukka@iki.fi>
Commit: plone/plone.formwidget.namedfile@0026d0f

Remove Plone 4 compatibility

Files changed:
M plone/formwidget/namedfile/utils.py
Repository: plone.formwidget.namedfile

Branch: refs/heads/master
Date: 2021-07-07T08:59:04+02:00
Author: Alessandro Pisa (ale-rt) <alessandro.pisa@gmail.com>
Commit: plone/plone.formwidget.namedfile@7ee1f45

Merge pull request #43 from plone/datakurre-generate-scales-on-save

Add to generate Plone image scales on save (opt-in)

Files changed:
A news/43.feature
A plone/formwidget/namedfile/datamanager.py
M plone/formwidget/namedfile/configure.zcml
M plone/formwidget/namedfile/interfaces.py
M plone/formwidget/namedfile/tests.py
M plone/formwidget/namedfile/utils.py
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.

6 participants