Determine optimal skin include file format

Ok, the following is a result of lots of discussion between @moonray and I about how the skins and groups need to be structured. There is documentation for each hook, and all the properties of each in the comments.

I added the full version of the plugin below (which is a working example) to GitHub. In addition to what's noted in the @todo's, I'll be creating more skins to test with, which implement more advanced functionality. These will all be added to the GitHub repository linked above, when finished:

1. A subtheme that enables a skin in its parent theme.
2. An example of how to alter a skin and group implementation.
3. Examples of skins that use more advanced file handling, such as external JS files, and IE conditional stylesheets.

Below are the contents of styles.skinr.inc (from the GitHub repo) for reviews:

/**
 * @file
 * Implementation of a Skinr plugin.
 *
 * @todo Decide on the naming convention for this style. This depends on whether
 * or not CTools will handle the plugin functionality. Currently proposed
 * naming convention which doesn't use CTools, is PLUGINNAME.skinr.inc. 
 */

/**
 * Implements hook_skinr_info().
 *
 * Define the API version of this Skinr plugin. This is required when
 * creating a new Skinr plugin. It checks to make sure your Skins are compatible
 * with the installed version of Skinr. The plugin will inherit information such
 * as what version of core it applies to from the module or theme implementing
 * it.
 *
 * The "hook" prefix is substituted with the name of the module or theme that
 * implements it, e.g. THEMENAME_PLUGINNAME_skinr_info(), or
 * MODULENAME_PLUGINNAME_skinr_info().
 *
 * @todo We are not sure this will be necessary, if we do not end up using
 * and external too such as CTools to handle plugin functionality. If we do use
 * CTools, it will be required and will need to contain more information.
 */
function skinr_styles_skinr_info() {
  return array('skinr api' => '2.0');
}

/**
 * Implements hook_skinr_groups().
 *
 * Optional. Defines group(s) that will contain skins. Groups are form element
 * containers used to organize skins categorically using vertical_tabs. If you
 * don't define a group, your skins will appear in Skinr's default group, which
 * is labeled "General."
 *
 * Group name(s):
 * Each group name must be unique. It is recommended to prefix the name of each
 * group with the name of the theme or module name implementing it, followed by
 * the name of the group. Note that you cannot define 2 groups with the same
 * the same name, even if they are in different plugins.
 *
 * Group properties:
 * - #title (required): Brief title of the tab.
 * - #description (optional): Description of the tab group.
 * - #weight (discouraged): Weight of the tab group; NULL by default.
 * - #admin_title (recommended): Administrative title of the group, displayed on
 *   the "Manage Skins" page.
 * - #admin_description (recommended): Administrative description of the group
 *   and a technical description of the skins it contains for the "Manage skins"
 *   page.
 * - #screenshot (optional): The path to an image containing a screenshot of the
 *   skin, which will be used on the "Manage Skins" page. Defaults to
 *   $path_to_plugin/screenshot.png, so only needs to be specified if different.
 *
 * The "hook" prefix is substituted with the name of the module or theme that
 * implements it, followed by the plugin name, e.g.
 * THEMENAME_PLUGINNAME_skinr_groups(), or MODULENAME_PLUGINNAME_skinr_groups().
 *
 * @todo Create Skinr's default group implementation. Skinr will provide a
 * plugin containing groups to provide some consistency in the UI, promote the
 * re-use of groups as categories, and ease the burden of creating skins.
 * @todo Document that collapsible/collapsed options are obsolete for groups in
 * the new version.
 */
function skinr_styles_skinr_groups() {
  $groups['skinr_typography'] = array(
    '#title' => t('Typography'),
    '#description' => t('Choose one or more styles to apply to content of this element.'),
    '#admin_title' => t('Skinr content styles'),
    '#admin_description' => t('Contains 3 @font-face fonts (Droid Sans, Fontin Sans and Museo) that style headings, and font sizing options designed for a base font-size of 13px.'),
  );
  return $groups;
}

/**
 * Implements hook_skinr_skins().
 *
 * Required. Define the skin(s) for this Skinr plugin. Each skin definition
 * consists of properties that define its form element and settings that are
 * needed to make it work, such as the class(es) Skinr should apply, which files
 * it should load, whether or not it should be disabled by default and which
 * theme hook(s) it was designed to work with.
 *
 * Skin name(s):
 * Each skin name must be unique. It is recommended to prefix the name of each
 * group with the name of the theme or module name implementing it, followed by
 * the name of the group. Note that you cannot define 2 groups with the same
 * the same name, even if they are in different plugins.
 *
 * Skin settings:
 * - #title (required): Label of the skin form element.
 * - #description (optional): End-user facing description of the skin.
 * - #group (optional): The group the skin is attached to; defaults to a Skinr
 *   provided group.
 * - #type (optional): The type of form element. Allowed values:
 *   - checkboxes (default): Use when single or multiple options can be chosen.
 *     Does not need to be set manually, as Skinr will apply this by default.
 *   - select: Use when a single option can be chosen, but many exist.
 *   - radios: Use when only single option can be chosen.
 * - #weight (discouraged): Sets the weight of the skin inside the group; NULL
 *   by default.
 * - #stylesheets (optional): A array containing information about stylesheet(s) the skin
 *   requires. This maps to the functionality of drupal_add_css() with one
 *   exception: paths are automatically assumed as relative to the plugin
 *   directory, unless external. Examples:
 *   - Simple:
 *       '#stylesheets' => array('css/skin-name.css')
 *     - Advanced: 
 *       '#stylesheets' => array(
 *         array('css/skin-name-ie.css', array(
 *           'media' => 'screen',
 *           'browsers' => array(
 *             'IE' => 'lte IE 8'
 *            ),
 *          ),
 *       )
 * - #scripts (optional): A array containing information about script(s) the
 *   skin requires. This maps to the functionality of drupal_add_js() with one
 *   exception: paths are automatically assumed as relative to the plugin
 *   directory, unless external. Examples:
 *   - Simple:
 *     '#scripts' => array('js/skin-name.js'),
 *   - Advanced: 
 *     #scripts' => array(
 *     array(
 *       'js/skin-name-advanced.js',
 *       array(
 *         'scope' => 'footer',
 *         'group' => JS_THEME,
 *       ),
 *     ),
 * - #options (required): An array containing one or more options (also arrays)
 *   for the user to choose from when applying skins. Each option key should be
 *   a machine name describing the option. An option should including the
 *   following keys:
 *   - #label (required): The option label.
 *   - #class (required): An array containing one or more classes the skin
 *     should apply. A single class may be entered as a string, but multiple
 *     classes should be entered as an array: For example:
 *     - Single:
 *       '#class' => 'class'
 *     - Multiple:
 *       '#class' => array('class-b', 'class-b') 
 *   - #stylesheets (optional): Same syntax as above, but applies to a specific
 *     option only.
 *   - #scripts (optional): Same syntax as above, but applies to a specific
 *     option only.
 * - #features (optional): An array containing certain allowed hooks, which
 *   allow you to limit where the skin can be used. Allowed options include:
 *   block, block__MODULE, comment, comment__NODETYPE, comment_wrapper,
 *   comment__wrapper_NODETYPE, node, node__NODETYPE, region,
 *   region__REGIONNAME, panels_display, panels_region, panels_pane, views_view,
 *   views_view__STYLENAME, views_view__DISPLAY_NAME, views_view__VIEWNAME, and
 *   views_view__VIEWNAME_DISPLAYNAME.
 * - #default_status (optional): Skins are disabled by default to keep UI
 *   clutter to a minimum. In some cases, like contrib themes, it makes sense to
 *   enable skins which are required to make the theme work properly by default.
 *   Setting this property to 1, will cause it to be enabled it by default for
 *   all installed themes.
 *
 * Programmatically added data:
 * - #status: An array containing each theme as the key and the status of the
 *   skin in it (0/1). These values override #default_status on a per theme
 *   basis. The "Manage skins" page only lists skins which are contained in an
 *   enabled theme or module. This setting is useful for sub-themes to override
 *   via an hook_skinr_skins_alter() to auto-enable skins located in a parent
 *   theme which may not be enabled.
 *   For example:
 *     '#status' => array(
 *       'theme_name' => 1,
 *      ),
 * - #source: An array containing the following information about the module or
 *   theme implementing the skin:
 *   - #type: Either module or theme.
 *   - #name: The name of the module or theme.
 *   - #plugin: The name of the plugin.
 *   - #path: The path to the skin's plugin directory.
 *
 * The "hook" prefix is substituted with the name of the module or theme
 * implementing it, followed by the plugin name, e.g.
 * THEMENAME_PLUGINNAME_skinr_skins(), or MODULENAME_PLUGINNAME_skinr_skins().
 *
 * @todo Document that support for #templates have been removed. The code does
 * not work properly and will eventually need an overhaul, so removing fluff.
 * @todo Document that collapsible/collapsed options are obsolete for groups in
 * the new version.
 * @todo A better name needs to be agreed upon for #features. This name is
 * confusing people to death, and doesn't accurately describe what it does.
 * @todo A helper function needs to be created for alter hooks to use to control
 * the status of a dependency skin.
 * 
 */
function skinr_styles_skinr_skins() {
  $skins['skinr_font'] = array(
    '#title' => t('Font family'),
    '#type' => 'select',
    '#group' => 'skinr_styles_typography',
    '#stylesheets' => array('css/typography.css'),
    '#options' => array(
      'font_1' => array(
        '#label' => t('Arial, Helvetica, Nimbus Sans L, Liberation Sans, FreeSans'),
        '#class' => 'font-1',
      ),
      'font_2' => array(
        '#label' => t('Lucida Grande, Lucida Sans Unicode, DejaVu Sans, Tahoma'),
        '#class' => 'font-2',
      ),
      'font_3' => array(
        '#label' => t('Geneva, Bitstream Vera Serif, Tahoma'),
        '#class' => 'font-3',
      ),
      'font_4' => array(
        '#label' => t('Georgia, Bitstream Vera Serif'),
        '#class' => 'font-4',
      ),
      'font_5' => array(
        '#label' => t('Tahoma, Geneva, DejaVu Sans Condensed'),
        '#class' => 'font-5',
      ),
      'font_6' => array(
        '#label' => t('Bitstream Vera Sans, Verdana, Arial'),
        '#class' => 'font-6',
      ),
      'font_7' => array(
        '#label' => t('Palatino Linotype, URW Palladio L, Book Antiqua, Palatino'),
        '#class' => 'font-7',
      ),
      'font_8' => array(
        '#label' => t('Free Serif, Times New Roman, Times'),
        '#class' => 'font-8',
      ),
    ),
  );
  $skins['skinr_font_size'] = array(
    '#title' => t('Font size'),
    '#type' => 'select',
    '#group' => 'skinr_styles_typography',
    '#stylesheets' => array('css/typography.css'),
    '#options' => array(
      'size_1' => array(
        '#label' => '0.846em (11px)',
        '#class' => 'size-1',
      ),
      'size_2' => array(
        '#label' => '0.923em (12px)',
        '#class' => 'size-2',
      ),
      'size_3' => array(
        '#label' => '1.077em (14px)',
        '#class' => 'size-3',
      ),
      'size_4' => array(
        '#label' => '1.231em (16px)',
        '#class' => 'size-4',
      ),
      'size_5' => array(
        '#label' => '1.385em (18px)',
        '#class' => 'size-5',
      ),
      'size_6' => array(
        '#label' => '1.538em (20px)',
        '#class' => 'size-6',
      ),
    ),
  );
  $skins['skinr_font_style'] = array(
    '#title' => t('Font style'),
    '#type' => 'radios',
    '#group' => 'skinr_styles_typography',
    '#stylesheets' => array('css/typography.css'),
    '#options' => array(
      'normal' => array(
        '#label' => t('Normal'),
        '#class' => 'fs-normal',
      ),
      'italic' => array(
        '#label' => t('Italic'),
        '#class' => 'fs-italic',
      ),
    ),
  );
  $skins['skinr_font_other'] = array(
    '#title' => t('Font weight'),
    '#group' => 'skinr_styles_typography',
    '#stylesheets' => array('css/typography.css'),
    '#options' => array(
      'bold' => array(
        '#label' => t('Bold'),
        '#class' => 'fs-bold',
      ),
      'small_caps' => array(
        '#label' => t('Normal'),
        '#class' => 'fv-smallcaps',
      ),
    ),
  );

  $skins['skinr_font_face'] = array(
    '#title' => t('Heading font family'),
    '#description' => t('Applies to headings level 1-3.'),
    '#group' => 'skinr_styles_typography',
    '#options' => array(
      'droid_sans' => array(
        '#label' => t('Droid Sans'),
        '#class' => array('ff', 'droid-sans'),
        '#stylesheets' => array('fonts/droid-sans/css/normal.css'),
      ),
      'droid_sans_bold' => array(
        '#label' => t('Droid Sans (bold)'),
        '#class' => array('ff', 'droid-sans-b'),
        '#stylesheets' => array('fonts/droid-sans/css/bold.css'),
      ),
      'fontin_sans' => array(
        '#label' => t('Fontin Sans'),
        '#class' => array('ff', 'fontin-sans'),
        '#stylesheets' => array('fonts/fontin-sans/css/regular.css'),
      ),
      'fontin_sans_bold' => array(
        '#label' => t('Fontin Sans (bold)'),
        '#class' => array('ff', 'fontin-sans-b'),
        '#stylesheets' => array('fonts/fontin-sans/css/bold.css'),
      ),
      'museo_300' => array(
        '#label' => t('Museo (300)'),
        '#class' => array('ff', 'museo-300'),
        '#stylesheets' => array('fonts/museo/css/300.css'),
      ),
      'museo_500' => array(
        '#label' => t('Museo (500)'),
        '#class' => array('ff', 'museo-500'),
        '#stylesheets' => array('fonts/museo/css/500.css'),
      ),
      'museo_700' => array(
        '#label' => t('Museo (700)'),
        '#class' => array('ff', 'museo-700'),
        '#stylesheets' => array('fonts/museo/css/700.css'),
      ),
    ),
  );

  return $skins;
}

FYI, just updated the codeblock above to tweak the docs and fix some errors. I'll probably continue tweaking the docs as we work on this, and I'll try to keep comment #2 updated, but the updates will go to GitHub first: http://github.com/jacine/skins

• Q: When do #properties make sense?
• A: #properties are a custom construct of Form API and the rendering system in D7. Both subsystems have one thing in common: They are recursively processing the passed in array. All element #properties are prefixed with a hash sign, in order to distinguish #properties of an element from child elements. Do not use #properties if the array data is not recursive.

  For example, this array needs to be processed recursively:

  $foo = array(
    '#title' => 'Foo',
    '#description' => 'Foodoo.',
    'bar' => array(
      '#title' => 'Bar',
      '#description' => 'Where you get beer.',
    ),
  );
  

  The keys starting with a hash sign belong to the respective element in question. Any other keys (not starting with a #-sign) do not belong to the element; i.e., they are sub-elements of the element.

Thanks @sun!

Although I don't understand the "recursive" stuff, this is what I thought needed to happen. I'll update it. :)

I'm also curious about using #attached for the CSS and JS files. From my understanding this is not limited to forms, and I would like to prevent using custom properties wherever possible and be as consistent as possible with the way core does things like this. The documentation is not fully updated yet, and I'm not sure how this would work or if it's the right thing to do here though. Can you comment on that?

Ok, I've updated this to remove the hashes from non-elements. The only one I am unsure about is #options. It's not the standard option used by FAPI, it's a custom version where we use label and class. I left the # off for now.

http://github.com/jacine/skins/blob/master/styles/styles.skinr.inc :)

[also] edited by sun

Goal

• A certain skin applies a certain CSS class to a page element, and loads corresponding CSS/JS required for it.
• Multiple skins can be applied to a certain element. (?) [Also multiple styles of a skin?]
• A skin can inherit another skin:
  • (and what this effectively means)
• A skin should belong to a group to help users to find/apply the skin they want. Skinr itself defines the default groups:
  

  Typography

  ...

• A skin is configurable, exposing a form to set its options [, normally self-contained, i.e., not relying on external factors?].
• To be continued...

#attached is a different beast, which we should certainly analyze in detail:

Pro

• Allows us to re-use drupal_process_attached() without a single line of own/custom code.
• Everyone and the world already knows the syntax and structure of #attached.
• Even allows to do funky shit, since #attached is really flexible.
• #attached works on any kind of array, which can also be a skin definition.

Contra

• #attached does not work on its own; the definition either needs to be merged into an existing one of an element, or just manually invoked. [Not really an issue, because this needs to happen either way. More interestingly, this imposes the question whether Skinr could work before theme() is invoked... separate issue.]

--
Overall issues I see with the proposed syntax/structure:

1. API version compatibility should be defined per skin.
2. hook_skinr_groups() should not have any single #-sign in it.
3. hook_skinr_groups() duplicates #title and #description in an odd way. It's not clear to me why we need that duplication.
4. All properties (not belonging to renderable arrays) should use spaces, no underscores.
5. Skin definitions need to be separated from form structures. All that custom stuff in #options breaks Form API. Most likely, to keep this as simple as possible, we should use a custom #property to map a skin's form #options to actual behavior, along the lines of this:
   

     $skins['skinr_font_other'] = array(
       '#title' => t('Font weight'),
       '#group' => 'skinr_styles_typography',
       '#skins' => array(
         'bold' => array(
           'attributes' => array(
             'class' => array('fs_bold'),
           ),
           '#attached' => array(
             'css' => array(
               'bold.css',
             ),
           ),
         ),
         'small_caps' => array(
           'attributes' => array(
             'class' => array('fv-smallcaps'),
           ),
         ),
       ),
       '#options' => array(
         'bold' => t('Bold'),
         'small_caps' => t('Normal'),
       ),
     );
   

   With #915936: Make it easier to define checkboxes/radios with customized sub-elements in place, we could attempt to move the #skins (or similar) property to the individual #options, but that's not yet possible.

/**
 * Implements hook_skinr_info().
 */
function skinr_skinr_info() {
  $skins['font_other'] = array(
    '#attached' => array(
      'css' => array('css/typography.css'),
    ),
    'variants' => array(
      'bold' => array(
        'class' => 'fs-bold',
      ),
      'small_caps' => array(
        'class' => 'fv-smallcaps',
      ),
    ),
  );
  return $skins;
}

/**
 * Implements hook_skinr_SKIN_form().
 */
function skinr_skinr_font_other_form($form, &$form_state) {
  // #group is automatically handled by Skinr UI.
  $form['font_other'] = array(
    '#title' => t('Font weight'),
    '#options' => array(
      'bold' => t('Bold'),
      'small_caps' => t('Normal'),
    ),
  );
  return $form;
}

API version compatibility should be defined per skin.

I was hoping to avoid this because it's tedious to enter, but I can live with it if it needs to happen.

hook_skinr_groups() duplicates #title and #description in an odd way. It's not clear to me why we need that duplication.

There needs to be an administrative title and description of a plugin. The title and description we display to and end-user is most always not the same as the title and description administrators need to see. Not having the ability to do this would be like not being able to have a description for a module name on the module listing page. I can't think of any other way to accomplish this, and I'm sure there is a better way, but it's definitely not duplication, and it is necessary at plugin-level. It is awkward at group level, I will admit that, but I don't know how to fix this. I want this information in hook_skinr_info(). I'd also like to enable/disable at plugin level, and then configure at skin level, but was told it wouldn't work there.

Skin definitions need to be separated from form structures. All that custom stuff in #options breaks Form API. Most likely, to keep this as simple as possible, we should use a custom #property to map a skin's form #options to actual behavior, along the lines of this:

The only issue I have with this and the above examples is that it makes it harder for themers to grok and be able to pick up. I don't like that it's completely abstracted and I would prefer not dealing with forms at all if this is the problem, and creating our own element.

Summary of the IM discussion yesterday:

• Separation of API/UI in #9 is basically appealing.
• hook_form() should be optional, to allow for progressive enhancement for advanced skins.
• hook_info() should be sufficient for simple skins.
• hook_info() should support simple skin options (like previous .info files).
• Challenge is to figure out a simple structure for hook_info() that mainly targets the API functionality of a skin, but also allows to define simple UI properties to build a simple options form out of it:
  • Doesn't have to (and probably shouldn't) be using a Form API structure; i.e., along the lines of hook_info() #9.
  • Must allow to define properties for the UI within or next to the API properties.
  • We need to be able to remove all UI properties in a quick way, so as to optimize our skin cache for performance later on. (The cached run-time data must only contain the information that is required to apply/load skins and nothing else.)

--
New proposal:

Note: Using a different array definition syntax this time to see whether it helps to understand the overall nested array structure. Keys and locations are the same as in #9, just written out differently.

/**
 * Implements hook_skinr_info().
 */
function skinr_skinr_info() {
  $skins['font_other'] = array();
  $skins['font_other']['#attached'] = array(
    'css' => array('css/typography.css'),
  );
  $skins['font_other']['variants'] = array(
    'bold' => array(
      'class' => 'fs-bold',
    ),
    'small_caps' => array(
      'class' => 'fv-smallcaps',
    ),
  );
  // Either define a simple form (without validation/submit handlers) inline...
  $skins['font_other']['form'] = array(
    '#type' => 'select',
    '#title' => t('Font weight'),
    '#options' => array(
      'bold' => t('Bold'),
      'small_caps' => t('Normal'),
    ),
  );

  // ...or specify a custom form callback:
  // Defaults to hook_skinr_SKIN_form().
  $skins['font_other']['form callback'] = 'my_skinr_super_skin_form':

  return $skins;
}

/**
 * Implements hook_skinr_SKIN_form().
 *
 * Just for comparison. The simple skin form above contains exactly the same. :)
 */
function skinr_skinr_font_other_form($form, &$form_state) {
  // #group is automatically handled by Skinr UI.
  $form['font_other'] = array(
    '#type' => 'select',
    '#title' => t('Font weight'),
    '#options' => array(
      'bold' => t('Bold'),
      'small_caps' => t('Normal'),
    ),
  );
  return $form;
}

This structure allows us to simply unset($skins[$name]['form']); in all skin definitions prior to caching them.

I'd suggest to discuss and agree on this detail first. Afterwards, let's discuss hook_info() properties, especially #attached.

At the risk of asking a silly question where would we define "features" as in block, panels pane, node etc. This is looking good.

So Jacine just tells me that the separation into 'form' is too complex, and I think she's right.

Thus, new proposal:

/**
 * Implements hook_skinr_info().
 */
function skinr_skinr_info() {
  $skins['font_other'] = array();
  $skins['font_other']['#attached'] = array(
    'css' => array('css/typography.css'),
  );
  // Either define simple skin options (without validation/submit handlers) inline...
  $skins['font_other']['form type'] = 'select';
  $skins['font_other']['form label'] = t('Font weight');
  $skins['font_other']['variants'] = array(
    'bold' => array(
      'label' => t('Bold'),
      'class' => 'fs-bold',
    ),
    'small_caps' => array(
      'label' => t('Small caps'),
      'class' => 'fv-smallcaps',
    ),
  );

  // ...or use a fully-fledged form callback:
  // Optional; defaults to hook_skinr_SKIN_form().
  $skins['font_other']['form callback'] = 'my_skinr_super_skin_form';
  $skins['font_other']['variants'] = array(
    'bold' => array(
      'class' => 'fs-bold',
    ),
    'small_caps' => array(
      'class' => 'fv-smallcaps',
    ),
  );

  return $skins;
}

/**
 * Implements hook_skinr_SKIN_form().
 *
 * Just for comparison. The simple skin form above contains exactly the same. :)
 */
function skinr_skinr_font_other_form($form, &$form_state) {
  // #group is automatically handled by Skinr UI.
  $form['font_other'] = array(
    '#type' => 'select',
    '#title' => t('Font weight'),
    '#options' => array(
      'bold' => t('Bold'),
      'small_caps' => t('Normal'),
    ),
  );
  return $form;
}

Thus, when collecting skin information for processing (only), then we'd invoke hook_info(), and unset() 'type', 'title', and also every 'title' in each variant.

1 - how about #include or #files instead of #attach?

  // The options below are programatically added.
  $skins['font_other']['default_status'] => 0;
  // Overrides default_status on a per-theme basis.
  $skins['font_other']['status'] => array(
    'bartik' => 1,
    'fusion' => 1,
  );

I'm confused by this, does it mean that if I were making a theme I would have to use $skins['font_other']['status'] and list my theme name with the status number? or could i use $skins['font_other']['default_status'] ?

TBH, I was similarly confused, and those 'default_status' and 'status'-per-theme properties make little sense to me. Would you ever want to have any skin *automatically* appear enabled on your site, just because it exists?

The situation and answer to that question is different when we're talking about exporting skin configurations into code - but that's an entirely different story and doesn't have anything in common with registering and describing skins (the task at hand).

One further property that has been added is 'source' -- all of what is visible in there does not have to be specified manually, since skins will be tied to a module or theme anyway now.

On the remaining, 'group' is cool and a no-brainer, and/but we likely want to rename 'features' into 'hooks' (or 'theme hooks').

TBH, I was similarly confused, and those 'default_status' and 'status'-per-theme properties make little sense to me. Would you ever want to have any skin *automatically* appear enabled on your site, just because it exists

Well this is not to have the skin actually applied to anything, its just making it available in the UI, so that it can be selected. Personally, I think ALL skins should be available by default, people are not gonna realize they have to enable them and I can see issues flooding in about it.. but thats just my opinion.

Anyway some clarification on the difference between 'default_status' and 'status' would be awesome cause i'm still a bit confused. :)

Updates

Here are some updates based on conversations earlier today:

1. hook_skinr_api() will be used to define API information. This already exists and looks like this:

   function skinr_styles_skinr_api() {
     return array(
       'api' => 2.0,
       'path' => drupal_get_path('module', 'skinr'),
     );
   }
   

2. admin_title, admin_description and screenshot settings have been removed for groups. These are purely Admin UI related, and there not an "essential" skin creation syntax issue. If there is a problem, we'll deal with it separately.

3. hook_skinr_skins() has been changed to hook_skinr_skins_info().

4. hook_skinr_groups() has been changed to hook_skinr_groups_info().

5. features has been changed to theme hooks.

6. Suggestions have been made to name settings keys in a way that clearly separates what is a form property vs. a skinr property. After some discussion and realizing that it doesn't matter what we name these, I am opting for the simple version that will be easy for skin creators to grok. The readability of processing code should have no bearing over this because the nature of these properties. We need to keep this simple.

7. attached is now being used for stylesheets and script additions. The actual functionality still needs to be discussed, but this format is a Drupal 7 standard that will be used elsewhere, so we should use it for consistency.

8. I've removed the label for the "variant/option" titles. These are titles at the end of the day, and they do not always end up as label's, so we should just standardize.

9. I'm not thrilled with "variants" and I think it represents a regression. A skin has options, period. The word variant just complicates things unnecessarily. Maybe it's just me, but in this context the word variant implies a variation of some default and there is no default. Sure, a skin can contain similar classes/files and options can ultimately end up being variants of each other, but that is not always the case. Options can be and often are completely independent of each other. Can't we just K.I.S.S.?

Summary

The example has been updated on Github: github.com/jacine/skins

hook_skinr_skins_info()

hook_skinr_groups_info()

An example of how this translates to a skin, with a lot more docs is also attached. Please review.

1. Not sure whether the example skinr_styles_skinr_api() was meant to be final. It should be hook_skinr_api() only. The compatible API version is unique for all Skinr-related code of a certain module/theme, so I do not see a need for adding a separate 'styles' key. Initially, this hook will merely return the compatible API version. Only in a later step, we will probably also return a path to an include directory, in which skins are defined in atomic include files. Therefore:
   

   function hook_skinr_api() {
     return array(
       'api' => '2.0',
       // Later...
       'path' => drupal_get_path('module', 'example') . '/skins',
     );
   }
   

2. hook_skinr_skin_info() and hook_skinr_group_info() should both be singular; i.e., "skin" instead of "skins". That is, because although the hook technically allows to register more than one skin, the fundamental "thing" that is being registered is a "skin", not a "skins". (And FWIW, I originally hoped that we could get away with just hook_skinr_info() for the primary skin info hook... however, if you think that hook_skinr_skin_info() is better, that's ok, too.)
3. OK with going with 'options'.
4. 'weight' should be removed. The API docs state "discouraged", and I cannot see a valid and/or useful reason for specifying a weight.
5. 'class' should always be an array. No exceptions. This is a new standard in D7 -- it's never ever a string. We should follow that lead.
6. The current phpDoc on hook_skinr_skin_info() intermixes skin definition with properties of stored, configured/applied skins. It's important to make that difference -- our skin "entities" stored in the database will have entirely different properties (e.g, ID, status, theme, etc.) and users will never ever see the info hook properties intermixed in such a stored skin.

   Code-wise, we will have to make sure that whenever we are dealing with a skin info definition, the variable is named $skin_info accordingly. Whenever the code deals with a stored skin, the variable will be $skin.

   Likewise, function names should follow accordingly:

   These invoke hooks + code:
   function skinr_get_skin_info()
   function skinr_skin_info_process()
   function skinr_skin_info_*()
   function skinr_get_group_info()
   function skinr_group_info_process()
   
   These do not deal with any info hooks + corresponding code:
   function skinr_skin_load()
   function skinr_skin_load_multiple()
   function skinr_skin_save()
   function skinr_skin_delete()
   function skinr_skin_delete_multiple()
   

7. Further looking at the example skin + docs, I wonder whether certain skins (e.g., that very font family skin)...
   
   1. shouldn't be able to be applied to "any" theme hook? Most likely, we could simply allow that by making 'theme hooks' optional; i.e., if not defined, then all theme hooks are potential targets. Current docs already state that 'theme hooks' is optional, but do not state what the default behavior is when omitting the property.
   2. shouldn't be able to be applied to a certain "category" of theme hooks? This idea can and certainly should be left for a later point in time, but what I'm thinking of is that a "block" and a "panels_pane" and whatnot, those all fall into a certain category of "thing" that is being output. Just mentioning, off-topic for this issue. Please do not follow up on this point, thanks.

1. One module or theme can only be either compatible or not compatible with Skinr's current API. There's no need to differentiate.

   Additionally, I think that most of the current "handler" functionality in the /modules directory is moot for D7. I'd recommend to leave out the 'path' key for hook_skinr_api() for now. FWIW, we're leaving the entire "one skin in an atomic include file" feature to a separate issue anyway right now. And before approaching that issue, we will have to tackle that module handler functionality first. Depending on the results, we may need separate 'skin path' and 'handler path' properties, or not. As of now, I bet we will be fine with a single 'path' property.

4. We can leave 'weight' in, but it makes little sense to me. Pre-emptively specifying a weight for an item within a list of unknown other items doesn't work. Contrary to that, what would work is to add a tableDrag to the administrative list of available skins to allow the admin to order all skins to their liking. However, that still means that it's pointless to specify a weight in the registered info per skin. Rather, Skinr UI would store the tableDrag weights in a system variable or similar.
6. I can perfectly understand that the current code leads to confusion, because it is repeatingly mixing skin information with a stored skin all over the place. However, the point is that a stored skin is the actual entity that we are working with. Without a stored (and therefore configured) skin entity, no skin is applied. The fact that a stored skin entity was registered and defined through an info hook and some corresponding form in the first place is completely peripheral. I'm going into extremes here, but strictly technically speaking, hook_skinr_skin_info() should actually be hook_skinr_ui_skin_info(), because it is the Skinr UI module that requires the registered skin information to collect available skins and configure them -- the Skinr API module has nothing to do with that. The Skinr API module merely stores and loads skin entities in order to apply them at run-time.

   Thus, the differentiation is in "available skin [information]" and "actual skin". The actual skin is the entity of the Skinr module. Available skins are just information.

7. It's OK to simply state that 'theme hooks' is optional. The internally stored value is of no interest for someone registering a skin. Internally, we should neither store '*' nor 'any', just nothing. The mere existence of a stored skin entity means that a skin needs to be applied.

p.s.
I was thinking that the work Jacine is doing with her example skin might be used as a base for #926740: Include a skinr_example.module with the Skinr module that includes an example skin. All it would need in addition is a wrapper module that probably just has the hook_skinr_api() in it.

Ok, sounds good. I'll update it.

It should be hook_skinr_api() only.

Amen! This makes me very happy.

'weight' should be removed. The API docs state "discouraged", and I cannot see a valid and/or useful reason for specifying a weight.

It's discouraged to do regularly. What it's good for is setting an extreme high or low weight as Bala mentioned. I'm all for the TableDrag implementation, but unless that is going to happen as part of this process, I don't want to see it removed.

And I agree with everything else, which is very exciting :)

Ok, here's the final for reference. Marking this fixed so we can move on :)

Fixed.

These API docs should naturally go into a skinr.api.php file.

@jacine: Can you commit an empty file for that? You may use title.api.php as template: http://drupalcode.org/viewvc/drupal/contributions/sandbox/sun/baseline/

Just ran into one small issue... skinr_skinr_api() should be skinr_styles_skinr_api() if we want to be consistent.

The hook_skinr_api() should be in the module or theme's template.php as far as I understand.

I can't fully comment on the direction because I am confused. I'd like it to remain on track as it's outlined here, but I am unclear on whether or not that's possible at this point.

@sun, there is a docs/docs.php file for that. I don't have time to work on it today, but I assume that is the file that this belongs in. When it's all done it can change to skinr.api.php.

Just ran into one small issue... skinr_skinr_api() should be skinr_styles_skinr_api() if we want to be consistent.

If I'm caught up and understand this discussion then I don't see why the plugin part of Skinr hook definitions are at all necessary.

For the latest format in #30 there is HOOK_skinr_api() and then HOOK_PLUGIN_skinr_skin_info(), but if plugins are defined for modules and themes only why is there a further key used?

From what I understand @moonray's issue in #31 was a false alarm and that hook_skinr_api() is defined once by the module (in the .module file) or the theme (in the template.php) implementing skins.

Other than that, all that's left here is to merge this documentation in with docs.php and move it to skinr.api.php, so changing the priority.

Apologies if it's been covered and is clear to everyone else, but I don't understand the 'plugin' part of the other hooks. Why is the module or theme name for HOOK_(skinr_hook) not enough? What does a variable for plugin do?

@coltrane, it basically allows for multiple sets of skin implementations to be used in the same module or theme. For example, let's say you have a theme, and you have 2 different collections of Skins:

• Grids: Comes w/960 Grid system which has like 6 skins.
• Styles: General styles for your theme (presentational styles, typography, etc) that has maybe 10 skins.

The "plugin" allows you do define "grids" and "styles" as separate entities. Each would likely have its own directory containing any CSS/JS files and an .inc file that contains their implementation. So... you'd have:

• zen_grids_skinr_skin_info()
• zen_styles_skinr_skin_info()

If we don't specify the "plugin" part, we don't get to do this, and you'd basically have to lump all the skins in one to be able to use them. This would make sharing and managing skins way harder.

Hopefully that makes sense. :)

Since the API docs are being added in this issue #956990: Clean up and optimize skinset handling code in skinr core, so this issue should be closed.

@coltrane Feel free to reopen, if you think it's necessary. :)

Reopening because I think the distinction between module plugins and skins should be stronger.

For example, as part of #956994: Write load and parse code for Skinr include files in PHP format the includes file loading is handling both module plugins and skins. Two separate include files are loaded in the same way and using the same function. That was not clear to me until I spoke with Jacine. In comments, skinr.api.php, and in the code we could use different terminology to make the distinction more clear.

So, I propose that for things providing integration with Skinr to allow for module-specific Skinr functionality we continue to refer to that as a "module plugin", or "plugins".

And for skins, could we drop any mention of them as plugins and just refer to them as what they are, "skins"? This would mean that instead of hook_plugin_skinr_skin_info() we'd have HOOK_SKINNAME_skinr_skin_info(). Jacine, to use your examples in #38 grids and styles would be the name for the separate entities providing the skins.

We originally called them "skinsets" because they are basically a set of individual "skins." That was no good, so plugins was the best we could come up with. As a themer, I feel plugins is a more accurate description (and this is how CTools does panels pane and layout plugins), but obviously that is confusing the crap out of developers, so I'm of course fine with changing it.

However, the reason we needed a different name in the first place is that each hook contains skins, so calling both the "group" and its contents "skins" is confusing from a code standpoint. Maybe this is not confusing, or less confusing? I don't know.

I'm happy with anything that allows us to move on at this point. I'm admittedly frustrated with the pace, and all the confusion surrounding this, because it is not much different than what's sitting in 6.x-2.x-dev right now. We are nowhere near having a functional 7.x release, despite spending a lot of time trying to improve the module and keep this process open. It's ultimately holding up contrib themes, preventing inclusion in books, and will begin to cause LOTS of problems for us as we drastically change things while people begin to use it with Drupal 7. We just need to iron this out ASAP so we can begin to address the gazillion other issues we need to get to.

So, the question I have is:

Is calling the referring to the hook group name and the skins themselves "skins" going to be too confusing from a code standpoint?

I'm now OK with "plugins" so long as the code, comments, and documentation were to make the distinction clear.

In http://drupal.org/node/956994#comment-3896396 I propose not using plugin entity grouping in the _skinr_skin_info() definitions. Instead using separate plugin files to differentiate. Of course, that method depends on the plugin.skinr.inc method being chosen. I do think HOOK_PLUGIN_skinr_skin_info() is unique and confusing would like to see it change no matter what the skin include file becomes.

Seems like skinr.api.php should be about skin plugins. Should it also provide info about the module plugin hooks like hook_skinr_config_info()? I think so.

FYI, There's a patch for API docs that include the skin hooks already that needs review: #956990: Clean up and optimize skinset handling code in skinr core. If there's anything else missing, we should definitely include it, but it would be nice to get this committed soon so that people creating skins have a good reference.

I think we can close this. The API docs have been committed and all discussion on this is happening in #956994: Write load and parse code for Skinr include files in PHP format.