#125494 Part 3: Table output through themes

Patch looks very nice. But it is incomplete since it leaves behind the now unused TableDiffFormatter in diffengine.php

Do we care? Is that class fundamental to the functionality from #198110? Seems like all it does is add a 1em width, and I find it hard to believe that fundamentally alters the behavior of the other patch.

that said, theme('table') is a mess. there's no point trying to fix it in D6, we're *way* past the point where such a far reaching API change would go in. there was talk on the devel list of big changes to theme('table') in D7, but no reason to hold up good patches in here for another 1+ years...

i say we just forget about the "diff-marker" class, lose the 1em width, and continue on.

hrm, interesting. thanks for the explanation, i've never been much of a CSS ninja...

in terms of what that means for this patch, i'm not sure. seems like theme('table') isn't flexible enough (once again) to do what we need, so it seems like we still can't use it. i'm not much of a theme ninja, either, so i don't know the best solution. ideas welcome.

This is great. This patch is getting pretty close to RTBC, and once it's in, there's only a little bit more goodness to harvest from http://drupal.org/node/125494 and we'll be in good shape to make progress on the rest of the issues in the queue.

So, here are my latest concerns about this patch:

A) Why are we making the caller responsible for sanitizing input we hand to contextLine() instead of just putting the check_plain() directly in that function? Seems safer that way than having a comment about it. Sure, there's only 1 call site now, but even then, it seems cleaner and needing less code to do the check_plain() "once" in the function itself.

[disclaimer] the rest of this isn't my strongest area of drupal knowledge [/disclaimer]

B) Seems a little evil to fork theme('table') like that, but perhaps that's our only option given the extra classes and such you're trying to add. I'd really like other thoughts on this before I'd be willing to commit the patch.

C) The DrupalDiffFormatter class looks like an improvement. However, it seems like it's still hard-coding a fair bit of HTML in its output formatting. If we really want to make this output themeable, don't we want to use theme functions directly inside this formatter class? For example:

  function _block_header( $xbeg, $xlen, $ybeg, $ylen ) {
    return array(
      array(
        'data' => '<strong>' . t('Line %nr', array('%nr' => $xbeg)) . '</strong>',
       ...

Could be something like:

   ...
        'data' => theme('diff_line_header', $xbeg),
   ...

and then, outside of the class entirely, just have a few of these:

function theme_diff_line_header($lineno) {
  return '<strong>'. t('Line %lineno', array('%lineno' => $lineno)) .'</strong>';
}

(Note also the code style for spacing around string concatenation and literals)...

If I wanted to alter the look of the diff output, and I was a designer, I'd much rather just tweak a few theme functions like this than have to understand and hack my own copy of the DrupalDiffFormatter class.

Thanks again!

This all looks good, except for that form of theme_table(). Have you tried to accomplish same by adding a class attribute to each cell? I know it isn't pretty, but we do this routinely. For example note sortable tables - every cell in the column which is the current 'sort by' has an 'active' class.

This page suggests that we could accomplish same by adding classes to each cell: http://www.htmldog.com/guides/htmladvanced/tables/

I think colgroup and col are useful and i would love if you submitted a patch to core for these. once accepted, we would happily use it here.

did you test with safari3? i have that available, if you could put up a test pge somewhere. if you tested with safari3 and it is still busted, then we can proceed with commit.

finally committed to HEAD and DRUPAL-5. thanks so much, moonray.