Possibility of overriding representation in atom reference field

The following patch should be applied to the module before starting work on this, to avoid future conflicts. https://www.drupal.org/node/1978828

Hi :)

awesome,

Maybe there was no issues as the other patch is mostly changing JS stuff.

There seems to be a typo at

+function atom_reference_update_7001() {
+
+  $new_field = array(
+    'description' => 'A caption for the referenced atom.',
+        'description' => 'An options set for rendering the referenced atom.',

We should check if we need that many changes in the formatter.

Isnt it like we only need to check for the context to not display the context defined in the field display but the one defined in the override if one exists?
And then the caption is handled by the representation, as usually all options are.

But then maybe im missing something.

@gifad:
I meant for example this part:

+        if ($display['settings']['caption']) {
+          if (!empty($options['caption']))
+            $markup .= '<figcaption class="atom-reference-caption">' . $options['caption'] . '</figcaption>';
+          elseif (!$display['settings']['title'])
+            $markup .= '<figcaption class="atom-reference-caption">' . scald_render($sid, 'title') . '</figcaption>';
+        }

Isnt figcaption handled by the image provider's representation?

The typo is still there though from my comment #3 the description is there twice.

@gifad : can you explain to me in more detail why we need to limit the display mode?

As far as i understand if i dragged an atom into a reference field, and i change the default representation set up in the field display settings, then it should be overriden in all displays. Cause its defined by the user on the field. And then on any display it should be displayed like how the user wanted.

So it seems to me that we simply have to override the representation value to the one in the options when it exists, and thats it.

Because otherwise this option would be confusing if it only worked for the full display.

But then the problem is that we should not provide a selection for each existing display, as that would be a UX fail i guess.

Maybe this need more thought into it...

For me it produced the following reject when applying on scald code with the following patches
- https://www.drupal.org/files/issues/atom_reference-1978828-61.patch
- https://www.drupal.org/files/issues/2393425-scald-widget-false-drop-72.p...

atom_reference.js.rej

--- modules/fields/atom_reference/atom_reference.js
+++ modules/fields/atom_reference/atom_reference.js
@@ -48,6 +48,10 @@
                 .find('input:text')
                 .val(dt)
                 .end()
+                .closest('td')
+                .find('input.atom-reference-caption')
+                .val(Drupal.dnd.Atoms[dt].meta.title)
+                .end()
                 .find('input:button')
                 .show();
               Drupal.attachBehaviors($this);

Can fix that later of course.

@gifad:
Seems like there is no way to query the view mode from the formatter_view function.
http://drupal.stackexchange.com/questions/39224/how-to-get-view-mode-fro...

And checked other sources as well.

Seems like this issue is not as easy as i thought :(

Maybe a good idea could be to create a separate formatter for the atom reference field.

Basically it would duplicate each format with something like "Overridable Title", "Overridable Editor Representation" and so on.

And then the site builder can select on which view modes he wants to enable this option. And then of course the context override would only work on this overridable formatter.
(The rest of the options can be carried through in either formatter though).

Maybe something along this line could be a good solution.

Though im not sure why we have each representation as a separate format, instead of having a format like "Representation" and "Overridable Representation" and then select the representation in the formatter settings.
Though changing it like that would surely require an update hook the least...
(Or maybe some trouble with the views integration?)

@gifad:
Have you read comment #15, and #16?
As i think thats the only way to make this work properly.

Doing it with debug_backtrace is not an option, pretty ugly.

And putting in a hard coded limit will not work generally anyways, what if i want this override to work for a custom view mode, what if i dont want it to work for custom view mode.
There is no way to do it properly without an extra formatter like i wrote in 15, or 16.

My comment was just about creating another formatter for the field type and then the site builder can select which formatter to use, the overrideable formatter or the non overridable formatter.

But your solution seems to be better with the formatter setting.

So then according to #17

This array is perfectly fine?

+  $new_field = array(
+    'description' => 'Options for the referenced atom.',
+        'description' => 'An options set for rendering the referenced atom.',
+        'type'        => 'blob',
+        'length'      => 'big',
+        'not null'    => FALSE,
+        'serialize' => TRUE,
+        'object default' => array(),
+        'initial' => serialize(array()),
+  );

The description array key is there twice, basically the second one will override the first one, i see no point in it...

Also there is one more task to do:

Currently we are altering the context names by drupal_alter('scald_wysiwyg_context_list', $contexts);
So in wysiwyg you see a customized list, where the labels can be different.
And for most of our clients it needs to be customized.

We should harmonize that, to also apply the same way for this new context selection. Of course changing the name would be problematic for backward compatibility. But at any rate there should be a way to make sure that the same list comes up for the user here than in the wysiwyg.

1. As i see we will have to change a test in scald core cause of the link setting value changes.
Then we either have to ignore that part failing and also change the test in the patch,
or have to make a separate patch that changes the test, and then commit that before the other to be able to test.

Or maybe there is a better way of doing it.

2. As i see the caption is currently a textfield. Maybe a text area would be better.
But I think this will be confusing for the user on certain areas, like in the scald_gallery to have a caption text field in addition to the description and other stuff there.
However this cant be the description of the scald_gallery either cause (apart from not being a textarea) the render would use this caption to produce a figcaption for the atom (in the current patch), while in scald_gallery the description is used for the galleria player to display a description below the images.

Otherwise the patch seems to work fine, nice work!

Of course more testing is required, and cleaning on the patch.

Patch can be recreated based on latest dev.

Separated out the wysiwyg context list alter problem : #2457005: Generalize hook_scald_wysiwyg_context_list_alter

Yes its the same in wysiwyg the server does not know what atoms you drag into the wysiwyg, so the list there is populated from JS, and its dynamic, as you open the dialog for a given atom.

Of course a different set of JS is needed for the other cases, but its the same there, we have the context list per type in the variables, we just have to use that to limit the items in the select list (and to rename them)

You would miss mostly content architecture.

- In a wysiwyg you cannot limit what types of atoms you can drag.
- In a wysiwyg managing multi value fields would be really a UX fail.
- In a wysiwyg you can put anything which you might not supposed to add. Like putting all kinds of text and other wysiwyg stuff next to the atom.
- You cannot say display only the first item of the multi value field, cause that would mean displaying the first atom of a wysiwyg?

There are countless problems :)

Its i guess the same argument as why dont we have a single textarea field, instead of having separate fields. Mainly due content architecture.
Often we have separate image fields, (like main image of a content type) instead of putting the whole render as a wysiwyg.

Also if the question is that we could create a multi value textarea to replace a multi value atom reference, then we have the following problems:
1. Even with specific ckeditor settings, we cannot deny the user from entering text or new lines or whatever into the editor on top of dragging any number of atoms into a single textarea. This is quite a big problem, as you will have no idea what is in a given field unless you parse it each time. And probably additional filters are also needed.
2. Previews cannot be properly guaranteed as they might not always fit into the ckeditor initial display.
3. It can potentially introduce a lot of separate ckeditor instances on a single page, which is a huge performance impact.

Generally when i add an atom reference field, i add it because id like if the user dragged some atom into that field, and then from code or on the display i could be sure that i have an atom, and i can display it or do whatever with it. Its a lot clearer and strait forward situation.

And about the need, we actually already had an occasion, where we had a main image for a node display, and the requirement was to let the user define the context or image style of the image in the atom reference field. There we've done it with custom code using a new image style reference field (https://www.drupal.org/sandbox/york/2383353) and a custom representation, but the atom reference was limited to images, and it would be nice to have a general solution for these cases.

And since we need this also for scald_pane linked above, i think its a nice to have feature in fact.

In order to get this into the upcoming scald 1.4, i ve decided to separate out and rework only the part that is relevant for the task description, which is the new options field and the context switching ability with the new javascript code.

I've removed the alters, as discussion about that can continue at #2457005: Generalize hook_scald_wysiwyg_context_list_alter
And because the altered list is already available for javascript, as shown in scald_pane, so now the list in atom reference field overrides and in wysiwyg on the same page going to be the same which is good.

I've also removed the title, link, and caption parts. To change as little as possible in this patch for now, also I'm still not sold on the idea of having another figcaption around the atom markup on top of the figcaption in the image provider. Plus having an extra caption is a problem for scald_gallery too, so likely we should not do that yet.
Adding those further fields can happen in followup issues.

I've tested the update hook, works fine. Testing the fresh install is still to be done, but will do that with simplytestme.

The identation of the atom_reference.js is wrong, but i did not want to change it in this patch, as that would ruin the diff, can do in a follow up commit.

I've added a use_the_default option in the representation selector on the atom reference, because actually that field cannot have a default value from anywhere, as the default value can be different per display formatter, also its easier for the user to select this option when he wants to get back the default, instead of remembering what was the default setting.

Otherwise the attached patch works fine for me, will need more testing.

Tested new install on simplytestme, seems to be fine.

Added one more check, even though there was no JS error.

Committed. Further work can be done in followup issues.

Thanks! Committed.