Add new UI for formatter settings options to the field settings panel

This is a bug :)

Thanks for linking the issue, this really helps here.

Just a short summary if someone want's to work on it: "Fun"

So you can now have settings per formatter, which means that there is only one "image" formatter but different settings,
for example the image style for a imagefield.

So views would need to integrate the form, store the settings and use it during rendering.

So a brief plan

option_definition: get the settings out of hook_field_formatter_info for this field.

options_form: add the additional form into $form['settings'], and use the stored values in views as the default values / replace instance['settings'] with it.

Perhaps use field_formatter_settings_summary in field::admin_summary.

Load the settings on field::render into the instance.

Hardcoding the image link options seems like the wrong way to go, but that's what the image module does.

Sorry. This will not be used this way.

We have to get the settings from the formatter manually and store it.

Image settings are not stored with the formatter, they are instance specific. I couldn't find a better way to get all of the appropriate settings. I must be missing something in the Field/Image APIs. Sorry, I tried!

Here's the beginnings of a solution. I am identifying a dummy 'instance' of the field so we can use hook_field_formatter_settings_form() to get the settings form. That form already has a 'type' field for the formatter itself, so I am just substituting the whole thing for the default 'type' form.

I don't know how to show/hide various settings if there is more than one formatter, so that needs to be done still. And I haven't done anything about trying to store the data or use it to alter the display of the field.

It raises a question about what to use for the 'view_mode'. I just used 'views' as a generic view_mode, figuring we won't actually save these values to the field, we will store them in the view. (Otherwise we would need a different 'view_mode' for every views field and display.)

This still needs lots of work, but maybe it can give others some ideas about where to take it next.

I'm working on a sequel in the exibition (lunch) hall.

Edit: I have it working. However, it makes baby Jesus cry (some cleanup needed). Will post a patch soon.

Oh come on, it wasn't *that* bad :)

But good, I'm glad someone is working on it.

@KarenS
I was actually referring to my part of the code :P

Here's a patch in progress.

I've found out that some fields (like the text field) have ifs in their settings forms, and return different settings for different view modes. That means that we can't just select the default one and hope for the best.
(Also, the previous approach of sending "views" as the views mode caused it to blow up with notices.)

An obvious solution is making a view mode selector and showing settings per view mode, but that complicates the code and is kind of a waste since we don't save the settings back into the view mode and pass those settings manually during render, instead of specifying a view mode.

So, is "default' enough, or do we need more? I guess testing will show.

P.S. I'm not really sure we need the static caching in _field_view_formatter_options() since the functions called already have their own.

This one's better. Works for the intended use case (images).

Note that it generates a warning:

Warning: Illegal offset type in image_field_formatter_settings_form()

But this smells like a bug in core...

That's all, folks.

And no, it wasn't a bug in core, it was mine, and I need some sleep.
I was passing the view_mode object instead of the key.

The patch is simpler now, fixes the issue, and can be used as the basis for further efforts (although i probably won't have more time...).

And, that was a wrong patch. Aaargh!

My final spam on this issue.

Some minor updates. This is already quite cool.

Just reviewed it, cool.
I like this.

Would like to know what KarenS thinks, and of course, we should test this with other field types and field settings.

w00t! Going to try this out now :)

I can also see if it will work for Date fields, where I also need this functionality. That will be a way to see if it is generalized enough to work for things other than imagefields.

sdaniel and me tested this today/yesterday night without problems just some errors.

I'm wondering about

      $form_function = $info['module'] . '_field_formatter_settings_form';
      $settings_function = $info['module'] . '_field_formatter_info';

      if (!function_exists($form_function) || !function_exists($settings_function)) {
        continue;
      }

      $settings_info = $settings_function();
      $default_settings = $settings_info[$formatter]['settings'];

      if (count($default_settings) < 1) {
        continue;
      }

      $formatter_options =& $options['formatters']['contains']; 
      $formatter_options[$formatter] = array('contains' => array());

      $form_state = array();
      $form = $form_function($field, $instance, $view_mode, array(), $form_state);
      foreach ($form as $settings_key => $definition) {
        $formatter_options[$formatter]['contains'][$settings_key] = array(
          'default' => $definition['#default_value'],
        );
      }

especially the last part. Can't we just use the default value of

function image_field_formatter_info() {
  $formatters = array(
    'image' => array(
      'label' => t('Image'),
      'field types' => array('image'),
      'settings' => array('image_style' => '', 'image_link' => ''),
    ),
  );

So basically iterate over each formatter get the settings defined in hook_field_formatter_info and store it in #default_value.
The form shouldn't have elements which aren't yet in the formatter_info.

I changed it and at least for me this still does work. Bojanz what do you think? This makes the code a bit less scarry.

I agree.
Some things were mostly copy&paste (most of which I cleaned out in the coder's lounge, but still...), so I'm really happy to see anything simplified / made less scary.

Will do testing today and see how it all works.

I would like to remove

      $formatter_options =& $options['formatters']['contains']; 

This makes it harder to read.

I'm not really emotionally attached to any part of the patch ;)

However, the array[access][does][get][a][bit][ugly][when][10][levels][deep]

Yes long an deep arrays sucks. The question is whether it's worth to have it for the two lines.

Let's do this in a follow up.

Okay, here's an updated patch with the reference removed.
The line's long, but it's not better than the alternatives.
And I've removed one of the long lines, since that initialization is not necessary (doesn't generate any notices or anything).

I am having trouble with multiple formatters, I see all the settings for all the formatters instead of only the settings for the selected formatter. It may be something I've done wrong in Date module, so you can try adding a second formatter to the image module to test that idea since the current code only shows a single formatter.

Do you have javascript disabled ? It basically renders every form for every formatter to be able to switch the form by the selection of the formatter based on the #state system.

Nope, I have javascript enabled. Maybe there's something funky in my setup. Has anyone tried setting up multiple formatters to confirm it works right? If so and it does, maybe it's just me.

Sorry for coming late to the issue :-/.

Rather than including all settings forms for all available formatters, and hide / show them with #states, isn't it possible to #ajax-update the form, like core does for the 'Manage display' screens ?

That obviously requires #561858: [Tests added] Fix drupal_add_js() and drupal_add_css() to work for AJAX requests too by adding lazy-load to AJAX framework to get in - but so does the #states approach... Right now, content in ajax-updated portions (like a 'View field settings' subpanel) cannot add js behaviors, be it #states or #ajax.

Better title.

So that's why the display fields screen works right for multiple formatters but this one does not, they are ajax-updating the form with only the settings for the selected formatter? I'm not sure how hard a change that would be to make, but it looks like that might work even without the core patch getting in.

Thanks to the D7 #ajax framework and the FAPI refactoring around multistep and $form_state persistence, the code to #ajax update the 'formatter settings' portions on the core screen was pretty straightforward to add (once agreeing on the overall UX).

In the case of the 'View field settings' form, we're inserting in a context we don't fully control, though.

Okay, I'm back at home, so I can move forward on this.

@KarenS
What setup do you use, which field with multiple formatters, etc?

@yched
What do you think of the way we do display modes? Is just specifying "default" okay?
It's strange in core how it currently only has "Default" and "Teaser' as the selectable defaults in "Manage Display", but the help text says:

Content items can be displayed using different view modes: Teaser, Full content, Print, RSS, etc. Teaser is a short format that is typically used in lists of multiple content items. Full content is typically used when the content is displayed on its own page.

I'd like to replicate the problem that Karen has before investigating alternatives to #states, I'm not sure I see the benefits (More code with similar effects, and we won't have many forms for one field anyway....)

Just add a second formatter to the imagefield code with the same settings as the current one to see what happens when there is more than one.

Doesn't this assume each formatter has a settings array? I'm getting notices about text_plain/text_default.

Yes, it needs one check more (isset) next to the count() one (the settings array might be empty, or it might not even be set). Haven't had more time to devote to this...

Attached is a updated patch that checks that the key exists before assigning it's value to the settings.

I think we need the check in options_form and options_definition, that's where the notices are coming from, not render().

In any case, this needs some heavy testing with multiple formatters per field.

Hopefully i'm not stepping on any toes here, but came across this cue after running into some config issues trying to setup some view blocks displaying images (and not seeing options for defining image styles within the view).

I've implemented 884730-formatter_settings_2.patch. This looks like it worked in displaying the formatter options but I now get this error:

"Notice: Undefined index: settings in views_handler_field_field->option_definition() (line 117 of /Applications/MAMP/htdocs/jibe/iqmetrixwww/modules/contrib/views/modules/field/views_handler_field_field.inc)."

The view blocks and images are displaying as expected though (minus the error).

This patch fixes the notices.
However, multiple formatter settings are broken, the #states approach definitely doesn't work, not sure why.

Needs work.

Edit: Tried retesting with the lazy-load patch, no luck.

tested #37 patch, it seems the image delta is missing, and duplicates are displayed.

Well the duplicates are handled in another issue.

What do you mean with "image delta" is missing?

if a node have multiple images,
1.) Feature request: How about having a delta option there? So, we can just show one of the images.
2.) Bug: The item is duplicated as many times as the number of the images.

Sorry for not getting back at this earlier, I was on vacation, and busy on core criticals when I returned.
I think the #states-based approach in the current patch is flawed. Taking a stab at this is the next item on my todo.

Working on it.

Great =)

Subscribe.

So, here's a working patch with #ajax-updated formatter settings form.
A #states-based approach might in fact be better UX-wise, this can be discussed - current patch is also a bit cleaner than the previous attempts :-).

For now, a couple issues (whether we go with #states or #ajax) :

- Some formatters rely on a couple common helper validator functions ('input is a number', 'input is a positive integer'...)
Those functions currently live in field_ui.admin.inc, and are thus not loaded when the 'configure View field' pane' is displayed. I opened #946646: Move helper #element_functions out of Field UI to move them to field.module.
For now, the patch redefines the function when they don't exist - hack, but works until core is fixed.

- It seems that validation errors in the 'field config' form are not reported to begin with.
E.g : node View, add Body field, configure it with 'Trimmed' formatter.
The 'Trim length' input should be required, and accept only a positive integer - but the form submits just fine if you put 'abc' or leave the field empty (the invalid value is not saved, though)

Awesome!
Will review this tomorrow.

The validation problem is a general Views problem (invalid value closes the form, but it doesn't get saved).

@bojanz : OK - we can leave that out for this patch then.

So, #ajax vs #states :

#states :
Pros :
- is snappier - no wait for the HTTP request
Although JS performance for that many show/hides should possibly be tested ?
- is more inline with the rest of Views UI (mostly client-side #states)
- degrades 'better' (?) without JS - you get a separate fieldset for the settings of each available formatter, only one of them is relevant (the one for the formtter selected when you submit)
Although this also means that with JS on, you get a 'Settings for @foobar format' fieldset around the settings forms, which is not inline with the rest of Views UI
Cons :
- means a heavier $form array (one 'settings fieldset + subform' per available formatter - I don't really expect more than 10 different formatters for a given field type, though, which should stay reasonable)

#ajax :
Pros :
- Allows values for settings common to several formatters to be persisted when changing the format.
Example : for the 'Body' field, the 'Trimmed' and 'Summary or trimmed' formatters both have a 'Trim length' setting.
If you change the format from 'Trimmed' to 'Summary or trimmed', the current value of 'Trim length' is kept.
In the #states approach it would be set back to the default value, 600 in this case, since the two 'Trim length' inputs are in fact different elements.
Cons :
- Does not degrade nicely. Without JS, you need to change the format, submit, and re-edit the field to get the settings for the new format.
Possible workaround : add a "Refresh settings form" button next to the 'Format' selector, and keep it hidden when JS is on.

subscribe
(and tested the patch in a "3 minutes-i-dont-have-more-time" run, seems to works for me, but need "real" review)

One thing I noticed after applying patch to latest dev build:

- I now see two formatter drop down boxes in the field settings for a view. One is above the Label settings, and the main one (the one that works) is right where it should be.

@jazzdrive3 - strange, I cannot reproduce. What's the field type ? Are you sure you applied the patch correctly ? :-)

Nope, I'm not sure :) As far as I know (which isn't much), I did. Thought I'd bring it up though.

It's an image field.

If it might make a difference, the field existed before I applied the patch.

Hm, actually for image fields, the whole 'configure field' form is duplicated. Odd.

Subscribe

subscribing...

After applying this patch I started to get this:

Error message
Notice: Undefined index: distinct in views_plugin_query_default->build() (line 1107 of sites/all/modules/contrib/views/plugins/views_plugin_query_default.inc).

Actually #54 wasn't related with this issue as noted in http://drupal.org/node/944692
My bad :)

I'm working with some of these same issue over at #946534: Upgrading node_body.inc content type (and the cck equivalent). DamZ has handed me a little code that I'm giving a trial run and I think it might solve a few problems here too. Once I finish that issue, I'll hop on this one and see if I can help. Some of this field instancing stuff is looking like we might need a nice little api for spitting out generic instances that aren't actually part of any bundle.

+    foreach ($field['bundles'] as $entity_type => $bundles) {
+      $entity_info = entity_get_info($entity_type);
+      if ($entity_info['base table'] == $view->base) {
+        $bundle = current($field['bundles'][$entity_type]);
+        $instance = field_info_instance($entity_type, $field['field_name'], $bundle);
+        break;
+      }
+    }

This looks very weird to me. First, the condition on $view->base is definitely wrong, a rendered field doesn't have to be attached to the base entity of the view.

Second, this code is arbitrary loading one instance, and as a consequence uses the settings of that instance as a starting point. This feels fragile, and could lead to inconsistencies if the list of entities / bundles available for this field changes.

I suggest we rather create a completely dummy instance, like this: http://pastie.org/private/8uqpgtzpyfj6nws4dbauq

"a rendered field doesn't have to be attached to the base entity of the view."
True. Relationships have been there for 2+ years. Duh.
A "neutral" $instance array set up for the occasion sounds fine.

+ found why the whole form was duplicated for image fields - lol @ #948584: Wrong implementations of hook_field_formatter_settings_form()

Will reroll for #57 asap, anyone feel free to beat me to it.

Also, it seems that you can't unlink the image. You can only link it to the node or the file. But I don't want the image to be a link at all. This was definitely an option in Views 2.

Thanks.

@jazzdrive3 : that's a core bug, now fixed in latest HEAD - #936536: Missing #empty_option on a couple selects

This patch uses the new function from #57 to create the fake instance

Thks dereine.
Patch has unrelated changes in views_plugin_query_default.inc ?

Still to be decided : #ajax (current patch) or #states - see #46.

Anyway, #946646: Move helper #element_functions out of Field UI has to get in core before this patch is committed. The hunks adding _element_validate_integer() et al make the Views interface work, but can raise fatal errors on Field UI pages if field.views.inc is loaded before field_ui.admin.inc ("cannot redeclare a function").

Oh i forgot to cvs update before diffing.

Both core patches landed.

Updated patch not including those core hacks now that the core issue has been committed.

I tried it out with the date field, which has multiple formatters, and it looks like it works fine.

The patch from post #65 solved the issue for me as well - after applying it I was able to assign predefined image styles to image fields displayed by views.

I'm personally more a fan of the #states approach, one look at yched's list of pros and cons just makes that clearer.

However, unlike the #states patch, the #ajax one actually works :P Plus it's smaller and cleaner.
So I have no problem with this getting in and solving the problem (at least for now).

Outstanding community work! Now this issue is fixed.

Everytime i try out this new interface with js it's quite handy to use.

@yched
Perhaps you could try out it with the date module and the normal "manage display" settings. It's a bit confusing, because the "as time ago" feature still displays a settings link, even there is definitive none available.
It's hard to describe.

@dereine (& @KarenS :-) ) : That's because date_default_formatter_settings_summary() should not return a 'settings summary' if the formatter has no settings.

LOL, still figuring that new formatter stuff out. Plus it may get some settings soon, now that it can.

Not sure if this is related, but there is also a bug in the formatters for dates.

So if I want to display the node post date, I can choose 3 of the exact same format (a bug in itself), a few "time agos" and the "custom" option. But when you select the custom option, there is no text field to enter in your custom format. And so I can't just display the month and the year as needed.

Couldn't find anything related in the bug queue so apologies if there is already an issue for this.

Am I missing something? I'm using the latest snapshot as of November 5, and Firefox on a PC. I select an image field in Fields, the formatter only contains Image. There is no button next to it, no subsequent configuration dialog when I click Update ... where is the image style selected??

This is what you can see

Agree with #73: Has this patch been committed? Upgrading views here broke my (manually patched) image formatter.

Personally i would say yes
http://drupalcode.org/viewvc/drupal/contributions/modules/views/modules/...

I don't see what's in the png at all. Running beta 2. Updated Views this morning, and have created a new view (in case the fix didn't work on existing views). Cleared views cache first. I have only the formatter, not the two fields below it.

Do you have some image presets configured already?

I got it to work by fresh installing views. Updating didn't seem to fix it.

I have three view styles configured. Is removing/installing Views necessary? Seems it shouldn't make any difference unless there's a database change that doesn't take effect when updating.

Try it out....

I never was able to get it working on Drupal 7-Beta 2 but the core Dev version that came out after that worked. Someone here posted that.

No joy. I deactivated views and removed it, and reinstalled it using the url. Here is what I still get.

@84

Which version of drupalcore do you have?

7.0-beta2

But it works fine for you when you install a new drupal version?

Afaik the formatter change was made before beta1 so there is no update path.

I'm not sure what you mean. I never had it working. I started with a fresh install of beta1 and updated that to beta2.

Looks like this functionality needs the latest D7 core (after beta2). You either need to use D7-dev, or wait for beta3 which should land any day now.

ok...I'll mark this postponed until beta3

This is fixed if it works with the current core dev :)

I have Views on two Drupal 7 Beta 3 installs and can't get the formatter to work on either one. I have another Beta 3 install that has it working fine, and this one was Drupal 7 Dev version up until Beta 3 arrived. I got it working with views a while back. I'm trying to solve why I have one out of 3 working now.

OK. This was working for me on Beta 2, and also on Beta 3, but has just stopped working having updated Views this morning. I'll dig around a bit...

Right. It seems that updating installed the old views without this patch over my newer install. Replacing the files in views/modules/field with my backups restored expected functionality. This is probably a new bug though.

Let's leave this issue be.
If you find a bug, and can provide reproduce-able steps, open a new issue and we'll look into it.

Additional it's important that you use the current dev of drupal core and views.