Updated: Comment #0

Problem/Motivation

Currently to enforce image crop and option can be set to make image crop mandatory. It is also possible to define a default crop size, which is pre-selected.

It is also possible to open the crop tool automatically.

Still the workflow requires many clicks just to get to a form that submits and validates correctly:

1) Select image
2) Click upload
3) Click "crop"
4) Click "save"

Rinse and repeat for all images to be uploaded.

Proposed resolution

Add an option to automatically crop already when the image is uploaded.

The crop can still be customized, but a crop is selected by default, by copying the JS from manualcrop.js and setting as #default_value.

Remaining tasks

* Review patch.

User interface changes

* Add a new option to set automatic default crop.

API changes

* Add manualcrop_default_crop_size function.

CommentFileSizeAuthor
#13 automatic_default_crop-2145939-13.patch4.35 KBnicola85
#12 interdiff.txt793 bytesDavid_Rothstein
#12 manualcrop--automatic_default_crop--2145939-12-APPROACH-FROM-EARLIER-IN-ISSUE.patch4.48 KBDavid_Rothstein
#9 manualcrop--automatic_default_crop--2145939-9.patch3.11 KBsardara
#8 manualcrop--automatic_default_crop--2145939-8.patch4.2 KBDavid_Rothstein
#4 manualcrop--automatic_default_crop--1.5.diff4.15 KBlucas.constantino
#1 manualcrop--automatic_default_crop--2145939.diff4.34 KBFabianx
manualcrop.diff4.35 KBFabianx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented
FileSize
manualcrop--automatic_default_crop--2145939.diff4.34 KB
Matthijs’s picture

Matthijs
Primary language Dutch
CreditAttribution: Matthijs commented

Thanks for the patch. I like your idea, but I'm wondering if it wouldn't be better to handle cropping at client side (JavaScript)? This way the crop-logic remains together and can be reused, instead of having PHP and JavaScript logic for all possible crop selections.

To do this the same code as for the default crop selection can be used, it should be triggered for each style after uploading...

What do you think?

Tess Bakker’s picture

Tess Bakker
they/them
Primary language Dutch
CreditAttribution: Tess Bakker as a volunteer commented
Category: Task » Feature request
Status: Needs review » Needs work

Nice feature! As Matthijs suggested, JS instead of PHP would be great.

lucas.constantino’s picture

lucas.constantino CreditAttribution: lucas.constantino commented
FileSize
manualcrop--automatic_default_crop--1.5.diff4.15 KB

Nice patch. Reroled it to version 1.5.

lucas.constantino’s picture

lucas.constantino CreditAttribution: lucas.constantino commented
Version: 7.x-1.x-dev » 7.x-1.5
Tess Bakker’s picture

Tess Bakker
they/them
Primary language Dutch
CreditAttribution: Tess Bakker as a volunteer commented
Status: Needs work » Closed (works as designed)

After some thinking, this isn't needed.

A required cropping is of course 'required' and user input is needed, therefore not automatically done by the system.

If an optional cropping needs a default fallback, just add 'Scale and Crop' or any other image effect as a fallback after the Manual Crop image effect.

Please reopen this issue if this statement is false.

lucas.constantino’s picture

lucas.constantino CreditAttribution: lucas.constantino commented
Status: Closed (works as designed) » Reviewed & tested by the community

I believe the statement is indeed false. A case where it would be false is when the aspect is not pre-defined; in that case, if a user selects a custom cropping - with an aspect different from the "Scale and Crop" filter below the Manual one - the image would get "re-cropped" to an undesired result.

Is it clear? Sounded odd reading it a second time :)

About the status, I'm changing it to "Reviewed and tested" because even though I agree that a JavaScript solution would make more sense, the one in the server-side is working pretty good and makes this featured completed. Anyone willing to redo it in the client-side would be just creating an improvement, not resolving the original feature request. Unless the maintainers understand otherwise.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein at Tag1 Consulting commented
Status: Reviewed & tested by the community » Needs review
FileSize
manualcrop--automatic_default_crop--2145939-8.patch4.2 KB 
       '#default_value' => (isset($value['manualcrop_selections'][$style_name]) ? $value['manualcrop_selections'][$style_name] : ''),
+      '#default_value' => (isset($value['manualcrop_selections'][$style_name]) ? $value['manualcrop_selections'][$style_name] : manualcrop_get_default_crop_size($fid, $style_name, $settings)),

That definitely doesn't look right. I rerolled it to remove the existing line instead.

sardara’s picture

sardara CreditAttribution: sardara at Randstad Digital commented
FileSize
manualcrop--automatic_default_crop--2145939-9.patch3.11 KB

We need the same default crop behaviour in our distribution. But we need to cover also default crop for previously uploaded images.
This is needed because we use Scald as media library, so the user can pick an already existing image and then select a crop size to be used.
So I've extended the manualcrop_crop effect to apply an image crop if no manualcrop data is selected.
The user can configure this setting in the effect form, along with the possibility to maximize the crop area, like it's done in the crop tool.

PieterDC’s picture

PieterDC
Primary language Dutch
Location Ghent
CreditAttribution: PieterDC at Randstad Digital commented

I reviewed Sardara his patch and I approved it for use in our distro. It nicely does the fallback without marking all image crop styles as ' (cropped)' which otherwise could confuse the user because she/he didn't define a crop selection for those styles.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx at Tag1 Consulting commented
Status: Needs review » Reviewed & tested by the community

Yes, that makes sense to me and is likely for most cases the better fix.

However for existing sites enabling the option might be disruptive as suddenly images that had not been cropped, would be cropped now.

So maybe we will keep #8 for ourselves.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein at Tag1 Consulting commented
FileSize
manualcrop--automatic_default_crop--2145939-12-APPROACH-FROM-EARLIER-IN-ISSUE.patch4.48 KB
interdiff.txt793 bytes

I'm not sure which approach is best, but I'm using #8 and noticed that manualcrop_get_default_crop_size() takes a file ID as input, then loads the file to get the URI. If file entity revisions are being used, this limits the function to working with the current version of the file only.

I have a use case that wants to call this function with older file revisions (that can have different URIs) so here's a simple reroll of #8 that breaks up the function so as to create a helper for that purpose.

This patch is not related to #9 so I won't change the RTBC status.

nicola85’s picture

nicola85 CreditAttribution: nicola85 commented
FileSize
automatic_default_crop-2145939-13.patch4.35 KB