Organize image formatters around settings

yay!

This could be really helpful for galleries!

Now that I'm familiar with this, let's see if i can do something of value here.

Let's see how many tests are broken

Fixed
- tests
- image_image_style_save() update formatter settings when preset name changed

PS: do not forget to change #606598: Human readable image-style names

> sets good practice for contrib formatters ('put image style as a setting rather than as separate formatters' - see http://drupal.org/project/hover_preview for why this sucked in D6)

This raises the question... can a contrib module such as hover_preview use hook_form_alter to add settings to an existing formatter, or does it have to define a new formatter for itself?

joachim, I think much easy to implement own formatter with own settings oppose altering

In D6, modules like hover_preview, lightbox2, thickbox, colorbox, and countless of others cloned all the image styles, using a identifying prefix or suffix for their special action.

In D7, with this patch, I could see two different ways of extensions:

1) Add a new "Lightbox2" formatter, providing image style select list like core's "Image" formatter, plus any additionally needed settings. (yay, AWESOME, additional settings!!! :)

2) Enhance core's "Image" formatter with an optional #states-driven checkbox, which may or may not expose an entire fieldset of additional settings. Use the theme system to squeeze a preprocess function in front of the normal processing (if that's sufficient), or entirely replacing the original preprocess/theme functions with wrappers/custom versions.

Now, you will tell: Technically, 1) is correct and good.

I'll agree, but have to amend: Usability-wise, it's still formatted as "Image", stuff like a lightbox or preview are only attached later to it, so 2) makes more sense from a user perspective.

--

This patch looks ready to fly for me. Not sure whether the functionality is covered by tests though.

I might be repeating what you already agree on, but well this is my take on it, not sure I understand every part of the discussion above... I think it makes sense to move some options to the settings UI let's say from:

• Thumb linking to node
• Thumb linking to full image
• Thumb linking to medium image
• Thumb linking to large image
• Lightbox: Thumb to full image
• Lightbox: Thumb to medium image
• Lightbox: Thumb to large image
• etc etc

To:

• Link one image style node
• Link one image style to another
• Lightbox: Link one image style to another
• Shadowbox: Link one image style to another

, and then in the options choose the visible image style, and the style to link to.

Usability-wise this approach for these kinds of cases does involve an extra step wish is bad. However it un-clutters the options the user sees which is good. We could tackle the first problem by opening up the options for the formatter that the user selects automatically for them when needed.

What do you think?

Tagged.

- fixed standard.profile
- added comments about isset()
- fixed test settings

By default every image field is set to full-image-without-link - this could be changed in install profile and possibly in field defaults. Next image_image_style_delete() already cares about replacement of new style.

isset($image_styles[$settings['image_style']]) cares about image styles that introduced in code and have no reaction on module_enable/disable. Same for link - contrib could insert own values in this setting.

Attached screenshots. Suppose we could remove Style: ... Link: ... if commited #606598: Human readable image-style names
So only style label and link type would be displayed

PS: same could be done for #857314: Add form for number formatter settings

In the edit form, we could remove the "Image" prefix for both settings? i.e., just have "Style" and "Link"?

Second, the label "Link" currently has a select list showing "Image linked to file". So what? :) I guess that the Link can be either "<none>", "File", "Content". In other words: Could we drop "Image linked to" ?

I make "Image style" and "Image link" to make it easy to translate because this adds a "context" meaning to option.
About options in "link" - I'm not so good in English but I think combination of "title" and "option value" should contain some verb:
Link to "file"
or Links to "content"

So I glad to see more reviews about stings, because we should care about how this could be translated.

Actually the context argument is invalid, because we have proper string contexts in D7 which we could pass to the strings.
We should try to find the most adequate and correct English strings.

+1, Exactly what I wanna say

We should try to find the most adequate and correct English strings.

About t()'s context, this is sad that core use this only for long month names.
It scares to update most of core's strings to context. But it shell be done someday...

+++ modules/image/image.field.inc	18 Jul 2010 18:48:46 -0000
@@ -428,52 +428,69 @@ function image_field_formatter_info() {
+  return implode ('<br />', $summary);

Stray space after implode.

@all / yched: We discussed elsewhere that it would probably be a good idea to let these formatter summary callbacks return an structured array containing labels and values, so as to allow contributed modules to implement better or alternate UIs, display this info elsewhere, alter the summaries, etc. Anyone up for tackling this in a new issue?

+++ modules/image/image.field.inc	18 Jul 2010 18:48:46 -0000
@@ -535,3 +552,25 @@ function theme_image_formatter($variable
+ * Helper function: returns image styles for formatter settings.
...
+ * Helper function: returns link types for formatter settings.

Please remove the "Helper function:" prefix.

46 critical left. Go review some!

Updated patch with #20
Also link information displays only if image really linked.

+++ modules/image/image.field.inc	19 Jul 2010 10:36:33 -0000
@@ -428,33 +428,58 @@ function image_field_formatter_info() {
+  if (isset($link_types[$settings['image_link']]) && $link_types[$settings['image_link']]) {

Was wrong! So new patch and screenshots. If 'No link' chosen then no summary for link

> This will lead to all being stuffed into the same and single 'image' formatter, which is not great UX wise. When I add a new formatter module, I prefer seeing new formatters available rather than having to open settings for existing formatters to discover where is the new candy.

I think it depends on the formatters.

Say you had a module that wanted to add an option for the hover on an image, or (made up example) the border style.
Would you want a whole new formatter which would duplicate imagecache and lightbox settings, or to get an extra dropdown in the existing formatter? I'd say for usability, the latter.

Previous patch mixed with hunk from #278425: Using basename() is not locale safe

When I add a new formatter module, I prefer seeing new formatters available rather than having to open settings for existing formatters to discover where is the new candy

Lets try not to focus on this edge case. Better is to craft the user experience assuming that there is just one "system" and we design the best user experience for it. Consider that many sites will be running distributions and downloading Features which have all these modules pre-configured and ready to go. Thats where we are headed. It might take a bit, but we should keep the goal in mind. The days of downloading individual modules that provide small formatters are waning.

when new module added it's formatter gets available in dropdown and you could optionally configure if defaults does not conform your needs.

I think core should introduce a Pattern for formatter settings/summary. There was no pros&cons about.

So let's decide:
1) a way to display settings form - small, understandable titles for elements with/without description
2) a way to display summary

Image settings:
Image style: [select] (no descr)
Link: [select] (no descr)

Image summary:
Image style: style_x | Original image
Linked to file | Linked to content | (no 2nd line if no link) - no title for tis option

Text settings: (#504564: Make summary length behave with fields)
Trim length: [text input]

Text summary:
Trim length: number

Number settings: (#857314: Add form for number formatter settings)
Thousand marker: [select]
Decimal marker: [select]
Scale: [select]
[checkbox] Display with prefix and suffix.

Number summary:
Preview: 1 234.567
Display with prefix and suffix. | (no 2nd line if no setting)

Yes, #29 makes sense.

Changes

- removed functions for options, using core's image_style_options()
- Settings form
1) Image style option starts with , seems more reasonable because if no style then we get 'Original image' in summary
2) Image link option starts with and summary displays only if link to assigned

Changed to 'None (display original image)'
Fixed "Link"

From an end user perspective, knowing that you won't be using any image style is not relevant. I think just using 'Original image' would make more immediate sense.

Agreed, "original" is a perfectly valid image style and would be sufficient. The only thing we need to prevent is having 2x "original" in the image style select options, from which one is a custom image style that happens to be named "original".

Either we prevent creating an image style called "original" or we add a prefix/suffix to the real one, i.e., <original> or similar.

Re-roll with 'None (original image)'

About naming - I still hope to see #606598: Human readable image-style names commited and then list of styles became more user-friendly

Because the select named as "Image style" so using 'None (original image)' makes more sense to me. Because this actually means "I don't need styles, just display original" - this is a widely used for search indexing and rss

@yched we should use isset() on fixed array and comment about contrib because nothing stops contrib from

  $instance['display']['default']['settings']['image_link'] = 'mymodule broken link';
  field_update_instance($instance);

I think we could make this extensible but this looks like over-engineering for this case and anyway out of scope of this patch

Sure, isset() just checks that setting in defined array to display only allowed values

Good point on #38 about using 'None (original image)', makes sense!

I think while we on it we should add a limit setting to limit a number of images to display for current view mode.

Suppose this feature would be very useful.

Also added a test for this setting.

New screenshot attached

A bit usability - limit list to actual field cardinality

Wrapped array with default formatter settings

-1 for adding this new "limit the number of images to be displayed" feature. This is really not the way to do it.

Reposting #40 that is RTBC.

This issue is a task for refactoring of image formatter so "limiter" should be discussed.
It is not API change, also test applied.

So let's get more reviews on #45 and #47. While this not commited I see no reason to open new issue with feature request.

Please open an separate issue for the feature request. This refactoring *task* is clearly not the place to discuss it.

Personally, I find the UI to be weak.

- What does: "Format settings: Image" (see http://drupal.org/files/issues/image_field-d7.png) mean?

- The "Link - to content" approach is awkward too. We never split up sentences like that.

I think we'd like to do some UX work on this.

Tagging for UX team suggestions.

I think "Format settings: Image" becomes clear from the context. First of all people will know the idea of a formatter from other fields. Also, before they get to this screen, they see "Image" in the Formatter column, so "Format settings" should make sense for them when they're editing, well, the settings.

Is there any ideas how to continue this issue?

Subscribe, will review.

"Format settings: Image" is the static text replacement for the choice you make in the previous screen:

It helps maintain context:

Maybe it helps if it said "format settings *for*: image" instead?

For the Link select:
- Capitalize the options
- Make the label a bit more descriptive. We want to fix this in the label, not in the options by adding redundant wording that makes them harder to scan.
- I don't see the problem with labels that trigger you to complete a sentence, on the contrary, it is a construct that helps you connect the dots and avoid redundant wording. Which is important here because there's little room to work with.

Suggestion: "Link image to". It's quite descriptive and puts the main trigger word first.

re-roll with proposed

  $link_types = array(
    '' => t('<none>'),
    'content' => t('Content'),
    'file' => t('File'),
  );
  $form['image_link'] = array(
    '#title' => t('Link image to'),
    '#type' => 'select',
    '#default_value' => $settings['image_link'],
    '#options' => $link_types,
  );

From a technical perspective, this patch was RTBC already. So I suggest and ask the UX team to RTBC this patch once they are happy with it, please.

I guess the issue with 'sentences' that start as Label and end in the select options is that they have poor translatability. You cannot ensure that the order stays the same in any random language.

Totally true, but those are very elegant and quite easy to get right.

Example from content_handler_field_multiple.inc in CCK6:

    // Make the string translatable by keeping it as a whole rather than
    // translating prefix and suffix separately.
    list($prefix, $suffix) = explode('@count', t('Show @count value(s)'));
    $form['multiple']['multiple_number'] = array(
      '#type' => 'textfield',
      '#size' => 5,
      '#field_prefix' => $prefix,
      '#field_suffix' => $suffix,
      '#default_value' => $options['multiple']['multiple_number'],
    );

For stuff which is technically a prefix only, like the "Link image to" label, it is always possible to make a convoluted translation, even in languages where this prefix would be more natural as a suffix.

OTOH 'to content' and 'to file' could confuse because without context it looks like verb

Image link target:
- Content
- File

subscribe

/me thinks @yched is totally over-thinking this label issue.

Link image to? Why wouldn't that work? I mean translation can easily go for something diffrent, no reason for them not to change the order when appropriate.

@yched I think that should be fine, I mean grammatical homogeneously might be a discussion point as I agree that isn't the "best" thing. But repeating the word is only adding more load, so - yup lets move this in. I would really love to test this someday

@yched yes like in #57. I am ok with that.

I decided to commit #58 but more than happy to follow-up with language tweaks and smaller UX changes.

Is there any proposals from UX and any tweaks?