Currently when scald is installed on a new system (dev version), it still comes with the legacy mode of embedding atoms.

I think that we should change a few default variables in the case of new installs, to make it closer to the new way of doing it for new sites.

Could be useful for the next stable release.

Comments

nagy.balint’s picture

As i see the steps to do in this task are:
- Updating the documentation, to list the dndck4 setup in the installation by default, and some notes on how to access the legacy version after.
- Changing the default value of mee_store_format to embed_div

However the other three steps here: https://www.drupal.org/node/1775718
Are manual steps.
Probably we shouldnt programmatically add the filter for all text formats.
The dnd plugin on ckeditor was always added manually before as well.
And downloading a custom build of the ckeditor with the widget support is also manual.

So at this time not a lot of things can be done.

gifad’s picture

not a lot of things can be done.

Two contributions about this, more related to Easier way of setting the mee_store_format variable, but this issue is fixed, and this is related to manage coexistence of the two systems;

1 - about the plugins
I can make two assertions :
a) The use of the legacy plugin makes sense if and only if mee_store_format is 'sas'
b) The use of the dndck4 plugin makes sense if and only if mee_store_format is 'embed_div'
If these are valid, then we can register only one plugin:

function dnd_ckeditor_plugin() {
  $plugins = array();
  $mee_store_format = (module_exists('mee')) ? mee_store_format() : '';
  if ($mee_store_format == 'sas') {
    $plugins['dnd'] = array(
      'name' => 'dnd',
      'desc' => t('Scald Drag and Drop integration'),
      'path' =>  drupal_get_path('module', 'dnd') . '/plugins/ckeditor/',
      'buttons' => array(),
    );
  }
  elseif ($mee_store_format == 'embed_div' && version_compare(ckeditor_get_version(), '4.3.0') >= 0) {
    $plugins['dndck4'] = array(
      'name' => 'dndck4',
      'desc' => t('Scald Drag and Drop integration - CKEditor 4 widgets'),
      'path' =>  drupal_get_path('module', 'dnd') . '/plugins/dndck4/',
      'buttons' => array(),
    );
  }
  return $plugins;
}

The user will still have to check boxes in ckeditor profiles, but he/she will be sure to select the "right" plugin

2 - About the input text filter
If mee_store_format is 'embed_div', the mee_filter_process() function MUST be executed, but it is not technically necessary that this be in a filter context;
So the filter activation may be an optimisation option, just as "Always rebuild rendered content"
mee_field_attach_view_alter() could manage both cases as follow:

function mee_field_attach_view_alter(&$output, $context) {
  $store_format = mee_store_format();
  if ($store_format == 'sas' || $store_format == 'embed_div') {
    list($id, $revision_id, $bundle) = entity_extract_ids($context['entity_type'], $context['entity']);
    $fields = field_info_instances($context['entity_type'], $bundle);
    foreach ($fields as $name => $field) {
      if (!empty($field['settings']['dnd_enabled']) && isset($output[$name])) {
        foreach (element_children($output[$name]) as $key) {
          if ($store_format == 'embed_div') {
            $input_format = $output[$name]['#items'][$key]['format'];
            $list = filter_list_format($input_format);
            if (empty($list['mee_scald_widgets']) || $list['mee_scald_widgets']->status != 1) {
              $output[$name][$key]['#markup'] = mee_filter_process($output[$name][$key]['#markup']);
            }
          }
          $output[$name][$key]['#markup'] = scald_sas_to_rendered($output[$name][$key]['#markup'], $field['settings']['context']);
        }
      }
    }
  }
}

How to check "the case of new installs" is another problem, but the two patches above can make life easyer ;-)

nagy.balint’s picture

Thanks!

Well basically, for old installs it should not change anything.
So far 1 and 2 does not do anything bad for old installs.
For new installs however this can indeed help.

Im thinking about one scenario for 1), What if I have an old install on 'sas', and then i switch the method to 'embed div' without unticking the plugin first. Will the old plugin be automatically disabled?

Otherwise it would be nice to have a patch for this, and test it a bit more. Also to set the default value of the mee store format variable in that patch.

gifad’s picture

The "wrong" plugin is automatically de-activated when you visit admin/config/content/ckeditor, which you have to do, in order to activate the new one for the given profile(s) - so there cannot be any conflict

Well, this is for the stable release of CKEditor module, but now that "CKEditor plugin system is modular and clean", everything can happen >>)

about "Also to set the default value of the mee store format variable", I have no idea how we can know that this is a "new install" (because we should not change the store format at an upgrade, not even at a disable/enable sequence).
Perhaps checking the mee_resource table is empty ? - it records the embedded atoms, so if it is empty, no storage_format has been in effect... but is it allowed to issue a db query in mee.install ?

nagy.balint’s picture

mee_store_format, is a variable, which is queried with:
function mee_store_format() {
return variable_get('mee_store_format', 'sas');
}

So all we need to do is to change 'sas' default value in the variable_get function to embed_div,

And also to fix the bug in dnd.admin.inc that the default value for the new UI option is not taken from this function, so
'#default_value' => variable_get('mee_store_format', 'sas'),
should be replaced with
'#default_value' => mee_store_format(),

So for old installs the value of the variable will not change, for new installs the default value is going to be embed_div, and the variable will be created like that as well, if you save the dnd settings form.

nagy.balint’s picture

Maybe one issue could be if the old install never actually created this variable, and always used the default value, in which case the change would be problematic... but maybe we can make an update hook that creates the variable in case it does not exist with the old default value. And then it will be 100% correct in all cases.

Because if its an old install and the variable does not exist, then we create the variable with the old value in the update hook.
If its an old install and the variable exists, then we do nothing.
If its a new install, then we wont be running the update hook, and the default is now embed_div, so its fine as well.

gifad’s picture

Status: Active » Needs review
StatusFileSize
new2.96 KB

Here is a patch for this;
I use the filter bypass for months, the plugin pre selection works with latest ckeditor dev, but the install trick is untested...

nagy.balint’s picture

StatusFileSize
new6.55 KB

I think that the default value of variable_get in mee_store_format() can be embed_div, because after the update hook it wont matter for old installs, and it looks better since thats the default anyways now.

Added that the UI for the variable only appears in dnd settings form if mee module is enabled, and now uses the function to get the value as well.

Fixed a few typos in mee.module.

Tested the update hook, it seems to work fine. Tested it on a site with the old plugin, and on a site with the new plugin. Tried having no filter and having a filter, and in both cases it seems to work fine, the condition is fine as well.

Likely the install hook works fine as well since it is just a variable_set.

Seems like this could go in, but you can double check.

gifad’s picture

you can double check.

Just a side note for testers/debuggers : the check of ckeditor_get_version() fails if you are using ckeditor-dev (from github) at the source level, because of :

    if ($ckeditor_version == '%VERSION%') {
      $ckeditor_version = '4.0.0';
    }

This is of course totally irrelevant for this issue, which is about "the next stable release"

nagy.balint’s picture

Actually this is the right issue.

However that check was already in before this patch, so its not something we introduced here.

nagy.balint’s picture

But maybe that check is unnecessary as having ckeditor 4.3 is not enough, you need a custom build of ckeditor with widget support. so a better check would be to see if the installed ckeditor has the widget plugin enabled or not, but not sure if its doable or not.

gifad’s picture

ckeditor_version was a joke : Right after preparing the patch, I installed latest ckeditor module (modular and clean) and latest ckeditor library (branch major) - I spent a couple of minutes to figure out why it was not working...

But now, any up to date site will use ckeditor from CDN, and dndck4 works with the default ckeditor settings
(//cdn.ckeditor.com/4.4.3/full-all) unaltered: the widget plugin is dynamically loaded after dndck4 statement :

CKEDITOR.plugins.add('dndck4', {
  lang: 'en',
  requires: 'widget',

Anyway, the plugin activation is at profile level, and scald cannot know which profile(s) is/are relevant for use with dnd/mee

nagy.balint’s picture

Tested new install on simplytestme, seems to be fine.

  • nagy.balint committed 9576c81 on 7.x-1.x authored by gifad
    Issue #2458875 by nagy.balint, gifad: Make the dndck4 ckeditor plugin to...
nagy.balint’s picture

Status: Needs review » Fixed

Committed, Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.