Ticket #13337 (closed Feature Request: fixed)

Opened 17 months ago

Last modified 5 months ago

plone.app.imaging should allow different default image quality to be set

Reported by: khink Owned by: khink
Priority: minor Milestone: 4.x
Component: General Version: 4.2
Keywords: patch Cc:

Description (last modified by khink) (diff)

plone.scale.scale.scaleImage uses a default image quality of 88 when rescaling ( https://github.com/plone/plone.scale/blob/master/plone/scale/scale.py). This may be too detailed for some, and too sloppy for others.

It would be a good thing if the scaled image quality were a setting in plone.app.imaging.

Change History

comment:1 Changed 17 months ago by khink

  • Type changed from Bug to Feature Request

comment:2 Changed 17 months ago by khink

  • Description modified (diff)

comment:3 Changed 17 months ago by kleist

  • Status changed from new to confirmed
  • Component changed from Unknown to General

comment:4 Changed 17 months ago by khink

  • Owner set to khink

comment:6 Changed 17 months ago by khink

  • Keywords patch added

comment:7 Changed 17 months ago by khink

And a fix for plone.namedfile. This makes your custom content types' image fields use the setting from plone.app.imaging, if it exists.  https://github.com/plone/plone.namedfile/pull/5

comment:8 Changed 11 months ago by khink

This approach is flawed, it introduces a dependency of plone.namedfile to plone.app.imaging.

I closed  https://github.com/plone/plone.app.imaging/pull/2 without merging. I reverted ( https://github.com/plone/plone.namedfile/commit/051c705b65dfb3fa88d213547ab6f8e5d50ecf0d) the changes introduced by  https://github.com/plone/plone.namedfile/pull/5.

Last edited 11 months ago by khink (previous) (diff)

comment:9 Changed 11 months ago by khink

Dependencies of related packages (p.a.blob and p.a.dexterity also included because people might use Plone without TinyMCE):

        Products.CMFPlone (4.2.x, 4.3.x, 4.4)
                    |       |
                    |       | CMFPlone < 4.4 
                    |       v
plone.app.dexterity |   plone.app.blob
    |               v           |
    |       Products.TinyMCE    |
    |       |           |       |
    |       v           v       v
    v plone.namedfile   plone.app.imaging
    [scales]                |
        |                   |
        |                   |
        v                   v
        plone.scale [storage]

I propose to define IScaledImageQuality in plone.scale. We can then use it in plone.app.imaging to let users set their images' scaling quality via the control panel.

See  https://github.com/plone/plone.app.imaging/pull/2 for some previously proposed code and discussion, and  https://github.com/plone/plone.namedfile/commit/051c705b65dfb3fa88d213547ab6f8e5d50ecf0d for the changes to plone.namedfile.

comment:10 Changed 11 months ago by khink

Some other points i took from the discussion in  https://github.com/plone/plone.app.imaging/pull/2, i think this sums them all up:

comment:11 Changed 11 months ago by khink

This involves three packages so it now has a buildout cfg of its own:  https://github.com/plone/buildout.coredev/blob/4.3/experimental/configurable-scaling-quality-13337.cfg

I cherry-picked the code from plone.namedfile and plone.app.imaging and made the necessary changes. Here are the branches:

There's no hard dependency on plone.scale:

  • plone.app.imaging has a zcml:condition="installed plone.scale.interfaces
  • plone.namedfile does a try-except import.

If you don't have the latest plone.scale, Plone will still start although your scaling quality setting won't take effect.

The final commit in plone.app.imaging ( https://github.com/plone/plone.app.imaging/commit/fcf6477537782e817166032dced940d794c587b2) *is* necessary to make this do anything for default Plone. I don't see much harm there. I agree with witsch ( https://github.com/plone/plone.app.imaging/pull/2#commitcomment-2227868) that "let's not monkey-patch even more stuff in AT's ImageField (but instead change it there)", but that should be a different issue. For good measure: #13583

Last edited 11 months ago by khink (previous) (diff)

comment:12 Changed 11 months ago by khink

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

All branches merged for all three.

comment:13 Changed 5 months ago by khink

In  https://github.com/plone/plone.app.imaging/commit/eefd46ae07d748625dd0d28565148b75fe4be50b, the quality setting was removed from plone.app.imaging (moved to Products.CMFPlone).

Note: See TracTickets for help on using tickets.