Moodle
  1. Moodle
  2. MDL-29624

Media embedding should be consistent and customisable

    Details

    • Testing Instructions:
      Hide

      Note: The behaviour tested here is pretty much the same as existing behaviour. There are not intended to be any really major changes, except occasionally that something works where it was previously broken.

      There may be some cases where the embedding behaviour is less than ideal - this change does improve some embedding behaviour including making the mobile device support better (especially on Android), but most of the embed code is brought forward from the old media filter, so this is not intended to fix all existing issues.

      This test includes a good range of formats but not every single one. The new (and existing) unit tests do basic testing of all the code so it's hopefully not necessary to manually test everything. All videos in the test course are format conversions of a 6 second video of me typing stuff into Word. The audio is also a format conversion of the soundtrack from this video.

      Test difficulty: EASY (but one section requires access to mobile devices).

      PREPARATION

      i. Before you install this patch, you should have default media filter settings. (These are for everything to be on except the HTML 5 options and Vimeo.) You will need to visit the admin page so that it has a chance to update the settings. Or, if you already upgraded and didn't have default settings, skip to point 15 below and set options up as per the 'verify' text before you begin the test.

      ii. A Moodle backup file is attached to this bug. Restore it onto your test system as a new course.

      iii. Your test computer needs to have QuickTime installed.

      RESOURCES

      1. The top area of the course is headed 'Resources'. Each resource in this area is named after a file format, and contains a File resource of that type.
      + All files should be embedded using a suitable player and should play.

      Note: The WMV file will only embed in Internet Explorer. In other browsers you should get a download link.

      URL

      2. In the second section of the course, click 'URL'. This is a link to an mp4 file on a remote server.
      + Should display embedded using QT.

      LESSON

      3. Click 'Lesson'. If necessary, switch to the Preview tab.
      4. Under 'linked media' on the right, click the link.
      + Popup window should appear; within this, the media file should display using QT.

      FILTER

      5. Click into the 'Page'.
      + Check that embedded items A through H display correctly as embedded videos, audio (C), YouTube (G), or Flash (H).
      + Items D and F, which were embedded using URLs ending in #640x480, should display at double size.
      + Item F ('alternative formats MP4 + FLV') should be displaying using the Flash player.

      Note: When displayed inline by the filter, MP3 files use a mini-player which is smaller than when displayed in a File resource; this is intended as it is the same as previous behaviour.

      MOBILE SUPPORT

      6. View the Page using an iDevice.
      + Items A, B, D, and F should all display using QuickTime.
      + Item C should display using HTML5 audio.
      + Items E and H should not display because they use Flash. These should show download links.

      7. View the Page using an Android device (2.2+).
      + Items A, B, and D should all display using HTML5 video.
      + If the device supports Flash, then items E and H should appear, and items F and C should display using Flash.
      + If the device does not support Flash then items E and H should show download links, item F should display using HTML5 video, and item C as HTML5 audio.

      SETTINGS

      8. Go to the new 'Media embedding' settings page under Site administration / Appearance.
      + Verify that all options are on except Vimeo (i.e. the options are same as they were pre-upgrade except the HTML5 options have been enabled, as they are now suitable to be on by default).

      9. Turn off QuickTime option and save settings.

      10. Go look at the Page again on a desktop browser which supports HTML5 including mp4 format (Chrome/Safari).
      + All videos should still display. Videos A, B, and D (i.e. all the ones which were previously QuickTime) should now display using HTML5 video. The others should still use the Flash player as before.

      FALLBACK

      11. Look at the Page on a desktop browser which supports HTML5 but not mp4 format (Firefox/Opera).
      + Everything should display as before except videos A, B, and D which cannot be embedded (now that QT is disabled in settings).
      + These videos should contain download links which you could use to download the video and play locally.
      + The download link should have the same text as the video filename (frog.mp4), except in video B where I used custom fallback text (Frog frog frog frog!).

      FLASH SECURITY

      12. Click into the Forum. Start a new discussion. Give it any subject.
      13. In the Message area, use the Moodle Media button to insert a .SWF file (if you need one, go grab the sample I used for the 'SWF' File item).
      14. Post the discussion containing the SWF link. Click into it to view it.
      + Unlike other media files, the SWF file should not be embedded and should remain as a link. This is because forum messages are not trusted.

      MOODLE MEDIA PREVIEW

      15. Reply to the post. In the reply field, click the 'Moodle media' button in the editor and this time select an .flv file (again, if you don't have one, use the 'FLV' File resource).
      + Once you have uploaded the file but before you insert it, verify that a preview displays in the Moodle Media dialog. When the player does not fit in the preview space, there will be scrollbars.

      Show
      Note: The behaviour tested here is pretty much the same as existing behaviour. There are not intended to be any really major changes, except occasionally that something works where it was previously broken. There may be some cases where the embedding behaviour is less than ideal - this change does improve some embedding behaviour including making the mobile device support better (especially on Android), but most of the embed code is brought forward from the old media filter, so this is not intended to fix all existing issues. This test includes a good range of formats but not every single one. The new (and existing) unit tests do basic testing of all the code so it's hopefully not necessary to manually test everything. All videos in the test course are format conversions of a 6 second video of me typing stuff into Word. The audio is also a format conversion of the soundtrack from this video. Test difficulty: EASY (but one section requires access to mobile devices). PREPARATION i. Before you install this patch, you should have default media filter settings. (These are for everything to be on except the HTML 5 options and Vimeo.) You will need to visit the admin page so that it has a chance to update the settings. Or, if you already upgraded and didn't have default settings, skip to point 15 below and set options up as per the 'verify' text before you begin the test. ii. A Moodle backup file is attached to this bug. Restore it onto your test system as a new course. iii. Your test computer needs to have QuickTime installed. RESOURCES 1. The top area of the course is headed 'Resources'. Each resource in this area is named after a file format, and contains a File resource of that type. + All files should be embedded using a suitable player and should play. Note: The WMV file will only embed in Internet Explorer. In other browsers you should get a download link. URL 2. In the second section of the course, click 'URL'. This is a link to an mp4 file on a remote server. + Should display embedded using QT. LESSON 3. Click 'Lesson'. If necessary, switch to the Preview tab. 4. Under 'linked media' on the right, click the link. + Popup window should appear; within this, the media file should display using QT. FILTER 5. Click into the 'Page'. + Check that embedded items A through H display correctly as embedded videos, audio (C), YouTube (G), or Flash (H). + Items D and F, which were embedded using URLs ending in #640x480, should display at double size. + Item F ('alternative formats MP4 + FLV') should be displaying using the Flash player. Note: When displayed inline by the filter, MP3 files use a mini-player which is smaller than when displayed in a File resource; this is intended as it is the same as previous behaviour. MOBILE SUPPORT 6. View the Page using an iDevice. + Items A, B, D, and F should all display using QuickTime. + Item C should display using HTML5 audio. + Items E and H should not display because they use Flash. These should show download links. 7. View the Page using an Android device (2.2+). + Items A, B, and D should all display using HTML5 video. + If the device supports Flash, then items E and H should appear, and items F and C should display using Flash. + If the device does not support Flash then items E and H should show download links, item F should display using HTML5 video, and item C as HTML5 audio. SETTINGS 8. Go to the new 'Media embedding' settings page under Site administration / Appearance. + Verify that all options are on except Vimeo (i.e. the options are same as they were pre-upgrade except the HTML5 options have been enabled, as they are now suitable to be on by default). 9. Turn off QuickTime option and save settings. 10. Go look at the Page again on a desktop browser which supports HTML5 including mp4 format (Chrome/Safari). + All videos should still display. Videos A, B, and D (i.e. all the ones which were previously QuickTime) should now display using HTML5 video. The others should still use the Flash player as before. FALLBACK 11. Look at the Page on a desktop browser which supports HTML5 but not mp4 format (Firefox/Opera). + Everything should display as before except videos A, B, and D which cannot be embedded (now that QT is disabled in settings). + These videos should contain download links which you could use to download the video and play locally. + The download link should have the same text as the video filename (frog.mp4), except in video B where I used custom fallback text (Frog frog frog frog!). FLASH SECURITY 12. Click into the Forum. Start a new discussion. Give it any subject. 13. In the Message area, use the Moodle Media button to insert a .SWF file (if you need one, go grab the sample I used for the 'SWF' File item). 14. Post the discussion containing the SWF link. Click into it to view it. + Unlike other media files, the SWF file should not be embedded and should remain as a link. This is because forum messages are not trusted. MOODLE MEDIA PREVIEW 15. Reply to the post. In the reply field, click the 'Moodle media' button in the editor and this time select an .flv file (again, if you don't have one, use the 'FLV' File resource). + Once you have uploaded the file but before you insert it, verify that a preview displays in the Moodle Media dialog. When the player does not fit in the preview space, there will be scrollbars.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-29624-master
    • Rank:
      19136

      Description

      There are three areas in Moodle which have media embedding code:

      1) Resource (used when you add a File or URL in 'Embed' mode, or in Lesson for the popup).
      2) Filter (used when you put a link to <a href="somevideo.mp4"> in any text).
      3) Moodle media preview (in TinyMCE when you select a media file to insert).

      All three areas have different, totally independent code. This means the same code is duplicated three times (twice in PHP, once in JavaScript). In practice, the situation is actually worse than this because the code in resource is significantly worse than the code in filter, and different formats are supported using different approaches which work (or not) in different browsers.

      In addition it is possible in a Moodle installation to customise #2 (by writing a new filter) but not possible to customise #1 and #3 at all without editing core code.

      After discussion with other developers (including Petr I think) the best approach to resolve this problem was agreed. This work implements it as follows:

      1) New core_media_renderer contains functions necessary to embed media. It is possible to override this renderer in a theme (achieving the customisation requirement).

      2) The back-end to this class is in medialib.php and consists of a number of ranked 'player' classes (currently these are lightweight classes, not actual moodle plugins). It is easy to add extra classes either in core or inside a theme that extends the renderer as above.

      The system works by outputting content using ALL player classes that support that content, in ranking order, via the object tag fallback mechanism.

      Player classes were implemented starting from the existing functions in filter_mediaplugin.

      There is significant unit testing for this feature.

      3) filter_mediaplugin was changed to implement using these classes. (Obviously it is now very much smaller.) Language files and settings were moved from the filter into core_media. Existing unit test was updated to still pass.

      4) I changed the resource (File) module to use this function as well, deleting all the resource code related to embedding media files. (Note: Resource still handles its own embedding of non-media files such as PDFs, HTML.)

      5) I also changed the 'Moodle media' TinyMCE plugin so that instead of the custom JS embedding code, it adds an iframe to a new php script which outputs the media using the render_media function. [This removes the need to duplicate the code in JavaScript.]

      6) I made necessary improvements to the HTML5 support, including:

      a) Browser detection so that we can now have html5 support left on by default, without breaking fallbacks in the case where the browser does not support the video format in use.

      b) Tweaks to work around bugs on Android versions which are still in wide use. This has now been tested to play on iDevices and Android when using any supported format.

      I also wrote documentation for this proposed feature on the Moodle Docs wiki:
      http://docs.moodle.org/dev/Media_embedding

      1. smurf_MDL-29624.xml
        106 kB
        Aparup Banerjee

        Issue Links

          Activity

          Hide
          Sam Marshall added a comment -

          Note: I filed this because it got filed on me as a bug and I need to fix the problem in our OU systems - but I would prefer to fix it in core than do a custom hack at our end (or create an 'oufile' module), if possible.

          Show
          Sam Marshall added a comment - Note: I filed this because it got filed on me as a bug and I need to fix the problem in our OU systems - but I would prefer to fix it in core than do a custom hack at our end (or create an 'oufile' module), if possible.
          Hide
          Sam Marshall added a comment -

          If HQ agrees with this bug I am also happy to develop it as above for Moodle 2.2.

          Show
          Sam Marshall added a comment - If HQ agrees with this bug I am also happy to develop it as above for Moodle 2.2.
          Hide
          Sam Marshall added a comment -

          After discussion (which may have happened in another bug I didn't find just now, or out-of-band) I believe my original approach was rejected, but an alternative was considered acceptable. I've changed the description of this bug to correspond to what was agreed.

          Show
          Sam Marshall added a comment - After discussion (which may have happened in another bug I didn't find just now, or out-of-band) I believe my original approach was rejected, but an alternative was considered acceptable. I've changed the description of this bug to correspond to what was agreed.
          Hide
          Sam Marshall added a comment -

          Corrected test instructions.

          Also note: the current resource plugin has a specific branch for RealAudio support, but it doesn't work; the system will use the 'generic' object tag for RealAudio. (The reason for not working is there is a condition on mime type which does not match any of the mime types from filelib.) I have made this new system do the same, which means I didn't need to include that code.

          Show
          Sam Marshall added a comment - Corrected test instructions. Also note: the current resource plugin has a specific branch for RealAudio support, but it doesn't work; the system will use the 'generic' object tag for RealAudio. (The reason for not working is there is a condition on mime type which does not match any of the mime types from filelib.) I have made this new system do the same, which means I didn't need to include that code.
          Hide
          Sam Marshall added a comment -

          Because I'm deleting the resourcelib functions that do audio/video embeds, I searched for uses in existing modules and found more duplicate code along the same lines in URL and Lesson. Added to the test case. (This is making the code a bit shorter in those modules.)

          Note: So far, I decided not to support Flash with this system (i.e. leave it using current logic) due to security implications and the assumption that nobody much needs to customise Flash embedding, not like you can use a different player... but this may be incorrect, not sure.

          Show
          Sam Marshall added a comment - Because I'm deleting the resourcelib functions that do audio/video embeds, I searched for uses in existing modules and found more duplicate code along the same lines in URL and Lesson. Added to the test case. (This is making the code a bit shorter in those modules.) Note: So far, I decided not to support Flash with this system (i.e. leave it using current logic) due to security implications and the assumption that nobody much needs to customise Flash embedding, not like you can use a different player... but this may be incorrect, not sure.
          Hide
          Sam Marshall added a comment -

          I've done the first part of this code so that it passes steps 1-6 of the test script (updated).

          Show
          Sam Marshall added a comment - I've done the first part of this code so that it passes steps 1-6 of the test script (updated).
          Hide
          Sam Marshall added a comment -

          Just to document: I decided to change approach. On consideration of the media filter code, I think this is better/more recent than the resource code that I based my initial work on.

          I'm going to rework my code so that it is based on the code used for the media filter. Will also hijack the media filter settings (in terms of supported players) as these should apply systemwide and not just to filter, especially where there are overlaps (html5 and QuickTime).

          Show
          Sam Marshall added a comment - Just to document: I decided to change approach. On consideration of the media filter code, I think this is better/more recent than the resource code that I based my initial work on. I'm going to rework my code so that it is based on the code used for the media filter. Will also hijack the media filter settings (in terms of supported players) as these should apply systemwide and not just to filter, especially where there are overlaps (html5 and QuickTime).
          Hide
          Sam Marshall added a comment - - edited

          I've implemented for the filter (not fully tested yet) and have also written a performance test script that is included in the code. The performance numbers (in average ms required to run filter on my test server, with all the 'enable' checkboxes turned on) are:

          Text with no links:
          BEFORE 0.02ms AFTER 0.02ms (no change)

          Text with a link which is not a media file:
          BEFORE 0.22ms AFTER 0.06ms (3.5x faster)

          Text with an mp3 file link:
          BEFORE 0.33ms AFTER 1.10ms (3.5x slower)

          Typically, fewer than half of links are actually media files, so this change should improve performance overall.

          Show
          Sam Marshall added a comment - - edited I've implemented for the filter (not fully tested yet) and have also written a performance test script that is included in the code. The performance numbers (in average ms required to run filter on my test server, with all the 'enable' checkboxes turned on) are: Text with no links: BEFORE 0.02ms AFTER 0.02ms (no change) Text with a link which is not a media file: BEFORE 0.22ms AFTER 0.06ms (3.5x faster) Text with an mp3 file link: BEFORE 0.33ms AFTER 1.10ms (3.5x slower) Typically, fewer than half of links are actually media files, so this change should improve performance overall.
          Hide
          Sam Marshall added a comment -

          I have now implemented resource module changes again and a version of the code in the finished structure (three commits) is now on GitHub. However, I expect to make further changes:

          a) Test and fix mobile support - this works slightly differently than it used to - and possibly change default values for the various settings or consider additional options to make behaviour different on mobile (e.g. an option to enable html5 but only on mobile devices).

          b) Update test script to make it even longer! Er, I mean, to fully include the media filter and other aspects.

          Show
          Sam Marshall added a comment - I have now implemented resource module changes again and a version of the code in the finished structure (three commits) is now on GitHub. However, I expect to make further changes: a) Test and fix mobile support - this works slightly differently than it used to - and possibly change default values for the various settings or consider additional options to make behaviour different on mobile (e.g. an option to enable html5 but only on mobile devices). b) Update test script to make it even longer! Er, I mean, to fully include the media filter and other aspects.
          Hide
          Sam Marshall added a comment -

          Mobile support (iOS and Android - approx 2.2 for video, approx 2.3 for audio) is now tested and fixed. I changed the text for all the settings and turned default on for html5 - there is no need to enable it only for mobile device as enabling it now does no harm, it is lowest-ranked anyway.

          Show
          Sam Marshall added a comment - Mobile support (iOS and Android - approx 2.2 for video, approx 2.3 for audio) is now tested and fixed. I changed the text for all the settings and turned default on for html5 - there is no need to enable it only for mobile device as enabling it now does no harm, it is lowest-ranked anyway.
          Hide
          Sam Marshall added a comment -

          After upgrade, the settings for the old filter have been deleted. Should anyone want to test switching between code branches (old code vs new code) you can run the following SQL statements after upgrade to add back in the default settings for the old filter (change db prefix as needed):

          INSERT INTO coremdl_config(name, value) VALUES('filter_mediaplugin_enable_youtube', '1');
          INSERT INTO coremdl_config(name, value) VALUES('filter_mediaplugin_enable_vimeo', '1');
          INSERT INTO coremdl_config(name, value) VALUES('filter_mediaplugin_enable_mp3', '1');
          INSERT INTO coremdl_config(name, value) VALUES('filter_mediaplugin_enable_flv', '1');
          INSERT INTO coremdl_config(name, value) VALUES('filter_mediaplugin_enable_swf', '1');
          INSERT INTO coremdl_config(name, value) VALUES('filter_mediaplugin_enable_qt', '1');
          INSERT INTO coremdl_config(name, value) VALUES('filter_mediaplugin_enable_wmp', '1');
          INSERT INTO coremdl_config(name, value) VALUES('filter_mediaplugin_enable_rm', '1');
          
          Show
          Sam Marshall added a comment - After upgrade, the settings for the old filter have been deleted. Should anyone want to test switching between code branches (old code vs new code) you can run the following SQL statements after upgrade to add back in the default settings for the old filter (change db prefix as needed): INSERT INTO coremdl_config(name, value) VALUES('filter_mediaplugin_enable_youtube', '1'); INSERT INTO coremdl_config(name, value) VALUES('filter_mediaplugin_enable_vimeo', '1'); INSERT INTO coremdl_config(name, value) VALUES('filter_mediaplugin_enable_mp3', '1'); INSERT INTO coremdl_config(name, value) VALUES('filter_mediaplugin_enable_flv', '1'); INSERT INTO coremdl_config(name, value) VALUES('filter_mediaplugin_enable_swf', '1'); INSERT INTO coremdl_config(name, value) VALUES('filter_mediaplugin_enable_qt', '1'); INSERT INTO coremdl_config(name, value) VALUES('filter_mediaplugin_enable_wmp', '1'); INSERT INTO coremdl_config(name, value) VALUES('filter_mediaplugin_enable_rm', '1');
          Hide
          Sam Marshall added a comment -

          Development complete

          I put the heading so anyone looking at the issue can easily skip down to this and ignore all the other comments that I wrote during coding!

          I'm asking Tim to initially review this. Once I fix problems Tim identifies we will pass it on to HQ developer (probably Petr) for final review.

          Show
          Sam Marshall added a comment - Development complete I put the heading so anyone looking at the issue can easily skip down to this and ignore all the other comments that I wrote during coding! I'm asking Tim to initially review this. Once I fix problems Tim identifies we will pass it on to HQ developer (probably Petr) for final review.
          Hide
          Sam Marshall added a comment -

          for some reason that comment didn't get emailed to tim, adding another one

          Show
          Sam Marshall added a comment - for some reason that comment didn't get emailed to tim, adding another one
          Hide
          Dan Poltawski added a comment -

          We want this too. Adding Andrew & Ruslan to watchers

          Show
          Dan Poltawski added a comment - We want this too. Adding Andrew & Ruslan to watchers
          Hide
          Tim Hunt added a comment -

          Generally, the new approach is much better. The code is well laid out, and has good PHP docs and unit tests. Just a few issues:

          Code review comments:

          1. We typically don't automatically change config settings on upgrade, so https://github.com/sammarshallou/moodle/commit/716d10ada0418f7995a1e97eff242372c81019f9#L1R63 is unconventional. You need to decide whether unconventional means right or wrong here.

          2. I have a feeling that filters should avoid referring to global $PAGE (https://github.com/sammarshallou/moodle/commit/716d10ada0418f7995a1e97eff242372c81019f9#L2R46) They should have a non-global link to the relevant page.

          OK, I am wrong. It should be like blocks, that use $this->page, instead of $PAGE, but whoever last changed how filters work does not agree.

          3. I think using static method on a renderer class like core_media_renderer::split_alternatives https://github.com/sammarshallou/moodle/commit/716d10ada0418f7995a1e97eff242372c81019f9#L2R101 is nasty. I think these should be methods of a different class, for example core_media_utils or even just core_media. Similarly for constants like core_media_renderer::OPTION_EMBED_OR_BLANK. Should be core_media::OPTION_EMBED_OR_BLANK

          4. Reading the code, I just had to hunt around to work out what $this->media was. Would be better to call it $this->mediarenderer. (https://github.com/sammarshallou/moodle/commit/716d10ada0418f7995a1e97eff242372c81019f9#L2R118)

          5. I think that test scripts like filter/mediaplugin/perftest.php are better stored inside the simpletest sub-folder.

          6. Do you need to update ->requires in the filter version.php? https://github.com/sammarshallou/moodle/commit/716d10ada0418f7995a1e97eff242372c81019f9#L7R29

          7. Coding style says two blank lines before a new class definition, and @copyright and @license PHPdoc attributes. https://github.com/sammarshallou/moodle/commit/716d10ada0418f7995a1e97eff242372c81019f9#L9R63 http://docs.moodle.org/dev/Coding_style#Classes_3 http://docs.moodle.org/dev/Coding_style#Class_declarations

          8. Bad indent: https://github.com/sammarshallou/moodle/commit/716d10ada0418f7995a1e97eff242372c81019f9#L9R362 (Affects several other places too)

          9. You seem to still have debug code: https://github.com/sammarshallou/moodle/commit/716d10ada0418f7995a1e97eff242372c81019f9#L9R384 (If that should still be there, convert it to coding_exception).

          10. I suggest that you give the array in protected function get_players_raw() { mnemonic array keys. https://github.com/sammarshallou/moodle/commit/716d10ada0418f7995a1e97eff242372c81019f9#L11R2812 That would let subclasses do something like $players = parent::get_players_raw(); unset($players['rm']); return $players;

          Whew! End of first patch. I am going to add this comment now, before my browser crashes, then to onto the other patches.

          Show
          Tim Hunt added a comment - Generally, the new approach is much better. The code is well laid out, and has good PHP docs and unit tests. Just a few issues: Code review comments: 1. We typically don't automatically change config settings on upgrade, so https://github.com/sammarshallou/moodle/commit/716d10ada0418f7995a1e97eff242372c81019f9#L1R63 is unconventional. You need to decide whether unconventional means right or wrong here. 2. I have a feeling that filters should avoid referring to global $PAGE ( https://github.com/sammarshallou/moodle/commit/716d10ada0418f7995a1e97eff242372c81019f9#L2R46 ) They should have a non-global link to the relevant page. OK, I am wrong. It should be like blocks, that use $this->page, instead of $PAGE, but whoever last changed how filters work does not agree. 3. I think using static method on a renderer class like core_media_renderer::split_alternatives https://github.com/sammarshallou/moodle/commit/716d10ada0418f7995a1e97eff242372c81019f9#L2R101 is nasty. I think these should be methods of a different class, for example core_media_utils or even just core_media. Similarly for constants like core_media_renderer::OPTION_EMBED_OR_BLANK. Should be core_media::OPTION_EMBED_OR_BLANK 4. Reading the code, I just had to hunt around to work out what $this->media was. Would be better to call it $this->mediarenderer. ( https://github.com/sammarshallou/moodle/commit/716d10ada0418f7995a1e97eff242372c81019f9#L2R118 ) 5. I think that test scripts like filter/mediaplugin/perftest.php are better stored inside the simpletest sub-folder. 6. Do you need to update ->requires in the filter version.php? https://github.com/sammarshallou/moodle/commit/716d10ada0418f7995a1e97eff242372c81019f9#L7R29 7. Coding style says two blank lines before a new class definition, and @copyright and @license PHPdoc attributes. https://github.com/sammarshallou/moodle/commit/716d10ada0418f7995a1e97eff242372c81019f9#L9R63 http://docs.moodle.org/dev/Coding_style#Classes_3 http://docs.moodle.org/dev/Coding_style#Class_declarations 8. Bad indent: https://github.com/sammarshallou/moodle/commit/716d10ada0418f7995a1e97eff242372c81019f9#L9R362 (Affects several other places too) 9. You seem to still have debug code: https://github.com/sammarshallou/moodle/commit/716d10ada0418f7995a1e97eff242372c81019f9#L9R384 (If that should still be there, convert it to coding_exception). 10. I suggest that you give the array in protected function get_players_raw() { mnemonic array keys. https://github.com/sammarshallou/moodle/commit/716d10ada0418f7995a1e97eff242372c81019f9#L11R2812 That would let subclasses do something like $players = parent::get_players_raw(); unset($players ['rm'] ); return $players; Whew! End of first patch. I am going to add this comment now, before my browser crashes, then to onto the other patches.
          Hide
          Tim Hunt added a comment -

          11. Best require_once config.php I have ever seen! https://github.com/sammarshallou/moodle/commit/92e206ba960c9e74d802e3930fc73c127c4b5c7d#L1R26 - should this be using dirname(_FILE_)?

          12. Should we have deprecated versions of resourcelib_embed_mp3 etc. https://github.com/sammarshallou/moodle/commit/e4048cf1f380d7653b5599704248c296b38c165d#L0L232 in deprecatedlib.php, that call the new code?

          At any rate, do we need to add some comments on the API changes to mod/upgrade.txt or lib/upgrade.txt

          13. Other potentially confusing $media variable names: https://github.com/sammarshallou/moodle/commit/e4048cf1f380d7653b5599704248c296b38c165d#L2R79 and https://github.com/sammarshallou/moodle/commit/e4048cf1f380d7653b5599704248c296b38c165d#L3R309

          14. Given that lesson, resource and url still contain three fairly similar if statements to embed various things, is there one more function that could be re-factored out into medialib.php?

          15. Do we need a new developer docs page explaining how to embed media in your corner of the code, if necessary; and how to write a new player, and add it in? (I know you cover that latter briefly in a PHP doc comment.)

          Yay! That's all. Great work. I'm sure the integrators will love this!

          I agree with you that it would be good if someone like Petr did a second peer review after you have addressed the issues I raise, and before you submit it for integration.

          Show
          Tim Hunt added a comment - 11. Best require_once config.php I have ever seen! https://github.com/sammarshallou/moodle/commit/92e206ba960c9e74d802e3930fc73c127c4b5c7d#L1R26 - should this be using dirname(_ FILE _)? 12. Should we have deprecated versions of resourcelib_embed_mp3 etc. https://github.com/sammarshallou/moodle/commit/e4048cf1f380d7653b5599704248c296b38c165d#L0L232 in deprecatedlib.php, that call the new code? At any rate, do we need to add some comments on the API changes to mod/upgrade.txt or lib/upgrade.txt 13. Other potentially confusing $media variable names: https://github.com/sammarshallou/moodle/commit/e4048cf1f380d7653b5599704248c296b38c165d#L2R79 and https://github.com/sammarshallou/moodle/commit/e4048cf1f380d7653b5599704248c296b38c165d#L3R309 14. Given that lesson, resource and url still contain three fairly similar if statements to embed various things, is there one more function that could be re-factored out into medialib.php? 15. Do we need a new developer docs page explaining how to embed media in your corner of the code, if necessary; and how to write a new player, and add it in? (I know you cover that latter briefly in a PHP doc comment.) Yay! That's all. Great work. I'm sure the integrators will love this! I agree with you that it would be good if someone like Petr did a second peer review after you have addressed the issues I raise, and before you submit it for integration.
          Hide
          Sam Marshall added a comment -

          Petr - when you have time, could you review this fairly large 2.3 change please? I think you are the appropriate person, set somebody else as reviewer if not. This is supposed to be roughly in line with your previous comments.

          Response to Tim code review comments:

          1. Done (added comment to explain why upgrade settings change is necessary to retain existing behaviour with html5 fallbacks).

          2. n/a (Tim withdrew comment within the comment itself)

          3. Done (moved consts and statics to core_media class) - previously I had not thought of a way to get medialib.php automatically required on time, but I now thought of one.

          4. Done (replaced all 'media' with 'media_renderer')

          5. Done (simpletest folder not really appropriate, but I made another folder just called test and put it there).

          6. Done (set ->requires to current weekly version).

          7. Done (added @copyright, @license to all class definitions).

          8. Done (fixed stupid indent created by eclipse PDT copy/paste being crap).

          9. Done (removed stupid debug code that breaks if you turn xmlstrictheaders on).

          10. Done (meaningful array keys in get_players_raw)

          11. Done (changed require to use dirname(_FILE_)) - note I used the /../ variant and not the one shown in code guidelines because the code guideline one would result in about 350 lines of nested dirname calls.

          12. Done (added note to lib/upgrade.txt, mod/upgrade.txt) - I don't think we need deprecated version of the deleted resourcelib functions, I guess not too many people used it. Could be wrong though.

          13. Done (as 4 above)

          14. Not done. Yes the resource stuff could be refactored (into resourcelib.php) but they're not quite identical and this code already made it better, does it need to go the whole way...? (I will do this if required.)

          15. Done (http://docs.moodle.org/dev/Media_embedding) - there's some nice detailed user-friendly documentation for you. Both parts are also covered very briefly in the phpdoc at top of medialib.php, as well as in detail in appropriate function phpdoc.

          In addition I ran codechecker and got to 0 errors on medialib.php and filter/mediaplugin and my part of lib/outputrenderers.php, and fixed the unit tests, and quickly checked that it still appears to be working.

          Show
          Sam Marshall added a comment - Petr - when you have time, could you review this fairly large 2.3 change please? I think you are the appropriate person, set somebody else as reviewer if not. This is supposed to be roughly in line with your previous comments. Response to Tim code review comments: 1. Done (added comment to explain why upgrade settings change is necessary to retain existing behaviour with html5 fallbacks). 2. n/a (Tim withdrew comment within the comment itself) 3. Done (moved consts and statics to core_media class) - previously I had not thought of a way to get medialib.php automatically required on time, but I now thought of one. 4. Done (replaced all 'media' with 'media_renderer') 5. Done (simpletest folder not really appropriate, but I made another folder just called test and put it there). 6. Done (set ->requires to current weekly version). 7. Done (added @copyright, @license to all class definitions). 8. Done (fixed stupid indent created by eclipse PDT copy/paste being crap). 9. Done (removed stupid debug code that breaks if you turn xmlstrictheaders on). 10. Done (meaningful array keys in get_players_raw) 11. Done (changed require to use dirname(_ FILE _)) - note I used the /../ variant and not the one shown in code guidelines because the code guideline one would result in about 350 lines of nested dirname calls. 12. Done (added note to lib/upgrade.txt, mod/upgrade.txt) - I don't think we need deprecated version of the deleted resourcelib functions, I guess not too many people used it. Could be wrong though. 13. Done (as 4 above) 14. Not done. Yes the resource stuff could be refactored (into resourcelib.php) but they're not quite identical and this code already made it better, does it need to go the whole way...? (I will do this if required.) 15. Done ( http://docs.moodle.org/dev/Media_embedding ) - there's some nice detailed user-friendly documentation for you. Both parts are also covered very briefly in the phpdoc at top of medialib.php, as well as in detail in appropriate function phpdoc. In addition I ran codechecker and got to 0 errors on medialib.php and filter/mediaplugin and my part of lib/outputrenderers.php, and fixed the unit tests, and quickly checked that it still appears to be working.
          Hide
          Sam Marshall added a comment -

          I have now attached a test backup file - I made a small 6 second video and converted it into a zillion formats so that the backup fits as a Jira attachment.

          Will now update test script so it uses backup instead of including setup instructions to add everything.

          Show
          Sam Marshall added a comment - I have now attached a test backup file - I made a small 6 second video and converted it into a zillion formats so that the backup fits as a Jira attachment. Will now update test script so it uses backup instead of including setup instructions to add everything.
          Hide
          Sam Marshall added a comment -

          Updated test script to use simple backup file, making it significantly shorter. Also reorganised and clarified test. (I re-ran it, too, except for the mobile parts.)

          Show
          Sam Marshall added a comment - Updated test script to use simple backup file, making it significantly shorter. Also reorganised and clarified test. (I re-ran it, too, except for the mobile parts.)
          Hide
          Sam Marshall added a comment -

          I have updated the backup file as there were a couple of 'players' that I don't think were exercised properly. Added .ogg audio and, for comedy value, a .rm video.

          Show
          Sam Marshall added a comment - I have updated the backup file as there were a couple of 'players' that I don't think were exercised properly. Added .ogg audio and, for comedy value, a .rm video.
          Hide
          Sam Marshall added a comment -

          Note: I have also rebased to latest master. It still works, yay.

          Show
          Sam Marshall added a comment - Note: I have also rebased to latest master. It still works, yay.
          Hide
          Lael... added a comment -

          I haven't downloaded or looked at this patch. Interested though - I was looking at replacing flowplayer with jwplayer for our installation. I'm sure many schools would want to do the same thing easily (as we fall under the non commercial licence) if possible.

          Show
          Lael... added a comment - I haven't downloaded or looked at this patch. Interested though - I was looking at replacing flowplayer with jwplayer for our installation. I'm sure many schools would want to do the same thing easily (as we fall under the non commercial licence) if possible.
          Hide
          Sam Marshall added a comment -

          Lael:

          Yes this patch will allow you to customise media embedding across Moodle (although it will require some coding to do so).

          We will be using it to incorporate our own media player, which is currently based on JWPlayer. (Although I think we are planning to change to one based on FlowPlayer... but anyway, not the standard Moodle one for sure.) Our code will be weird and no use to anybody else, but just to say, this will definitely be possible.

          Show
          Sam Marshall added a comment - Lael: Yes this patch will allow you to customise media embedding across Moodle (although it will require some coding to do so). We will be using it to incorporate our own media player, which is currently based on JWPlayer. (Although I think we are planning to change to one based on FlowPlayer... but anyway, not the standard Moodle one for sure.) Our code will be weird and no use to anybody else, but just to say, this will definitely be possible.
          Hide
          Sam Marshall added a comment -

          I have rebased this patch against latest moodle master again in case anyone want sto review it (hint)

          Show
          Sam Marshall added a comment - I have rebased this patch against latest moodle master again in case anyone want sto review it (hint)
          Hide
          Sam Marshall added a comment -

          Removed peer reviewer name to indicate that this is awaiting peer review.

          Show
          Sam Marshall added a comment - Removed peer reviewer name to indicate that this is awaiting peer review.
          Hide
          Dan Poltawski added a comment -

          First comments - sorry for the delay in review:

          1. It'd be great if you can convert the unit tests to phpunit
          2. filter/mediaplugin/test/perftest.php - should that be there, not quite sure why we should be including this in the distribution? (Also now phpunit tests are in the tests directory it could be mistaken for a unit test directory)
          3. I'm not so sure about the hardcoded rank, just doesn't feel right to me (should our non-codey users be able to change that rank without a custom renderer?)
          4. get_filename() says:
                 * Obtains the filename from the moodle_url. This would be easier if
                 * moodle_url had get methods for some of its fields...
            

            Should we add the moodle_url methods for it?

          5. Strict standards:
            Strict Standards: Declaration of core_media_player_external::list_supported_urls() should be compatible with that of core_media_player::list_supported_urls() in /Users/danp/git/moodle/lib/medialib.php on line 487 Strict Standards: Declaration of core_media_player_swf::list_supported_urls() should be compatible with that of core_media_player::list_supported_urls() in /Users/danp/git/moodle/lib/medialib.php on line 1004 Strict Standards: Declaration of core_media_player_link::list_supported_urls() should be compatible with that of core_media_player::list_supported_urls() in /Users/danp/git/moodle/lib/medialib.php on line 1239
            
          6. Missing space after comment //NOTE: we can not use any link fallback because it breaks built-in player on iOS devices
          Show
          Dan Poltawski added a comment - First comments - sorry for the delay in review: It'd be great if you can convert the unit tests to phpunit filter/mediaplugin/test/perftest.php - should that be there, not quite sure why we should be including this in the distribution? (Also now phpunit tests are in the tests directory it could be mistaken for a unit test directory) I'm not so sure about the hardcoded rank, just doesn't feel right to me (should our non-codey users be able to change that rank without a custom renderer?) get_filename() says: * Obtains the filename from the moodle_url. This would be easier if * moodle_url had get methods for some of its fields... Should we add the moodle_url methods for it? Strict standards: Strict Standards: Declaration of core_media_player_external::list_supported_urls() should be compatible with that of core_media_player::list_supported_urls() in /Users/danp/git/moodle/lib/medialib.php on line 487 Strict Standards: Declaration of core_media_player_swf::list_supported_urls() should be compatible with that of core_media_player::list_supported_urls() in /Users/danp/git/moodle/lib/medialib.php on line 1004 Strict Standards: Declaration of core_media_player_link::list_supported_urls() should be compatible with that of core_media_player::list_supported_urls() in /Users/danp/git/moodle/lib/medialib.php on line 1239 Missing space after comment //NOTE: we can not use any link fallback because it breaks built-in player on iOS devices
          Hide
          Sam Marshall added a comment -

          Dan: Thanks very much for looking at this. I don't have time to fix these points today but will do so next week. Here are comments to your points:

          1. Yes OK, will do that, it will be my first time using phpunit, I need to learn sometime...

          2. Not certain about this, but I think the test should be included because if there are future changes to this code it would be useful to be able to compare performance? Regarding the directory name, I agree this is now confusing, perhaps a name like 'developers' would be appropriate?

          3. You can't just change rank; it was determined to be in a particular order that works, due to the way in which the various fallbacks operate. So I don't think users should be able to alter it - or if there were cases where they could alter it this would be limited, like a specific config checkbox which would cause the ranks of two players to switch, rather than just 'set any rank to anything'.

          The purpose of the rank setting is so that if you create a new 'player' object (i.e. yes you do need coding ability) then you can decide the correct place to slot it in among the existing filters (again to make it work in terms of the fallbacks). That's why I used the 10, 20, 30, 40 etc (leaving gaps). Possibly this situation is not explained clearly enough in comments?

          4. Yes probably, I will look into adding a get_filename method to moodle_url instead of adding sarky comments.

          5. Ooops, will fix.

          6. Will fix. (That comment is actually from the original code I didn't change but obviously as I touched the line, I should fix it.)

          Show
          Sam Marshall added a comment - Dan: Thanks very much for looking at this. I don't have time to fix these points today but will do so next week. Here are comments to your points: 1. Yes OK, will do that, it will be my first time using phpunit, I need to learn sometime... 2. Not certain about this, but I think the test should be included because if there are future changes to this code it would be useful to be able to compare performance? Regarding the directory name, I agree this is now confusing, perhaps a name like 'developers' would be appropriate? 3. You can't just change rank; it was determined to be in a particular order that works, due to the way in which the various fallbacks operate. So I don't think users should be able to alter it - or if there were cases where they could alter it this would be limited, like a specific config checkbox which would cause the ranks of two players to switch, rather than just 'set any rank to anything'. The purpose of the rank setting is so that if you create a new 'player' object (i.e. yes you do need coding ability) then you can decide the correct place to slot it in among the existing filters (again to make it work in terms of the fallbacks). That's why I used the 10, 20, 30, 40 etc (leaving gaps). Possibly this situation is not explained clearly enough in comments? 4. Yes probably, I will look into adding a get_filename method to moodle_url instead of adding sarky comments. 5. Ooops, will fix. 6. Will fix. (That comment is actually from the original code I didn't change but obviously as I touched the line, I should fix it.)
          Hide
          Sam Marshall added a comment -

          Requesting review again after fixes:

          1. Converted test to phpunit (was really easy!)

          2. Renamed confusing 'test' directory to 'dev' and added to the comment at the top explaining that the perftest script is to use if comparing performance of future changes.

          3. Added full explanation of rank.

          4. I have added get_path to moodle_url, including unit test, as a new first commit in this sequence. And changed code to call it

          5. Fixed.

          6. Fixed.

          Show
          Sam Marshall added a comment - Requesting review again after fixes: 1. Converted test to phpunit (was really easy!) 2. Renamed confusing 'test' directory to 'dev' and added to the comment at the top explaining that the perftest script is to use if comparing performance of future changes. 3. Added full explanation of rank. 4. I have added get_path to moodle_url, including unit test, as a new first commit in this sequence. And changed code to call it 5. Fixed. 6. Fixed.
          Hide
          Dan Poltawski added a comment -

          Did you push the change with the phpunit tests? Can't see them?

          Show
          Dan Poltawski added a comment - Did you push the change with the phpunit tests? Can't see them?
          Hide
          Sam Marshall added a comment -

          Dan: That's odd, while I do sometimes forget, this time I actually did push it - just checked now using the diff link in this bug, and the file list visible on github includes lib/tests/medialib.php. Could you have accidentally used a link to the commits instead?

          Diff link just to confirm (anchor tag is to the test script):

          https://github.com/sammarshallou/moodle/compare/master...MDL-29624-master#diff-16

          Show
          Sam Marshall added a comment - Dan: That's odd, while I do sometimes forget, this time I actually did push it - just checked now using the diff link in this bug, and the file list visible on github includes lib/tests/medialib.php. Could you have accidentally used a link to the commits instead? Diff link just to confirm (anchor tag is to the test script): https://github.com/sammarshallou/moodle/compare/master...MDL-29624-master#diff-16
          Hide
          Sam Marshall added a comment -

          Sorry, the test file that I forgot to convert is now converted.

          Show
          Sam Marshall added a comment - Sorry, the test file that I forgot to convert is now converted.
          Hide
          Dan Poltawski added a comment -

          Looks good, submitting for integration.

          Show
          Dan Poltawski added a comment - Looks good, submitting for integration.
          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
          Sam Marshall added a comment -

          Rebased and fixed conflict (by applying same minor change from MDL-32423 rawencodeurl->addslashes_js affecting 2 files used here, now only 1 copy due to reducing duplication...)

          Show
          Sam Marshall added a comment - Rebased and fixed conflict (by applying same minor change from MDL-32423 rawencodeurl->addslashes_js affecting 2 files used here, now only 1 copy due to reducing duplication...)
          Hide
          Andrew Nicols added a comment -

          Just noticed a bug from a rebase - tinymce 3.5 is now included and used so the preview.php when inserting moodle media is currently broken (preview.php missing in 3.5-dev1 too)

          Show
          Andrew Nicols added a comment - Just noticed a bug from a rebase - tinymce 3.5 is now included and used so the preview.php when inserting moodle media is currently broken (preview.php missing in 3.5-dev1 too)
          Hide
          Sam Marshall added a comment -

          Currently I hope to rebase this on Monday unless something else comes up.

          Andrew: Did you mean it's broken when you rebase this bug onto current, or is it broken anyway before you even apply this?

          Show
          Sam Marshall added a comment - Currently I hope to rebase this on Monday unless something else comes up. Andrew: Did you mean it's broken when you rebase this bug onto current, or is it broken anyway before you even apply this?
          Hide
          Andrew Nicols added a comment -

          It was broken before I tried a rebase.

          In the version of master this is based on, there are two TinyMCE folders for 3.4.6 and 3.5dev1. The only file in 3.4.6 is preview.php, and it's missing entirely from 3.5dev1.

          On latest master, TinyMCE 3.5 has now been integrated instead of 3.5dev1 so preview.php will need to be there instead.

          Show
          Andrew Nicols added a comment - It was broken before I tried a rebase. In the version of master this is based on, there are two TinyMCE folders for 3.4.6 and 3.5dev1. The only file in 3.4.6 is preview.php, and it's missing entirely from 3.5dev1. On latest master, TinyMCE 3.5 has now been integrated instead of 3.5dev1 so preview.php will need to be there instead.
          Hide
          Aparup Banerjee added a comment -

          thanks for that catch, just linking the tinymce upgrade to 3.5 here.

          Show
          Aparup Banerjee added a comment - thanks for that catch, just linking the tinymce upgrade to 3.5 here.
          Hide
          Sam Marshall added a comment -

          Thanks - I have pushed a rebased branch which now appears to work correctly (there was something else broken recently as well as the preview.php being in the wrong place).

          Show
          Sam Marshall added a comment - Thanks - I have pushed a rebased branch which now appears to work correctly (there was something else broken recently as well as the preview.php being in the wrong place).
          Hide
          Aparup Banerjee added a comment -

          Hi Sam,
          Thanks for rebasing that.

          i've had a look and have run this thru some checks, comments :

          • not really sure about the perftest being in there. however it looks safe+harmless - note however that the require config.php is falling short by one '../' for me.
          • the name core_media::OPTION_EMBED_OR_BLANK sounds like a question than an option, perhaps consider something like the name OPTION_FALLBACK_TO_BLANK. Hmm, OPTION_NO_LINK could be renamed to OPTION_NO_FALLBACK too - or would that be too generic?
          • unit test :
            • has $SERVER as global. did you mean $_SERVER?
          • summary from the (attached) smurf.xml file:
            code style
            • lib/medialib.php have define() but @var used in comment block. (http://docs.moodle.org/dev/Coding_style#Defines)
            • lib/tests/medialib_test.php
              • line 237, Private method name "medialib_testcase::assertContents" must be in lower-case letters only
              • line 515, space after 'foreach' and spaces around '=>' needed.
              • line 115 (whole file), it seems theres a bunch of missing scopes in lib/tests/weblib_test.php : 'public' scope missing from test_moodle_url_get_path() test. (not sure if this has to be fixed now, up to you, since its prevalent everywhere in this file.)

          there are a few phpdoc issues with the APIs : lib/medialib.php and lib/outputrenderers.php

          • core_media_player_external has @var in doc for constant.

          General thoughts:

          looks awesome!

          The whole separation between PDF's, 'other' files and audio/video+(urls-youtube) seems slightly biased towards audio/video media only when really what we are solving here in one library is simply the embedding of any embeddable files within the current display. I see this more in the name core_media_renderer::get_players() implying they are only playable content. However i can't think of a better term than 'player' (maybe err get_embeddable_objects()? ).

          I also wonder if the whole solution should be structured under core_files_xxx where xxx could be 'embedding'. To somehow say that its not just video/audio but really about embedding of any files for display within the current view. perhaps it might be a good idea to further discuss the name before it goes in.

          All that said though, i do like the sound of core_media :-p and its accompanying doc (http://docs.moodle.org/dev/Media_embedding).

          Everything else seems looks fine to me, with a few tweaks and establishing that 'core_media' name is fine, i'd feel safe that this goes into core. (ofcourse http://docs.moodle.org/dev/Frankenstyle#Core_subsystems etc would need to be updated too after it goes into core. )

          thats all, cheers!

          Show
          Aparup Banerjee added a comment - Hi Sam, Thanks for rebasing that. i've had a look and have run this thru some checks, comments : not really sure about the perftest being in there. however it looks safe+harmless - note however that the require config.php is falling short by one '../' for me. the name core_media::OPTION_EMBED_OR_BLANK sounds like a question than an option, perhaps consider something like the name OPTION_FALLBACK_TO_BLANK. Hmm, OPTION_NO_LINK could be renamed to OPTION_NO_FALLBACK too - or would that be too generic? unit test : has $SERVER as global. did you mean $_SERVER? summary from the (attached) smurf.xml file: code style lib/medialib.php have define() but @var used in comment block. ( http://docs.moodle.org/dev/Coding_style#Defines ) lib/tests/medialib_test.php line 237, Private method name "medialib_testcase::assertContents" must be in lower-case letters only line 515, space after 'foreach' and spaces around '=>' needed. line 115 (whole file), it seems theres a bunch of missing scopes in lib/tests/weblib_test.php : 'public' scope missing from test_moodle_url_get_path() test. (not sure if this has to be fixed now, up to you, since its prevalent everywhere in this file.) there are a few phpdoc issues with the APIs : lib/medialib.php and lib/outputrenderers.php core_media_player_external has @var in doc for constant. General thoughts: looks awesome! The whole separation between PDF's, 'other' files and audio/video+(urls-youtube) seems slightly biased towards audio/video media only when really what we are solving here in one library is simply the embedding of any embeddable files within the current display. I see this more in the name core_media_renderer::get_players() implying they are only playable content. However i can't think of a better term than 'player' (maybe err get_embeddable_objects()? ). I also wonder if the whole solution should be structured under core_files_xxx where xxx could be 'embedding'. To somehow say that its not just video/audio but really about embedding of any files for display within the current view. perhaps it might be a good idea to further discuss the name before it goes in. All that said though, i do like the sound of core_media :-p and its accompanying doc ( http://docs.moodle.org/dev/Media_embedding ). Everything else seems looks fine to me, with a few tweaks and establishing that 'core_media' name is fine, i'd feel safe that this goes into core. (ofcourse http://docs.moodle.org/dev/Frankenstyle#Core_subsystems etc would need to be updated too after it goes into core. ) thats all, cheers!
          Hide
          Aparup Banerjee added a comment -

          ps: noting that prior to the patch some players appear in as full a width as possible and its not so after the patch

          Show
          Aparup Banerjee added a comment - ps: noting that prior to the patch some players appear in as full a width as possible and its not so after the patch
          Hide
          Sam Marshall added a comment -

          Just to document - looking at this now - I'm starting with the smurf file...

          Show
          Sam Marshall added a comment - Just to document - looking at this now - I'm starting with the smurf file...
          Hide
          Sam Marshall added a comment -

          Thanks very much for your review comments. I've taken action based on them. Here are my responses:

          1) I tried to fix all errors and warnings from the smurf file, however I'm not able to re-run that (Eloy did show me where the script is, but it's too complicated) so I may have missed a few.

          There are some exceptions where I think it was giving an incorrect error re phpdoc or where I think it is better to change the coding style than my code (see the talk page for coding style). The main case of this is that I think it's better to not document functions that override a base class method (compared to copy-pasting the base class documentation onto every implementation, creating unhelpful duplication and making the code harder to read and maintain). So I've intentionally left these.

          Regarding files which are a mess in general, I have tried not to expand the scope of my changes i.e. I am only fixing coding style errors on lines that I touch. However, I do believe that in general, it is correct to always fix coding style errors on lines that you do change, so I've tried to do that.

          2) I understand why people (including me) feel that including the perf test script is a bit odd, however I spent some time on that script and it's pretty vital for this type of change - filters are frequently cited here as the cause of all our performance woes, and if you 'do it wrong' it is definitely possible to slow things down a bit. Maybe it can be deleted later if somebody decides that sort of thing shouldn't be in the source tree, or invents a proper place for it.

          3) You're right that the perf test script was also broken due to wrong require, I have fixed it and checked it actually works

          4) Regarding $SERVER in unit test. This was wrong but also kind of broken in other ways - the reset code in unit test was not necessary because Petr has since made the unit test code to automatically reset both $CFG and $_SERVER. I fixed it and removed the reset code.

          5) Regarding OPTION_EMBED_OR_BLANK, I have renamed it to OPTION_FALLBACK_TO_BLANK as suggested. Regarding OPTION_NO_LINK I think using the word 'NO_FALLBACK' would be confusing - typically this system actually does do 'fall back' (e.g. from flash player -> html 5) in most cases, so 'no link' is more accurate and I haven't changed it.

          6) About general comment re. embedding other things. I really want to avoid scope creep as it has taken an approximately infinite amount of time so far to get this in anyway The key focus here is embedding audio/video because these are really important and it was previously done in 3 different ways in 3 different parts of the code, which was terrible and meant it was impossible to customise.

          It would definitely be possible to use the same kind of code to embed other things apart from audio/video but I would much prefer to leave that for somebody else to do in a future enhancement. The current code for that is only in 1 place (afaik) (resourcelib, called from mod/resource, lesson, and url).

          If we did add embedding later for other things, I think the naming I've used is okay. The word 'media' is, IMO, fine for other types of things you might want to embed (a PDF, or a PowerPoint presentation, say). The word 'player' is sketchy but it's actually already sketchy for what I'm using it for - I have a 'player' that just outputs a link, for instance. I think player in this case is sort of more intended as a probably-unique word for something that would otherwise be totally generic like component or widget or whatever, and could well be used elsewhere in moodle.

          I rebased and pushed new version now, I think this is ready for consideration again. Waiting for full unit test run to complete in case anything else is wrong though (will post when it does).

          Show
          Sam Marshall added a comment - Thanks very much for your review comments. I've taken action based on them. Here are my responses: 1) I tried to fix all errors and warnings from the smurf file, however I'm not able to re-run that (Eloy did show me where the script is, but it's too complicated) so I may have missed a few. There are some exceptions where I think it was giving an incorrect error re phpdoc or where I think it is better to change the coding style than my code (see the talk page for coding style). The main case of this is that I think it's better to not document functions that override a base class method (compared to copy-pasting the base class documentation onto every implementation, creating unhelpful duplication and making the code harder to read and maintain). So I've intentionally left these. Regarding files which are a mess in general, I have tried not to expand the scope of my changes i.e. I am only fixing coding style errors on lines that I touch. However, I do believe that in general, it is correct to always fix coding style errors on lines that you do change, so I've tried to do that. 2) I understand why people (including me) feel that including the perf test script is a bit odd, however I spent some time on that script and it's pretty vital for this type of change - filters are frequently cited here as the cause of all our performance woes, and if you 'do it wrong' it is definitely possible to slow things down a bit. Maybe it can be deleted later if somebody decides that sort of thing shouldn't be in the source tree, or invents a proper place for it. 3) You're right that the perf test script was also broken due to wrong require, I have fixed it and checked it actually works 4) Regarding $SERVER in unit test. This was wrong but also kind of broken in other ways - the reset code in unit test was not necessary because Petr has since made the unit test code to automatically reset both $CFG and $_SERVER. I fixed it and removed the reset code. 5) Regarding OPTION_EMBED_OR_BLANK, I have renamed it to OPTION_FALLBACK_TO_BLANK as suggested. Regarding OPTION_NO_LINK I think using the word 'NO_FALLBACK' would be confusing - typically this system actually does do 'fall back' (e.g. from flash player -> html 5) in most cases, so 'no link' is more accurate and I haven't changed it. 6) About general comment re. embedding other things. I really want to avoid scope creep as it has taken an approximately infinite amount of time so far to get this in anyway The key focus here is embedding audio/video because these are really important and it was previously done in 3 different ways in 3 different parts of the code, which was terrible and meant it was impossible to customise. It would definitely be possible to use the same kind of code to embed other things apart from audio/video but I would much prefer to leave that for somebody else to do in a future enhancement. The current code for that is only in 1 place (afaik) (resourcelib, called from mod/resource, lesson, and url). If we did add embedding later for other things, I think the naming I've used is okay. The word 'media' is, IMO, fine for other types of things you might want to embed (a PDF, or a PowerPoint presentation, say). The word 'player' is sketchy but it's actually already sketchy for what I'm using it for - I have a 'player' that just outputs a link, for instance. I think player in this case is sort of more intended as a probably-unique word for something that would otherwise be totally generic like component or widget or whatever, and could well be used elsewhere in moodle. I rebased and pushed new version now, I think this is ready for consideration again. Waiting for full unit test run to complete in case anything else is wrong though (will post when it does).
          Hide
          Sam Marshall added a comment -

          The unit test run completed, so I think this is ready to go again.

          Show
          Sam Marshall added a comment - The unit test run completed, so I think this is ready to go again.
          Hide
          Aparup Banerjee added a comment -

          Thanks Sam, i'll look at your changes now.

          regarding the terms, thanks for your feedback and yes lets not scope creep

          Show
          Aparup Banerjee added a comment - Thanks Sam, i'll look at your changes now. regarding the terms, thanks for your feedback and yes lets not scope creep
          Hide
          Aparup Banerjee added a comment -

          Ok this is ready for testing now on some more different environments.

          This has been integrated into master now.

          Hmm, we'll have to tweak the code checker policies on phpdoc requirements for implementations for abstract methods.

          Sam, theres always http://docs.moodle.org/dev/Coding_style#Useful_tools too.

          Show
          Aparup Banerjee added a comment - Ok this is ready for testing now on some more different environments. This has been integrated into master now. Hmm, we'll have to tweak the code checker policies on phpdoc requirements for implementations for abstract methods. Sam, theres always http://docs.moodle.org/dev/Coding_style#Useful_tools too.
          Hide
          Dan Poltawski added a comment -

          Apu: Sams point about the coding checker is he can't check just the diff with those tools!

          Show
          Dan Poltawski added a comment - Apu: Sams point about the coding checker is he can't check just the diff with those tools!
          Hide
          Aparup Banerjee added a comment -

          ah sorry, missed that out in dev chat

          Show
          Aparup Banerjee added a comment - ah sorry, missed that out in dev chat
          Hide
          Rossiani Wijaya added a comment -

          This issue is working great in desktop browser.

          Currently, I don't have access to test this on mobile devices.

          Will update the test result for mobile device later.

          Show
          Rossiani Wijaya added a comment - This issue is working great in desktop browser. Currently, I don't have access to test this on mobile devices. Will update the test result for mobile device later.
          Hide
          Rossiani Wijaya added a comment -

          This is working fine in android and iPhone.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This is working fine in android and iPhone. Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This is now part of Moodle and a few millions people around the globe will be using it soon. Isn't that awesome?

          Many, many thanks and don't forget http://youtu.be/4N7dPaP5Z8U

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This is now part of Moodle and a few millions people around the globe will be using it soon. Isn't that awesome? Many, many thanks and don't forget http://youtu.be/4N7dPaP5Z8U Closing, ciao
          Hide
          Petr Škoda added a comment - - edited

          oh! people, you can not just add hacks to tinymce plugins - we MUST maintain all our changes in https://github.com/moodle/custom-tinymce - I was working on MDL-33114 when I noticed this, please next time create separate pulls for any tinymce hacks, thanks.

          Show
          Petr Škoda added a comment - - edited oh! people, you can not just add hacks to tinymce plugins - we MUST maintain all our changes in https://github.com/moodle/custom-tinymce - I was working on MDL-33114 when I noticed this, please next time create separate pulls for any tinymce hacks, thanks.
          Hide
          Petr Škoda added a comment -

          Hmmm, this seems to break big time when slasharguments are not supported. Aparup and Sam: did anybody from you considered/tested it?

          Show
          Petr Škoda added a comment - Hmmm, this seems to break big time when slasharguments are not supported. Aparup and Sam: did anybody from you considered/tested it?
          Hide
          Dan Poltawski added a comment - - edited

          Petr: thanks for spotting this and sorry we missed this. Unfortunately slasharguments are one of those things which are easy to miss. It'd be good if lib/editor/tinymce/readme_moodle.txt had clearer instructions on customsiation changes.

          Though Petr - Sam wanted your expertise on this issue and was hoping for a review between December and April when you were assigned as a peer reviewer on this so its a shame you were only able to spot this after it was integrated.

          Show
          Dan Poltawski added a comment - - edited Petr: thanks for spotting this and sorry we missed this. Unfortunately slasharguments are one of those things which are easy to miss. It'd be good if lib/editor/tinymce/readme_moodle.txt had clearer instructions on customsiation changes. Though Petr - Sam wanted your expertise on this issue and was hoping for a review between December and April when you were assigned as a peer reviewer on this so its a shame you were only able to spot this after it was integrated.
          Hide
          Martin Dougiamas added a comment -

          Helen can you please check the docs and 2.3 release notes? Despite the tags it looks like this was not documented well.

          Show
          Martin Dougiamas added a comment - Helen can you please check the docs and 2.3 release notes? Despite the tags it looks like this was not documented well.
          Hide
          Helen Foster added a comment -

          Apologies for the delay in documenting this improvement. It's now explained in http://docs.moodle.org/23/en/Media_embedding and mentioned in http://docs.moodle.org/23/en/Multimedia_plugins_filter and http://docs.moodle.org/dev/Moodle_2.3_release_notes (thanks to Michael d).

          Sam, or anyone watching, if you have chance to review the documentation and fix or let me know if I've got anything wrong, that would be great!

          Show
          Helen Foster added a comment - Apologies for the delay in documenting this improvement. It's now explained in http://docs.moodle.org/23/en/Media_embedding and mentioned in http://docs.moodle.org/23/en/Multimedia_plugins_filter and http://docs.moodle.org/dev/Moodle_2.3_release_notes (thanks to Michael d). Sam, or anyone watching, if you have chance to review the documentation and fix or let me know if I've got anything wrong, that would be great!
          Hide
          Sam Marshall added a comment -

          Thank you Helen! I just added one line to the documentation to indicate that if you only want it to work in File module embedding, you don't necessarily need to enable the media filter, other than that I didn't spot anything incorrect.

          (Sorry for slow response as I was away & am still catching up on Moodle bugs)

          Show
          Sam Marshall added a comment - Thank you Helen! I just added one line to the documentation to indicate that if you only want it to work in File module embedding, you don't necessarily need to enable the media filter, other than that I didn't spot anything incorrect. (Sorry for slow response as I was away & am still catching up on Moodle bugs)
          Hide
          Helen Foster added a comment -

          Thanks Sam, I was actually wondering about whether the multimedia filter was necessary for file and URL resources and the lesson activity, but didn't have chance to test it out!

          Show
          Helen Foster added a comment - Thanks Sam, I was actually wondering about whether the multimedia filter was necessary for file and URL resources and the lesson activity, but didn't have chance to test it out!
          Hide
          Aparup Banerjee added a comment - - edited

          added api_change (just in case it was missed before) as i'm not seeing this in http://docs.moodle.org/dev/Core_APIs (eyes are blurry atm so i might have missed it)
          Edit: i've added a stub @ http://docs.moodle.org/dev/Core_APIs#Media_API_.28media.29 but the page name might need to be renamed perhaps, please have a look oh magical ones.

          Show
          Aparup Banerjee added a comment - - edited added api_change (just in case it was missed before) as i'm not seeing this in http://docs.moodle.org/dev/Core_APIs (eyes are blurry atm so i might have missed it) Edit: i've added a stub @ http://docs.moodle.org/dev/Core_APIs#Media_API_.28media.29 but the page name might need to be renamed perhaps, please have a look oh magical ones.
          Hide
          Sam Marshall added a comment -

          I tweaked your stub text slightly but agree maybe the page needs renaming, the magical ones can decide that...

          I also did a quick read-through of the media embedding documentation page itself and updated a minor detail where somebody left a question in the page. Not sure it needs the 'work in progress' template any more either, but I left removing that to the magical ones.

          Show
          Sam Marshall added a comment - I tweaked your stub text slightly but agree maybe the page needs renaming, the magical ones can decide that... I also did a quick read-through of the media embedding documentation page itself and updated a minor detail where somebody left a question in the page. Not sure it needs the 'work in progress' template any more either, but I left removing that to the magical ones.
          Hide
          Marina Glancy added a comment -

          Hi Sam, I know this issue was closed year ago but can you tell me what was the idea for filtering the string here:
          https://github.com/moodle/moodle/blob/master/admin/settings/appearance.php#L138
          It's related to MDL-37979
          Cheers,
          Marina

          Show
          Marina Glancy added a comment - Hi Sam, I know this issue was closed year ago but can you tell me what was the idea for filtering the string here: https://github.com/moodle/moodle/blob/master/admin/settings/appearance.php#L138 It's related to MDL-37979 Cheers, Marina
          Hide
          Sam Marshall added a comment -

          Marina (sorry for slow response, I am catching up with a backlog of tracker email):

          I think that string was done using markdown format just so that I could have formatting in it without needing to put HTML code into the language file. I don't think it should actually be filtered, so it would probably be better to turn off filtering for that string (can we still have markdown format without running filters?).

          Show
          Sam Marshall added a comment - Marina (sorry for slow response, I am catching up with a backlog of tracker email): I think that string was done using markdown format just so that I could have formatting in it without needing to put HTML code into the language file. I don't think it should actually be filtered, so it would probably be better to turn off filtering for that string (can we still have markdown format without running filters?).
          Hide
          Marina Glancy added a comment -

          Sam, thanks for answer.
          Yes we can indicate 'filter'=>false in options to do only formatting without filtering. I'll fix it in my issue myself, I was just asking if maybe there was a reason for filtering.

          Show
          Marina Glancy added a comment - Sam, thanks for answer. Yes we can indicate 'filter'=>false in options to do only formatting without filtering. I'll fix it in my issue myself, I was just asking if maybe there was a reason for filtering.

            People

            • Votes:
              12 Vote for this issue
              Watchers:
              16 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: