Adding edit and view links bellow the atom reference field

It does not check whether the user has permission to edit the atoms. However we can't do it right now. I'll file another bug in order to be possible to fetch the permissions.

#1979014: Allow to fetch user's permissions on an atom

While we are at it, the same could be done for the wysiwyg menu, if i right click an atom in ckeditor, and could click on edit to open up the ctools modal dialog for the selected atom.

It's exactly what I was thinking when I see this issue. However there was some discussion about a more exposed option for wysiwyg: hover the atom, then we have a contextual menu with 1/ edit properties 2/ edit atom 3/ delete atom from the textarea etc. Let's do that in a separate issue, which is more complicated than this one.

Reviewed the patch:

- Fixed an issue where the edit link got the view class and the view link the edit class (and that also caused some links to get duplicated).
- Added the necessary ctools classes, and manually calling the ctools JS behaviour to work on the edit links. (For now found no other way to make the edit link work with the modal, because these links are added with javascript).
- Changed the links to not use ?q= format as we dont use it anywhere in the code.

todo:
- probably need some css to make the links look nice, or make them look as a button.

There was a suggestion that the user should be educated that the "edit" action in the reference field is meant to be a global modification of the atom, not only in that field context. I'm not sure how to incorporate it here.

I do think the CToolsDetachBehaviors event is triggered after atom update (submit -> update -> receive response -> detach). The problem is at:

Drupal.dnd.fetchAtom('new_' + (new Date()).getTime(), atom_id, function() {

I think you want to bypass the cache. In that case, just remove the context (Drupal.dnd.Atoms[i].contexts['sdl_editor_representation']) then fetch the atom with the correct context. When you pass 'new_' + (new Date()).getTime() as context, it fetches nothing other than meta data.

Marking as needs work as you are working on this, it is not ready for review.

Reviewed the patch.

I have the following observations:
1. Instead of
'Beware that the modifications you made when using the edit link aren't local to the current entity edition but on the resource itself.'
I think something simpler like
'Modifications you make using the edit link will globally change the settings of the atom.'
would be better.

2. It seems that if the atom reference is in a vertical tab, then the "remove resource reference" button becomes 100% width.
This is because of the /misc/vertical-tabs.css

.vertical-tabs .form-type-textfield input {
width: 100%;

So possibly this needs to be overriden.

3. There seems to be some ctools cache issue. If i drag an image to the atom reference field, and then remove the reference and drag another image in there, then click on the edit link then i get the form for the original image and not for the current image.

4. Related to 3, if i have edit access on an atom, then i remove the reference and drag another atom where i have no edit access, then the edit link remains, does not disappear, and brings up the edit form for the previous atom where i had edit rights.

5. If i dont have edit right on an image then when i drag an image it has only the view link which is correct, but then when i drag an atom where i have edit rights, the edit link gets added to the end, so they are in a different order than when i have edit access right away, cause then the edit link comes first.

After "Edit resource", if CTools window is dismissed by 'Finish" or "Cancel", the representations of Atom references on the page are empty, for all Atoms which have not been embedded in the library pages previously displayed.
This is caused by :

          Drupal.dnd.fetchAtom('sdl_editor_representation', atom_id, function() {
            $this
              .empty()
              .append(Drupal.dnd.Atoms[atom_id].editor);

The .editor property is populated by renderLibrary, it is not by fetchAtom.
The fix coud be :

              .append(Drupal.dnd.Atoms[atom_id].contexts['sdl_editor_representation']);

(if you insist on using sdl_editor_representation instead of context set up in admin/structure/types/manage/story/display, but that's another issue)

Have a nice week-end-end ;)

- Issue description: Let's do a simple test :

1. Have a library with more than 10 atoms (the size of the library pager)
2. Create a node, drop a reference from an atom on page 2 of the libary
3. Save the node
4. Edit the node, don't touch the library widget
5. Scroll to the atom reference, click Edit Resource
6. Click Cancel

The div "atom_reference_drop_zone" is now empty...
If you re-edit the node, but with advancing the library widget at page 2 before setp 5, it's ok...

- About "sdl_editor_representation" : I am attached to the concept of wysiwyg, and would like that the atom reference be rendered with the context specified in the "Manage display" tab, even in Edit mode; I added this just to explain why I said "The fix could be" instead of "The fix is"...

Sorry again for my so bad english;

About the editor representation:
For wysiwyg i see the reasoning behind having something else than editor representation being the default, because you cannot control that from the manage display, and in fact the atoms will be with different representations anyways, as the user will change representations inside the wysiwyg.

But for atom reference its a bit different, because you control the display of the field in the manage display tab as you mention, but you can have several displays assigned to a node for example (especially with display suite), and then which representation it should choose to show up on the edit form? The one defined on the default display, or some other display?
I think that showing the editor representation in the "editor" was a deliberate choice, that's why its called editor representation. But in fact if we decide not to use the editor representation and only use lets say the one defined on the Default display in the editor, then the editor representation becomes useless.

Sorry to keep off topic,
just to answer nagy's question :

which representation it should choose to show up on the edit form?

In function atom_reference_field_widget_form_process(&$element), the $element contains #entity_type, #bundle, and #field_name, which is just a key of field_config_instance table, where the data field contains the context whch could (should IMHO) be used in the scald_render call line 224;
(I'm talkiing SQL because I'm experienced in, but I'm sure there is a Field API for that)

I do not think its off topic, since this relates to the patch, whether it should keep using editor representation or not.

We are still talking about the atom reference field in this issue (so not about the wysiwyg fields). In the atom reference field there is no context setting. The context is defined on the display that gets displayed, one of the many that can be defined. In the example you gave me, that means that yes the config will contain representations, but it will contain as many representations as many displays you have set up.
So in fact i can set many representation to a single atom reference field instance on the manage display tab, simply by setting it differently in each display. And since the editor representation was intended to be used on the edit form, and not one of the representations set in one of the displays, that this JS has the editor representation in the code is just how it should work, unless we want to throw out the editor representation and implement a different logic.

Different logic would be to add the default representation used at the edit form to the setting of the field (which is not there at the moment), or to use the representation from one of the displays. Should create a separate issue to decide about that, but in this issue and according to the current logic, using the editor representation in atom reference field is completely fine in my opinion, as it was intended to work this way.

I'm not sure tu understant the case of "many displays"; does this snippet solves your use case ?

function atom_reference_field_widget_form_process(&$element) {
  // Get the default value, and format the placeholder accordingly.
  $default = $element['#value'];
  if ($default) { // dsm($element);
    $info = field_info_instances($element['#entity_type'], $element['#bundle']); // dsm($info);
    $context = $info[$element['#field_name']]['display']['default']['type']; // dsm($context);
  //$context = variable_get('dnd_context_default', 'sdl_editor_representation');
    $prefix = '<div class="atom_reference_drop_zone">' . scald_render($default, $context) .'</div>';

BTW,
In my tests "beyond the borders", I found an interesting "feature" : I happened to specify the "Title" context for the atom reference; Then the View and Edit buttons did not show, because the "actions" array was not in the DOM, because atom was not fetched, because its représentaton did not match :
var match_atom_id = /

/g.exec($this.html());
Another :
If the Library is present due to the presence of an Atom Reference field, it allows Drag and Drop into the Body field, even if it NOT dnd-enabled...
The atom shows is editor représentaton during edit, but in view mode, it shows just the SAS représentaton...

The resources of multidimensional architecture of scald are endless...

@gifad:

Your snippet explicitely shows the problem @nagy.balint is talking about (even though you shouldn't be using field_info_instances, use field_info_instance instead to fetch data about a specific field instance). You're fetching the context of the 'default' display key, which is matching the configuration of the 'default' view mode, but there's a lot of other possible keys in there that could make sense.

Your test with the Title context failed, because only context that are marked as "Parseable" will be parse-able ; the title context is not parse-able, so you won't ever be able to find out which atom it was.

The atom being droppable in any textfield is #1965834: It is currently possible to drag an atom to any textfield

This patch is working perfectly for me, for 2 weeks now;

I'm not competent as a reviewer, but it has been extensively tested by the my community ;)

so, please commit
Thanks

Patch looks good in general. Some thoughts:

* Needs work:
- The text "Modifications you make using the edit link..." should only display when the edit link is available

* Some more nitpicks:
- The default dnd context is not always sdl_editor_representation (the patch was committed not long time ago).
- Use less specific CSS selectors ("div.class" could be ".class"? it seems work, but I didn't try every case).
- Use less specific text (View, Edit like in the library, instead of "View resource", "Edit resource").
- Use canonical link for view (atom/% instead of atom/%/view)

* Suggestion:
Could CTools dropbutton be used?
- Because I think those links are rarely used compared to the Remove link.
- The big button and a list of links are quite instrusive.
In this case: 1/ we don't need the text in my first remark 2/ we use simply "Remove" (or "Delete" whatever) instead of "Remove resource reference", because it is already implied in the context.

Yes it is, but it's a different issue :
dnd library does not know the target of its items : it is up to the target to fetch the correct context, as suggested in #4.

Now, that's fine ;)
Just fixed an issue : visual representation of the atom was not updated after “Edit” action, because representations are cached in the DOM (and Drupal.dnd.fetchAtom did not re-fetch it); so I added a delete just before...

I test the patch again.

- In a multivalue field, drop the second atom removes the arrow in the first dropbutton and we can't choose to edit or view that atom.
- This patch changes the default behavior: it use the default display context instead of sdl representation context. It should not. If we want to change the context used in field widget, there is #2046979: Atom reference widget can have different context per field instance.
- The dropbutton has a black border. I don't know where it comes though. It's a minor issue that should not hold this patch.

other minor issue : The dropbutton button does not work in the “Quick edit” mode of the Edit module...

Your patched applied, lines 238+ of atom_reference module are :

  if (isset($info['field_atom_reference']['settings']['rendering_context'])) {
    $rendering_context = $info['field_atom_reference']['settings']['rendering_context'];
  }

where field name is hardcoded as ['field_atom_reference']; it does not work if field has custom name...
You should use :

  if (isset($info[$element['#field_name']]['settings']['rendering_context'])) {
    $rendering_context = $info[$element['#field_name']]['settings']['rendering_context'];
  }

then everything works as designed ;)

NB: my issue #33 was fixed in #34, thanks

I haven't tested this new patch yet, just a quick review here because I really want to get it committed. Now a few remarks by skimming through the patch (so some could be incorrect):
- Atom reference field widget should not be global settings. I think we need something like an attributedata-widget-context="..." in the field widget.
- Delete a value in Drupal.dnd.Atoms may cause problem because the legend goes away. The legend currently can not be retrieved with Drupal.dnd.fetchAtom() - there is/was an issue for that. The bug indeed already exists, but this patch makes it more severe. Can we perhaps just remove the value in the used context?
- The widget context should be name "widget_context" (or "widget-context") and not "rendering context" (rendering makes me think about the context used in field display settings, not in the field widget).

Noted a bug in the settings.atom_reference.rendering_context variable;
It's initially correct, but if, after a first drop, you click Add another item, it becomes an array of strings, each one with the same context name, with a dimension equal to the number of atom reference fields in the node...
So subsequent drop fails (in fetchAtom); The "value" field is ok though, so click again Add another item, and the atom shows...
As a workaround, I added

if (Object.prototype.toString.call(rendering_context) === '[object Array]')
  rendering_context = rendering_context[0];

but I suspect the bug comes from

  $element['#attached']['js'][] = array(
    'data' => array('atom_reference' => array('rendering_context' => $rendering_context)),
    'type' => 'setting',
  );

(I don(t know the behaviour of this #attached attribute)

Other bug : The buttons don't show right after drop : you should replace:

              $this
                ...
                .find('input:button')
                .show();
by
              $this
                ...
                .find('div.atom_reference_operations')
                .show();

To clarify "rendering context": rendering context, or context in short, mean the same thing. I wanted to distinguish context used in field widget and context used in field display.

Attached the interdiff 39-42 that will help me review easier.

To clarify “The Drupal.dnd.Atoms deletion” :
Drupal.dnd.Atoms array acts as a cache, indexed by atom id, and context name;
If you apply a modification to the atom, such as change title or author, the fetchAtom will not fetch the modified atom, it will return the cached one; deleting Drupal.dnd.Atoms[atom.sid] will force download of the updated atom..
NB: to experience this, you have to use a context displaying some atom attributes...

Let's see if this patch still works

Could we get a reroll of this please? The patch is vastly out of date now.

Trying a reroll of the patch from #42...

Reroll of the patch from #42.

The 2nd remark of jcisio at #40 is correct : my deletion of the whole atom info is abusive (it deletes the legend, and more importantly (in the context of the upcoming dndck4), it deletes the sas)
fix is easy (lines #25~37)

       $('div.atom_reference_drop_zone.atom_reference_processed').each(function() {
         var $this = $(this);
         var rendering_context = $this.attr('data-rendering-context');
         var match_atom_id = /<!-- scald=(\d+):.*-->/g.exec($this.html());
         if (match_atom_id) {
           var atom_id = match_atom_id[1];
-          delete Drupal.dnd.Atoms[atom_id]; // to force reload
+          delete Drupal.dnd.Atoms[atom_id].contexts[rendering_context]; // to force reload
           Drupal.dnd.fetchAtom(rendering_context, atom_id, function() {
             $this
               .empty()
               .append(Drupal.dnd.Atoms[atom_id].contexts[rendering_context]);
           });

For some reason, I'm not able to produce a valid patch, sorry for that

The reason was that #2136415 (the widget plugin) was just being committed (Yay !!)
Attached patch should work...

@gifad
There is a huge difference between the patch 56 and 57.
I tried the patch, but it seems the ctools dropbutton is broken now, and indeed some reference to that is missing from the patch 57 compared to 56. And also when i dropped a text atom inside the atom reference, where before we got the rendered result now its just an empty box.

Fresh rebuild of this patch, against scald-7.x-1.3+1-dev ...

Okey this works better thanks, we will test this over the next weeks, and we will report if we see anything wrong.

While we are at it, the same could be done for the wysiwyg menu, if i right click an atom in ckeditor, and could click on edit to open up the ctools modal dialog for the selected atom.

Was it done in some way, or at least is there a separate issue for this?

No, this patch is only about the atom reference.
Lets move the ckeditor idea to a new issue, to not delay the patch getting into scald core.

not delay the patch getting into scald core

agree,
patch in use for 3 weeks, no trouble !

Same here :)

Tested #61 and works perfectly on our platform.

Updating the patch against latest dev to make drush make builds working again.

That is weird the #61 applies cleanly for me on the scald dev.

#61 works good for me as well.
Cleaning up the files

Drush make does not apply:

SourceTree does not apply:

PhpStorm applies correctly:

@drikc Thank you! correct. I added a gist
https://gist.github.com/wroxbox/587f264e34a2dffdd085

The new embed plugin has not been working well yet with picture&breakpoint responsive images so that was the main idea not to go with the latest codebase.

@Alex Bukach

I have just created a Feature Request issue for adding this functionality to the CKEditor context menu:

https://www.drupal.org/node/2453363

Committed #61.

Thanks!

The new embed plugin has not been working well yet with picture&breakpoint responsive images so that was the main idea not to go with the latest codebase.

@wroxbox: If this is still an issue for you, then please create a new issue, and please provide more details. As currently i cant see how this could be an issue, since the picture module is handled in the representation in the image module, and therefore it should work fine.

To use picture's responsive images with scald, I found that the "Make images responsive with the picture module " input filter (in the text format), and the "Support responsive images with the Picture module." plugin (in the CKEditor profile) must NOT be activated - that is, the scald_image transcoder has already done all the job...