Not sure if I had to file this as bug, feature request, or support request.

When building the skinr ui form I need to render or omit things based on theme settings of the theme that is being 'skinned'. However, because the seven theme is loaded @ the ui form a call to theme_get_setting request a setting from the Seven theme. So we would have to know the theme-context within the skinr ui form so that you can do theme_get_setting('setting', $theme) and get the right setting for the theme that is acted on.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jeff Burnz’s picture

Subscribe :)

moonray’s picture

Skinr UI adds a set of form elements with the following structure. You can deduce the currently used theme from there:

$form['skinr_settings'][$module . '_type'][$theme->name]['groups'][...]

noland’s picture

just wondering if this has anything to do with blocks only being rendered with the skinr settings applied for the site-wide default theme and ignoring their setting for another (enabled) theme whose template is actually being used?

I have 2 sub themes of Acquia Marina and each use skins from that base. One of my sub themes, ThemeA is the default, ThemeB is also enabled and used only in the Organic Groups context (using OG Theme). The group context rendered with ThemeB in that the logo and slogan of the site change but all the blocks and panes retain their ThemeA skins even though I set (and the settings persist) different skins for them under ThemeB. Theme Dev says that the blocks use ThemeB template.

Is this related to this issue?

moonray’s picture

ngermain: doesn't sound realted at all. please open a separate ticket. Also, going to need a bit better description. can't quite make out the problem you're having.

noland’s picture

Ok. I think it actually is a bug and I think I have a fix for it. see http://drupal.org/node/1311046

JurriaanRoelofs’s picture

The $form variable appears to be unavailable in the .inc files in my theme. get_defined_vars() also doesn't give me any theme-context in the .inc file or within the hook implementations.

Maybe the solution to this, and other advanced theme integrations is to complete the @todo at skinr_ui.module#490:

// Create widget.
$field = array();
if (!empty($skin_info['form callback']) && function_exists($skin_info['form callback'])) {
// @todo Allow for custom form callbacks.
}

JurriaanRoelofs’s picture

I'm trying to create a patch that does allows a skin to implement a callback function, but as we would want the callback function to be included, the MY_THEME/core.inc the inc file must be loaded otherwise the function does not exist when loading the skinr form.
Do you know if there is a function in the current skinr codebase that rounds up all the .inc files and includes them? We'll have to make it work so that we only call the .inc files that implement callbacks but that shouldn't be hard. I'm just asking to make sure I don't replicate any code in my patch.

Right now the .inc files are only loaded once after you clear cache and after that the form seems to be cached or something.

This what I got so far.

in skinr_ui.module around line #490

      // Check if any function needs to be called for this skin.
      // Provide theme and module context via $params.
      $field = array();
      if (!empty($skin_info['skin callback']) && function_exists($skin_info['skin callback'])) {
        $skin_info['skin callback']($params);
      }

      // Create widget.
      $field = array();
      if (!empty($skin_info['form callback']) && function_exists($skin_info['form callback'])) {
        // @todo Allow for custom form callbacks.
      }

in my .inc file

function _tundra_core_my_callback_function() {
  $media_total = theme_get_setting('media_total', $params['theme']);
  drupal_add_css( etc. etc.
}

for ($media_count = 1; $media_count <= $media_total; $media_count++) {
  $medium = $media[$media_count-1];

  $skins["grid_width{$media_count}"] = array(
    'skin callback' => '_tundra_core_my_callback_function',
    'title' => t('@medium Grid-width', array('@medium' => $medium)),
    'description' => t("Change the width of this block."),
    'type' => 'select',
    'options' => _tundra_core_skinr_grid_width_options($media_count),
    'group' => 'layout',
    'theme hooks' => array('block', 'panels_display', 'panels_pane', 'panels_panel'),
    'default status' => TRUE,
  );
moonray’s picture

Use skinr_load_include() to load your includes. See skinr_hook() for an example of how to get the right path for your include and how to invoke skinr_load_include().

moonray’s picture

Another thought... the form callback should probably include a $context array with relevant data (such as $theme, $skin_name, and maybe $skin_info; these are derived from the variables used in the foreach loops in skinr_ui_form_alter()).

JurriaanRoelofs’s picture

Assigned: Unassigned » JurriaanRoelofs
Status: Active » Needs review
FileSize
2.72 KB

Allright sorry for the delay, here is my patch. I've reworked it a few times and I think now I better understand the Skinr code. I've tried to mimic skinr's variable names, caching strategy and code organisation so I've put the heavy stuff in a separate function in skinr.module and it only runs if there are actually callback functions that need to be loaded.

This is the first time I've made a patch file for a module, let me know if anything needs fixing.

Currently the callback function just loads skinr info files, searches for callback functions, and runs them with a contextual function argument that provides the currently enabled theme.

moonray’s picture

Finally got around to reviewing this patch. It doesn't seem quite right...

What is the purpose of these 'skin callback' functions? What are you trying to accomplish with it? Do you have an example callback function?

What we envisioned originally is for a 'form callback' to be an alternative to the automatic widget creation.
If you look at skinr_ui.module around line 491 you'll see the following code which should be expanded upon:

<?php
      if (!empty($skin_info['form callback']) && function_exists($skin_info['form callback'])) {
        // @todo Allow for custom form callbacks.
      }
?>

I'm working on a patch that does the original intended behavior, and should (hopefully) work for your purposes.

JurriaanRoelofs’s picture

Easiest way to see what's going on is hitting the edit-skin form in this profile I made that has the patch applied:
http://drupal.org/project/starterkit_arti

Generally speaking, the skin_callback functions are just hooks that any skin can use to do something before the form is built. In my case I use it to determine how many form fields I need and to theme the form.

This is one example callback function, just adding some CSS:

function _arctica_configurator_form($params) {
  $arctica_theme_path = drupal_get_path('theme', 'arctica');
  drupal_add_css($arctica_theme_path . '/styling/css/arctica.configurator.css', array('group' => CSS_THEME, 'weight' => 10));
}

Another example of stuff I need to do in the skinr form to communicate with the theme settings:

/**
 * @code
 * Due to the order of steps that build the skirn form it's not possible
 * to get the current theme for every theme fieldset in the form so we
 * assume user is theming the currently enabled theme.
 */

$current_theme = skinr_current_theme(TRUE);
if (theme_get_setting('responsive_enable', $current_theme)) {
  $media_total = theme_get_setting('media_queries', $current_theme);
  $media = array();
  if ($media_total && is_numeric($media_total)) {
    for ($i = 1; $i <= $media_total; $i++) {
      $media[] = 'medium' . $i;
    }
  }
}
else {
  $media = array('Default');
  $media_total = 1;
}

And then after this code when I define my widgets I loop over the number of media queries that the theme is using to target different devices:

for ($media_count = 1; $media_count <= $media_total; $media_count++) {
  $medium = $media[$media_count-1];

  $skins["grid_width{$media_count}"] = array(
    'title' => t('@medium Grid-width', array('@medium' => $medium)),
    'description' => t("Change the width of this block."),
    'type' => 'select',
    'options' => _arctica_skinr_grid_width_options($media_count),
    'group' => 'layout',
    'theme hooks' => array('block', 'panels_display', 'panels_pane', 'panels_panel'),
    'default status' => TRUE,
    // @todo maak functie die active grid system kent en juiste opties geeft
    // 'options' => arctica_skins_grid_options(),
  );

  $skins["grid_clear-{$media_count}"] = array(
    'title' => t('@medium Grid-width', array('@medium' => $medium)),
    'title' => t('Float Clearing'),
    'description' => t("Select Clear:left to start a new grid row. This is not if grid units within a region have equal heights."),
    'type' => 'select',
  'options' => array(
    'clear-both' => array(
      'title' => 'Clear both',
      'class' => array("media-{$media_count}-clear-both"),
    ),
    'clear-left' => array(
      'title' => 'Clear left',
      'class' => array("media-{$media_count}-clear-left"),
    ),
    'clear-right' => array(
      'title' => 'Clear right',
      'class' => array("media-{$media_count}-clear-right"),
    ),
    'clear-none' => array(
      'title' => 'Clear none',
      'class' => array("media-{$media_count}-clear-none"),
    ),
  ),
    'group' => 'layout',
    'theme hooks' => array('block', 'panels_display', 'panels_pane', 'panels_panel'),
    'default status' => TRUE,
  );

  $skins["grid_prefill-{$media_count}"] = array(
    'title' => t('@medium Grid postfill', array('@medium' => $medium)),
    'title' => t('Grid prefill'),
    'description' => t("Push the block towards the far end of the layout"),
    'type' => 'select',
    'options' => _arctica_skinr_grid_prefill_options($media_count),
    'group' => 'layout',
    'theme hooks' => array('block', 'panels_display', 'panels_pane', 'panels_panel'),
    'default status' => TRUE,
  );

  $skins["grid_postfill-{$media_count}"] = array(
    'title' => t('@medium Grid-width', array('@medium' => $medium)),
    'title' => t('Grid postfill'),
    'description' => t("Push the block towards the first edge of the layout"),
    'type' => 'select',
    'options' => _arctica_skinr_grid_postfill_options($media_count),
    'group' => 'layout',
    'theme hooks' => array('block', 'panels_display', 'panels_pane', 'panels_panel'),
    'default status' => TRUE,
  );
}
moonray’s picture

Assigned: JurriaanRoelofs » moonray
Status: Needs review » Needs work
FileSize
2 KB

Got the code for it written, just need tests.

Below is an example skin plugin file that uses a form callback.
The $context parameter is an array with the following items:
$context['theme'] // theme name
$context['skin_name'] // current skin name
$context['skin_info'] // current skin info array

<?php
/**
 * Implements hook_skinr_skin_PLUGIN_info()
 */
function grids_skinr_skin_wires_info() {
  $skins['skinr_ui_test_color'] = array(
    'title' => t('Color'),
    'form callback' => 'grids_skinr_skinr_ui_test_color_form',
    'group' => 'general',
    'theme hooks' => array('block__system', 'comment_wrapper__page', 'node__page'),
    'default status' => 1,
    'options' => array(
      'color_white' => array(
        // Note: 'title' is not needed for form callback skins.
        'class' => array('color-white'),
      ),
    ),
  );
  return $skins;
}

function grids_skinr_skinr_ui_test_color_form($form, $form_state, $context) {
  $form = array(
    '#type' => 'checkboxes',
    '#title' => t('Color'),
    '#options' => array(
      'color_white' => t('White'),
    ),
  );
  return $form;
}
?>
moonray’s picture

Status: Needs work » Needs review
FileSize
3.96 KB

And here's a patch that includes tests.
Please review.

moonray’s picture

Status: Needs review » Fixed

Due to lack of feedback I've decided to commit this patch. If you have problems with it, reopen this issue.

nedjo’s picture

Status: Fixed » Active

The commit contained a stray pass by reference that is causing fatal errors as reported in #1395386: Skinr Jan 5th dev release makes web site inaccessible.

nedjo’s picture

Status: Active » Needs review
FileSize
490 bytes

Patch attached.

moonray’s picture

Status: Needs review » Fixed

Thanks for catching that, nedjo. Committed.

Status: Fixed » Closed (fixed)

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

JurriaanRoelofs’s picture

Thanks bala, well done you did a much cleaner job than I did. Sorry for being slow I was preoccupied with some projects.
I've done some small preliminary tests and it seems to work properly. I'll put this in my base theme soon.

JurriaanRoelofs’s picture

Status: Closed (fixed) » Active

This patch is not working as intended. I had used it to ensure the my inc file gets loaded but now I actually need the intended purpose of this patch, namely to provide the theme context and use it to produce skins.

Problems with the patch:
1. The color-white example doesn't work, if I safe the form it doesn't remember the state of the checkbox, it's always false. Even if it does work, it seems that the callback is not supposed to be used for adding more or less skinr classes right? so what is this form callback intended for?
2. It doesn't provide theme context to the scope where it is useful. The $context['theme'] variable is not available in the scope of the hook_skinr_skin_PLUGIN_info info so it can't be used to create theme-specific skins.

What I want to do:
Create different sets of skins for different themes in hook_skinr_skin_PLUGIN_info, by querying theme settings.
For example: in the edit-skin form I see 2 fieldsets for 2 enabled themes e.g. bartik and seven.
Bartik and seven both have a theme setting called 'device_count' that specifies how many layout contexts they support.
Now if Bartik has this setting set to 5, in the edit-skin form I might need 5 "Block width for device X" skins where X is the index number of the device. If Seven has this setting set to 2, it will only need 2 layout skin widgets in the edit skin form.
This example is a simplification of the code I posted in comment #12, where I produce sets of skins in a loop, where the theme setting determines how many times the loop loops.

Because this patch provides the $context_theme after skins are defined in hook_skinr_skin_PLUGIN_info, there is still no way to define theme-specific skins as far as I know.

Do you think it's at all possible the get the theme context in the inc file? Because I notice the inc file gets loaded only once, not once for every theme.

moonray’s picture

The color white example listed above worked perfectly for me.
Note that it's being limited to system blocks, comments on the page node, and the page node (see the 'theme hooks' variable).

Context outputs the following for me, which I believe should be sufficient for you to get the theme settings you require.
Do remember that all possible options need to be defined in the hook_skinr_skin_PLUGIN_info() for them to be available...

<?php
array(
  'theme' => 'bartik',
  'skin_name' => 'skinr_ui_test_color',
  'skin_info' => array(
    'title' => 'Color',
    'form callback' => 'grids_skinr_skinr_ui_test_color_form',
    'group' => 'general',
    'theme hooks' => array(
      'block__system',
      'comment_wrapper__page',
      'node__page',
    ),
    'default status' => 1,
    'options' => array(
      'color_white' => array(
        'class' => array(
          'color-white',
        ),
      ),
    ),
    'name' => 'skinr_ui_test_color',
    'type' => 'checkboxes',
    'description' => '',
    'attached' => array(),
    'weight' => NULL,
    'status' => array(
      'bartik' => 1,
      'garland' => 1,
      'seven' => 1,
    ),
    'source' => array(
      'path' => 'sites/all/modules/grids/skins/wires',
      'filename' => 'wires.inc',
      'type' => 'module',
      'name' => 'grids',
      'version' => '7.x-1.0',
      'directory' => 'skins',
    ),
  ),
)
?>
lolmaus’s picture

I'm on Skinr 7.x-2.0-beta1.

I've got a custom setting in my theme, that i access with theme_get_setting.

In my skins/myplugin/myplugin.inc i have two functions:
mytheme_skinr_group_myplugin_info() and mytheme_skinr_skin_myplugin_info().

When i call theme_get_setting from mytheme_skinr_skin_myplugin_info(), i get the setting. But from mytheme_skinr_group_myplugin_info(), theme_get_setting returns an empty result. :(

Thus, i fail to create groups dynamically based on that theme setting. I have to put all my settings in one bloated group. :(

How can i call theme_get_setting from mytheme_skinr_group_myplugin_info()?

rooby’s picture

This feature does not seem to be documented.
Can someone give some information on its purpose/what you would use it to do?

I'm not sure I understand from reading back over some of the comments because it seems to me like something that could be done via form_alter as per #2 (although then it would not be within the plugin itself but the module that the plugin belongs to - is that the reason this is here?)

moonray’s picture

@rooby See comment #13. The purpose for having form callbacks is to allow custom form elements not defined by Skinr. You could use a textfield instead of checkboxes/etc. or add a set of form elements to set up the skin settings you wish to capture.

moonray’s picture

@lolmaus I'm not sure why you're getting the results you are. There isn't any code in Skinr that would cause theme_get_settings to return empty results. If you can get it in mytheme_skinr_skin_myplugin_info() then you should get it in mytheme_skinr_group_myplugin_info() (the callbacks are called right after each other in skinr_ui_form_alter(), and in all other cases skin info is initialized before groups, so it's wouldn't be a matter of the theme settings not being available yet).

rooby’s picture

That is actually exactly what I currently need.

Should mean I can do it cleaner than with form alter.

rooby’s picture

Status: Active » Needs review
FileSize
574 bytes

There is a problem I have come across that might be what the others are experiencing.

The default value is not passed through so you set a skin, then edit it and your setting is not repopulated.

This patch addresses it for me.

All you should have to do is add this to your element:
<?php 'default_value' => $context['default_value'], ?>

moonray’s picture

Status: Needs review » Needs work

Can you either provide a test or describe the steps required to repeat this error?

rooby’s picture

I can't even remember now. When I get a chance I will roll back my patch and see what the error was.

I don't know how you would do without the default value as per the patch though. How else would you set the existing value when someone loads the form?

moonray’s picture

I was confused with other issues.
You are right, we're not passing the default value to the custom form.

The patch in #28 should fix that. However, I prefer using (value instead of default_value for the context) 'value' => isset($defaults[$theme->name][$skin_name]) ? $defaults[$theme->name][$skin_name] : array(),

moonray’s picture

Issue tags: +Needs tests

Also, we're going to need tests to make sure this doesn't get broken again in the future.

moonray’s picture

Status: Needs work » Needs review
FileSize
4.54 KB

Attached patch adds tests, and documentation in skinr.api.php.

moonray’s picture

Status: Needs review » Fixed

Committed.

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