Bartik: template.php cleanups

+++ themes/bartik/template.php	11 Jul 2010 11:35:23 -0000
@@ -78,18 +69,7 @@ function bartik_process_page(&$variables
-  $variables['hide_site_name']   = theme_get_setting('toggle_name') ? FALSE : TRUE;
-  $variables['hide_site_slogan'] = theme_get_setting('toggle_slogan') ? FALSE : TRUE;

@@ -144,3 +124,21 @@ function bartik_page_alter(&$page) {
+  $variables['show_site_name']   = theme_get_setting('toggle_name') ? FALSE : TRUE;
+  $variables['show_site_slogan'] = theme_get_setting('toggle_slogan') ? FALSE : TRUE;

1) It looks like the names have changed, but not the logic/values. I.e., FALSE is TRUE now. ;)

2) Ideally, some other theme and markup component freaks should comment here on the general variable naming. I'm still not sure whether it wouldn't be a better idea to just re-use the existing theme setting name, i.e., "toggle_name" instead of "show_site_name". For sure, toggle_name is not very clear, but at least, themers (should) already know what's contained and its meaning from the theme setting.

+++ themes/bartik/templates/page.tpl.php	11 Jul 2010 11:35:23 -0000
@@ -29,10 +29,10 @@
- * - $hide_site_name: TRUE if the site name has been toggled off on the theme
+ * - !$show_site_name: TRUE if the site name has been toggled off on the theme
  *   settings page. If hidden, the "element-invisible" class is added to make
  *   the site name visually hidden, but still accessible.
- * - $hide_site_slogan: TRUE if the site slogan has been toggled off on the
+ * - !$show_site_slogan: TRUE if the site slogan has been toggled off on the
  *   theme settings page. If hidden, the "element-invisible" class is added to
  *   make the site slogan visually hidden, but still accessible.

oh. Of course, these need to be fully updated, too.

43 critical left. Go review some!

UPDATE: Ignore this patch ... see #4

sorry ... lost some stuff in the shuffle when I broke up the big giant patch

Straight reroll (#845928: System menu blocks don't get the same styling as menu module blocks removed code from bartik_preprocess_block).
I'll revisit this tomorrow.

After some time thinking on this, and discussing it with some other themers, I really think this patch shouldn't go in.

Yes, normally show is used instead of hide, but in this case it shouldn't be.
Setting a variable that always needs to be negated in use is just extra work. show_site_* is ALWAYS preceded by !, which is just another roadblock for PHP-noob themers.

Also, having to write if ($site_slogan) {... if (!$show_site_slogan) {...} } seems very confusing and introduces ambiguity over the differences between $site_slogan and $show_site_slogan. The differences between $site_slogan and $hide_site_slogan are more evident.

Just my 0.02 cents :)

Here's a reroll of just the helper function portion of the patch.

well, I'm not Bartik's maintainer, but from a pure code and synaptic perspective, negated variables are almost always bad, since humans need to think around one more corner to understand the actual meaning. Most often, a constant source of bugs.

Also, when themers start to familiarize themselves with theme settings, those negated variables simply get weird. To understand the effect of the 'toggle_name' theme setting, you need to think. Don't make me think.

+++ themes/bartik/templates/page.tpl.php	11 Aug 2010 19:58:21 -0000
@@ -96,24 +96,24 @@
-          <div id="site-slogan"<?php if ($hide_site_slogan) { print ' class="element-invisible"'; } ?>>
+          <div id="site-slogan"<?php if (!$show_site_slogan) { print ' class="element-invisible"'; } ?>>

I think in the case I need to agree with tim.plunkett... Given this change, $hide_site_slogan makes you think less than !$show_site_slogan.

I'm fine with #6 but I dont think I can mark it RTBC

Powered by Dreditor.

Why exactly are we hiding site name and slogan with element-invisible? I can somewhat understand site name, but site slogan? If I toggle this stuff off in theme settings I assume its not getting printed at all - gone, not hiding, I cant see a logical reason to leave either of these in the markup - can someone please explain the thinking here.

I take it we're all in agreement here that we should not do this? Can we close this as a won't fix and clear one more away.

Is there anything from the original issue we need to still look at?

The patch at #6 just takes shared code from bartik_process_maintenance_page and bartik_process_page and moves it to _bartik_process_page, which is still a good idea IMO.

It still applies cleanly, can someone either RTBC or won't fix it?

+++ themes/bartik/template.php	13 Aug 2010 02:32:38 -0000
@@ -45,18 +45,9 @@ function bartik_process_page(&$variables
+  _bartik_process_page($variables);

This is the only thing in the function with no comment to explain what it is - maybe we need a comment here?

+++ themes/bartik/template.php	13 Aug 2010 02:32:38 -0000
@@ -143,3 +123,21 @@ function bartik_page_alter(&$page) {
+  // Always print the site name and slogan, but if they are toggled off, we'll
+  // just hide them visually.

This entire comment refers to things that dont happen here...

+++ themes/bartik/template.php	13 Aug 2010 02:32:38 -0000
@@ -143,3 +123,21 @@ function bartik_page_alter(&$page) {
+    // If toggle_name is FALSE, the site_name will be empty, so we rebuild it.

This comment refers to the toggle setting, shouldn't it refer to the variable?

Docs/comments aside its good - either RTBC now and shift to a docs issue or fix em now and then RTBC?

Powered by Dreditor.

#6: drupal-bartik-850728-6.patch queued for re-testing.

This makes me really sad. It was basically RTBC three months ago, but it is way too late to change something like this. I blame myself for not following up.

Reroll for git, comment changes from #12.

Still applies with small offset, reduces duplicate code, and Bartik still works for me after applying.

#16: drupal-bartik-850728-16.patch queued for re-testing.

Oops, all the core/ paths changed, rerolled for HEAD.

Rerolling during core mentoring.

Sorry, I uploaded the wrong file here. The right one follows immediately

Reviewed patch #25, which is OK.
Maybe the documentation above _bartik_preprocess_node can be more generic as the focus doesn't need to be on site name and slogan only?

It looks like this patch added some unnecessary characters? It was throwing complaints in Sublime anyway.

Indeed, good catch @LewisNyman! Follow-up is RTBC.

Committed 1ba7895 and pushed to 8.x. Thanks!

Should have really be a separate issue.