I have implemented a patch that adds two new features for the forum module block.

The options provide more granularity for the forum block, when the admin can select to show only active topics, or new topics, or both.

Patch is attached.

CommentFileSizeAuthor
#23 forum.module-block-cvs-3.patch2.63 KBkbahey
#18 forum.module-block-cvs-2.patch2.59 KBkbahey
#12 forum.module-block-cvs.patch2.59 KBkbahey
#6 forum.module-block-3.patch2.19 KBkbahey
#1 forum.module-block-2.patch3.32 KBkbahey
forum.module-block.patch2.72 KBkbahey
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kbahey’s picture

kbahey CreditAttribution: kbahey commented
FileSize
forum.module-block-2.patch3.32 KB

If only one option is selected, then having two headers is a bit redundant.

Hence, this 2nd version of the patch eliminates this, and only displays two headers if the two checkboxes are selected.

Dries’s picture

Dries CreditAttribution: Dries commented

Your patch is reversed and does not conform our coding standards. Won't commit.

Bèr Kessels’s picture

Bèr Kessels CreditAttribution: Bèr Kessels commented

-1 from me. I am fighting aginat new options :).

What i would like much better, is when you add two new blocks, then you (a) Do not need additional options in the settings sections, and (b)leave that to the blocks adminstration.

Bèr

kbahey’s picture

kbahey CreditAttribution: kbahey commented

Dries,

Can you explain more what you mean by 'reversed' ? Do you mean that the patch works the opposit of what it should do? Or you mean 'backed out'? Also, can you be more specific about how it is not up to coding standards?

Ber,

I understand your reluctance for new options. Your suggestion of two blocks is a better solution. I will give it a thought.

Junyor’s picture

Junyor CreditAttribution: Junyor commented

I was looking at this exact issue last night. There should be two separate blocks enabled/disabled through the block administration page.

That leaves one option on the forum settings page about blocks: # of items to show. I think that option is something that should be more integrated into block administration and available for all blocks (where applicable) rather than just the forums block. I've had to hack code just to change the number of items in a block, which isn't user-friendly at all.

kbahey’s picture

kbahey CreditAttribution: kbahey commented
FileSize
forum.module-block-3.patch2.19 KB

Here is a patch that makes them two blocks, one for active, and one for new.

The admin can select either or both, and place each in a different place. Thanks for Ber for his suggestion.

I understand now what Dries meant by reversed. It is corrected now.

Junyor’s picture

Junyor CreditAttribution: Junyor commented
Component: base system » forum.module

Don't forgot to remove the settings from the forum settings. And set this issue to patch. :)

kbahey’s picture

kbahey CreditAttribution: kbahey commented

Changing to patch.

There are no settings in this one.

Junyor’s picture

Junyor CreditAttribution: Junyor commented
Component: forum.module » base system

Right. I meant remove the current settings.

Junyor’s picture

Junyor CreditAttribution: Junyor commented

Nevermind, I see that the settings are removed. +1 for this patch. Applies cleanly to 4.5.2, too.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented

+1

However, I think this patch was made against 4.5.x and not CVS. There are some changes to the block hook which move the settings which I see are not properly handled by this code. I would guess that it does not apply.

kbahey’s picture

kbahey CreditAttribution: kbahey commented
Component: base system » forum.module
FileSize
forum.module-block-cvs.patch2.59 KB

Here is the patch against CVS HEAD.

I have no CVS installation at present, so this is not tested.

Can someone please test it and report here?

tangent’s picture

tangent CreditAttribution: tangent commented

The patch applies. It does not comply with the style guidelines though. The spacing and use of tabs is incorrect.

Also, this patch removes the option of having both types of forum links in the same block in favor of putting them in separate blocks. I think it makes sense to leave them in the same block and this removes the temptation to use block titles of the form "Forum: Active topics" which isn't very consistant or elegant.

Bèr Kessels’s picture

Bèr Kessels CreditAttribution: Bèr Kessels commented

tangent: Drupal has no other blocks that contain two differten lists. It does also not have two different ficuntions in one block either.
In other words: having a (forum) block that has two lists, is very inconsistant, much more that two different titles.
Can we not change the titles into:
Active forum topics
New forum topics

so +1 for moving them into their own blocks, and -1 from keeping them in one block with more config options.

Anonymous’s picture

(not verified) CreditAttribution: commented

Yeah, changing the titles to:

Active forum topics
New forum topics

would be preferable.

Stefan Nagtegaal’s picture

Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commented

I have to agree with Ber on this one.
As i think the functionality is quite handy for some sites, I am against having options for everything. The best solution in this way will be - as Ber suggests - having two blocks. for an example of how multiple blocks could be implemented in a module, take a look at the block example page of drupaldocs.org, which is very clear..

Besides that, please take some time to let your code be as we want it to be: Coding standards

tangent’s picture

tangent CreditAttribution: tangent commented

Can we not change the titles into:
Active forum topics
New forum topics

I was going to suggest these block titles at first but I thought it might be better to keep all forum stuff in a single block. I don't think its that important though so +1 on those block title changes.

kbahey’s picture

kbahey CreditAttribution: kbahey commented
FileSize
forum.module-block-cvs-2.patch2.59 KB

Thanks for stefan for pointing out the coding standards, rather than just stating that my code is not compliant with them.

I reviewed them, and they are not to my personal liking (e.g. braces are K&R style and not on the same column and on a line by themselves, ...etc.). However, a standard is a standard, whether we like it or not. Conforming to a less than ideal standard is better than no standard at all. So, I will stick to them, since they are the way of doing business around here. By the way, does anyone have pointers to vim settings for these standards?

Regarding the rest of the comments, it seems the concensus is to leave the patch functionality as it is (two blocks, with titles as they are).

So, I am changing the style to be in conformance, and here is a patch for that.

Anonymous’s picture

(not verified) CreditAttribution: commented

Any text presented in the UI needs to be wrapped in a call to the t() function for translation purposes.

The line
$title = 'Active forum topics';
in your patch should be changed to:
$title = t('Active forum topics');

Same problem with 'New forum topics'.

Other than that, +1.

kbahey’s picture

kbahey CreditAttribution: kbahey commented

Hello Anonymous

If you look a bit down, there is a t($title), which does the same thing you want, to theme to the title of the block.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Title: New options for forum block » Separating forum blocks

kbahey, please! You need to wrap the literal string into t(), and not a variable, so the extractor can pick it up. Otherwise +1 on separating forum blocks. Also renamed the issue to reflect this goal.

tangent’s picture

tangent CreditAttribution: tangent commented

The switch block also contains some tabs which should be converted to spaces.

kbahey’s picture

kbahey CreditAttribution: kbahey commented
FileSize
forum.module-block-cvs-3.patch2.63 KB

Here is the latest version of the patch, responding to the requests made:

1. The t() only has a literal as an argument

2. Indentation is with spaces and not tabs.

TDobes’s picture

TDobes CreditAttribution: TDobes commented

+1 -- I manually created a PHP block for just active forum topics on my site years ago. This would be a welcome improvement.

Dries’s picture

Dries CreditAttribution: Dries commented

Committed to HEAD.

Anonymous’s picture

(not verified) CreditAttribution: commented