From 85874eb36fc95485ba609601c5035cb507ffd79b Mon Sep 17 00:00:00 2001 From: Maurits van Rees Date: Fri, 12 Aug 2022 16:16:20 +0200 Subject: [PATCH 1/2] Use "scale" mode as default. This cleans up more confusion between mode and direction. See also https://github.com/plone/plone.namedfile/issues/102. Previously our definition of the `IImageScaleFactory` interface had the deprecated `direction="thumbnail"`. Other parts used `mode="contain"` by default, which does cropping, where in Plone we are used to simple scaling almost everywhere. Depending on your point of view, you could call this a bug fix (fixing the default to what Plone expects, to avoid surprises). Or you could call this a breaking change, because the default changes. It is beta stage, so I am okay with it either way. --- news/102.bugfix | 6 ++++++ plone/scale/interfaces.py | 10 ++++++---- plone/scale/scale.py | 22 +++++++++++----------- plone/scale/storage.py | 3 ++- plone/scale/tests/test_scale.py | 13 ++++++++----- plone/scale/tests/test_storage.py | 15 +++++++++++++-- 6 files changed, 46 insertions(+), 23 deletions(-) create mode 100644 news/102.bugfix diff --git a/news/102.bugfix b/news/102.bugfix new file mode 100644 index 0000000..61a9734 --- /dev/null +++ b/news/102.bugfix @@ -0,0 +1,6 @@ +Use "scale" mode as default. +This cleans up more confusion between mode and direction. +See also `plone.namedfile issue 102 `_. +Previously our definition of the ``IImageScaleFactory`` interface had the deprecated ``direction="thumbnail"``. +Other parts used ``mode="contain"`` by default, which does cropping, where in Plone we are used to simple scaling almost everywhere. +[maurits] diff --git a/plone/scale/interfaces.py b/plone/scale/interfaces.py index 87103e0..41a9b72 100644 --- a/plone/scale/interfaces.py +++ b/plone/scale/interfaces.py @@ -12,9 +12,9 @@ class IScaledImageQuality(Interface): class IImageScaleFactory(Interface): """Creates a scale""" - def _call__( + def __call__( fieldname=None, - direction="thumbnail", + mode="scale", height=None, width=None, scale=None, @@ -30,8 +30,10 @@ def _call__( ``fieldname`` name of the field to scale - ``direction`` - is same as PIL direction on scale + ``mode`` + See ``scalePILImage`` for the values that should be accepted. + This used to be called "direction", which may still be accepted, + but is deprecated. ``width`` and ``height`` target size diff --git a/plone/scale/scale.py b/plone/scale/scale.py index 3fe7be6..bb8b42d 100644 --- a/plone/scale/scale.py +++ b/plone/scale/scale.py @@ -45,7 +45,7 @@ def scaleImage( image, width=None, height=None, - mode="contain", + mode="scale", quality=88, result=None, direction=None, @@ -153,17 +153,17 @@ def _scale_thumbnail(image, width=None, height=None): def get_scale_mode(mode, direction=None): if direction is not None: warnings.warn( - "the 'direction' option is deprecated, use 'mode' instead", + "The 'direction' option is deprecated, use 'mode' instead.", DeprecationWarning, ) mode = direction - if mode in ("scale-crop-to-fit", "down"): - mode = "contain" - elif mode in ("scale-crop-to-fill", "up"): - mode = "cover" - elif mode in ("keep", "thumbnail"): - mode = "scale" + if mode in ("scale", "keep", "thumbnail", None): + return "scale" + if mode in ("contain", "scale-crop-to-fit", "down"): + return "contain" + if mode in ("cover", "scale-crop-to-fill", "up"): + return "cover" return mode @@ -178,7 +178,7 @@ def __init__(self, original_width=0, original_height=0): def _calculate_all_dimensions( - original_width, original_height, width, height, mode="contain" + original_width, original_height, width, height, mode="scale" ): """Calculate all dimensions we need for scaling. @@ -334,7 +334,7 @@ def _calculate_all_dimensions( def calculate_scaled_dimensions( - original_width, original_height, width, height, mode="contain" + original_width, original_height, width, height, mode="scale" ): """Calculate the scaled image dimensions from the originals using the same logic as scalePILImage""" @@ -345,7 +345,7 @@ def calculate_scaled_dimensions( return (dimensions.final_width, dimensions.final_height) -def scalePILImage(image, width=None, height=None, mode="contain", direction=None): +def scalePILImage(image, width=None, height=None, mode="scale", direction=None): """Scale a PIL image to another size. This is all about scaling for the display in a web browser. diff --git a/plone/scale/storage.py b/plone/scale/storage.py index dbde4a3..3986d4a 100644 --- a/plone/scale/storage.py +++ b/plone/scale/storage.py @@ -241,7 +241,8 @@ def pre_scale(self, **parameters): height = parameters.get("height") orig_width, orig_height = value.getImageSize() mode = get_scale_mode( - parameters.get("direction") or parameters.get("mode") or "contain" + parameters.get("mode"), + parameters.get("direction") ) width, height = calculate_scaled_dimensions( orig_width, orig_height, width, height, mode diff --git a/plone/scale/tests/test_scale.py b/plone/scale/tests/test_scale.py index d620661..4ffaf42 100644 --- a/plone/scale/tests/test_scale.py +++ b/plone/scale/tests/test_scale.py @@ -226,6 +226,7 @@ def testAlternativeSpellings(self): ``direction`` arguments instead of ``mode``. """ + # Test mode contain. This can do cropping. # scale-crop-to-fit img = PIL.Image.new("RGB", (20, 20), (0, 0, 0)) img_scaled = scalePILImage(img, 10, 5, direction="scale-crop-to-fit") @@ -335,19 +336,19 @@ def testDeprecations(self): scaleImage(PNG, 16, 16, direction="keep") self.assertEqual(len(w), 1) self.assertIs(w[0].category, DeprecationWarning) - self.assertIn("the 'direction' option is deprecated", str(w[0].message)) + self.assertIn("The 'direction' option is deprecated", str(w[0].message)) def test_calculate_scaled_dimensions_contain(self): - """Test the calculate_scaled_dimensions function. + """Test the calculate_scaled_dimensions function with mode "contain". You pass it: original_width, original_height, width, height - Plus an optional mode, by default "contain"`. + Plus an optional mode, by default "scale"`. Alternative spellings: `scale-crop-to-fit`, `down`. """ - calc = calculate_scaled_dimensions + calc = functools.partial(calculate_scaled_dimensions, mode="contain") self.assertEqual(calc(1, 1, 1, 1), (1, 1)) self.assertEqual(calc(10, 10, 1, 1), (1, 1)) self.assertEqual(calc(1, 1, 10, 10), (10, 10)) @@ -381,8 +382,10 @@ def test_calculate_scaled_dimensions_scale(self): """Test calculate_scaled_dimensions function with mode "scale". Alternative spellings: `keep`, `thumbnail`. + "scale" is the default """ - calc = functools.partial(calculate_scaled_dimensions, mode="scale") + # calc = functools.partial(calculate_scaled_dimensions, mode="scale") + calc = calculate_scaled_dimensions self.assertEqual(calc(1, 1, 1, 1), (1, 1)) self.assertEqual(calc(10, 10, 1, 1), (1, 1)) # Mode "scale" only scales down, not up: diff --git a/plone/scale/tests/test_storage.py b/plone/scale/tests/test_storage.py index e643fe0..e98531b 100644 --- a/plone/scale/tests/test_storage.py +++ b/plone/scale/tests/test_storage.py @@ -112,7 +112,9 @@ def testPreScaleForNonExistingScale(self): self.assertIn("key", scale) self.assertEqual(scale["data"], None) self.assertEqual(scale["width"], 50) - self.assertEqual(scale["height"], 80) + # Note: the original image is 60x40. + # By default we scale without cropping, so you do not get height 80. + self.assertEqual(scale["height"], 33) self.assertEqual(scale["mimetype"], "image/jpeg") # Request the same pre scale. scale2 = storage.pre_scale(width=50, height=80) @@ -129,6 +131,15 @@ def testPreScaleForNonExistingScale(self): self.assertEqual(new_scale["height"], 23) self.assertEqual(new_scale["mimetype"], "image/png") + # Try cropping as well. + scale = storage.pre_scale(width=50, height=80, mode="contain") + self.assertIn("uid", scale) + self.assertIn("key", scale) + self.assertEqual(scale["data"], None) + self.assertEqual(scale["width"], 50) + self.assertEqual(scale["height"], 80) + self.assertEqual(scale["mimetype"], "image/jpeg") + def testPreScaleForNonExistingField(self): self._provide_dummy_scale_adapter(None) storage = self.storage @@ -151,7 +162,7 @@ def test_get_or_generate(self): self.assertEqual(placeholder["uid"], uid) self.assertIsNone(placeholder["data"]) self.assertEqual(placeholder["width"], 50) - self.assertEqual(placeholder["height"], 80) + self.assertEqual(placeholder["height"], 33) self.assertEqual(placeholder["mimetype"], "image/jpeg") # 'get_or_generate' gets the pre generated placeholder info # and generates the scale. From b1ba10f71f56d4e7e2ffc956f162e0160ee0884f Mon Sep 17 00:00:00 2001 From: Maurits van Rees Date: Thu, 29 Sep 2022 12:34:49 +0200 Subject: [PATCH 2/2] No longer test Plone 5.2 on 3.6 and Plone 6.0 on 3.7. --- .github/workflows/tests.yml | 2 -- news/3637.breaking | 2 ++ tox.ini | 6 ++---- 3 files changed, 4 insertions(+), 6 deletions(-) create mode 100644 news/3637.breaking diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 25cbef5..79cf5b2 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -12,10 +12,8 @@ jobs: matrix: config: # [Python version, tox env] - - ["3.6", "plone52-py36"] - ["3.7", "plone52-py37"] - ["3.8", "plone52-py38"] - - ["3.7", "plone60-py37"] - ["3.8", "plone60-py38"] - ["3.9", "plone60-py39"] - ["3.10", "plone60-py310"] diff --git a/news/3637.breaking b/news/3637.breaking new file mode 100644 index 0000000..000e271 --- /dev/null +++ b/news/3637.breaking @@ -0,0 +1,2 @@ +No longer test Plone 5.2 on 3.6 and Plone 6.0 on 3.7. +[maurits] diff --git a/tox.ini b/tox.ini index 053ec86..543f7fa 100644 --- a/tox.ini +++ b/tox.ini @@ -1,9 +1,7 @@ [tox] envlist = - plone52-py36, plone52-py37, plone52-py38, - plone60-py37, plone60-py38, plone60-py39, plone60-py310, @@ -18,13 +16,13 @@ commands = pip list zope-testrunner --test-path={toxinidir} {posargs:-vc} -[testenv:plone52-py{36,37,38}] +[testenv:plone52-py{37,38}] commands_pre = pip install mxdev mxdev -c sources-52.ini pip install -rrequirements-52-mxdev.txt -[testenv:plone60-py{37,38,39,310}] +[testenv:plone60-py{38,39,310}] commands_pre = # for libvcs pin, see https://github.com/bluedynamics/mxdev/issues/10 pip install mxdev "libvcs<0.12"