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.

new admin configuration for focal point

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bleen’s picture

bleen’s picture

iStryker’s picture

Title: Setting per image field » Settings per image field
FileSize
1.93 KB

Attach 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).

iStryker’s picture

Adding Screenshot #3 for clarity Change to fields setting page

iStryker’s picture

Issue summary: View changes
FileSize
21.05 KB
6.85 KB

New Patch containing:

  1. Admin settings updated
  2. Update hook to move from old settings to new settings
  3. Fix bug in patch 3 from preventing it showing on node edit form

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.

iStryker’s picture

Status: Active » Needs work

Installed 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.

  • Figure out how to get the field settings in the form alter
  • Add the setting to the entity properties
  • Rewrite module to add a field to the entity, like title and alt.
anbarasan.r’s picture

Add 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.

iStryker’s picture

@anbuindia you have a patch to test?

anbarasan.r’s picture

Please find the attached patch to test it out.

bleen’s picture

Status: Needs work » Needs review

setting to "needs review"

bleen’s picture

  1. +++ b/docroot/profiles/publisher/modules/contrib/focal_point/focal_point.module
    
    
    @@ -158,6 +158,17 @@ function focal_point_form_file_entity_edit_alter(&$form, &$form_state) {
    +    if ($field['type'] == 'image') {
    
    @@ -195,12 +206,55 @@ function _focal_point_form_append_focal_point_preview(&$form, $file) {
    +  if ($field['type'] == 'image') {
    ...
    +    if ($field_type == 'image')
    

    What about media fields? There is a "supported_field_type" function somewhere that we can use

  2. +++ b/docroot/profiles/publisher/modules/contrib/focal_point/focal_point.module
    @@ -158,6 +158,17 @@ function focal_point_form_file_entity_edit_alter(&$form, &$form_state) {
    +    if ($field['type'] == 'image') {
    +      $form[$name][[LANGUAGE_NONE][0]][0]['#media_options']['global']['focal_point'] = $form_state['field'][$name][LANGUAGE_NONE]['instance']['settings']['focal_point'];
    

    Something seems wrong here ... at minimum, it seems like this would not work with a field that has a cardinality > 1

  3. +++ b/docroot/profiles/publisher/modules/contrib/focal_point/focal_point.module
    @@ -195,12 +206,55 @@ function _focal_point_form_append_focal_point_preview(&$form, $file) {
    +  $instance = $context['instance'];
    +  $settings = $instance['settings'];
    

    These two lines can be combined since $instance isnt actually used anywhere

anbarasan.r’s picture

Attached the updated patch

iStryker’s picture

Patch #12 looks much better than #9. I'll try to find some time this week to test.

just.andy.shilton’s picture

I 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.

anbarasan.r’s picture

Re rolling the patch to keep the default value to TRUE to keep the existing fields in enabled status by default.

samberry’s picture

I'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.

+/**
+ * Implements hook_form_FORM_ID_alter().
+ */
+function focal_point_form_node_form_alter(&$form, &$form_state) {
+  foreach($form_state['field'] as $name => $field) {
+    if($name === '#parents') {
+      continue;
+    }
+    $lang = $form[$name]['#language'];
+    if (_focal_point_supported_field_type($field[$lang]['field']['type'])) {
+      $delta = $form[$name][$lang][0]['#delta'];
+      $form[$name][$lang][$delta]['#media_options']['global']['focal_point'] = $form_state['field'][$name][$lang]['instance']['settings']['focal_point'];
+    }
+  }
+}

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.

+/**
+ * Process callback for the media form element.
+ *
+ * @see focal_point_element_info_alter()
+ */
+function focal_point_element_process($element, &$form_state, $form) {
+  // Call parent hook.
+  $element = media_element_process($element, $form_state, $form);
+  if (isset($form_state['field'][$element['#field_name']][$element['#language']]['instance']['settings']['focal_point'])
+  && $form_state['field'][$element['#field_name']][$element['#language']]['instance']['settings']['focal_point'] == 1) {
+    $element['edit']['#href'] = $element['edit']['#href'] . '/use_focal_point';
+  }
+  return $element;
 }
bleen’s picture

Status: Needs review » Needs work
  1. +++ b/docroot/sites/all/modules/contrib/focal_point/focal_point.module
    @@ -138,7 +138,7 @@ function focal_point_file_load($files) {
    +  if (isset($form_state['step']) && $form_state['step'] == 4 && isset($form['#options']['focal_point']) && $form['#options']['focal_point'] == 1) {
    

    This should probably be fixed in a separate issue.

  2. +++ b/docroot/sites/all/modules/contrib/focal_point/focal_point.module
    @@ -153,8 +153,52 @@ function focal_point_form_file_entity_edit_alter(&$form, &$form_state) {
    +      $delta = $form[$name][$lang][0]['#delta'];
    +      $form[$name][$lang][$delta]['#media_options']['global']['focal_point'] = $form_state['field'][$name][$lang]['instance']['settings']['focal_point'];
    

    We only want to do this if media module exists, no? We should check for that

  3. +++ b/docroot/sites/all/modules/contrib/focal_point/focal_point.module
    @@ -153,8 +153,52 @@ function focal_point_form_file_entity_edit_alter(&$form, &$form_state) {
    +  if (isset($types['media'])) {
    

    for good luck we should also check that module_exists(media).

  4. +++ b/docroot/sites/all/modules/contrib/focal_point/focal_point.module
    @@ -153,8 +153,52 @@ function focal_point_form_file_entity_edit_alter(&$form, &$form_state) {
    +    // See media module media_element_info().
    

    @see

  5. +++ b/docroot/sites/all/modules/contrib/focal_point/focal_point.module
    @@ -153,8 +153,52 @@ function focal_point_form_file_entity_edit_alter(&$form, &$form_state) {
    +function focal_point_element_process($element, &$form_state, $form) {
    

    If this is only meant for media fields then we should add "media" to the function name

  6. +++ b/docroot/sites/all/modules/contrib/focal_point/focal_point.module
    @@ -153,8 +153,52 @@ function focal_point_form_file_entity_edit_alter(&$form, &$form_state) {
    +    $element['edit']['#href'] = $element['edit']['#href'] . '/use_focal_point';
    

    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

iStryker’s picture

@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.

bleen’s picture

@18: I'mnot following. Can you be more specific?

iStryker’s picture

@bleen, I am just looking at the code in the post

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.

Now that I took a second look, I see it was introduced in patch #9.

millionleaves’s picture

I 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).

Liam Morland’s picture

The patch #16 needs to be re-rolled; Git complains that it is corrupt.

ron_s’s picture

Status: Needs work » Needs review
FileSize
20.6 KB

Attached is a new patch for review. This version has a number of updates, including the following:

  1. Incorporates feedback from @bleen in #17.
  2. Incorporates feedback from @iStryker and @millionleaves to apply to all entities, not just nodes.
  3. Changes approach for adding previews to file entities. The previous version would not support an image field added to a file entity. A use case where we need this to work are video bundles that have an additional image field used as a preview image (background image when a video is paused).
  4. Adds support to enable/disable the "Image Preview" link per field. We have a number of cases where we don't want it shown.
  5. Adds support to select which image styles are shown on the "Image Preview" page, per field. We have a large number of focal point-supported image styles, and only some of them should be shown with certain image fields.
  6. Adds support to enable/disable focal point support for file entities with image mimetypes.
  7. Cleans up some typos, including one that causes a current field description to not be shown.
  8. Combines hook_form_file_entity_edit_alter and hook_form_alter into one function.

Let me know your feedback, thanks.

ron_s’s picture

Also to be clear, this patch is against 7.x-1.x-dev. It will not apply cleanly to beta6.

muschpusch’s picture

Status: Needs review » Reviewed & tested by the community

Thanks a lot for this patch! It works fine.

iStryker’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
22.3 KB

Re-roll against the latest dev

iStryker’s picture

Status: Needs review » Needs work
FileSize
36.67 KB

Looks 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.

millionleaves’s picture

FileSize
32.01 KB

Patch 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:

updated admin configuration for focal point

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.

ron_s’s picture

Status: Needs work » Needs review
FileSize
25.85 KB

@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=c5cdf56

Also 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.

ron_s’s picture

Issue summary: View changes
FileSize
78.59 KB

Also here is a screenshot of how the admin page should look after applying the patch to 7.x-1.x-dev.

ron_s’s picture

Status: Needs review » Needs work

I think I found an issue in the patch I just posted. Let me make some changes and roll a new version.

ron_s’s picture

Status: Needs work » Needs review
FileSize
25.88 KB

Ok, 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.

ron_s’s picture

Found a couple of lingering bugs that caused error messages to be displayed on output.

See the updated patch and interdiff with changes. Thanks.

ron_s’s picture

Confirming that patch #33 still applies cleanly to the newest 7.x-1.x-dev posted on Sept 11.

millionleaves’s picture

Status: Needs review » Reviewed & tested by the community

Confirming 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.

ron_s’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
26.48 KB
1.04 KB

Have 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.

firewaller’s picture

+1

firewaller’s picture

Patch #36 works for me.

ron_s’s picture

Patch #36 still applies cleanly to the new Focal Point 7.x-1.2 release.

Liam Morland’s picture

Status: Needs review » Reviewed & tested by the community

University of Waterloo has been running it for over a year.

ron_s’s picture

Thanks Liam. We've been running it on a number of sites for a while with no issues.

yan’s picture

Patch from #36 applied fine with latest 1.2 Version of the module.

LeDucDuBleuet’s picture

The 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!

markabur’s picture

Status: Reviewed & tested by the community » Needs work

I'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 the focal_point_previewsetting, but I can't figure out how to check that setting for the field.