Hello!

Right now, comment_alter_comment_view() is using the Diff module to generate the table at the top of comments, which shows the differences between the previous revision and the revision created by the given comment.

This should be customizable in some way! Although, I'm not entirely sure how yet.

Maybe we just move the diff code into a theme function which takes old_vid and new_vid? Then the themer could override the way this table is generated.

However, something for site builders who don't want to write PHP in the theme would be nice! Maybe we could list a few options/modes when editing the comment settings for the node?

Personally, I don't like the way the Diff module outputs the difference - I wouldn't feel comfortable giving that to my clients. I'd prefer something more like what casetracker, project_issue or comment_driven output:

comment_driven:

Selection_076.png

project_issue:

Selection_077.png

So, maybe we can have a couple selectable modes:

  1. A "basic" mode, which puts together an array of the difference and passes it to a theme function.
  2. If the Diff module is available, a mode which does what comment_alter_comment_view() is doing currently.

I'll keep thinking about this and post a patch if I come up with something!

Regards,
David.

CommentFileSizeAuthor
#2 comment_alter-customize-diff-1973082.patch4.63 KBdsnopek
#2 Selection_078.png20.56 KBdsnopek
#1 Selection_077.png7.58 KBdsnopek
Selection_076.png3.14 KBdsnopek

Comments

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
StatusFileSize
new Selection_077.png7.58 KB

Here's how project_issue on Drupal.org looks:

Selection_077.png

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Status: Active » Needs review
StatusFileSize
new Selection_078.png20.56 KB
new comment_alter-customize-diff-1973082.patch4.63 KB

Ok, I've created a patch that doesn't give any options and it still depends on the Diff module. BUT it outputs a table that looks much more similar to comment_driven, casetracker and project_issue.

The patch is attached!

Here is a screenshot from Bartik:

Selection_078.png

Please let me know what you think!

Personally, I think this is a much better default...

boobaa’s picture

boobaa
he/him
commented
Status: Needs review » Needs work

Well, I don't say I'm satisfied with diff.module's default output, but:

  1. comment_alter_comment_view() is mostly a stripped-down version of diff_diffs_show(). If diff.module had such an API call that allows passing two node objects (most likely two different revisions of the same node), I would have stuck with it. In other words: it would be better if diff.module would be more generic, but anyway, we can go on with this implementation for the time being.
  2. Diff.module is pluggable, which means that if it displays "No visible changes", then it means there's no plugin yet for that particular field type, but not a bug in comment_alter nor diff. I don't want to reinvent the wheel: if there's a pluggable mechanism for displaying diffs between nodes (revisions), I'm all in to use it.
  3. Diff.module's output _IS_ themeable, but I must admit it's not the easiest thing to do so. Again: if there's already something that's 1. used by quite a many sites like diff.module, 2. is pluggable, 3. is themeable, then I don't want to reinvent the wheel. OTOH, if extra fields (like comment_altered ones on comment entity's bundles) would support having different display settings, I would be all in to provide some different display modes for the comment_altered fields. I don't know if this is possible or not, but I'm against hardcoding any non-generic solutions.

Your patch fails at 2nd and 3rd points: it does comparison by itself (instead of relying on diff.module's pluggable mechanism), and it hardcodes a specific display solution (that's even less generic than diff.module's one). However, your basic idea is good, but needs a more generic thinking, eg. support for selectable different display modes for comment_altered extra fields (if it's possible at all, which I don't know yet).

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Status: Needs work » Needs review

All good points, thanks!

Yeah, this patch was just experimenting with outputting something more similar to what users are used to. I really do *want* to implement a generic or pluggable solution (as I described in earlier comments). This was just an initial attempt at a slightly better default. :-)

To directly answer your points:

Points #1 and #2: My patch is still using the Diff.module! It's not doing the comparison itself - it's calling diff_compare_entities(), which is more or less an "API call that allows passing two node objects." ;-) Underneath it calls hook_entity_diff() to build up the return value that I'm then putting into a table.

Point #3: Yes, the Diff.module output is themable globally. But with the current code, you can't theme *just* it's output by comment_alter without also effecting the Diff.module's output on the "Revisions" tab, for example. In my patch, it gives #theme => "table__comment_alter__diff" which would allow you to theme just this table (unlike the existing code which passes #theme => "table__diff__standard").

So, even though my patch isn't perfect and the actual output should probably be more customizable, I don't think it's any worse than the existing implementation (it still uses Diff.module) but it's a little more themeable and little closer to what users expect.

Could you please take another look at the patch?

Thanks again for your review!
David.

boobaa’s picture

boobaa
he/him
commented
Status: Needs review » Fixed

I have considered your patch and reused its best; eg. diff_compare_entities() is being used by now, and theming is much more generic _and_ replaceable. Latest changes have been pushed to git.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented

This looks really awesome! Yeah, I agree this is the right way to do it: generating the table in a theme function so someone can replace that entirely. Thanks so much!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Included another example image.