Settings for the theme used by jQuery UI

Is it possible for you to upload this in patch form so it could be reviewed?

Ok, had a chance to sit down and review this tonight. Overall, this looks like really great functionality to have.

- There's a lot of code in the form_alter function which has nothing to do with adding jQuery UI theme settings. Stuff like making the existing fieldsets collapsible, etc. Though this is a nice usability improvement, it's out of scope for this module, as it just ends up being more code that needs to be maintained. :\

- It's against the CVS guidelines for us to commit 3rd party code, so we should have this work with the default themes jQuery UI provides, as well as those generated from http://ui.jquery.com/themeroller. So it should be looking @ JQUERY_UI_PATH . '/themes' rather than /css.

- I spot some typos as I'm going through, like * Module "pubblic" functions.

- I'm not sure the .inc file requirement really buys us anything. Your patch here still breaks with jQuery UI 1.5, because flora_all.css got renamed to flora.all.css.

- All of the hunks fail against HEAD. :( But I guess this is rolled against 5.x, so that's to be expected.

I'm going to try rolling a version of this for 6.x.

Ok, here's a more simplified patch that's a bit easier to wrap one's head around. I'm not saying that your approach earlier was wrong, just it's a bit over my head as to why all of that extra stuff is needed. The only thing we're doing is choosing a theme to use, no?

To solve the awkard weighting issues on this form (yuck!) I just went with a -10 weighted fieldset, collapsed by default. It's not the most gorgeous thing in the world, but it does reduce the amount of code required by a great deal which makes the code easier to parse.

This patch does hard-code the theme search logic to where it finds:

JQUERY_UI_PATH . "/themes/$theme_name/$theme_name.css";

Since this follows the standard recommended at http://docs.jquery.com/How_to_Write_a_Theme, and since it correctly finds both flora and a themeroller theme I dumped into that directory, I'm comfortable having this hard-coded, at least for now.

The biggest issue (and I can't tell from reading your patch how this works, either) is I don't understand how to actually get jQuery UI to respect the new theme selection? Any ideas?

This is against 6.x for now, and can be back-ported once we have it figured out.

Oops. That one had some cruft in it.

Stupid thing ate my reply. :P

Actually, the jQuery UI theme I used was a little modified version of the flora theme used by the plugin. The plug-in CSS files, i.e., used the folder i for the images, which I changed in images. Also the rest of the CSS files are changed to have a more consistent style.

That's cool, but the rule against 3rd party code still applies. But that sounds like a great patch to submit to jQuery UI for flora theme! :)

My idea was to use a .inc file so that the theme used by jQuery UI could be changed at runtime, and adapted to the theme used by Drupal. Or it can changed at runtime so that there isn't the need to have different themes which just use a different color scheme. I.e., it could be possible to have a theme like flora, but that uses red shadows.

The problem with the .inc file requirement is that you now need to know PHP in order to use a jQuery UI theme, which is out of the depth of most people. So I'd prefer to just say "download a theme and pop it into the jquery.ui/themes directory," even if it means some duplication between "Red flora" and "Blue flora."

We can do the run-time theme switching with drupal_add_css() in hook_menu op (!$may_cache) (Drupal 5) or hook_module_preprocess_page (Drupal 6).

The code that simply hides the fieldsets is not necessary, but makes the settings form a little more "user friendly", especially when used with the setting form of garland, which takes most of the space with the color selector. It even more true when it is also used themesettings.module, which adds more fieldsets.
Maybe that code can be put into a little module that does the work (something I am thinking to do).

Yeah, that sounds great! I would use that on several sites, only one of which actually needs jQuery UI module. :) You should submit a Drupal core patch too, while you're at it, so that in Drupal 7 this page is a lot easier to deal with for third-party modules in the future.

Why do you use such code when there is the function theme_get_settings()?

It was my understanding that theme_get_settings() would only get me all the settings for a theme, not a particular theme setting. But that said, that variable_get() call should definitely be changed to theme_get_settings($key). Thanks!

Also, I'm changing the version back to Drupal 6, because it has to get fixed there first and then back-ported.

Nice. Another thing that's missing in core's "js libraries" approach.

@Kiam: If you still have this code/patch at hand, could you re-roll adhering to Drupal's coding standards (like webchick did) ?

 /**
- * Implementation of hook_uninstall().
+ * Implementation of hook_update_N().
  */

Here, we document what actually is done in the update function instead. The exception of an exception. (Not, that I've mentioned this elsewhere in a core issue,...)

+function jquery_ui_update_6100() {
+  // Make sure the module is the last one to be called.
+  db_query("UPDATE {system} SET weight = 8 WHERE name = 'jquery_ui'");
+  
+  return array();
+}

8 is not sufficient. If you really want it to be last - for the moment it is installed - you have to grab maximum module weight of all other modules first and assign jquery_ui that MAX + 1 - ideally using a minimum of 10.

Additionally, we want to properly setup $ret = array(); and use update_sql() here.

-define('JQUERY_UI_PATH', drupal_get_path('module', 'jquery_ui') . '/jquery.ui');
+ define('JQUERY_UI_PATH', drupal_get_path('module', 'jquery_ui') .'/jquery.ui');

Improper change, please remove.

+/*******************************************************************************
+ * Drupal hooks.
+ ******************************************************************************/
+

We do not do that in Drupal modules.

+    '#type'          => 'fieldset',
+    '#title'         => t('jQuery UI'),
+    '#collapsible'   => TRUE,
+    '#collapsed'     => TRUE,
+    '#weight'        => -25,
+    '#tree'          => TRUE,

Do not indent array values like this - just use their natural indentation. Because further patches affecting this code will have to re-align the entire array.

The rest, I want to review after a new patch has been submitted.

well, this patch slightly overlaps with the patch in #388384: Allow usage of jQuery UI themes from the themeroller - I guess either this or the other issue is a duplicate in reality (though the issue titles are different, which adds to the confusion). However, based on the code, both patches do more or less the same.

ok, let's ensure that the other patch contains everything from this issue then.

Marking as duplicate of #388384: Allow usage of jQuery UI themes from the themeroller. You can follow up on that issue to track its status instead. If any information from this issue is missing in the other issue, please make sure you provide it over there.

However, thanks for taking the time to report this issue.