Excellent work on the patch to get insert and picture to play ball together (http://drupal.org/node/1885766). I have it working locally perfectly (other than a small bug which I will ticket seperately).

In the site I am speccing out, it would be great if insert module worked in a similar fashion, is this functionality already out there, or something someone would need to patch as a new request to one or other of the 2 modules?

Thanks for the advice,
Dan

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

attiks’s picture

I think we'll need a patch to support insert, long time since I used insert, so check first if it works and report back.

kmonty’s picture

Any tips as to how you would like to implement this? I don't have oodles of time to work on a patch, but am willing to dip my toes into the water...

attiks’s picture

You can have a look at http://drupalcode.org/project/insert.git/blob/refs/heads/7.x-1.x:/includ... for some ideas, I think we need a separate template as well. The easiest will be to do the insert as the ckeditor dialog is doing it, so you only have to care about the insert part.

If you have any questions, leave a comment and thanks for doing this.

retrodans’s picture

Did anyone have a chance to look into this? If not, I may have time this week, after thinking about it all, I expect this is a patch for the insert module, not for the picture module (although feel free to let me know if you think otherwise).

Below are the changes I would need this patch to cater for, so again let me know your thoughts and I will try to consider them as I do this. The only bit I am unsure about is which theme function to use to render the new item into the insert module. Is it 'theme_picture($variables)'?

Also, once I start, I expect an issue around how to differentiate between images and breakpoint groups, but will assess that when I get to it.

Field Admin
-----------
* Below 'Enabled insert styles' add a new list titled 'Enabled insert breakpoints' which lists all the breakpoints available with an identical checkbox list - insert_styles_list() in insert.module
* Change 'Default insert style' so it says 'Default insert style/breakpoints' - in insert.module
* Populate the above list so it has breakpoints mixed in - insert_styles_list() in insert.module

Entity edit
-----------
* Rename item 'Style' with 'Style/breakpoint' - in insert-widget.tpl.php
* On insert, if breakpoint is selected, then render using a theme function

retrodans’s picture

Okay, managed to have a bit of a play today, and will get a quick concept patch up tomorrow at some point so you can all see and let me know thoughts before I tidy up my code. One issue I have had which you may be able to advise me on though is using

theme('picture', $variables).

Within variables there is a part called breakpoints, so to populate this I tried out (passing in my group id):

breakpoints_breakpoint_group_load()

then passed in the breakpoint variable from that. But sadly the output source doesn't look right (double slash instead of preset name), so believe I have passed in the wrong variable. What should I use instead?

<span data-picture="">
<span data-src="http://local.drupal7.com/sites/default/files/styles//public/testimages/judge-dredd.jpg?itok=bD6afE3x"  data-width="100px" data-height="100px"></span>
<noscript><img typeof="foaf:Image" src="http://local.drupal7.com/sites/default/files/styles//public/testimages/judge-dredd.jpg?itok=bD6afE3x" width="100px" height="100px" alt="" /></noscript>
</span>

Thanks,
Dan

attiks’s picture

Thanks for working on this, I think it better to do the same as the ckeditor is doing, inserting an IMG tag with some metadata. Ckeditor doesn't like custom span tags.

kmonty’s picture

I was just thinking what @attiks said in #6. Maybe it might make more sense to tokenize what is inserted and then process the code using an input filter? (Or regex on hook_node_view_alter()?)

attiks’s picture

That is what ckeditor dialog is doing now, it inserts an IMG tag with some data- attributes.

retrodans’s picture

Status: Active » Needs review
FileSize
4.4 KB

Thanks for the advice, here is an initial patch of the current master branch using the similar functionality you suggested and a first stab at the admin UI. Let me know any thoughts, and I can amend accordingly.

attiks’s picture

Status: Needs review » Needs work
FileSize
3.96 KB

I reworked the patch a bit, it didn't apply and I changed the terminology from 'breakpoint' to picture

I have no idea if this will get accepted by the insert module, since that module is build using include for each insert type, I guess picture should use the same principle like include/image.inc

So all logic specific to picture should be moved to that file.

Besides that: nice work, it works!

retrodans’s picture

Thanks. Okay, so in terms of good forum etiquette, what would be best. Shall I change the project for this ticket from Picture to Insert so it can go into that queue and get relevant feedback from their developers?

If so, I will get that happening, as would be good to get this done right, and then put somewhere for everyone to use.

Thanks,
Dan

attiks’s picture

#11 Yes feel free to change the project

retrodans’s picture

Project: Picture » Insert

Changing project for this ticket, as patch has been written for insert module.

retrodans’s picture

@attiks which git branch were you using for your version of the patch, as it doesn't apply for me (I was using 7.x-1.x from version control pages docs).

attiks’s picture

#14 7.x-1.x last commit is 0bd9301

retrodans’s picture

Thanks, I see where I went wrong, I did a reset back to master rather than the branch, all reset locally, and appears to work now, thanks.

m.david’s picture

This looks promising! Are there any plans to commit the patch and make it work with Insert 7.x-1.3 or later versions?

mengi’s picture

I can confirm the patch from #10 works. Using insert-1.3 and latest dev of picture.

retiredpro’s picture

Is it possible to get alt and title attributes inserted as well with patch #10?

mengi’s picture

Issue summary: View changes
Status: Needs work » Needs review

Changing to 'needs review'. Patch is in #10.

I have been using the patch in #10 for a while and works great. CKEditor module version 1.13 and CKeditor version 4.0

Nicolas Bouteille’s picture

Status: Needs review » Needs work

I carefully applied the patch #10 manually but it does not work for me :(
First I get this error
Notice: Undefined variable: style_options in insert_element_process() (line 300 of /...mypath.../insert/insert.module).
Then when I click on Insert button nothing happens.
I can however properly select the Picture group in my field insert settings.

Nicolas Bouteille’s picture

Ok I get it. For it to work I must enable at least one picture group in the insert settings. Which should not be the case. We should be able to only select a picture group in the dropdown "Default insert style/picture group" which instructions are "Select the default style which will be selected by default or used if no specific styles above are enabled." So this should be fixed.

Also, "Maximum image insert width" is not respected which I find very useful to set to 400px by default so that end users can see the whole Picture and resize it to their needs (I use CKEditor Enhanced Image non-default plugin). I don't want Picture to define the final width of the image. I only use it so that original image is 1200px max-width on mobiles and 2000px max-width on tablets and up. This should be included too :)

Nicolas Bouteille’s picture

in insert.js, this if returns false when it should return true:

<?php
    // Check for a maximum dimension and scale down the width if necessary.
    // This is intended for use with Image Resize Filter.
    var widthMatches = content.match(/width[ ]*=[ ]*"(\d*)"/i);
    var heightMatches = content.match(/height[ ]*=[ ]*"(\d*)"/i);
    
    if (settings.maxWidth && widthMatches && parseInt(widthMatches[1]) > settings.maxWidth) {
?>

but settings.maxWidth does equal 400 so the value is correctly passed

Nicolas Bouteille’s picture

When using image styles, widthMatches equals to width="2000",2000 for example. With Picture groups as of now it equals to null.

Nicolas Bouteille’s picture

Looks like insert.js reads the width and height from insert-image.tpl
and width and height variables are set in includes/image.inc

<?php
/**
 * Theme the content that will be inserted for Image styles.
 */
function template_preprocess_image_insert_image(&$vars) {
  $vars['file'] = file_load($vars['item']['fid']);

  // Determine dimensions of the image after the image style transformations.
  $image_info = @image_get_info($vars['file']->uri);
  $vars['width'] = isset($image_info['width']) ? $image_info['width'] : NULL;
  $vars['height'] = isset($image_info['height']) ? $image_info['height'] : NULL;
?>

But I'm having a hard time dpm() from here

jessking’s picture

The patch works for me, however I get this error on saving the node:

Notice: Undefined index: standard_mappings in _picture_filter_prepare_image() (line 863 of /sites/all/modules/picture/picture.module).
Notice: Undefined index: standard_mappings in _picture_filter_prepare_image() (line 873 of /sites/all/modules/picture/picture.module).

I also get broken images on IE

retiredpro’s picture

I finally got around to getting the alt and title tags inserted into the wysiwyg along with the images.

After using patch #10. Look for the following set of code in insert.module.

    // Loop through the picture groups, appending to the final list
    $picture_groups = insert_picture_groups();
    $insert_picture_groups = (isset($widget_settings['insert_picture_groups']) ? $widget_settings['insert_picture_groups'] : array());
    foreach ($insert_picture_groups as $picture_group_name => $enabled) {
      if ($enabled && isset($picture_groups[$picture_group_name])) {
        // Setup the image html using similar token format as ckeditor used as
        // then it will be parsed using a prebuilt and shared function.
        $filepath = parse_url(file_create_url($item['uri']));
        $filepath = $filepath['path'];
        $variables = array(
          'path' => $filepath,
          'attributes' => array(
            'data-picture-group' => $picture_group_name,
            'class' => !empty($widget_settings['insert_class']) ? $widget_settings['insert_class'] : '',
          ),
        );

You just need to set the alt and title values into the attributes array as shown below. Then JS will recognize the placeholders and replace them accordingly when you click the insert button.

$variables = array(
  'path' => $filepath,
  'attributes' => array(
  'data-picture-group' => $picture_group_name,
  'class' => !empty($widget_settings['insert_class']) ? $widget_settings['insert_class'] : '',
    'alt' => '__alt__',
    'title' => '__title__',
  ),
);
stewart.adam’s picture

Status: Needs work » Needs review
FileSize
3.97 KB

Attached patch is a reroll against latest git and is for use with Picture 7.x-2.x (requires a rename of attribute from 'data-picture-group' to 'data-picture-mapping').

Chris Burge’s picture

The patch from #28 works in my testing. I have rerolled it to 1) include #27 (alt and title attributes) and 2) change the style options select list to display the picture group's human-readable name and not its machine name.

Has anyone tested to confirm that Insert will continue to function normally if Picture is not enabled?

mr. chips’s picture

I've used the 29 patch and Insert seems to work without Picture installed. I didn't test it much 'cause I really want to use Picture too. The patch works pretty well but I have some strange issues regarding Insert - Picture - Breakpoint:

Note: I have the Image field format set to Hidden in Manage Display for the content type so it doesn't display twice.

It's working except on my two iOS devices, both ver 7.1.2. I only have an iPhone and iPad mini (which I don't want to upgrade because of the wifi problems). On the iOS devices none of the images inserted by Insert will display in any node (teaser or full), until I navigate to Manage Display of the content type and click Save. I don't change anything, I just click Save.

Then all the images inserted by Insert will display, in all nodes.

If I edit ANY node (just clicking Edit and Save is enough), I need to go back to Manage Display and click Save again or none of the images in any node will display. I'm only working with Articles at this point and they all have pictures but I can replicate this every time.

attiks’s picture

#30 If it is only on some devices, this sounds like a caching problem

mr. chips’s picture

Looks like you were on the right track Attics, thanks! I've been developing on my local Mac with MAMP, and finally realized that my Mac was the only device that worked 100% of the time. Once I uploaded the site to a real world server it all started working. Bummer... I haven't had a problem like this with MAMP 'til now. Guess I need to upgrade again...

portulaca’s picture

Thank you so much for working on this!

I tried patch from #29 and it's working, but I'm getting this message after inserting a picture mapping and saving the node:

Notice: Undefined index: content_image_mapping in _picture_filter_prepare_image() (line 1772 of /var/www/html/name/sites/all/modules/picture/picture.module).

Note: I had to adjust the Text format filter order to make "Convert line breaks into HTML (i.e. <br> and <p>)" come before "Make images responsive with the picture module" to make <picture> code work inside Body field.

Is there a way to also make it work with Colorbox? So Picture + Colorbox + Insert to make it insert responsive image (that the patch is already providing) but then also link it through Colorbox to open a larger version in a modal? Picture mapping and Colorbox already work well together as a field formatter, but that is useless on multivalue fields that aren't meant to be displayed as a gallery but rather have the values/images interspersed throughout the content.

Snater’s picture

Browsing through old tickets while working on the module's D8 version. I tend towards closing this ticket as outdated, but will have it rest for a few more weeks.

The latest patch seems to be flawed as to the most recent comment above. Apart from that, implementing the interface directly into "core" Insert is no good design. The interface should be in a separate module, or in a sub-module of Insert or Picture.

Anyway, it would be great to support D8's Picture / Responsive Image and in that case, the place to put the interface would, of course, need to be Insert. Yet, still, the interface should be in a sub-module depending on Insert and Responsive image, instead of the code being baked directly into Insert. I have opened a ticket for that task: #2988476: Support Responsive Image module

Snater’s picture

Status: Needs review » Closed (outdated)

As per last comment.