Clean up and optimize skinset handling code in skinr core

This patch should entirely ignore #956932: Determine optimal skin include file format and all other issues for now.

The goal is very focused: Turn current 'skinsets' into 'skins'.

Keeping their current syntax and structure, loading, and all other behavior. Potentially cleaning up related functionality, but not necessarily.

For review should this just be tested that skinr works normally? or is there any particular test/review of functionality that should be done?

Isn't part of the point of this to remove all references to skinsets? I still see lots of references to skinsets all over the place.

Ok, first off it's really great that Skinr is working again. Yay :)

At almost 100k, this patch is really hard to review, because there is no general summary of what was done. Please summarize the main points, or changes from the previous patch next time, so we can get through these reviews easier.

Here's my attempt at a summary (mostly for my own sanity – I want to make sure I understand), and my review of the patch.

Summary

• Changed:

  #	Before	After
  1	_skinr_fetch_config_defaults()	skinr_config_info_default()
  2	hook_skinr_config()	hook_skinr_config_info()
  3	skinr_fetch_config()	skinr_fetch_config_info()
  4	skinr_get()	skinr_skin_load()
  5	skinr_group_default()	skinr_group_info_default()
  6	skinr_skin_default()	skinr_skin_info_default()
  7	skinr_set()	skinr_skin_save()
  8	skinr_skin_data()	skinr_get_group_info()
  9	skinr_skinset_statuses()	skinr_get_skin_info()
  10	skinr_skinsets()	skinr_skin_info_process()
  11	_skinr_ui_admin_skinsets_build_row()	_skinr_ui_admin_skin_infos_build_row() *
  12	skinr_ui_admin_skinsets()	skinr_ui_admin_skin_infos() *
  13	skinr_ui_admin_skinsets_settings()	skinr_ui_admin_skin_infos_settings() *
  14	skinr_ui_admin_skinsets_submit()	skinr_ui_admin_skin_infos_submit() *
  15	skinr_ui_admin_skinsets_settings_submit()	skinr_ui_admin_skin_infos_settings_submit() *
  16	skinr_ui_sort_by_info_name()	skinr_ui_sort_by_title() *
  17	theme_skinr_ui_admin_skinsets_fieldset()	theme_skinr_ui_admin_skin_infos_fieldset() *
  18	theme_skinr_ui_admin_skinsets_info()	theme_skinr_ui_admin_skin_infos_summary() *
  19	theme_skinr_ui_admin_skinsets_incompatible()	theme_skinr_ui_admin_skin_infos_incompatible() *
  20	theme_skinr_ui_admin_skinsets_settings_fieldset()	theme_skinr_ui_admin_skin_infos_settings_fieldset() *
  21	theme_skinr_ui_admin_skinsets_settings_info()	theme_skinr_ui_admin_skin_infos_summary() *
  22	theme_skinr_ui_admin_skinsets_settings_info()	theme_skinr_ui_admin_skin_infos_settings_summary() *

  * See "Infos" below.

• Added:

  1. skinr_plugin_hooks() – not functional.
  2. skinr_skin_delete().
  3. skinr_skin_delete_multiple().
  4. skinr_skin_info_status_default().
  5. skinr_skin_load_multiple() – not functional.

• Removed

  1. _skinr_add_file()
  2. _skinr_skinset()
  3. skinr_skins_default()
  4. skinr_ui_overlay_enabled()

Apologies if any of the above is wrong. That's what I was able to grok.

$skin->skins

I realize that this is a @todo, but reading through the code it's really, really confusing:

@todo Rename $skin->skins to $skin->options throughout the module.

Is there a reason why we aren't doing this now?

infos?

All references to "infos" in function names and variables is literally making me cry. It doesn't make any sense at all to me. "plugins" seems more appropriate in most of these places, and there are way too many theme functions here, no? I realize this is a work in progress and this will not be the final version by any means, but I think they all (#12-22 in changed summary) have to change. Also, the comment blocks for all these functions still refer to "skinsets."

Table on admin/appearance/skinr/skins

This table is way too verbose, and there are too many theme functions used to generate it. I realize we'll have to figure out how to better display this information, but in the meantime, the following tweaks should be made:

1. Run "theme hooks" through theme_item_list(). Give it #theme => item_list__skinr.
2. Run "operations" though theme_links(). Give it a #theme => links__skinr and give it an .links & .inline class. Also, make sure you check if $skin_info['links'][$key] is set before doing anything with it.
3. Nix the <h2> for the skin name.
4. Remove the description. It's totally wrong in this context.
5. Change "source name" to Source and just leave the module or theme name there. A module and theme cannot be named the same so this should be enough.
6. Obviously, remove the "source type" column, per 5.
7. Get rid of the "source" prefix for version.
8. Also, if you are going to add classes to the table cells, make sure to do it for all of them so it's consistent.

One last thing regarding that page... The default fieldset has no legend. It should be labeled "General."

Other comments

+++ skinr.install
@@ -286,9 +157,29 @@ function skinr_update_7001() {
+/**
+ * No longer used: Install {skinr_skinsets} tables.
+ */
+
+/**
+ * No longer used: Install {skinr_skins} table.
+ */
+
+/**
+ * Create new {skinr_rules}.rule_type field to allow region rules.
+ */
+function skinr_update_7002() {

I don't get this. I thought they might be placeholders for functions you need to write, but skinr_update_7200() drops both tables. Maybe you forgot to delete this?

+++ skinr.module
@@ -44,6 +44,8 @@ function skinr_init() {
+  * @todo Account for drupal's caching being enabled and make it work.

Drupal should be capitalized.

+++ skinr.module
@@ -264,52 +236,64 @@ function skinr_skin_get_files($skins, $type, $theme = NULL) {
+function in_multiarray($needle, $haystack) {
+ ...
 }

Do we really need this? I see you are using it twice in this patch. If so, shouldn't it be named more appropriately named as a helper function with a skinr prefix?

+++ skinr.module
@@ -713,311 +729,265 @@ function skinr_group_default() {
+      if (strpos($file[0], 'http://') === 0) {

What about https?

+++ skinr_ui.module
-function skinr_ui_overlay_enabled() {
- ...
-}

We also need to:

1. Delete the skinr_overlay_width and skinr_overlay_height variables in an update function.
2. Remove admin/appearance/skinr/settings from hook_menu().
3. Remove skinr_ui_settings().
4. Remove dialog_js_load() in skinr_ui.module.
5. And whatever else I'm missing

Reviewed this *giant* patch. Didn't have the guts to go into all details, but overall, this patch looks like a huge improvement, code-wise. I'd recommend to commit it under the following premise:

*Lots of* follow-up patches that are going to revamp the API "anyway".

- Removed most of the .install file changes. Let's care for the upgrade/update path later on.

- $skin['_options'] and similar properties having starting with an underscore will have to be completely revised in a next step.

- Most of the added @todos are in line with the idea of major API rewrites "anyway".

- Some CSS classes that actually belong to Skinr module have been changed from "skinr" prefix to "skin" prefix. That'll have to be fixed in a follow-up patch. ("skin" is outside of the module's namespace.)

I understand that Jacine is probably not able to make much sense of the code just yet. However, I'd still recommend to commit this patch as-is anyway, but as said, only under the premise that we're going to heavily refactor the code afterwards.

Reasoning:

This patch doesn't really change the existing code. It merely renames a couple of functions and variable names throughout the module to comply with the terminology and ideas we came up with in the other "determine..." issue. So, in turn, trying to understand this code is the same WTF of understanding the current code, with the sole exception that with this patch, function and variable names are a bit more human-readable.

Ok, patch in #17 is committed in the interest of moving things along, but I still stand by all my comments in #17. :)

http://drupal.org/cvs?commit=456360

Marking needs work, so we can get to the next step. Please change the title to whatever that is accordingly. :)

I'm fine with this patch except for the _skinr_in_multiarray() function. It's better that it has been renamed but I find it odd to include in the first place. Seems to me that it would be better to just do the looping in place, but I'll leave that to others to comment on further. Marking "needs work" based on that for now.

We need to defer removal of that _skinr_in_multiarray() function until further clean-ups. As mentioned before, we're far from done with this issue yet.

Committed: http://drupal.org/cvs?commit=461516 :)

Setting to needs work, as the focus of this issue will now be on the handlers.

The patch in #22 broke Skinr. @moonray has a feeling that the modules/*skinr.inc files are not loading and therefore it's completely not functional right now.

Proposed fix from @moonray attached.

Here's an updated patch.

Committed patch in #27: http://drupal.org/cvs?commit=462660

Needs work for API docs.

+++ skinr.module	10 Dec 2010 21:31:24 -0000
@@ -425,7 +425,12 @@ function skinr_rule_visible($rid) {
+    if (function_exists('drupal_get_path')) {

I do not understand this condition. skinr_module_load_all_includes() is invoked from skinr_init() only, and hook_init() itself is only invoked when Drupal reached full bootstrap, so all subsystems and modules have been loaded. Can we clarify what's happening here?

Powered by Dreditor.

Can anyone clarify #30 for @sun, please?

If skinr_module_load_all_includes() is only called from skinr_init() I also don't see why the function_exists() is necessary.

Committed! Thanks :) http://drupal.org/cvs?commit=466110

Setting to needs work for API docs.

Here's a patch for the docs.

Err, Here's one without trailing white-space problems.

Forgot alter hooks.

Thanks! This patch should address all those concerns.

Patch given on 12/20 broken after patch

While the code in CVS isn't "working" right now, this is the only issue that is actually holding up the basic functionality: #956994: Write load and parse code for Skinr include files in PHP format, and it has working patches. I've got the one in #29 running locally to use for my themes: http://drupal.org/files/issues/patch_commit_4790e14e3753.patch

@ http://drupal.org/node/959686

applied. Fixed by taking added lines from that patch and applying them after the above patch. Here's the patch.

Hopefully this addresses all of @moonray's points in #42.

+++ skinr.api.php	7 Jan 2011 04:33:02 -0000
@@ -93,18 +93,220 @@ function hook_skinr_config() {
+function hook_plugin_skinr_group_info() {

s/plugin/PLUGINNAME/

+++ skinr.api.php	7 Jan 2011 04:33:02 -0000
@@ -93,18 +93,220 @@ function hook_skinr_config() {
+ * - default_status (optional): Skins are disabled by default to keep UI

All of the skin info properties specify more or less "defaults", so 'status' should be sufficient.

+++ skinr.api.php	7 Jan 2011 04:33:02 -0000
@@ -93,18 +93,220 @@ function hook_skinr_config() {
+function hook_plugin_skinr_skin_info() {

Actually, we should (or need to) move the "dynamic" (arbitrary) PLUGINNAME to a later position.

That is, because several namespace conflicts exist with having the plugin name directly after hook. The "plugin" of one module may clash with the module name of another module.

Simple example for a nameclash: Someone thinks that Skinr's default settings or whatnot are bad and thus creates the skinr_default module. The hook namespace for this module therefore is skinr_default, but Skinr module already contains a skin "plugin" that is named default, so we potentially have the identical function defined twice.

To prevent this, the pattern should be:

hook_skinr_skin_PLUGIN_info()

Powered by Dreditor.

Here we go.

Ok, committed. Thanks :)

+++ skinr.api.php	9 Jan 2011 11:56:10 -0000
@@ -93,18 +93,281 @@ function hook_skinr_config() {
+ * - default_status (optional): Skins are disabled by default to keep UI

I forgot to rename default_status into status, but we can also do this in #956994: Write load and parse code for Skinr include files in PHP format or separate issue.

Powered by Dreditor.