Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ticket/10650] Last post subject in forum list. #654

Merged
merged 8 commits into from Apr 4, 2012

Conversation

rahulr92
Copy link
Contributor

[develop] Last Topic Title on Forum list

The most recent topic title of a forum can now be displayed on the
board index. An option is provided in the ACP under the 'General Forum
Settings' which allows the admin to enable/disable this feature.
NOTE: This PR was closed but its merge was reverted.
New PR: #721

http://tracker.phpbb.com/browse/PHPBB3-10650

@@ -1109,6 +1109,7 @@ function get_schema_struct()
'forum_options' => array('UINT:20', 0),
'display_subforum_list' => array('BOOL', 1),
'display_on_index' => array('BOOL', 1),
'display_last_subject' => array('BOOL', 1),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be disabled (0) by default, imo. When a new addition is added that changes the existing functionality, it should be as unobtrusive as possible (i.e. current functionality should be retained by default, and then un-required changes/improvements should be configurable via the ACP). This also applies in the SQL files

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree, this is adding a new feature but adding an option for it to be disabled.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a big deal so if everyone really thinks this needs to be enabled by default, then fine. I just think that it's a good rule of thumb that when changing something (we are changing the way the last post info is displayed), it should default to how the user expects it to, and then give them the option to change it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one of the 5 most downloaded MODs, I doubt anyone will complain.

People expect things to change when they update from 3.0 to 3.1

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with unknownbliss.

@rahulr92
Copy link
Contributor Author

Hi. I have made the suggested changes and fixed the whitespaces. Please review and suggest further modifications if required.

@callumacrae
Copy link
Contributor

Looks good.

@nickvergessen
Copy link
Contributor

This will have a merge conflict with the event branch. Also I dunno whether this should be a core feature or kept as an easy sample extension

@michaelcullum
Copy link
Member

Not the end of the world having a merge conflict. We can move the event so it changes the variable (php event not template). As your extension is more advanced than this.

@@ -57,6 +57,8 @@
'DELETE_ALL_POSTS' => 'Delete posts',
'DELETE_SUBFORUMS' => 'Delete subforums and posts',
'DISPLAY_ACTIVE_TOPICS' => 'Enable active topics',
'DISPLAY_LAST_SUBJECT' => 'Display last added post title on forum list',
'DISPLAY_LAST_SUBJECT_EXPLAIN' => 'If set to yes the title of the last added post will be displayed in the forum list with a hyperlink to the post.',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use one of subject and title?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think title is most often used, imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So should I change all variables from subject to title?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems to be more work than expected. All other variables and database entries refer to the topic title as 'subject'. Even the user is displayed a 'subject' line in the posting page. So maybe I should just change the entries in the language files from title to subject. But as imkingdavid said, I think users will find 'title' more familiar. Can I have a final word on this?

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each post has a subject, and it should stay that way. The topic title is the post subject of the first post. So what this should say depends on whether you display the last subject or the topic title of the last added post. The two are different things. Since we are displaying the subject of the last added post, it should indeed say "subject" in the explanation too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in that case, just change "title" to "subject" in the language part, and then do igorw's request and I think this is about ready to merge.

Rahul and others added 6 commits April 5, 2012 00:45
The most recent topic title of the forum can now
be displayed on the board index. An option is provided
in the ACP under the 'General Forum Setting' which
allows the admin to enable or disable this feature.

PHPBB3-10650
Ran create_schema_files.php and added
the generated static sql files to the commit.

PHPBB3-10650
The entry in languages have been changed from
title to subject. Also the IF condition in forumlist_body.html
has been properly intented.

PHPBB3-10650
Inserted a space before 'true' as per coding
guidelines.

PHPBB3-10650
The subject being displayed in the forum list have been
shortened to 30 characters. Also it is now being shown
in a separate line.

PHPBB3-10650
Passworded forums and ones in which user doesn't have
read access will be excluded. Also uft8 based string functions and
html encode/decode functions have been used to sanitise subject.

PHPBB3-10650
{
$last_post_subject = htmlspecialchars(utf8_substr(htmlspecialchars_decode($last_post_subject, 0, 30)));
$last_post_subject .= '...';
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation messed up? ^^

Corrected the intendation of if condition.

PHPBB3-10650
Now empty subjects will not be displayed in a
new line on the forum list.

PHPBB3-10650
@imkingdavid imkingdavid merged commit 3e9711d into phpbb:develop Apr 4, 2012
p added a commit to p/phpbb3 that referenced this pull request Apr 4, 2012
The pull request (phpbb#654) was merged a little too quickly.

In particular, display_last_subject per-forum option needs consensus.

This reverts commit b1fb34a, reversing
changes made to 7d6b289.

PHPBB3-10650
@p
Copy link
Contributor

p commented Apr 4, 2012

Merge reverted in #710.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants