Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Request:
Currently there is only a setting for all image fields. It would be a great feature to have this on a per field bias.
Motivation:
Multiple image fields across multiple content types. Focal crop style is only use on one image field for the entire site. It is confusing for content editors to have, choosing the focal point, image preview link and help text, on images that will never use it.
Comment | File | Size | Author |
---|---|---|---|
#36 | interdiff-2480947-33-36.txt | 1.04 KB | ron_s |
#36 | focal_point-settings_per_field-2480947-36.patch | 26.48 KB | ron_s |
#30 | focal_point_admin_page.png | 78.59 KB | ron_s |
#28 | focal_point_defaults.png | 32.01 KB | millionleaves |
#27 | Default_image_focal_point.png | 36.67 KB | iStryker |
Comments
Comment #1
bleen CreditAttribution: bleen commentedComment #2
bleen CreditAttribution: bleen commentedComment #3
iStryker CreditAttribution: iStryker commentedAttach is a patch that makes it works per field. This is a clone of how I do it for the image_field_caption module.
Still need to figure out the admin interface. Currently there is 2 options. All for image fields. All for media image fields. This patch does not do media image fields (at least to my knowledge).
Comment #4
iStryker CreditAttribution: iStryker commentedAdding Screenshot #3 for clarity
Comment #5
iStryker CreditAttribution: iStryker commentedNew Patch containing:
When you enable the module, it turns on focal point for every image field by default. To prevent this expect behavior from disappearing, I have created an update hook 7104. Moving forward, When you download and enabled the module, it will be enabled per image field, but not on by default.
TODO get it working for Media image field and test.
Comment #6
iStryker CreditAttribution: iStryker commentedInstalled File Entity and Media. The settings all show up correctly. Focal point still shows up correctly. The only problem you cannot turn it off.
In
focal_point_form_field_ui_field_edit_form_alter
It has no reference to the field settings. All it has is the file entity.Three options I can think of.
Comment #7
anbarasan.r CreditAttribution: anbarasan.r commentedAdd the field settings value to the media field of ['#media_options']['global'] array and get the values from $form['#options'] in focal_point_form_field_ui_field_edit_form_alter function and write the condition to display the focal point.
Comment #8
iStryker CreditAttribution: iStryker commented@anbuindia you have a patch to test?
Comment #9
anbarasan.r CreditAttribution: anbarasan.r commentedPlease find the attached patch to test it out.
Comment #10
bleen CreditAttribution: bleen commentedsetting to "needs review"
Comment #11
bleen CreditAttribution: bleen commentedWhat about media fields? There is a "supported_field_type" function somewhere that we can use
Something seems wrong here ... at minimum, it seems like this would not work with a field that has a cardinality > 1
These two lines can be combined since $instance isnt actually used anywhere
Comment #12
anbarasan.r CreditAttribution: anbarasan.r commentedAttached the updated patch
Comment #13
iStryker CreditAttribution: iStryker commentedPatch #12 looks much better than #9. I'll try to find some time this week to test.
Comment #14
just.andy.shilton CreditAttribution: just.andy.shilton commentedI have applied #12 on one of my sites and have to admit it's working very well indeed. Is a very nice feature to be able to select which fields do/don't use focal point.
Thanks for the work, very much appreciated. I hope this patch makes it into a future release.
Comment #15
anbarasan.r CreditAttribution: anbarasan.r commentedRe rolling the patch to keep the default value to TRUE to keep the existing fields in enabled status by default.
Comment #16
samberry CreditAttribution: samberry at Deeson commentedI've applied #15 on one of my sites and it seems to work very well. But I noticed a couple of issues that I've addressed and added to a patch.
Firstly in the following function I have changed the foreach loop so that it is not looping over all of the fields on the site, but only those that are on the node form. Also, I have changed the way that the delta is being extracted as the "$form[$name][$lang]['#file_upload_delta']" variable is only set by the media module when the field has a cardinality of > 1. So this is not set if the cardinality is 1. Finally I have changed the way the type is referenced as it didn't seem to be set at $field['type'] on my configuration.
Also, on focal_point_element_process I have added an isset around the focal point setting and checked to see if this is equal to 1. This is because we are using paragraphs (https://www.drupal.org/project/paragraphs) in our configuration, this variable is not always available here.
Comment #17
bleen CreditAttribution: bleen commentedThis should probably be fixed in a separate issue.
We only want to do this if media module exists, no? We should check for that
for good luck we should also check that module_exists(media).
@see
If this is only meant for media fields then we should add "media" to the function name
This seems really hackey ... I get that you are trying to avoid checking every field on every node edit form before displaying the preview, but this is an ugly solution
Comment #18
iStryker CreditAttribution: iStryker commented@16, I think it should be for all fields, not node forms, because the field could be added to an entity that is not a node type.
Comment #19
bleen CreditAttribution: bleen commented@18: I'mnot following. Can you be more specific?
Comment #20
iStryker CreditAttribution: iStryker commented@bleen, I am just looking at the code in the post
Now that I took a second look, I see it was introduced in patch #9.
Comment #21
millionleaves CreditAttribution: millionleaves as a volunteer and commentedI think the point in #18 relates to the fact that image fields can be added to entities that are not nodes, i.e. taxonomy terms, users, and custom entity types such as Commerce products. This means we need to be able to disable Focal Point on individual image fields on all entities, not just nodes.
I note that the above patches break on the latest release (beta-6).
Comment #22
Liam MorlandThe patch #16 needs to be re-rolled; Git complains that it is corrupt.
Comment #23
ron_s CreditAttribution: ron_s commentedAttached is a new patch for review. This version has a number of updates, including the following:
hook_form_file_entity_edit_alter
andhook_form_alter
into one function.Let me know your feedback, thanks.
Comment #24
ron_s CreditAttribution: ron_s commentedAlso to be clear, this patch is against 7.x-1.x-dev. It will not apply cleanly to beta6.
Comment #25
muschpusch CreditAttribution: muschpusch at Factorial GmbH commentedThanks a lot for this patch! It works fine.
Comment #26
iStryker CreditAttribution: iStryker commentedRe-roll against the latest dev
Comment #27
iStryker CreditAttribution: iStryker commentedLooks like I get an undefined index on 938 with the new patch when I preview an image. In addition what happens to image default settings (see attached image). It's been awhile since I touch this issue, but what I recall is that with adding this feature every new image field could have this setting turned on by default.
Comment #28
millionleaves CreditAttribution: millionleaves as a volunteer and commentedPatch in #26 works for me on a clean install. I'm not experiencing the undefined index issue referenced in #27.
I was confused by the screenshot uploaded in #27, which doesn't match what I'm seeing with this patch:
However, I think the point being made is that there is no option with this patch to set the default (on/off) on image fields. Upon applying this patch, it was immediately enabled on all my image fields. I don't believe this should happen by default, and would prefer that the focal point option be disabled on installation, so it can be enabled field by field as needed. A single configuration page to turn it on/off for all image fields would be nice but isn't necessary IMHO.
Other than that, I'm loving the features added by this patch. So much better than explaining to customers (or writing inline help to explain) that the focal point feature doesn't work for all images.
Comment #29
ron_s CreditAttribution: ron_s commented@iStryker, in your re-roll, the code for
$default_value
was not included in the_focal_point_form_append_focal_point_preview
function, and actually causes it to be removed. It was a commit that was added in December: http://cgit.drupalcode.org/focal_point/commit/?id=c5cdf56Also there were a few places that the second line in comments for
@param
values had been changed from two spaces to one. The correct coding standard is to have two spaces.@millionleaves, the screenshot you posted in #28 is correct. That is what you should be seeing with patch #26. My guess is the original image posted by iStryker is a mockup, because I cannot find anything referencing those checkboxes in the repository.
As for the comment about having the focal point option disabled by default, I think it really depends on the need. For example, we have sites with hundreds of image fields, and all of them need Focal Point enabled. Expecting the customer's staff to set each field individually would be a nightmare. I can also understand that others might want it disabled by default.
I've updated the patch to incorporate this concept for default settings. Please take a look and let me know your feedback.
Comment #30
ron_s CreditAttribution: ron_s commentedAlso here is a screenshot of how the admin page should look after applying the patch to 7.x-1.x-dev.
Comment #31
ron_s CreditAttribution: ron_s commentedI think I found an issue in the patch I just posted. Let me make some changes and roll a new version.
Comment #32
ron_s CreditAttribution: ron_s commentedOk, looks like the issues are fixed. Had a couple of other spots that needed to be modified to support the default options.
Please review the attached patch. Thanks.
Comment #33
ron_s CreditAttribution: ron_s commentedFound a couple of lingering bugs that caused error messages to be displayed on output.
See the updated patch and interdiff with changes. Thanks.
Comment #34
ron_s CreditAttribution: ron_s commentedConfirming that patch #33 still applies cleanly to the newest 7.x-1.x-dev posted on Sept 11.
Comment #35
millionleaves CreditAttribution: millionleaves as a volunteer and commentedConfirming that patch #33 still applies cleanly to the newest 7.x-1.x-dev posted on 4 Oct 2017.
Also confirming that I've been using this patch for a while on several sites without issue. Marking Reviewed and Tested. Would be great to get this committed.
Comment #36
ron_s CreditAttribution: ron_s commentedHave found a couple of new bugs. If the Media module is being used in a custom Form API form (as described here: https://www.drupal.org/node/2288487), patch #33 will cause a fatal error "white screen of death."
Custom form fields have no
$form_state['field']
values. Eventually this needs to be fixed so Focal Point can support generic Form API forms, but for right now I just break out of the function to let processing continue.There is also code in the
focal_point_form_alter
that needs to account for situations where$form[$name]
is empty, as can be the case with fields on commerce products.Please see the attached for review, thanks.
Comment #37
firewaller CreditAttribution: firewaller commented+1
Comment #38
firewaller CreditAttribution: firewaller commentedPatch #36 works for me.
Comment #39
ron_s CreditAttribution: ron_s commentedPatch #36 still applies cleanly to the new Focal Point 7.x-1.2 release.
Comment #40
Liam MorlandUniversity of Waterloo has been running it for over a year.
Comment #41
ron_s CreditAttribution: ron_s commentedThanks Liam. We've been running it on a number of sites for a while with no issues.
Comment #42
yan CreditAttribution: yan commentedPatch from #36 applied fine with latest 1.2 Version of the module.
Comment #43
LeDucDuBleuet CreditAttribution: LeDucDuBleuet commentedThe patch is still working fine in 2021, it would be great if this was included in a new release! Thank you for a great module!
Comment #44
markabur CreditAttribution: markabur commentedI'm trying #36 with a regular imagefield and I still see the "Image Preview" link even though that option is unchecked for the field.
I can see that
focal_point_preprocess_image_widget()
needs to change depending on thefocal_point_preview
setting, but I can't figure out how to check that setting for the field.