- several php warnings fixed..
- niced up book page friendly link
- removal of comment links corrected, author link now gets stripped too
- added a 'comments' header in case someone archives the pdf this lets them distinguish comments from content more easily
now it works nicely on my book page http://hfopi.org/idea :D
only thing left is the order of the links on a book page, the printpdf link really should be next to the print link but the stats are not publicly shown on my site anyways so..
now back to studying :/

CommentFileSizeAuthor
#5 print-enhancements.patch5.31 KBeMPee584
#3 print-enhancements.patch4.15 KBeMPee584
print-enhancements.patch2.72 KBeMPee584

Comments

jcnventura’s picture

jcnventura
Primary language English
commented
Status: Needs review » Needs work

Thanks for the patch!

I will for sure commit the fix of the php warnings and the removal of the author's comment link.

The change to the book link is wrong, as it will not call the export/html path, resulting in the printout of the current page/chapter only. I don't think that you got the ideia behind the 'take control of the book module link'.. It's just that, take control but let the link as it was. What you created was actually something that would require another setting called 'Show normal node-only link in book pages' (or something of the sort).

The other 2 changes regarding comments (printing a title and changing the pattern to ul) will break correct behaviour now. The first will print a duplicate Comments header (look at http://www.venturas.org/6/print/2 where the current code is running without your patch). The second will start to produce invalid XHTML as the ending div will be removed, but not the starting tag. Also, in all my tests, the current pattern does the job nicely.. Why did you have to change it??

João

jcnventura’s picture

jcnventura
Primary language English
commented

Hi,

Having a look at your site, I think that the reason for the duplicate comments header and your need to change the links pattern is due to your local changes.. As such, these fixes should not go into the official module code.

João

eMPee584’s picture

eMPee584 commented
StatusFileSize
new print-enhancements.patch4.15 KB

yeah the pattern i forgot to change the second div to ul.. here's my current patches, about that book link i have almost not used books (only got one page) and thus you are probably correct. revert previous patch apply this one and try..

eMPee584’s picture

eMPee584 commented

and btw, i don't have any local changes just plain 6.x-dev comments module.. what about duplicate comments header, I don't see any? drupal never set a comment title iirc.. and with the book link think, revert this in that case I didn't know it creates loss of function I just disliked the cryptic path print/book/export/html/1 (why printpdf/1 though?) and second it does not print the comments in case of book/export/html.. anyways you may just revert this

-        $links[$module]['href'] = PRINT_PATH ."/". $link['href'];
+        $links[$module]['href'] = PRINT_PATH ."/". $node->nid;

but the rest should be ok

eMPee584’s picture

eMPee584 commented
StatusFileSize
new print-enhancements.patch5.31 KB

oh my daze mental shit
should test my code before i submit it
fixed the node type issue didn't see there already was a var named type
+ one more php warning
and the div.indented thing is for nested comments, i think padding is more sensible than margin..here you go last one for today bye :)

jcnventura’s picture

jcnventura
Primary language English
commented
Status: Needs work » Needs review

The link I sent before is a Drupal install of 6.1 + print module. Yours at least has a different theme. I suspect your theme is the one responsible for stripping out the Comments header.. Or something else is doing it..

The pdf link in book nodes is partly buggy :) It should also redirect to the export/html/nid path (for consistency).. Right now it prints the current page only. Actually the book module's lack of support for exporting a book with comments is what causes the comments to disappear..

I am applying your patch to my internal HEAD and will commit it soon..

João

jcnventura’s picture

jcnventura
Primary language English
commented
Status: Needs review » Postponed (maintainer needs more info)

Hello again.

I reviewed your patch and committed what I felt safe for now.

Some important points:
1. isset() is dangerous! Doing $false = false; $bool = isset($false); will result in $bool being true! This invalidates some of your suggestions, as I do care about their boolean value. I think !empty() might be a better choice, but I have to test it better.
2. The author pattern matching does not match on standard Drupal.. I have changed it to remove the author link in Garland, but it will definitively not apply to you anymore.
3. The CSS you suggested is superfluous to standard Drupal, so I removed it.

Last, the fact is that I never see these warnings (and I do have error reporting enabled). I have strong reasons to believe that you only see them because you're running a dev version of Drupal. See http://drupal.org/node/34341 on how to disable them, or upgrade to 6.1.

João

eMPee584’s picture

eMPee584 commented

Hi João,
what you refer to as 'standard drupal' is the Garland theme, which overrides the core template files with a lot of stuff.. just had a look at it and here:

template.php:
function phptemplate_comment_wrapper($content, $node) {
  if (!$content || $node->type == 'forum') {
    return '<div id="comments">'. $content .'</div>';
  }
  else {
    return '<div id="comments"><h2 class="comments">'. t('Comments') .'</h2>'. $content .'</div>';
  }
}

Without this override, core returns the commens as unordered list <ul class="comments"> without the header...
I have a CSS only theme so even if Garland is the default theme I'd consider an option in the settings 'Garland-derived theme' which defaults to (path_to_theme() == "/themes/garland").. Of course the indented CSS is only included in the Garland theme aswell. Again, same applies for the author pattern.
About the isset thing you're definitly right it has to be handled cautiously but I think I added in one place and replaced it in the other where it really should only be set if it is true.. better check for that newurl case..
About the book thing, imho 'print this page' should print exactly what is being displayed right now, not what the book module thinks should be printed. Maybe next week I'll add an option for that..
cya

eMPee584’s picture

eMPee584 commented

...another thing about that error display.. it is true I had it activated in my error reporting settings which defaults to log only, but any published module really shouldn't flood the log with php undefined foo warnings... so better turn it ON while developing this! After fixing all those warnings I switched it off now because it looks kinda unprofessional on a live site ;)

jcnventura’s picture

jcnventura
Primary language English
commented

Hi, zuriel.

I also came to the conclusion that the ul pattern is more general.. It will mean an empty div clause, but as long as it works better for everyone...

You didn't understand my comment on the warnings. My Drupal 6.1 site is also enabled with errors to the screen and to the log. It's just that it's not configured to log such low-level warnings at all. That log level is usually enabled only during Drupal development.

João

jcnventura’s picture

jcnventura
Primary language English
commented
Status: Postponed (maintainer needs more info) » Fixed

Hi,

I have committed the isset changes (in the end, it was the same as !empty), and the ul links to remove the comment links.

The only changes from your patch not committed are the book link changes which needs to be handled in another way, the author link change and the CSS. I need to investigate more if it's possible to have something working in Garland and other themes without drowning the code in special cases.

João

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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