Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-43341

Black icon (using Chrome) with SCORM resources user complete report

    Details

    • Testing Instructions:
      Hide

      Using Chrome

      1. Create a course, enrol student, add a scorm package
      2. as a student view course
      3. as a teacher view this user (through Participants), view Activity reports > Complete Report.
      4. Make sure the TOC displays correctly (not with black boxes like shown in screenshot)
      Show
      Using Chrome Create a course, enrol student, add a scorm package as a student view course as a teacher view this user (through Participants), view Activity reports > Complete Report. Make sure the TOC displays correctly (not with black boxes like shown in screenshot)
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull Master Branch:

      Description

      Using Chrome browser, the "spacer" transparent icon (https://github.com/moodle/moodle/blob/master/mod/scorm/pix/spacer.gif) used in Activity reports / ► Complete report is displayed as a small black square under scorm resources in the user complete report

      Steps:
      1) upload a SCORM FILE
      2) click on Participants
      3) select a student
      4) click on Profile settings for that student
      5) click Activity reports/ ► Complete report

        Gliffy Diagrams

          Activity

          Hide
          nobelium vignesh p added a comment - - edited

          Hi, The problem was with the spacer.gif(https://github.com/moodle/moodle/blob/master/mod/scorm/pix/spacer.gif) image. I replaced it with a new image of the same dimensions and file type and its working fine now.

          https://github.com/nobelium/moodle/compare/master...MDL-43341

          Show
          nobelium vignesh p added a comment - - edited Hi, The problem was with the spacer.gif( https://github.com/moodle/moodle/blob/master/mod/scorm/pix/spacer.gif ) image. I replaced it with a new image of the same dimensions and file type and its working fine now. https://github.com/nobelium/moodle/compare/master...MDL-43341
          Hide
          nobelium vignesh p added a comment -

          Can anyone help me with creating a patch?

          Show
          nobelium vignesh p added a comment - Can anyone help me with creating a patch?
          Hide
          matteo Matteo Scaramuccia added a comment -

          Hi Vignesh,
          just seen your diff URL in Chrome and saw the black square vs the transparent spacer; creating a patch is quite a simple process if you are confident with git. Please, read these docs below:

          and the peer reviewer will act as describe below:

          Otherwise, I'll try to create the pull request on your behalf, maybe already tomorrow.

          HTH,
          Matteo

          Show
          matteo Matteo Scaramuccia added a comment - Hi Vignesh, just seen your diff URL in Chrome and saw the black square vs the transparent spacer ; creating a patch is quite a simple process if you are confident with git . Please, read these docs below: http://docs.moodle.org/dev/Git_for_developers#Preparing_a_patch http://docs.moodle.org/dev/Commit_cheat_sheet and the peer reviewer will act as describe below: http://docs.moodle.org/dev/Peer_reviewing_checklist Otherwise, I'll try to create the pull request on your behalf, maybe already tomorrow. HTH, Matteo
          Hide
          nobelium vignesh p added a comment -

          Thanks Matteo, The fix to this issue is on https://github.com/nobelium/moodle/tree/MDL-43341. Please take a look and do the needful.

          Show
          nobelium vignesh p added a comment - Thanks Matteo, The fix to this issue is on https://github.com/nobelium/moodle/tree/MDL-43341 . Please take a look and do the needful.
          Hide
          danmarsden Dan Marsden added a comment -

          Thanks Vignesh, see the links Matteo has posted above - particularly the commit cheat sheat related to the correct format of your commit messages. - we don't need a standalone patch file, a link to your github repo is great.

          A better solution here would be to convert the SCORM code to use the function $OUTPUT->spacer instead of maintaining a spacer image in the SCORM module. If you could do this and use the correct commit message format it would be great!

          Matteo FYI - there are a few potential GSOC students who will probably appear in the tracker over the next few weeks.

          Show
          danmarsden Dan Marsden added a comment - Thanks Vignesh, see the links Matteo has posted above - particularly the commit cheat sheat related to the correct format of your commit messages. - we don't need a standalone patch file, a link to your github repo is great. A better solution here would be to convert the SCORM code to use the function $OUTPUT->spacer instead of maintaining a spacer image in the SCORM module. If you could do this and use the correct commit message format it would be great! Matteo FYI - there are a few potential GSOC students who will probably appear in the tracker over the next few weeks.
          Hide
          nobelium vignesh p added a comment - - edited

          Hi Dan, I have changed the commit message as per the coding standards. Please take a look. https://github.com/nobelium/moodle/tree/MDL-43341

          With regard to the solution that you suggested, {$OUTPUT->spacer} function gets the image from the function

          {render_pic_icon}

          https://github.com/moodle/moodle/blob/master/lib/outputrenderers.php#L1964 the

          {pix_url}

          function reads the image path and gives it to the source attribute. Do you want it to give a base64 source. I don't understand your solution.

          Show
          nobelium vignesh p added a comment - - edited Hi Dan, I have changed the commit message as per the coding standards. Please take a look. https://github.com/nobelium/moodle/tree/MDL-43341 With regard to the solution that you suggested, {$OUTPUT->spacer} function gets the image from the function {render_pic_icon} https://github.com/moodle/moodle/blob/master/lib/outputrenderers.php#L1964 the {pix_url} function reads the image path and gives it to the source attribute. Do you want it to give a base64 source. I don't understand your solution.
          Hide
          danmarsden Dan Marsden added a comment -

          Please look at examples of existing code that use $OUTPUT->spacer - I could give more specific information but a succesful GSOC student is expected to be able to work some of this out on their own. If you are still stuck after looking at this more closely let us know!

          Show
          danmarsden Dan Marsden added a comment - Please look at examples of existing code that use $OUTPUT->spacer - I could give more specific information but a succesful GSOC student is expected to be able to work some of this out on their own. If you are still stuck after looking at this more closely let us know!
          Hide
          nobelium vignesh p added a comment -

          Hi Dan, This is cannot be fixed by changing the SCORM code to use $OUTPUT->spacer https://github.com/nobelium/moodle/blob/MDL-43341/lib/outputrenderers.php#L2215 because the underlying image that the $OUTPUT->render function uses it self is causing the problem. Take a look at https://github.com/nobelium/moodle/compare/master...MDL-43341 using chrome browser. You can see that the image itself is not being displayed properly. The SCORM code that produces the image uses $OUTPUT->pix_url('spacer', 'scorm')https://github.com/moodle/moodle/blob/master/mod/scorm/lib.php#L452. I can change it to use $OUTPUT->spacer but it won't fix the issue.

          Show
          nobelium vignesh p added a comment - Hi Dan, This is cannot be fixed by changing the SCORM code to use $OUTPUT->spacer https://github.com/nobelium/moodle/blob/MDL-43341/lib/outputrenderers.php#L2215 because the underlying image that the $OUTPUT->render function uses it self is causing the problem. Take a look at https://github.com/nobelium/moodle/compare/master...MDL-43341 using chrome browser. You can see that the image itself is not being displayed properly. The SCORM code that produces the image uses $OUTPUT->pix_url('spacer', 'scorm') https://github.com/moodle/moodle/blob/master/mod/scorm/lib.php#L452 . I can change it to use $OUTPUT->spacer but it won't fix the issue.
          Hide
          nobelium vignesh p added a comment - - edited

          Sorry, my bad. The $OUTPUT->spacer function uses another copy of the spacer image in (pix/spacer.gif). I have replaced the code that generates the spacer to use $OUTPUT->spacer. I have not replaced the spacer image file. Thanks Dan. Please look at the commit now and do the needful. https://github.com/nobelium/moodle/compare/master...MDL-43341

          Show
          nobelium vignesh p added a comment - - edited Sorry, my bad. The $OUTPUT->spacer function uses another copy of the spacer image in (pix/spacer.gif). I have replaced the code that generates the spacer to use $OUTPUT->spacer. I have not replaced the spacer image file. Thanks Dan. Please look at the commit now and do the needful. https://github.com/nobelium/moodle/compare/master...MDL-43341
          Hide
          danmarsden Dan Marsden added a comment -

          yeah - this stuff can be pretty confusing! - I think you're saying that the spacer image in (pix/spacer.gif) works correctly - is that right?

          I've added your repository to the pull fields on this tracker issue and added the "cime" label which will trigger our automated system to check the patch against Moodle's coding guidelines - I can see a few coding guideline issues with the patch but will let CIBot pick them up - it will add a comment here with a report that will tell you what the issues are - thanks for working on this!

          Show
          danmarsden Dan Marsden added a comment - yeah - this stuff can be pretty confusing! - I think you're saying that the spacer image in (pix/spacer.gif) works correctly - is that right? I've added your repository to the pull fields on this tracker issue and added the "cime" label which will trigger our automated system to check the patch against Moodle's coding guidelines - I can see a few coding guideline issues with the patch but will let CIBot pick them up - it will add a comment here with a report that will tell you what the issues are - thanks for working on this!
          Hide
          cibot CiBoT added a comment -

          Results for MDL-43341

          • Remote repository: git://github.com/nobelium/moodle.git
          Show
          cibot CiBoT added a comment - Results for MDL-43341 Remote repository: git://github.com/nobelium/moodle.git Remote branch MDL-43341 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2056
          Hide
          danmarsden Dan Marsden added a comment -

          hmm - for some reason it didn't like that repository path - trying a different format.

          Show
          danmarsden Dan Marsden added a comment - hmm - for some reason it didn't like that repository path - trying a different format.
          Hide
          cibot CiBoT added a comment -

          Results for MDL-43341

          • Remote repository: git@github.com:nobelium/moodle.git
          Show
          cibot CiBoT added a comment - Results for MDL-43341 Remote repository: git@github.com:nobelium/moodle.git Remote branch MDL-43341 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2057 Error: Unable to fetch information from MDL-43341 branch at git@github.com:nobelium/moodle.git.
          Show
          cibot CiBoT added a comment - Results for MDL-43341 Remote repository: https://github.com/nobelium/moodle.git Remote branch MDL-43341 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2058 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2058/artifact/work/smurf.html
          Hide
          nobelium vignesh p added a comment -

          Hi Dan,
          Yes, the image in "pix/spacer.gif" works correctly. I have done a `git reset --soft` and edited the code to follow the coding guidelines and committed again with the same commit message. The branch is ready to be merged.

          Show
          nobelium vignesh p added a comment - Hi Dan, Yes, the image in "pix/spacer.gif" works correctly. I have done a `git reset --soft` and edited the code to follow the coding guidelines and committed again with the same commit message. The branch is ready to be merged.
          Hide
          matteo Matteo Scaramuccia added a comment -

          Hi Vignesh,
          a trivial issue in your commit message, now it is:

          MDL-43341 SCORM module report: Replaced usage of mod/scorm/pix/spacer.gif with moodle/pix/spacer.gif
           
          The spacer image (/srv/http/moodle/mod/scorm/pix/spacer.gif) has some problem and it is not being displayed by chrome correctly.
          SCORM code to generate spacer uses $OUTPUT->spacer() instead of  $OUTPUT->pix_url('spacer', 'scorm')

          i.e. a too long "short description". It could be:

          MDL-43341 SCORM Report: spacer.gif must be managed with $OUTPUT->spacer.
           
          The spacer image (mod/scorm/pix/spacer.gif) has some problem and it is not being correctly displayed by Chrome.
          Besides the SCORM code uses $OUTPUT->spacer() to generate that spacer, instead of OUTPUT->pix_url('spacer', 'scorm').

          Note the clean up of the paths, relative to the Moodle dirroot.

          If I'd asked to write a commit message for this issue I would write:

          MDL-43341 SCORM Report: spacer.gif must be managed with $OUTPUT->spacer.

          'cause the info in the "long description" are already available in the comments here in the Tracker.

          HTH,
          Matteo

          Show
          matteo Matteo Scaramuccia added a comment - Hi Vignesh, a trivial issue in your commit message, now it is: MDL-43341 SCORM module report: Replaced usage of mod/scorm/pix/spacer.gif with moodle/pix/spacer.gif   The spacer image (/srv/http/moodle/mod/scorm/pix/spacer.gif) has some problem and it is not being displayed by chrome correctly. SCORM code to generate spacer uses $OUTPUT->spacer() instead of $OUTPUT->pix_url('spacer', 'scorm') i.e. a too long "short description". It could be: MDL-43341 SCORM Report: spacer.gif must be managed with $OUTPUT->spacer.   The spacer image (mod/scorm/pix/spacer.gif) has some problem and it is not being correctly displayed by Chrome. Besides the SCORM code uses $OUTPUT->spacer() to generate that spacer, instead of OUTPUT->pix_url('spacer', 'scorm'). Note the clean up of the paths, relative to the Moodle dirroot . If I'd asked to write a commit message for this issue I would write: MDL-43341 SCORM Report: spacer.gif must be managed with $OUTPUT->spacer. 'cause the info in the "long description" are already available in the comments here in the Tracker. HTH, Matteo
          Hide
          nobelium vignesh p added a comment -

          Hi Matteo, Thanks for pointing it out. I have cut short the short description(not just this one, I have edited my commit messages in all other issues as well)

          Show
          nobelium vignesh p added a comment - Hi Matteo, Thanks for pointing it out. I have cut short the short description(not just this one, I have edited my commit messages in all other issues as well)
          Hide
          danmarsden Dan Marsden added a comment -

          Thanks Vignesh,

          This looks ready to integrate to me - but it should go into stable releases as well as master - can you please create a new branch for 2.5 and 2.6 with your commit on top? - then I can add those to this issue and push through for integration! - great work tracking this down!

          Show
          danmarsden Dan Marsden added a comment - Thanks Vignesh, This looks ready to integrate to me - but it should go into stable releases as well as master - can you please create a new branch for 2.5 and 2.6 with your commit on top? - then I can add those to this issue and push through for integration! - great work tracking this down!
          Hide
          nobelium vignesh p added a comment - - edited

          Hi Dan, I have cherry picked my commit into MDL-43341_25 and MDL-43341_26 and pushed it to my repository.
          https://github.com/nobelium/moodle/tree/MDL-43341_25 & https://github.com/nobelium/moodle/tree/MDL-43341_26. Thanks for looking into it.

          Show
          nobelium vignesh p added a comment - - edited Hi Dan, I have cherry picked my commit into MDL-43341 _25 and MDL-43341 _26 and pushed it to my repository. https://github.com/nobelium/moodle/tree/MDL-43341_25 & https://github.com/nobelium/moodle/tree/MDL-43341_26 . Thanks for looking into it.
          Hide
          matteo Matteo Scaramuccia added a comment -

          Hi All,
          apologize for this out-of-time feedbacks but:

          1. @Vignesh, have you already checked that spacer.gif is used only in that place inside mod/scorm?
          2. @Dan, what about removing that file from the SCORM module within this commit?

          HTH,
          Matteo

          Show
          matteo Matteo Scaramuccia added a comment - Hi All, apologize for this out-of-time feedbacks but: @Vignesh, have you already checked that spacer.gif is used only in that place inside mod/scorm ? @Dan, what about removing that file from the SCORM module within this commit? HTH, Matteo
          Hide
          nobelium vignesh p added a comment -

          Hi Mat, I did a global search(`grep -nr ./ -e "spacer"`) and spacer.gif is being used only in that place in scorm and if Dan agrees we can remove that pic from scorm module.

          Show
          nobelium vignesh p added a comment - Hi Mat, I did a global search(`grep -nr ./ -e "spacer"`) and spacer.gif is being used only in that place in scorm and if Dan agrees we can remove that pic from scorm module.
          Hide
          danmarsden Dan Marsden added a comment -

          yes - good point, the old file should be removed from mod/scorm as well - thanks Matteo.

          Show
          danmarsden Dan Marsden added a comment - yes - good point, the old file should be removed from mod/scorm as well - thanks Matteo.
          Hide
          nobelium vignesh p added a comment -

          Hi Dan, I have removed mod/scorm/pix/spacer.gif in all 3 branches(master, MDL-43341_25, MDL-43341_26).

          Show
          nobelium vignesh p added a comment - Hi Dan, I have removed mod/scorm/pix/spacer.gif in all 3 branches(master, MDL-43341 _25, MDL-43341 _26).
          Hide
          danmarsden Dan Marsden added a comment -

          great, bouncing up for integration - thanks for working on this!

          Show
          danmarsden Dan Marsden added a comment - great, bouncing up for integration - thanks for working on this!
          Hide
          stronk7 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
          stronk7 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
          nobelium vignesh p added a comment -

          (Y) All branches are ready to be merged.

          Show
          nobelium vignesh p added a comment - (Y) All branches are ready to be merged.
          Hide
          cibot CiBoT added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          Hide
          marina Marina Glancy added a comment -

          Hello Vignesh, everything looks/works great. We can not remove the file from stable version, only from master. Will you be able to update your stable branches?

          Show
          marina Marina Glancy added a comment - Hello Vignesh, everything looks/works great. We can not remove the file from stable version, only from master. Will you be able to update your stable branches?
          Hide
          nobelium vignesh p added a comment -

          Hi Marina, I have add the image back. Its good to merge.

          Show
          nobelium vignesh p added a comment - Hi Marina, I have add the image back. Its good to merge.
          Hide
          marina Marina Glancy added a comment -

          Thanks, integrated in 2.5, 2.6 and master

          Show
          marina Marina Glancy added a comment - Thanks, integrated in 2.5, 2.6 and master
          Hide
          marina Marina Glancy added a comment -

          Tested on 2.5, 2.6 and master, passing

          Show
          marina Marina Glancy added a comment - Tested on 2.5, 2.6 and master, passing
          Hide
          marina Marina Glancy added a comment -

          Thanks for your awesome code, it is now part of Moodle. Don't forget to submit another issue next week (and peer review two).

          Show
          marina Marina Glancy added a comment - Thanks for your awesome code, it is now part of Moodle. Don't forget to submit another issue next week (and peer review two).

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                12/May/14