Improve formatter summaries

1. +++ b/jw_player.module
   @@ -118,9 +118,11 @@ function jw_player_field_formatter_settings_form($field, $instance, $view_mode,
          '#default_value' => ($settings['jwplayer_preset']) ?  $settings['jwplayer_preset'] : FALSE,
          '#options' => $options,
   +      '#weight' => 0,
        );
   

   I like to keep some spacing when using explicit weights. E.g. 0 ... 5 ... 10. Otherwise, if you ever want to add a new option, you have to move them all around again.

   The other approach would be to move the fields into the proper order instead of relying on #weight. Would make the patch a lot bigger, but if we try to keep other changes in the settings form as minimal as possible, then I'd be OK with that.

   Not exactly sure what you mean with not showing when default is selected.

2. +++ b/jw_player.module
   @@ -218,36 +225,114 @@ function jw_player_field_formatter_settings_summary($field, $instance, $view_mod
   +          case 'sharing_sites_show':
   +            $val_sharing = array();
   +            foreach ($val as $item_key => $item_val) {
   +              if ($item_val) {
   +                switch ($item_key) {
   +                  case 'googleplus':
   +                    $format_key = 'Google Plus';
   +                    break;
   +                  case 'linkedin':
   +                    $format_key = 'LinkedIn';
   +                    break;
   +                  default:
   +                    $format_key = drupal_ucfirst($item_key);
   +                    break;
   +                }
   

   You mentioned something about sharing, I think that's the part for showing it, but it's not actually in yet?

   Btw, have you seen https://www.drupal.org/project/jw_player_sharing? It's linked on the project page. I have no idea what it does exactly, never used it. I'd also be OK with including that feature in the main project, it hasn't been updated in a long time and is possibly obsolete anyway.

3. +++ b/jw_player.module
   @@ -218,36 +225,114 @@ function jw_player_field_formatter_settings_summary($field, $instance, $view_mod
   +          case 'responsive':
   +            $key = t('Dimensions');
   +            $val = $preset_settings['responsive'] ? t('responsive') : t('fixed');
   +            break;
   +          case 'width':
   +            $val = $preset_settings['responsive'] ? $val . '%' : $val;
   +            break;
   +          case 'height':
   +            $display_summary = !($preset_settings['responsive']) ? TRUE : FALSE;
   +            break;
   +          case 'aspectratio':
   +            $key = 'aspect ratio';
   +            $display_summary = $preset_settings['responsive'] ? TRUE : FALSE;
   +            break;
   

   Can we somehow find a way to combine those 4 settings into single point? e.g. 600x400 is obvious that it is fixed, while a response aspect ratio could be shown like 60% width (16:9)?

   I think the code is trying to be more generic now than it has to be, see also below.

4. +++ b/jw_player.module
   @@ -218,36 +225,114 @@ function jw_player_field_formatter_settings_summary($field, $instance, $view_mod
   +      if ($display_summary) {
   +        $summary[] = t('@key: @val', array(
   +          '@key' => drupal_ucfirst(str_replace('_', ' ', $key)),
   +          '@val' => !empty($val) ? $val : t('default')
   

   This is a similar problem. You can't really "translate" this string.

   As mentioned above, Maybe we can simplify it by avoiding trying to be so generic with nested loops and switch statements. Just have a bunch of if isset($preset_settings['some_key']) { add a summary line for that setting/group of settings. Then we can have a fixed order based in importance, should get a lot easier to understand the code. might even be less code in the end.

   In 99% of the cases, all those non-plugin based keys are always present anyway as you fill out the form and submit all form elements.

   As an additional idea, we could move everything that's just based on the preset into a helper function. If someone ever wanted to show a similar summary (We could for example show it on the preset overview..?), then it would be easy to re-use.

   I'm also open to leaving out some keys that we don't consider worth showing in summary. if you need to know details., the form is now two clicks away.

5. +++ b/jw_player.module
   @@ -218,36 +225,114 @@ function jw_player_field_formatter_settings_summary($field, $instance, $view_mod
   +    // Add preview image settings.
   +    if (isset($settings['preview_image_field'])) {
   +      $split = explode('|', $settings['preview_image_field']);
   +      $info = field_info_instance($instance['entity_type'], $split[1], $instance['bundle']);
   +      $summary[] = t('Preview image field: @field',
   +        array('@field' =>  $info['label'] . ' (' . $split[0] . ')'),
   +        array('context' => 'jw_player_formatter_summary')
   +      );
   +    }
   +    if (isset($settings['preview_image_style'])) {
   +      $styles = image_styles();
   +      $summary[] = t('Preview image style: @style',
   +        array('@style' =>  $styles[$settings['preview_image_style']]['label']),
   

   Maybe this could be combined into a single string too, something like Preview: @field_label (@image_style), Image style should probably show Original if not set, as that's what we'd show I think.

One more idea to make it more compact: Show the true/false cases as a list of options: "Enabled options: Autostart, Mute, Controls" or so.

Nice, I like that output. Feedback below, mostly on translatability, also I think we can simplify the function a bit.

1. +++ b/jw_player.module
   @@ -219,46 +224,173 @@ function jw_player_field_formatter_settings_summary($field, $instance, $view_mod
   +        $preview_image_style = 'Original';
   +      }
   +      $preview_image_string = $info['label'] . ' (' . $preview_image_style . ')';
   +      $summary[] = t('Preview: @field',
   

   similar as below, try to get full sentences into t(), not just the prefix. So, something like t('Preview: @label (@image_style)'), no need for a context then I think, pretty specific at that point.

2. +++ b/jw_player.module
   @@ -219,46 +224,173 @@ function jw_player_field_formatter_settings_summary($field, $instance, $view_mod
   + * @param string $option (optional)
   + *   Preset values to return. Options include:
   + *   - 'all': all values (default)
   + *   - 'name': preset name
   + *   - 'description': preset description
   + *   - 'skin': player skin defined by the preset settings
   + *   - 'dimensions': preset size and stretching settings
   + *   - 'enabled': enabled options such as autostart, mute, sharing, and controls
   + *   - 'sharing': sharing sites that have been enabled for the preset
   + *
   + * @return string
   + *   A string including the formatter settings will be returned.
   

   Do we really need that kind of flexibility?

   It looks like you currently call it separately so you can put some other things inbetween.

   What about just return the full array, always? Should simply the function quite a bit.

   What we could do is use keys in $summary, e.g. $summary['skin'] = ..

   Then if you really only want something specific, you can pick it out from that array?

3. +++ b/jw_player.module
   @@ -219,46 +224,173 @@ function jw_player_field_formatter_settings_summary($field, $instance, $view_mod
   +    if (!empty($preset_settings['responsive'])) {
   +      $dim = $preset_settings['width'] . '% width (' . $preset_settings['aspectratio'] . ')';
   +    }
   +    else {
   +      $dim = $preset_settings['width'] . 'x' . $preset_settings['height'];
   +    }
   +    if (!empty($preset_settings['stretching'])) {
   +      if ($preset_settings['stretching'] == 'exactfit') {
   +        $stretch = 'exact fit';
   +      }
   +      else if ($preset_settings['stretching'] == 'uniform') {
   +        $stretch = 'uniform';
   +      }
   +      else if ($preset_settings['stretching'] == 'fill') {
   +        $stretch = 'fill';
   +      }
   

   Hm. This is tricky stuff for translation, very dynamic. Neither stretch options nor things like width are translatable now.

   What happens if no stretching option is given explicitly? we could just fall back to that in that case I think, then we can always display something and it becomes less dynamic.

   Then, maybe we can do two t() calls on the settings, something like

   if (responsive) {
   t('Dimensions: @width% width (@ratio), @stretching)
   }
   else {
   similar t() hre.
   }

   the stretch options can then be t()'d on their own.

4. +++ b/jw_player.module
   @@ -219,46 +224,173 @@ function jw_player_field_formatter_settings_summary($field, $instance, $view_mod
   +      $enabled[] = 'Autostart';
   ...
   +      $enabled[] = 'Mute';
   ...
   +      $enabled[] = 'Controls';
   ...
   +      $enabled[] = 'Sharing';
   

   t()

5. +++ b/jw_player.module
   @@ -219,46 +224,173 @@ function jw_player_field_formatter_settings_summary($field, $instance, $view_mod
   +  // Sharing sites.
   +  if ($option == 'sharing' || $option == 'all') {
   +    $sharing_sites = array();
   ...
   +      if ($value) {
   +        if ($key == 'googleplus') {
   +          $format_key = 'Google Plus';
   +        }
   +        else if ($key == 'linkedin') {
   +          $format_key = 'LinkedIn';
   

   What about leaving this out for now and instead adding it in the issue that adds this feature?

   I would recommend to just have a function that returns a list of sharing options with a key and a label. Then this can be simplified to 1-2 lines or so:

   $sharing_sites = array_intersect_key(jw_player_sharing_sites(), array_filter($preset_settings['sharing_sites_show']));

   And then you just have to sort and implode.

Would be very helpful if you could provide interdiffs: https://www.drupal.org/documentation/git/interdiff (that has various options, I just have a branch per issue and I'm diffing against the previous commit to get the interdiff).

That makes it way easier for me to review just the changes since the last patch.

The other patch is in, this needs a reroll.

+++ b/jw_player.module
@@ -218,47 +223,173 @@ function jw_player_field_formatter_settings_summary($field, $instance, $view_mod
+  return ($format == 'string') ? implode('<br />', $summary) : $summary;

This function looks easier already, I think we can also avoid this? If someone needs a string, they can just to that implode themself?

We tried to get rid of functions in Core that can return either a string or an array depending on an argument.

+++ b/jw_player.module
@@ -207,6 +203,15 @@ function jw_player_field_formatter_settings_form($field, $instance, $view_mode,
+  if (!empty($presets)) {
+    $element['check_support'] = array(
+      '#title' => t('Check Media Support'),
+      '#description' => t('Enable verification of JW Player media support in formatted file. If the file does no contain supported media formats, no player is displayed.'),
+      '#type' => 'checkbox',
+      '#default_value' => !empty($settings['check_support']),
+    );

I noticed that this feature also got lost in 8.x, opened #2716625: Re-add "Check Media support" feature to add it back.

Thanks. I think we can take care of this in #2713701: Add preset sources, sharing, mute and validation for the 8.x port.