comment_render contains hardcoded HTML

It'd be nice if we could move that to theme_comment_view. Looks difficult.

First attempt: I’ve moved the code to theme_comment_view() by adding an arg for the depth. Seems to work exactly as before.

Is this what you were expecting? It makes theme_comment_view() quite complex. I haven’t quite got my head around hooks etc., but I assume someone would have to cut & paste this into their template.php in order to customise it.

Would like to use a recursive function to add opening and closing tags ’fore & aft’ in the same call, eliminating the need for $depth.

Am I on the right track – your thoughts and comments(LOL) would be appreciated.

Changing the API by adding the depth is definitely d7 area and not needed. You should be able to work through $comment->depth, It's already in there. No need to pass it off as an extra parameter.

Yes, I thought that might be a sticking point, so I was careful to give it defaults making it an optional parameter, in such a way that all existing calls would still work exactly as before. I admit, it does change the API, but out of interest, is this true?

$depth is different to $comment->depth being the change required. Can I instead add a static variable to theme_comment_view, to keep track?

Using a static variable: much neater. Uses $comment->cid = NULL as flag to close all <div> tags.

Nice work, the only problem is that your calling theme('comment_view'). There are a bunch of other instances of theme('comment_[flat|threaded]_[collapsed|expanded]') that takes care of calling theme('comment_view').

*->cid & *->depth should also not be reset.

Although it works, it doesn't flow as expected making debugging harder later.

I am calling theme('comment_view'). It’s difficult to tell from the patch, but at that point, execution has just fallen out of the while loop.

The other instances are used to output the actual comments themselves; the theme('comment_view') is purely to close the <div>’s. I have set $comment->cid & $comment->depth explicitly to make it clear that this call does not involve a comment.

I’ve realised that db_fetch_object() hasn’t returned a null object as I thought, but has set $comment to FALSE, so isn’t even an object at this point. Setting $comment->cid & $comment->depth explicitly, makes it into an object again so that it can be an arg to theme('comment_view'). I’ve changed the name to make this obvious, and avoid confusion with $comment.

I haven’t used one of the other instances, as they don’t make a distinction between Threaded and Flat, so I have to call _comment_get_display_setting(): is this expensive enough to justify putting in a static variable?

Hope this is more sensible as I’m not sure what you mean by it doesn't flow as expected. The flow, as I understand it, is exactly the same as before. The chunk of code before the call to theme('comment_[flat|threaded]_[collapsed|expanded]') has moved to the beginning of theme('comment_view').

I would like to change

        if ($mode == COMMENT_MODE_FLAT_COLLAPSED) {
          $comments .= theme('comment_flat_collapsed', $comment, $node);
        }
        else if ($mode == COMMENT_MODE_FLAT_EXPANDED) {
          $comments .= theme('comment_flat_expanded', $comment, $node);
        }
        else if ($mode == COMMENT_MODE_THREADED_COLLAPSED) {
          $comments .= theme('comment_thread_collapsed', $comment, $node);
        }
        else if ($mode == COMMENT_MODE_THREADED_EXPANDED) {
          $comments .= theme('comment_thread_expanded', $comment, $node);
        }
 

to

        $modes[COMMENT_MODE_FLAT_COLLAPSED]     = 'comment_flat_collapsed';
        $modes[COMMENT_MODE_FLAT_EXPANDED]      = 'comment_flat_expanded';
        $modes[COMMENT_MODE_THREADED_COLLAPSED] = 'comment_thread_collapsed';
        $modes[COMMENT_MODE_THREADED_EXPANDED]  = 'comment_thread_expanded';
.
.
.
        global $modes;

        $comments .= theme($modes[$mode], $comment, $node);
 

Does that gain anything except readability?

forgot the slightly changed patch.

The other instances are used to output the actual comments themselves; the theme('comment_view') is purely to close the

’s. I have set $comment->cid & $comment->depth explicitly to make it clear that this call does not involve a comment.

That's the problem. theme_comment_view should be consistent on what is returned. Calling it a second time to add the closing div's is a bit ugly and can cause confusion for anyone trying to override that function.

There is still a way to nicely close the div's outside your technique this but it is tricky. theme_comment_view needs to know what and where it is. Specifically, the depth of the comment for threaded views and the last one being rendered on the page. I'll try to work on it a little later but if you find a solution, feel free and put up another patch.

I wanted there to be only one place that specified the string '</div>', and I thought that it should be in theme_comment_view() (with the opening '<div >'), so that it could be themed.

Do you mean putting these in a (themeable?) ‘helper’ function, so that the comments can be themed separately from the markup designed to indent?

I’m slowly getting my head around hooks, callbacks, naming .tpl.php files etc.

zeta, right.. It should be in one place and this patch does just that without reusing the same function just to close the div's on the second run. That was the only problem I saw with it. It should run once and produce consistent output.

I tested in threaded collapsed/expanded views with pagers and every other odd combination. The markup is output consistently.

The information we needed was the *last comment* so anything beyond 1 or more levels in depth gets closed properly. I had to use the global pager variables with some simple math. The alternative to getting the last comment would be to pull in into two loops. First gathering all the comments then doing count() then re-looping to render the theme functions.

@Gabor or Dries, this doesn't change the API. It might affect themers who took over this function but it's so rare that I think it's worth getting in.

Subscribe.

For D7 it seems like perhaps we need a $op of 'list' for hook_comment to build a structured array of comment objects with theme_comment_list where there could be an attribute to override site-wide settings so a module developer could have a module that uses collapsed threaded comments in one place while the site is set with expanded flat comments in another... Or modularly to control comments on blog posts one way and forums & containers differently instead of being so difficult to override.

I was happy with the extra call because all previous indents are closed off before the next comment is output. In tci_II.patch, the final closing off is done before a non-existent comment, using the same code. In tci_III.patch, the final closing off is done after the final comment as a special case. Thus you have the closing tag required in two places (though in the same function) when they always need to be identical. And the overriding function has to handle this special case too, in the same way. With tci_II.patch, the overriding function merely needs to not output a non-existent comment.

Would str_repeat('</div>', divs) be useful for the closing tags?

I had completely neglected the issue of paging comments (I wasn’t aware it is possible), so no surprises that tci_II.patch doesn’t close off correctly.

I have discovered a wrinkle in paging – see screen-shots. The first comment on page is never indented more than 1 level no matter how deep it is. When its parent is bumped onto the page with it, its indentation changes. Not a big wrinkle! Should this be another issue?

Are you aware that $i and ∴ $comment->position are effectively 1-based? If you initialised $i to $comment_count and decremented it, you wouldn’t have to check $i != $comment_count for every comment, and the last one would be 0 ($comment->position would have to be $comment->rev_position).

We don’t need $num_rows anymore if we have $comment_count (it wasn’t the number of rows anyway).

A structured array of comment objects would make this a lot easier too, with recursion keeping track of how deeply indented we are. The indenting tags could simply be in a wrapped around the comments. Paging might be more difficult, but not impossible.

Uh.. Want to review the patch I posted? It resolves all those issues. The points JBrauer brought up should be reserved for D7.

Forgot to check for depth which doesn't exist when viewing single comments and previews. Same as III otherwise.

Re-tested _III & _IIII: still seeing all those issues here.

NB screen shots are from 2^nd page. Titles reflect what indenting should be, as seen on *_IV.

Hrm.. That existed previously and maintaining the indents across pages is not necessary. The indents are good for showing the relative associations on the *same page*.

edit: Well, I'll just state that the indenting behavior should behave *exactly* as it did before. I'm not sure how you got that result zeta. I have no time to waste here so I'll leave it at that. Cheers. :)

But what about using comment_render just to create the (paged) array of comments and pass the data to a theme_comment_list_view function?

We can move there the "logic of the presentation":

          if ($comment->depth > $divs) {
            $divs++;
            $comments .= '<div class="indented depth-' . $comment->depth . '">';
          }
          else {
            while ($comment->depth < $divs) {
              $divs--;
              $comments .= '</div>';
            }
          }

I need to change the way comment are presented: I need to nest from zero level, i.e. collapsing all a comment thread, not only replies, and currently my only option is to patch the comment_render itself (bad bad bad).

I'll include a patch as soon.

EmanueleQuinto, if we were early in the 6.x-dev cycle but now we're in 7.

Looks like we'll have to live with this for another release.

EmanueleQuinto, I tried replying to your email but got an error so I'll do it here:

Hi EmanueleQuinto, if you can provide a patch that doesn't alter the API in any significant way, then great. Provide a patch and I'll review. I thought your approach would do so and that's why I moved it into the 7 queue. I could be totally off base.

If your approach does dig too deep into Drupal 7 territory, then if you can, it would be great if you reviewed the patch I put out. And mark it ready –if you think it is.

http://drupal.org/node/162747#comment-716039

Either way, as long as it gets fixed, I'll be happy. It just cannot be a huge change or Gábor will reject it.

thanks,
-joon

And here is the patch.

The main reason to add a theme_comment_list is because it could be necessary to add a div to the first level.

With this patch all comments (replies are already enclosed currently with a class "indented") are enclosed in a div with a class "depth-n" (added to "indented" on replies).

ema

Sorry, I don't follow. Add div to the first level?

I don't see how this approach is any better. Even for 7, it doesn't make it easier. Zeta had a good start, and I think I finished it at #16 which behaves just like before and could have made it into 6. I doubt it now.

The approcah I suggest allows to differentiate between 2 presentation step needed to create a commented list: one the list itself (and the way elements are rendered) and items (comments).

I guess should be better to separate those 2 steps: if I need to change something in the list I wouldn't touch the item presentation. Putting all in one theme function (i.e. checking if is the firt or last element, which level...) is not really helpfull if you need to change just one aspect.

Of course I know that adding div to first level is just something I need in a project, it's not an issue here and maybe none will have to face this situation, but in principle could something needed and separating list and item rendering should solve this problem.

This is a straight update of the patch in #22 to the current version of HEAD.

Though with the new cycle we should make sure it's the best approach that we follow.

the problem with the patch at #16 is that theme_comment should probably be allowed to not indent, for things like Views 2 or comment blocks. it should ONLY display the comment requested, as there may be other things than just comment_render that wants to print a comment.

+1 to #25

re-reading the thread, it looks like maybe you addressed that by examining $comment->depth. still it seems a bit overly complex and prone to breakage if overridden. #25 appeals to my aesthetics.

+1 on applying this to 7.x-dev.

To make even more use of this new comments-structure, could we add a hook in just prior to:

      $output .= theme('comment_list', $comments, $node);

?

See the attached patch (modified version of the last patch above).

This allows other modules to inject dynamically-generated comments:

function example_comment_prerender($node, $mode, &$comments) {
	$newcomment = new StdClass();
	$newcomment->subject = 'Dynamic Comment!';
	$newcomment->comment = 'This is a Dynamically-generated Comment.';
	$newcomment->depth = 0;
	$newcomment->cid = -1;
	$newcomment->nid = $node->nid;
	$newcomment->format = FILTER_FORMAT_DEFAULT;
	$newcomment->uid = 0;
	$newcomment->name = '';
	$newcomment->timestamp = time() - 86400;
	$newcomment->status = COMMENT_PUBLISHED;

	array_splice($comments, 0, 0, array($newcomment));
}

...or to modify existing comments.

the divs are still there but now this is easily changed in hook_page_alter(). i think that satisfies most of the pain.