Moodle
  1. Moodle
  2. MDL-33206

Add a link to display the print dialog in the print book tool

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3, 2.4
    • Fix Version/s: 2.5
    • Component/s: Book
    • Testing Instructions:
      Hide

      The following has to be tested on all supported browsers

      • Create a book resource in a course and add at least one chapter
      • In the book administration, click on "Print book".
      • In the top left of the print book page, click on the link "Print book"
        You should see the print dialog box again
      • In the book administration, click on "Print this chapter".
      • In the top left of the print chapter page, click on the link "Print this chapter"
        You should see the print dialog box again
      Show
      The following has to be tested on all supported browsers Create a book resource in a course and add at least one chapter In the book administration, click on "Print book". In the top left of the print book page, click on the link "Print book" You should see the print dialog box again In the book administration, click on "Print this chapter". In the top left of the print chapter page, click on the link "Print this chapter" You should see the print dialog box again
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-33206-master
    • Rank:
      41120

      Description

      From https://github.com/skodak/moodle-mod_book/issues/22

      Users get confused when printing book/chapter – the window shows content to be printed, but there's no "print" button.

      Yes, Ctrl+P helps, but a lot of users don't know that.

      I suggest adding javascript print dialog opening in the end of HTML output of print page:

      diff --git a/tool/print/index.php b/tool/print/index.php
      index b1949dd..d616eca 100644
      --- a/tool/print/index.php
      +++ b/tool/print/index.php
      @@ -111,6 +111,8 @@ if ($chapter) {
           $chaptertext = file_rewrite_pluginfile_urls($chapter->content, 'pluginfile.php', $context->id, 'mod_book', 'chapter', $chapter->id);
           echo format_text($chaptertext, $chapter->contentformat, array('noclean'=>true, 'context'=>$context));
           echo '</div>';
      +
      +    echo '<script type="text/javascript">window.print();</script>';
           echo '</body> </html>';
      
       } else {
      @@ -172,6 +174,8 @@ if ($chapter) {
               echo '</div>';
               //echo '<a href="#toc">'.$strtop.'</a>';
           }
      +
      +    echo '<script type="text/javascript">window.print();</script>';
           echo '</body> </html>';
       }
      

        Issue Links

          Activity

          Hide
          Gilles-Philippe Leblanc added a comment - - edited

          I would add more code that displays the dialog box to print a link "Print" in case the pop-pup does not work or that the user would like to see the html content to be printed before printing it.

          Also, this problem applies at least to the glossary and wiki modules.

          Show
          Gilles-Philippe Leblanc added a comment - - edited I would add more code that displays the dialog box to print a link "Print" in case the pop-pup does not work or that the user would like to see the html content to be printed before printing it. Also, this problem applies at least to the glossary and wiki modules.
          Hide
          Jean-Philippe Gaudreau added a comment -

          Patches have been submitted for 2.3 and master branches.

          Improvement includes :

          • Auto-display of print dialog
          • Link button in the page to prompt the print diablog
          Show
          Jean-Philippe Gaudreau added a comment - Patches have been submitted for 2.3 and master branches. Improvement includes : Auto-display of print dialog Link button in the page to prompt the print diablog
          Hide
          Frédéric Massart added a comment -

          Hi Jean-Philippe, thanks for working on this issue. Your patch looks good, I'd just remove the 'javascript:' from the onclick statement and add a 'return false;' so that the link is not triggered.

          Let me know when your branch is updated and I'll push it for integration.

          Cheers!

          Show
          Frédéric Massart added a comment - Hi Jean-Philippe, thanks for working on this issue. Your patch looks good, I'd just remove the 'javascript:' from the onclick statement and add a 'return false;' so that the link is not triggered. Let me know when your branch is updated and I'll push it for integration. Cheers!
          Hide
          Jean-Philippe Gaudreau added a comment -

          Hi Frédéric, branches are updated.

          thx!

          Show
          Jean-Philippe Gaudreau added a comment - Hi Frédéric, branches are updated. thx!
          Hide
          Frédéric Massart added a comment -

          Thanks Jean-Philippe. Pushing for integration now.

          Show
          Frédéric Massart added a comment - Thanks Jean-Philippe. Pushing for integration now.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Jean-Philippe Gaudreau added a comment -

          Branches rebased!

          Show
          Jean-Philippe Gaudreau added a comment - Branches rebased!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Personally I hate auto-print-dialog html pages.

          Although that does not matter, really, it's only a personal POV.

          But the important point is if we want to open the door to those auto-print-dialog pages within Moodle or no. From memory, I don't remember any place where we are allowing that (and we have certainly other "printable" pages in Moodle, but never have put them being "auto-printable" way.

          So, I'm going to reopen this to be considered @ HQ (MD for sure, ping him about it). And if it serves for something, my vote is -1 to this (and any other auto-print-window) to land.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Personally I hate auto-print-dialog html pages. Although that does not matter, really, it's only a personal POV. But the important point is if we want to open the door to those auto-print-dialog pages within Moodle or no. From memory, I don't remember any place where we are allowing that (and we have certainly other "printable" pages in Moodle, but never have put them being "auto-printable" way. So, I'm going to reopen this to be considered @ HQ (MD for sure, ping him about it). And if it serves for something, my vote is -1 to this (and any other auto-print-window) to land. Ciao
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Jean-Philippe Gaudreau added a comment -

          Hi Eloy.

          heemmm... What you say sounds a little wierd to me since you created the task with the initial patch But in another way, everyone can change his mind hehe!

          Ok so I'll wait for feedback from HQ. And what about the button in the print page? Is it still a good idea? We could leave only this option and remove the auto-display.

          Cheers!

          Show
          Jean-Philippe Gaudreau added a comment - Hi Eloy. heemmm... What you say sounds a little wierd to me since you created the task with the initial patch But in another way, everyone can change his mind hehe! Ok so I'll wait for feedback from HQ. And what about the button in the print page? Is it still a good idea? We could leave only this option and remove the auto-display. Cheers!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hehe, Jean-Philippe,

          I just created this issue as part of the adoption/integration of book into core, so we moved all the requests/issues present from github to the Tracker. There was no "desire" in such a move at all, hehe, just the move to have everything here.

          So let's decide it now... perhaps ppl thinks it's better to start adding such auto-print-window buttons/links, who knows. Personally I don't like them.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hehe, Jean-Philippe, I just created this issue as part of the adoption/integration of book into core, so we moved all the requests/issues present from github to the Tracker. There was no "desire" in such a move at all, hehe, just the move to have everything here. So let's decide it now... perhaps ppl thinks it's better to start adding such auto-print-window buttons/links, who knows. Personally I don't like them. Ciao
          Hide
          Jean-Philippe Gaudreau added a comment -

          Haha ok I see Well let's keep in touch to know if we continue or close this task.

          In my opinion I think there should be at least a print button in the print page for user-friendly (ergonimic) purpose.

          Show
          Jean-Philippe Gaudreau added a comment - Haha ok I see Well let's keep in touch to know if we continue or close this task. In my opinion I think there should be at least a print button in the print page for user-friendly (ergonimic) purpose.
          Hide
          Frédéric Massart added a comment -

          My +1 for

          • No automatic print
          • Print button added to the page
          Show
          Frédéric Massart added a comment - My +1 for No automatic print Print button added to the page
          Hide
          Mart Mangus added a comment -

          Hei! I understand the other view, but

          my +1 for automatic print dialog display

          – what's the point of opening the print view, if there's no goal to print it?

          Show
          Mart Mangus added a comment - Hei! I understand the other view, but my +1 for automatic print dialog display – what's the point of opening the print view, if there's no goal to print it?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          – what's the point of opening the print view, if there's no goal to print it?

          Save paper? Sorry, couldn't resist, lol!

          Show
          Eloy Lafuente (stronk7) added a comment - – what's the point of opening the print view, if there's no goal to print it? Save paper? Sorry, couldn't resist, lol!
          Hide
          Jean-Philippe Gaudreau added a comment -

          Negative, you can also print to pdf or other portable formats

          Show
          Jean-Philippe Gaudreau added a comment - Negative, you can also print to pdf or other portable formats
          Hide
          Jean-Philippe Gaudreau added a comment -

          I have some spare time and decided to give this issue another try As I can see from the comments above, can we agree on this ?

          • Change the patch to only add the print button on the page.

          I'll submit a new patch from the current master.

          Show
          Jean-Philippe Gaudreau added a comment - I have some spare time and decided to give this issue another try As I can see from the comments above, can we agree on this ? Change the patch to only add the print button on the page . I'll submit a new patch from the current master.
          Hide
          Jean-Philippe Gaudreau added a comment -

          New patch submitted for master branch with minor improvements. I had to change DOCTYPE to make it work on latest versions of IE... (9 and 10)

          Show
          Jean-Philippe Gaudreau added a comment - New patch submitted for master branch with minor improvements. I had to change DOCTYPE to make it work on latest versions of IE... (9 and 10)
          Hide
          Frédéric Massart added a comment -

          Hi Jean-Philippe,

          Thanks for your patch, I think the discussions above lead to the right decision. I just have a few minor comments:

          1. If you go through our coding style, you will notice that we try (especially for new lines) to avoid lines longer than 120 characters.
          2. I'd suggest you to use the class icon for the pix_icon as it was re-thought in 2.4 and would be use to determine the size of icons site-wide, so you don't have to specify 16x16 in .your own CSS rules.
          3. I'd also recommend using icon-pre (introduced in 2.4) as your icon is positioned before some text and will need padding. The benefit of using this combination of classes is that if you swtich to Right-to-Left, the paddings will be inverted automatically.
          4. You code makes me think that we could add general @media print rule to hide elements. Not sure how to properly do that though, so we can leave that like that for now.

          We could probably backport this, although it's not required. If you do so, please check the availability of the classes mentioned above.

          Many thanks,
          Fred

          Show
          Frédéric Massart added a comment - Hi Jean-Philippe, Thanks for your patch, I think the discussions above lead to the right decision. I just have a few minor comments: If you go through our coding style, you will notice that we try (especially for new lines) to avoid lines longer than 120 characters. I'd suggest you to use the class icon for the pix_icon as it was re-thought in 2.4 and would be use to determine the size of icons site-wide, so you don't have to specify 16x16 in .your own CSS rules. I'd also recommend using icon-pre (introduced in 2.4) as your icon is positioned before some text and will need padding. The benefit of using this combination of classes is that if you swtich to Right-to-Left, the paddings will be inverted automatically. You code makes me think that we could add general @media print rule to hide elements. Not sure how to properly do that though, so we can leave that like that for now. We could probably backport this, although it's not required. If you do so, please check the availability of the classes mentioned above. Many thanks, Fred
          Hide
          Jean-Philippe Gaudreau added a comment - - edited

          Hi Frédéric,

          Thx for the review. I've just submitted a new patch with your feedbacks :

          1. I've changed the code for my lines to be less than 120 characters. It would be a good idea to also change the documentation (see http://docs.moodle.org/dev/Coding_style#Maximum_Line_Length) because I though 132 was recommended and less than 180 acceptable...
          2. Done. Using the icon class
          3. Done. Also using the icon-pre class
          4. Yes I think it would be a great idea to set some "@media print" classes in Moodle core if it's to be used elsewhere. Maybe create a new general issue on that matter?

          There's something I'm not sure about this patch... I had to explicitly link "theme/base/style/core.css" to add the icon and icon-pre classes. Do you think of a better way of doing it? Long term, I think index.php should be rewrite using Moodle API to generate the output...

          I'll backport the patch when we find an agreement for the above problem.

          J-P

          Show
          Jean-Philippe Gaudreau added a comment - - edited Hi Frédéric, Thx for the review. I've just submitted a new patch with your feedbacks : I've changed the code for my lines to be less than 120 characters. It would be a good idea to also change the documentation (see http://docs.moodle.org/dev/Coding_style#Maximum_Line_Length ) because I though 132 was recommended and less than 180 acceptable... Done. Using the icon class Done. Also using the icon-pre class Yes I think it would be a great idea to set some "@media print" classes in Moodle core if it's to be used elsewhere. Maybe create a new general issue on that matter? There's something I'm not sure about this patch... I had to explicitly link "theme/base/style/core.css" to add the icon and icon-pre classes. Do you think of a better way of doing it? Long term, I think index.php should be rewrite using Moodle API to generate the output... I'll backport the patch when we find an agreement for the above problem. J-P
          Hide
          Frédéric Massart added a comment -

          Hi Jean-Philippe,

          I must have had my mind somewhere while reviewing your patch, sorry! You are right, the line length should be 132. I don't know why but I've been coding with 120 for 8 months now... no idea where I read that!

          This idea of using icon and icon-pre didn't make sense. As you noticed you had to link to the file manually, and the body is generated itself so there is not even a gain for RTL users. I'd suggest to revert to your previous patch, manually setting padding/size on the icon.

          Yes, in the future it'd be good to have print CSS files, and some better way to generate them as it is still done manually, at least in that case. Not sure what the approach should be.

          Please feel free to push your corrected patch for integration if you have the rights to do it, and if not let me know and I will.

          Désolé de t'avoir fait travailler pour rien .

          Cheers,

          Fred

          Show
          Frédéric Massart added a comment - Hi Jean-Philippe, I must have had my mind somewhere while reviewing your patch, sorry! You are right, the line length should be 132. I don't know why but I've been coding with 120 for 8 months now... no idea where I read that! This idea of using icon and icon-pre didn't make sense. As you noticed you had to link to the file manually, and the body is generated itself so there is not even a gain for RTL users. I'd suggest to revert to your previous patch, manually setting padding/size on the icon. Yes, in the future it'd be good to have print CSS files, and some better way to generate them as it is still done manually, at least in that case. Not sure what the approach should be. Please feel free to push your corrected patch for integration if you have the rights to do it, and if not let me know and I will. Désolé de t'avoir fait travailler pour rien . Cheers, Fred
          Hide
          Jean-Philippe Gaudreau added a comment -

          Hi Frédéric,

          Never mind for the waste of time it helped me improve the first patch! So I've re-submitted the patch with lines less than 132 characters and css classes for "book_print_icon" exactly the same as the combination of "icon" and "icon-pre". I also did the backport for 2.3 and 2.4 branches.

          Hope everything is fine! unfortunately, I can't push for integration so i'm putting it back to peer review.

          Merci et à la prochaine!

          Show
          Jean-Philippe Gaudreau added a comment - Hi Frédéric, Never mind for the waste of time it helped me improve the first patch! So I've re-submitted the patch with lines less than 132 characters and css classes for "book_print_icon" exactly the same as the combination of "icon" and "icon-pre". I also did the backport for 2.3 and 2.4 branches. Hope everything is fine! unfortunately, I can't push for integration so i'm putting it back to peer review. Merci et à la prochaine!
          Hide
          Frédéric Massart added a comment -

          Thanks! Pushing for integration.

          Show
          Frédéric Massart added a comment - Thanks! Pushing for integration.
          Hide
          Dan Poltawski added a comment -

          I've integrated this now, thanks Jean-Philippe!

          As a new feature this has only been integrated into master.

          Show
          Dan Poltawski added a comment - I've integrated this now, thanks Jean-Philippe! As a new feature this has only been integrated into master.
          Hide
          Rajesh Taneja added a comment -

          Thanks for fixing this Jean-Philippe,

          I can see the print link and clicking that opens print dialog.

          Show
          Rajesh Taneja added a comment - Thanks for fixing this Jean-Philippe, I can see the print link and clicking that opens print dialog.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks!

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks! Closing as fixed, ciao
          Hide
          Mary Cooch added a comment -

          Removing docs_required label as have added an updated screenshot to http://docs.moodle.org/25/en/Book_settings

          Show
          Mary Cooch added a comment - Removing docs_required label as have added an updated screenshot to http://docs.moodle.org/25/en/Book_settings

            People

            • Votes:
              2 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: