Don't show disabled themes on blocks settings page

Pasqualle - November 16, 2007 - 14:32
Project:Drupal
Version:6.x-dev
Component:block.module
Category:bug report
Priority:normal
Assigned:Unassigned
Status:closed
Description

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

#1

Pasqualle - November 16, 2007 - 14:42

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

#2

gaele - December 12, 2007 - 12:41
Category:feature request» bug report
Status:active» needs review

Only show enabled themes.

Is this a feature request or a usability bug?

AttachmentSizeStatusTest resultOperations
block.module_1.patch1.21 KBIgnoredNoneNone

#3

gaele - December 12, 2007 - 12:46

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

#4

gaele - December 15, 2007 - 18:30

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)

#5

Pasqualle - December 16, 2007 - 04:11
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.

#6

Gábor Hojtsy - December 16, 2007 - 10:30
Status:reviewed & tested by the community» fixed

Agreed, committed, thanks.

#7

hass - December 16, 2007 - 15:05

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.

#8

Pasqualle - December 16, 2007 - 12:05

@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.

#9

Gábor Hojtsy - December 16, 2007 - 13:37

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.

#10

Ralf - December 16, 2007 - 14:11
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

#11

Pasqualle - December 16, 2007 - 14:58

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.

#12

hass - December 16, 2007 - 15:18

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

#13

Gábor Hojtsy - December 16, 2007 - 15:20

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.

#14

Pasqualle - December 16, 2007 - 16:10

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

#15

Ralf - December 16, 2007 - 17:41
Status:needs work» needs review

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

AttachmentSizeStatusTest resultOperations
block-list-all.txt3.7 KBIgnoredNoneNone

#16

Gábor Hojtsy - December 16, 2007 - 18:04
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.

#17

Jody Lynn - December 16, 2007 - 21:02

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.

#18

Gábor Hojtsy - December 16, 2007 - 21:37

Any settings page submission should properly invalidate the settings cache.

#19

Jody Lynn - December 16, 2007 - 21:47

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.

#20

Crell - December 16, 2007 - 23:44

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. :-) )

#21

Jody Lynn - December 17, 2007 - 01:06
Status:active» needs review

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

AttachmentSizeStatusTest resultOperations
block-editable-themes.patch1.57 KBIgnoredNoneNone

#22

Pasqualle - December 17, 2007 - 01:54

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

AttachmentSizeStatusTest resultOperations
block-editable-themes_1.patch1.65 KBIgnoredNoneNone

#23

Pasqualle - December 17, 2007 - 02:24

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

#24

Jody Lynn - December 17, 2007 - 05:38

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

#25

Gábor Hojtsy - December 17, 2007 - 10:05

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

#26

Jody Lynn - December 19, 2007 - 18:48
Status:needs review» reviewed & tested by the community

Tested successfully and rerolled for HEAD.

AttachmentSizeStatusTest resultOperations
block-editable-themes_2.patch1.65 KBIgnoredNoneNone

#27

Gábor Hojtsy - December 19, 2007 - 19:11
Status:reviewed & tested by the community» fixed

Thanks, committed.

#28

Anonymous - January 2, 2008 - 19:11
Status:fixed» closed

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

#29

Gábor Hojtsy - August 7, 2009 - 14:24

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

 
 

Drupal is a registered trademark of Dries Buytaert.