reparsing of GeSHi language definition files

Attaching the patch file, which didn't seem to make it last time.

--- ~/Downloads/geshifilter/geshifilter.admin.inc          2008-05-01 14:26:14.000000000 -0600
+++ ~/Sites/all/modules/geshifilter/geshifilter.admin.inc  2008-06-10 11:25:10.000000000 -0600
@@ -483,14 +483,27 @@ function _geshifilter_generate_languages
   $geshi_library = _geshifilter_check_geshi_library();
   if ($geshi_library['loaded']) {
     require_once('geshifilter.pages.inc');
+    // Create GeSHi header for entire stylesheet
+    $output .= "/**\n";
+    $output .= " * GeSHi Filter dynamically generated stylesheet\n";
+    $output .= " * (http://drupal.org/project/geshifilter)\n";
+    $output .= " * GeSHi (C) 2004 - 2007 Nigel McNie, 2007 - 2008 Benny Baumann\n";
+    $output .= " * (http://qbnz.com/highlighter/ and http://geshi.org/)\n";
+    $output .= " */\n\n";
+    // Iterate through all enabled languages
     $languages = _geshifilter_get_enabled_languages();
     foreach ($languages as $langcode => $language_full_name) {
-      // create GeSHi object
+      // Create GeSHi object for current language
       $geshi = _geshifilter_GeSHi_factory('', $langcode);
       _geshifilter_override_geshi_defaults($geshi, $langcode);
-      // add CSS rules for current language
-      $output .= $geshi->get_stylesheet(FALSE) ."\n";
-      // release GeSHi object
+      // Add CSS group and rules for current language; exclude header (4 lines)
+      $output .= "/* @group {$language_full_name} */\n";
+      $styles = $geshi->get_stylesheet(true);
+      $arr = explode("\n", $styles);
+      $arr = array_slice($arr, 4);
+      $output .= implode("\n", $arr);
+      $output .= "/* @end */\n\n";
+      // Release GeSHi object
       unset($geshi);
     }
   }
@@ -510,18 +523,16 @@ function _geshifilter_generate_languages
   file_check_directory($path, FILE_CREATE_DIRECTORY);
   // Make stylesheet file name (depends on enabled languages).
   $stylesheet_file = $path .'/geshifilter-languages-'. substr(md5(serialize($languages)), 0, 8) .'.css';
-  if (!file_exists($stylesheet_file)) {
-    // build stylesheet
-    $stylesheet = _geshifilter_generate_languages_css_rules();
-    // save stylesheet
-    $ret = file_save_data($stylesheet, $stylesheet_file, FILE_EXISTS_REPLACE);
-    if ($ret) {
-      variable_set('geshifilter_languages_css', $ret);
-      drupal_set_message(t('Generated external CSS file %file.', array('%file' => $ret)));
-    }
-    else {
-      drupal_set_message(t('Could not generate external CSS file. Check the settings of your !filesystem.',
-        array('!filesystem' => l(t('file system'), 'admin/settings/file-system'))), 'error');
-    }
+  // Build stylesheet content and save to file
+  $stylesheet = _geshifilter_generate_languages_css_rules();
+  $ret = file_save_data($stylesheet, $stylesheet_file, FILE_EXISTS_REPLACE);
+  // Report success or failure
+  if ($ret) {
+    variable_set('geshifilter_languages_css', $ret);
+    drupal_set_message(t('Generated external CSS file %file.', array('%file' => $ret)));
+  }
+  else {
+    drupal_set_message(t('Could not generate external CSS file. Check the settings of your !filesystem.',
+      array('!filesystem' => l(t('file system'), 'admin/settings/file-system'))), 'error');
   }
 }

Posted the patch in the clear. Stupid attachments...

First request: Not only is readability increased and the file conforms to CSS norms, but the file size is decreased. When I started tweaking the Objective-C definition for GeSHi, I looked at the CSS file a lot to see what rules matched up where. While readability is not the primary concern, it always helps in such situations. If you've ever used something like CSSEdit (http://macrabbit.com/cssedit/) you'll know why the groups are nice. If you haven't, you should really try it... ;-)

This ties with the second request, because although the CSS file is "auto-generated", it is only generated once for each unique set of enabled languages, hence the MD5 hash excerpt in the filename. However, if one of the language definition files changes (as was the case frequently while I was modifying the Objective-C file) the CSS remains stagnant. Even when flushing the cache of available languages.

My claim of low overhead is based on the assumption that the function _geshifilter_generate_languages_css_file() is called infrequently; as far as I saw, it was only when the settings for enabled languages was changed. It seems that in such situations, and *especially* given the name of the function, it would be prudent to actually generate the file. My only workaround without this patch was to manually delete the file with the matching MD5 sum and refresh so the function would actually regenerate it.

We don't want to use inline CSS in our situation, because that makes each page larger, and the CSS cannot be cached from page to page. (In fact, cached pages will continue to have the incorrect CSS unless the cache is flushed. Also, if the external file for that language set ever existed, it will still not be re-generated afterward.) That may be a good solution for someone editing the language defs that also has rights to clear the Drupal caches, but this is not always the case. I realize that twiddling with language files *might* be fairly irregular, but on our site, we are not averse to changing the colors to match our whim and fancy.

I know nobody wants to impose extra overhead, and I'm trying to avoid that. By my count, the function in question is called in exactly 2 places, both in geshifilter.admin.inc:

- geshifilter_admin_general_settings_submit():221
- geshifilter_admin_per_language_settings_submit:318

Unless the overhead imposed by regenerating the CSS file only when one of those functions asks it to is excessive, I think the added convenience is well worth it. This is a simple patch, and it runs exactly the same, except it's easier to understand. How can you lose?

For an example of the new CSS stylesheet format, see this page:

http://cocoaheads.byu.edu/sites/default/files/geshifilter/geshifilter-la...

Sorry for the delay, but I've been working on my Master's thesis and publications.

No offense, but I think some of your assumptions are flawed. Editing the language files and expecting it to propagate to auto-generated CSS is not like hacking Drupal core, especially since I have submitted my language patch to GeSHi. The problem with your suggested approach of editing the CSS manually is that any CSS changes get thrown out the window if I ever change to a different set of enabled languages. (That's an optimization feature of your module, not GeSHi, and one that I find somewhat debatable. I simply don't the potential for a huge computation hit, since I don't imagine that people switch enabled languages frequently enough to merit it.)

I like the auto-generated CSS file. I'm even fine with the distinct versions using MD5 hashes, although I think it's silly and wastes more space with inactive CSS files than it would waste processing time. I just don't see why I can't even choose to actually regenerate the file when my language files have changed, not just when my enabled set has changed.

Perhaps this scenario will make sense. Say I install an update to GeSHi itself, and some of the language files have been updated, but I haven't changed my set of enabled languages. I'd like to be able to refresh the highlighting to match any changes in the language files by regenerating the CSS. Currently, your filter won't do that if the file already exists. I'd have to go find the CSS, delete the one with the right MD5 digest (or all of them) and then get it to regenerate. Not at all intuitive.

To me, the option to add only classes and no rules seems like a step backward. I can't imagine wanting to supply all my own CSS rules for highlighting--that's why we're using GeSHI and your filter. However, if you add an optional feature like that, it makes absolutely no sense that I can't tell the filter to regenerate my CSS file! Not even automatically, just when I ask it to! I think that the _geshifilter_generate_languages_css_file() function should do what it says it does, and not try to guess that I actually don't want to recreate the file since it already exists. I'd even settle for a button or checkbox that allows me to specify this behavior. Heck, there's one for generating empty CSS rules.

(Never mind the fact that my patch also includes fixes to the CSS that makes it easier to edit in the case where I might want to modify it directly. The file is smaller and more readable. Why is there such resistance to that idea? I don't get it.)

Sorry to rant, this is just a bit frustrating. These features seem like obvious wins. I'll try to calm down now. :-)