Skip to:

Opened 14 years ago

Closed 10 years ago

Last modified 10 years ago

#2768 closed enhancement (fixed)

Use WP comments' threaded comment depth settings

Reported by: djpaul's profile DJPaul Owned by: boonebgorges's profile boonebgorges
Milestone: 2.0 Priority: normal
Severity: normal Version:
Component: Activity Keywords: has-patch commit
Cc: zacharydubois@…


Related to #1870, I think that the Activity Stream should respect WP comments' threaded comment depth settings (thread_comments and thread_comments_depth in the options table).

I think that from a user's point of view, an activity stream item is the same as a comment on a blog post, and #2729 would only serve to blur the lines between these types of replies.

Attachments (4)

2768.patch (3.2 KB) - added by DJPaul 13 years ago.
2768.02.patch (3.2 KB) - added by boonebgorges 10 years ago.
2768.02b.patch (602 bytes) - added by r-a-y 10 years ago.
2768.02c.patch (1.0 KB) - added by boonebgorges 10 years ago.

Download all attachments as: .zip

Change History (22)

#1 @boonebgorges
14 years ago

I disagree that users will necessarily equate activity comments with blog comments, though I like your proclivity for parsimony :)

Suggestion: bp_activity_can_comment(), before returning true or false, checks to see whether the WP threaded comments box is checked. If so, then it returns true. bp_activity_can_comment_reply(), in turn, will check the value of $comment->mptt_left against the max depth set on the WP discussion panel, and returns true or false accordingly.

The advantage of this method is that activity depth will mirror blog comment depth by default, but will be filterable.

#2 @DJPaul
14 years ago

Good idea, that's probably the best approach if we were to patch this. I'm interested to hear any feedback from others.

#3 @DJPaul
13 years ago

  • Owner set to DJPaul
  • Status changed from new to assigned

13 years ago

#4 @DJPaul
13 years ago

This is not as trivial as it seems. We can't simply count() everything at the start of the activity stream item loop, or use bp_activity_get_comment_count(), as those will look down the entire depth of any set of items. We need to count down each level of the tree to see if any particular set has a number of items higher than the threaded comments limit. I took the approach of passing a counter variable through the process.

This needs testing, please. I think it works ok. Some remaining issues:

1) When submitting a reply via AJAX, if that reply would cause you to equal the threaded comments limit, the returned HTML still has the "reply" link. Probably something to do with the AJAX activity stream loop requesting a specific item rather than starting from the root item.

2) RE: 1, if the user hits reply on the AJAX, it will get posted, ignoring the threaded comments limit. I suggest we need to amend the "add activity stream to DB" function and for "activity_comment" items', check the depth. If it is at or has passed the threaded comment limit, we'll bump it back to a root level reply -- I'm sure we can fiddle the ajax return so that it is inserted visually in the correct place, too.

#5 @DJPaul
13 years ago

  • Keywords needs-testing added

#6 @DJPaul
13 years ago

  • Keywords needs-testing removed
  • Milestone changed from 1.3 to Future Release
  • Owner DJPaul deleted


#7 @boonebgorges
10 years ago

  • Keywords has-patch added
  • Milestone changed from Future Release to 2.0
  • Severity set to normal

Marked #5321 as a duplicate.

Since we've got a patch, and there's demand for the feature, let's look at it for 2.0.

#8 @Zachary DuBois
10 years ago

  • Cc zacharydubois@… added

Thanks @boonebgorges

#9 @boonebgorges
10 years ago

  • Keywords needs-testing added

I've reworked DJPaul's patch a bit.

Instead of passing a value around in the templates (unappealing now that our comment markup is generated by a real WP template), I've opted to build the depth index when the comments are fetched. To make the AJAX play nicely, and because of the somewhat odd way we fake out the global when building the markup for AJAX-inserted comment, I had to manually build the depth in the AJAX callback as well.

Ready for testing and feedback.

#10 @DJPaul
10 years ago

I'll test this patch next week, but it looks good.

#11 @boonebgorges
10 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from assigned to closed

In 8201:

Use WP's comment depth setting to limit activity comment depth

Fixes #2768

#12 @r-a-y
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#13 @r-a-y
10 years ago

  • Keywords needs-testing removed

Came across a loop of debug notices when on my own profile (eg. that was timing out my server.

PHP Notice: Undefined index: in wp-content\plugins\buddypress\bp-activity\bp-activity-classes.php on line 1016

I've attached a patch that I believe fixes this. Could use a onceover.

10 years ago

#14 @boonebgorges
10 years ago

Good catch, r-a-y. This only happens when the parent is not retrieved in the current loop, which would only happen in certain instances when display_comments=stream and the parent is out of scope. I've managed to reproduce it.

Your fix will essentially short-circuit the depth calculation in these sorts of situations, by forcing the depth to be set to 1. This prevents the infinite recursion, and doesn't really hurt anything in the interface, since we don't allow comments-on-comments when display_comments=thread. But it does result in an incorrect value for 'depth', which may be misleading down the road.

2768.02c.patch modifies your patch to fetch the direct parent when it's not found in the already-queried items. In some cases, this will result in additional query overhead. But it will also guarantee correct depth calculations, which IMO is a fair tradeoff for this fairly unusual circumstance. Could you give it a quick sanity check?

#15 @r-a-y
10 years ago

  • Keywords commit added

I think 02c.patch works. We could open up activity comments-on-comments in the future, so it's better to be future-proof :)

#16 @boonebgorges
10 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 8226:

Better checks for existence of parent item when recursing to calculate activity comment depth

The changes introduced in r8201 involve calculating tree depth for each queried
activity comment in a loop. The technique used involves traversing up a tree by
fetching the parent item until the root level is reached. However, the original
technique did not account for the fact that in certain cases - namely, when
display_comments=threaded - it may not be the case that all of the comment
ancestors are available in the current query. This failed logic resulted in
infinite regression in these sorts of cases.

The correction involves performing a separate query for the direct activity
parent when it is not found in the queried items.

Props r-a-y

Fixes #2768

This ticket was mentioned in IRC in #buddypress-dev by boonebgorges. View the logs.

10 years ago

#18 @boonebgorges
10 years ago

In 8402:

Disable activity reply threading if WP comment threading is disabled

In r8201, BP adopted WP's thread_comments_depth setting for its own activity
reply threading. See #2768. However, the thread_comments toggle was not
respected. The current changeset adds support for thread_comments, so that
disabling WP comment threading will also enable nested activity comments
(though the top level of comments will still be allowed).

Fixes #5562

Note: See TracTickets for help on using tickets.