Merge Garland and Minnelli into one theme

Makes loads and loads of sense, on first glance at the screenshot, it looks like there's an error and Garland has been duplicated in the listing, would be great not to have that any more.

My main concern with this patch once upon a time would've been that we don't have any other sub-themes in core and so it provides us with a way of testing them. But now we have an automated testing framework and the ability to add 'hidden' themes to do all sorts of crazy madness, so I would support this change.

Tagging as something to look at before UX freeze.

Great idea, just keep in mind the upgrade path, i.e. resetting the theme if someone upgrades, and then migrating the blocks.

Yep, Jen, +1 for merging these. As a newbie, it baffled me why there were two nearly-identical themes.

Here's updated screenshot.png, base.png, and preview.png that removes the "fluid width" text from those images.

The patch removes Minnelli completely.

The default width in the CSS is the fixed width option. If garland_preprocess_html() adds a fluid-width class, then the width is setup as fluid. garland_preprocess_html() will add the fluid-width class by default and/or if Garland is configured to use the new "Content width: fluid width" option on the new configure theme settings form for Garland (click "configure" next to Garland on the Appearance page.) The default width in the theme setting is the fluid width, so Garland will continue to be fluid width by default.

Note that for the install, update, and db-offline maintenance pages the garland_preprocess_html() function will not be run and the content width will use the CSS-defined default of fixed width. This is the equivalent of using Minnelli for those pages.

Wait, wait! Before you go down this path, let's get #547068: D7UX: use Seven for installation / updates in. It looks like you're changing almost exactly the same lines.

I don't mind waiting for #547068: D7UX: use Seven for installation / updates to go in first. I have bazaar; I can re-roll anything. :-)

Ok, done. GO. ;)

Wait. We are removing the only sub-theme example here.

I'm totally opposed to this. This core implementation served very well as sub-theme example, and, instead of removing it, we should advance on it. Not only to have an example, but also to have a working implementation for testing.

sun: We have a themes/tests/ directory where we have inheritance examples for testing. Minelli is a poor example of sub-theming anyway. There's no reason fixed vs. fluid couldn't be a setting, and then we could show off how to use the theme settings API.

I would absolutely love to have 10 gorgeous themes that piggyback off of Drupal's default template files to include in core as sub-theming examples, but not having those, this is a pretty poor case for leaving such a bizarre visual discrepancy.

Ditto what webchick said. :-)

Here's a re-roll hopefully fixing the MenuIncTest.

+1. Patch works well and is easy to understand for a semi-newb like me, which is always a plus.

No upgrade path?

Added upgrade path for catch. :-)

Looks great. Didn't test any of it but from a code point of view it's a lot cleaner and the upgrade path looks sane.

Good idea.

Per catch's sharp eyes in IRC, this patch fixes the coding style on an inline comment in garland/theme-settings.php. No code changes, so leaving at RTBC.

+++ modules/simpletest/tests/menu.test	2009-11-22 21:59:17 +0000
@@ -56,11 +56,12 @@ class MenuIncTestCase extends DrupalWebT
   function testThemeCallbackMaintenanceMode() {
     variable_set('maintenance_mode', TRUE);
+    variable_set('maintenance_theme', 'seven');
 
     // For a regular user, the fact that the site is in maintenance mode means
     // we expect the theme callback system to be bypassed entirely.
     $this->drupalGet('menu-test/theme-callback/use-admin-theme');
-    $this->assertRaw('minnelli/minnelli.css', t("The maintenance theme's CSS appears on the page."));
+    $this->assertRaw('seven/style.css', t("The maintenance theme's CSS appears on the page."));

This is breaking the test's purpose.

See my patch in the other maintenance theme issue on how to alter it properly.

I'm on crack. Are you, too?

+    variable_set('maintenance_theme', 'seven');
 
     // For a regular user, the fact that the site is in maintenance mode means
     // we expect the theme callback system to be bypassed entirely.
     $this->drupalGet('menu-test/theme-callback/use-admin-theme');
-    $this->assertRaw('minnelli/minnelli.css', t("The maintenance theme's CSS appears on the page."));
+    $this->assertRaw('seven/style.css', t("The maintenance theme's CSS appears on the page."));

What @sun said :)

More specifically, the test only serves a purpose if it visits a page whose default theme is different from the maintenance theme. You should just be able to get rid of the variable_set() that was added and then assert 'garland/style.css'....

+  if (variable_get('theme_default', 'garland') == 'minnelli') {

(minor) We really don't need 'garland' as the variable_get() default here - just variable_get('theme_default') == 'minnelli' should be fine.

+    // Set the theme setting width to "fixed" to match Minnelli's old layout.
+    $settings = variable_get('theme_garland_settings', array());
+    $settings['garland_width'] = 'fixed';
+    variable_set('theme_garland_settings', $settings);

Can't Minelli have 'theme_minnelli_settings', color module settings, etc that should be migrated over as well?

+  if (variable_get('admin_theme', 'seven') == 'minnelli') {
+    variable_set('admin_theme', 'seven');
+    db_update('system')
+      ->fields(array('status' => 1))
+      ->condition('type', 'theme')
+      ->condition('name', 'seven')
+      ->execute();
+  }

Why would we switch them to Seven here? If they're using Minnelli as their admin theme, shouldn't we replace that with Garland?

+    '#weight' => '-2',

(minor) Does this really need to be a string?

Updated patch given sun's and David's comments.

The color files are generated using color.module's color_scheme_form_submit() (a submit handler for form_system_theme_settings_alter). :-p I don't see an easy way to re-create this functionality inside an system update function, so I've just transfered the Minnelli's form settings to Garland; users will just have to go to that form and hit submit to re-create all the color files.

Bot

No code review from me, but definately a UX +1 to doing this for all the obvious reasons already mentioned here.

Actually applied and tested this patch. Works as advertised. It's a great relief to not see the same preview image twice on the Appearance page anymore.

Since it has been RTBC before, setting it back to that. Don't forget to commit the images in #6 as well.

The patch looks good. It did occur to me, though, that the update path probably still isn't complete - thinking about things like themes which are enabled but not the default, $user->theme, etc, which are not dealt with in this patch... deleting a theme gets complicated very fast :)

However, it's probably good enough for starters, so we could try to perfect the update path later - or, maybe we could decide to just move Minnelli to contrib (where it could then evolve into something better) rather than killing it completely, and then we don't need to worry about an update function at all?

Didn't actually mean to change the status - like I said above, this seems good enough to commit.

Oh, I'd like to point out that now that #491214: implement the top level Appearance / Choose Theme admin page has been committed, we don't need the screenshot.png in comment #6 anymore. Also, the patch in #24 needs a re-roll since it doesn't apply anymore due to changes in default.settings.php.

So here's a re-rolled patch. And the 2 updated color files for Garland again.

Ah, looks like the testAdministrationTheme test produces a warning when there are no disabled themes. Fixed that. No other changes. Images from comment #30 are still the ones to use.

Whoops. Didn't quite fix that last test failure. Now I have. (I hope.) :-)

Again, images from comment #30 are still the ones to use.

Yay! Committed to HEAD. I think. ;) That was pure CVS hell; it'd be nice for someone to confirm everything got there okay.

Just did a cvs up. Looks good to me! :-)

Hmm. I happened to have minnelli as the default theme on my sandbox, so I did cvs up, and now update.php is WSOD...

@matt2000. But update.php uses the Seven theme to display its content, so the removal of Minnelli wouldn't affect its ability to be displayed. I don't think this issue has anything to do with your WSOD on update.php.