Do not show disabled themes on blocks settings page (admin/build/block)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Pasqualle’s picture

or add a link to hide/show disabled themes (Show all/Hide disabled)

gaele’s picture

Category: feature » bug
Status: Active » Needs review
FileSize
1.21 KB

Only show enabled themes.

Is this a feature request or a usability bug?

gaele’s picture

Yes this is a bug. The current behavior is inconsistent with admin/build/themes/settings, which only shows enabled themes.

gaele’s picture

And forgot to mention: this is another result from Factory Joe's review.

"The list of themes here is really confusing. I don't get it."

http://groups.drupal.org/node/7043 (see under Blocks, about 2/3 down)

Pasqualle’s picture

Status: Needs review » Reviewed & tested by the community

works nicely

the change: 1 line status check

sidenote:
Checked the section module (http://drupal.org/project/sections), which also shows disabled themes, as a dropdown list. But if the theme is disabled the section can not use it.
So there is no need to manage blocks of disabled themes for the section module.

Can't get any other idea where it would be required to manage blocks for disabled themes, so RTBC.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Agreed, committed, thanks.

hass’s picture

Status: Needs work » Fixed

About Sections only:

As co-maintainer of sections i can say - listing disabled themes make big sense. You are able to configure something that is currently disabled or if you have configured a section earlier that have now an inactive theme - your configuration get's broken if you look inside this section configuration. e.g you have configured to be 3 col theme and you decide to disable this 3 col theme, the section configuration must remain as with "3col" item selected in the selectbox. I often thought about adding some extra info in the select box telling about "disabled" theme status. Maybe with a "*" behind the theme name and a help text explaining this special behavior... but removing the menu item break things.

Pasqualle’s picture

@hass:
You are right, and I absolutely agree. Did not thought of it this problem before. If the theme is a property, which is stored, then the disabled themes could/should be listed.
But I think it is still broken if I remove the theme, but who cares..

Yes. a little explanation in section settings would be really cool, because it is not obvious why the section does not show up.

Gábor Hojtsy’s picture

hass: If you can provide a *core* use case, feel free to object. Otherwise you can add menu items / tabs to the disabled themes yourself, and fill in their page handler callbacks just as the core blocks code does.

rstamm’s picture

Status: Fixed » Needs work

the patch is only listing enabled themes but not the admin theme if the admin theme is a disabled theme.

a better solution is to add another tab:
list -> enabled and admin theme
list all -> all installed themes

Pasqualle’s picture

admin theme does not need to be enabled? that is the problem, and should be fixed somehow..
it does not show up in theme settings page either.

hass’s picture

Status: Fixed » Needs work

Well, this sounds strange... i never tried with a disabled admin theme yet...

Gábor Hojtsy’s picture

Well, it is kind of a 'feature' that the admin theme should not be enabled, as users cannot choose it as the site theme for example. (Although it cannot have its settings modified as well). It is easy to fix this to also include a tab for the admin theme if it is disabled. Just variable_get the admin theme and output a tab for that too.

Pasqualle’s picture

please then fix the admin/build/themes/settings page also..

rstamm’s picture

Status: Needs work » Needs review
FileSize
3.7 KB

Patch adds a new tab item to block configuration page.

tab "List": Displays only enabled themes and admin theme
tab "List all": Displays all installed themes

Gábor Hojtsy’s picture

Status: Needs review » Active

Uhm, why complicate our life? As said, core and nearly all of contrib is happy with listing only enabled themes (+ the admin theme), and those who are not happy can have their contrib module add the "missing" tabs. Let's not make the core block admin harder to grasp.

All I see is that we need the admin theme listed in the tabs, as I said in #13.

Jody Lynn’s picture

if ($theme->status || variable_get('admin_theme', '0') == $key) { This is the change (Line 166) however the problem is that if you change the administration theme this menu item will not update to reflect that until you clear the cache.

Gábor Hojtsy’s picture

Any settings page submission should properly invalidate the settings cache.

Jody Lynn’s picture

I think the problem is the menu cache - these changes are right in block_menu so they don't get updated until the menu cache clears. I'm confused as to how to make non-caching menu items in D6.

Crell’s picture

There is no such thing as cached or non-cached menu items in Drupal 6. All menu items are (can be) dynamic, which is not the same as uncached.

If you need the menu rebuilt, I'd say just fire a menu_rebuild() in the form submit handler and be done with it. (Although chx will no doubt smack me down and say there is some other method. :-) )

Jody Lynn’s picture

Status: Active » Needs review
FileSize
1.57 KB

OK, I got around the problem by using an access callback.

Pasqualle’s picture

changes:
renamed the function to follow the naming conventions in core
removed one access argument. maybe I am missing something, but it does not looks like to me that it is needed
added function documentation

tested, the patch works for me

Pasqualle’s picture

patch for the admin/build/themes/settings page
http://drupal.org/node/201577

Jody Lynn’s picture

Marked http://drupal.org/node/103717 as a duplicate: there you can see the origin of a lot of this stuff.

Gábor Hojtsy’s picture

Indeed, the access callback seems to be much more menu API friendly. Let me know if Lynn tested it and found it working right.

Jody Lynn’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.65 KB

Tested successfully and rerolled for HEAD.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, committed.

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

Gábor Hojtsy’s picture

Turns out special casing the admin theme was not a good idea at all: #542828: Do not special case disabled admin theme