[drupal-devel] [bug] Make block configuration understandable
drumm
drupal-devel at drupal.org
Tue May 3 00:04:29 UTC 2005
Issue status update for http://drupal.org/node/11875
Project: Drupal
Version: cvs
Component: block.module
Category: bug reports
Priority: normal
Assigned to: killes at www.drop.org
Reported by: drumm
Updated by: drumm
Status: patch
Can this be closed yet?
drumm
Previous comments:
------------------------------------------------------------------------
October 21, 2004 - 13:42 : drumm
Attachment: http://drupal.org/files/issues/tmp (42.68 KB)
The primary goal of this patch is to take the 'custom' and 'path'
columns of the block overview page and make them into something
understandable. As of Drupal 4.5 'custom' lacked an explanation which
wasn't buried in help text and path required dealing with regular
expressions.
Every block now has a configuration page to control these options. This
gives more space to make form controls which do not require a lengthy
explanation. This page also gives modules a chance to put their block
configuration options in a place that makes sense using new operations
in the block hook.
The only required changes to modules implementing hook_block() is to be
careful about what is returned. Do not return anything if $op is not
'list' or 'view'. Once this change is made, modules will still be
compatible with Drupal 4.5. Required changes to core modules are
included in this path.
An additional optional change to modules is to implement the additional
$op options added. 'configure' should return a string containing the
configuration form for the block with the appropriate $delta.
'configure save' will come with an additional $edit argument, which
will contain the submitted form data for saving. These changes to core
modules are also included in this patch.
I have posted screenshots at http://foo.delocalizedham.com/tmp/blocks/.
Additional work, such as further rearrangement of the block overview
page, is as always a task for another patch.
------------------------------------------------------------------------
October 21, 2004 - 15:56 : drumm
Attachment: http://drupal.org/files/issues/block_0.diff (42.79 KB)
Now with a few more comments.
------------------------------------------------------------------------
October 21, 2004 - 16:14 : drumm
Attachment: http://drupal.org/files/issues/blocks.sql (211 bytes)
And the DB changes.
------------------------------------------------------------------------
October 23, 2004 - 11:06 : Dries
This patch marks a significant usability improvement. Excellent job. I
had a quick look at the patch:
1. Taken from the patch:
<?php
+ else if ($op == 'configure' && $delta = 3) {
?>
There are a number of '$delta = 3's that probably should be '$delta ==
3's.
2. I would use 'save' rather than 'configure save'.
3. block_box_add() is a little weird:
3.a. Is the $bid parameter being used?
3.b.
<?php
+ drupal_set_message(block_box_save($edit));
+ drupal_goto('admin/block');
?>
I would move the drupal_set_message() and the drupal_goto() inside
block_box_save(), if possible without having to add extra code.
3.c.
<?php
+ $form = block_box_form();
+ $form .= form_submit(t('Save block'));
+ $output .= form($form);
?>
Any reason we can't move all form stuff into block_box_form()?
3.d. It would be good if we could get rid of the block vs box thing.
4. We need proper database updates.
5. [?+ php block['pages_show'] =
$old_blocks[$module][$delta]->pages_show;
+ $block['pages'] = $old_blocks[$module][$delta]->pages; ?]
What is the difference between 'pages' and 'pages_show'? It is not
clear from the names so maybe we should try to come up with more
explanatory names.
6. The changes will need to be documented in a new 'Converting modules
from 4.5.x to CVS' page.
7. Has the block API Doxygen comments?
------------------------------------------------------------------------
October 23, 2004 - 14:40 : moshe weitzman
this is an improvement.
one nit: don't use a form_group() around all the form fields on a page.
There is no need to group if everything is of the same type. If you need
to name that single group of forms, use the page title
------------------------------------------------------------------------
October 23, 2004 - 15:13 : Steven
I agree, this is a very nice patch.
1) One thing I do miss is that there is no (longer?) a clear overview
of blocks (which blocks are always enabled, which are contextual, ...).
However, this is really the fault of the giant configuration table.
Reworking that is a different patch.
2) I have a usability issue with block.module which could go in this
patch: the description field is really only useful for module-defined
blocks where the title is not descriptive. Custom blocks have static
titles, so the description is really redundant. I know I never fill it
in on my own site, only to curse myself when I see the administration
screen and have a bunch of unknown blocks sitting there. So either:
- Get rid of the description field for custom blocks and use the title
instead.
- Keep the description field, but use the block title if the
description field was left empty.
I prefer the first.
3) Here's a reworked contextual help text for the block admin. The
giant paragraph that we have now is not going to be read by anyone:
"Blocks are the boxes in the left and right side bars of the web site.
They are made available by modules or created manually.
Only enabled blocks are shown. You can position the blocks by deciding
which side of the page they will show up on (region) and in which order
they appear (weight).
If you want certain blocks to disable themselves temporarily during
high server loads, check the 'Throttle' box. You will need to enable
the auto-throttle on the [[throttle configuration page]] after having
enabled the throttle module.
"
4) The general module help needs work:
"If the block\'s throttle checkbox is checked, the throttle level must
me low." (me -> be)
The paragraph about "A block is visible when:" is also structured
oddly. The part about the throttle is confusing and the last option
does not match the preceding fragment at all (it starts a new
sentence).
Maybe it's better to say:
"
A block's visibility depends on:
- Its enabled checkbox. Disabled blocks are never shown.
- Its throttle checkbox. Throttled blocks are hidden during high server
loads.
- Its path options. Blocks can be configured to only show/hide on
certain pageS.
- User settings. You can choose to let your users decide whether to
show/hide certain blocks.
- Its function. Dynamic blocks (such as those defined by modules) may
be empty on certain pages and will not be shown.
"
This is a bit vaguer, but shorter and easier to understand. Where the
exact options are located is not so important, because they are in a
very logical place, and people are bound to run into them when making
their own blocks.
------------------------------------------------------------------------
October 23, 2004 - 18:05 : Dries
I'm OK with removing the description.
------------------------------------------------------------------------
October 28, 2004 - 18:42 : drumm
Attachment: http://drupal.org/files/issues/block_1.diff (44.79 KB)
#3:
1. Changed '=' to '=='.
2. Changed 'configure save' to 'save'.
3. Cleaned up block_box_add(), specific code is there because
block_box_form() and block_box_save() are reused for editing. The
distinction between block and box is useful, boxes are user created
blocks, however box might not be the best name.
4. Added database updates for mysql.
5. Pages stores the list of pages for the textarea, pages_show stores
whether those are pages to show, or not show, on for the radio buttons.
6. Yes it will. Text can probably be copied from #1.
7. Finishing those up now. I can commit them to contrib cvs myself.
#4:
It works well on blocks with specific configurations. I expect a
limited number of additional options such as block name will be added
before 4.6. That will create more groupings.
#5:
1. Yes, lets save that for another patch.
2. I have a good idea for the description's needed removal which I plan
to work into a patch which allows every block to be renamed.
3. Replaced with the suggested help text.
4. Changed 'me' to 'be'.
#6:
See #5.2.
------------------------------------------------------------------------
October 29, 2004 - 01:39 : stefan nagtegaal
I think - to be more consistent with the rest of drupal - that if the
throttle.module is not enabled, you should _not_ display the throttle
fields at all. We do this all the time inside drupal, right?
I suggest you do something like:
<?php
- $header = array(t('Block'), t('Enabled'), t('Throttle'), t('Weight'),
t('Region'), array('data' => t('Operations'), 'colspan' => 2));
+ $header = array(t('Block'), t('Enabled'), (module_exist('throttle') ?
t('Throttle')), t('Weight'), t('Region'), array('data' =>
t('Operations'), 'colspan' => 2));
?>
and:
<?php
- $rows[] = array($block['info'], array('data' =>
form_checkbox(NULL, $block['module'] .']['. $block['delta']
.'][status', 1, $block['status']), 'align' => 'center'), array('data'
=> form_checkbox(NULL, $block['module'] .']['. $block['delta']
.'][throttle', 1, $block['throttle'], NULL, module_exist('throttle') ?
NULL : array('disabled' => 'disabled')), 'align' => 'center'),
form_weight(NULL, $block['module'] .']['. $block['delta'] .'][weight',
$block['weight']), form_radios(NULL, $block['module'] .']['.
$block['delta'] .'][region', $block['region'], array(t('left'),
t('right'))), l(t('configure'), 'admin/block/configure/'.
$block['module'] .'/'. $block['delta']), $operation);
+ $rows[] = array($block['info'], array('data' =>
form_checkbox(NULL, $block['module'] .']['. $block['delta']
.'][status', 1, $block['status']), 'align' => 'center'),
(module_exist('throttle') ? array('data' => form_checkbox(NULL,
$block['module'] .']['. $block['delta'] .'][throttle', 1,
$block['throttle'], NULL)), 'align' => 'center'), form_weight(NULL,
$block['module'] .']['. $block['delta'] .'][weight', $block['weight']),
form_radios(NULL, $block['module'] .']['. $block['delta'] .'][region',
$block['region'], array(t('left'), t('right'))), l(t('configure'),
'admin/block/configure/'. $block['module'] .'/'. $block['delta']),
$operation);
?>
The above patch is untested, and i'm not fully trusted with the fact
that it works, but your should get an idea of what I mean..
------------------------------------------------------------------------
October 29, 2004 - 14:41 : drumm
Actually this patch leaves the overview page as is, excpet for removing
the columns that are expanded into the configuration page.
I believe the standard is to show the throttle configuration, but
disabled if not applicable. See the modules administration page.
------------------------------------------------------------------------
October 29, 2004 - 15:15 : Dries
I tried the patch: running the update.php script disabled all blocks.
Can't look into it right now (lunch) but it is probably an easy fix.
user error: Unknown column 'pages_show' in 'field list' query: INSERT
INTO blocks (module, delta, status, weight, region, pages_show, pages,
custom, throttle) VALUES ('user', '3', 0, 5, 1, 0, '', 0, 1) in
/mnt/redhat/foo/cvs/web/drupal.org/cvs/includes/database.mysql.inc on
line 125.
------------------------------------------------------------------------
October 30, 2004 - 00:20 : stefan nagtegaal
Drumm, I know how the current overview looks and that's why I am asking
you to make this fix.. The throttle-checkbox should _not_ be displayed
instead of being disabled when the module is not enabled..
If a module is disabled, we don't display their help text and
configuration options at all, right? So why are we doing that for the
throttle module?
Stefan.
------------------------------------------------------------------------
October 31, 2004 - 01:38 : Dries
Committed. The UI could do with a tad more polish though, but that can
be part of a new patch. Please make sure to update the documentation.
------------------------------------------------------------------------
April 2, 2005 - 12:52 : killes at www.drop.org
Attachment: http://drupal.org/files/issues/update_2.patch (823 bytes)
The upgrade simply dropped all the stored regexes. I understand that an
upgrade patch was difficult to provide, but we should at least document
this.
Attached patch does this.
------------------------------------------------------------------------
April 8, 2005 - 07:04 : Dries
I'll add this to the release announcement together with the other
upgrade issues. Better to centralize everything imho.
More information about the drupal-devel
mailing list