Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.6
    • Fix Version/s: 2.6
    • Component/s: SCORM
    • Labels:
    • Testing Instructions:
      Hide
      1. Enter a SCORM as a student and make sure no new JS errors occur (ignore any JS errors that previously existed) from the YUI3 code
      2. Test the new settings that are added -
        • scorm/nav - Module level setting - Controls wether the SCORM navigation panel is disabled, under content or floating.
          • No - when set to 'No' the navigation panel should not be displayed.
          • Under content - when set to 'Under content' the navigation panel should be displayed below the SCORM content in the center.
          • Floating - when set to 'Floating' the navigation panel should appear at position specified by the settings - scorm/navpositionleft and scorm/navpositiontop. The navigation panel when set to floating can be dragged around anywhere in the window.
            • scorm/navpositionleft and scorm/navpositiontop should be disabled when scorm/nav is not set to 'Floating'.
        • scorm/collapsetocwinsize - Site level setting - set a value and enter a SCORM package.
          • Reduce the size (width) of the browser window and notice the TOC should collapse automatically below the corresponding window size value.
          • Increase the width of window and the TOC should re-appear.
      3. TOC toggle button - It should appear next to title of the SCORM package. On pressing the toggle button in state < - will collapse the TOC. When pressed in state > - will expand the TOC.
        • When the TOC is forced collapsed by pressing the toggle button the TOC should not re-appear on increasing/decreasing the width of the browser window.
        • Once TOC is un-collapsed by pressing the TOC toggle button >, the TOC should again collapse when the window size is reduced below scorm/collapsetocwinsize value.
        • When TOC is disabled the TOC toggle button should not be visible.
      4. Responsive - The TOC and content should adjust to the available window size (width). The TOC will be in collapsed state by default on small screen devices like iPads and mobile phones, if scorm/collapsetocwinsize is appropriately set.
      5. TOC Tree - The TOC should be correctly rendered with YUI3 sm-treeview.
        • Any clickable nodes of the tree should navigate to the corresponding content.
        • The active content item should be highlighted in the TOC tree, also while navigating with navigation buttons.
        • TOC can be resized as before.
      6. Navigation buttons - the Next/previous navigation buttons should be enabled/disabled when content can/cannot be navigated forward/backward respectively.
      7. Content should not be trimmed at the bottom - specially on iOS devices (Safari) and should be scrollable with touch gestures.
      8. Upgrading - while upgrading the new scorm/nav setting should replace the scorm/hidenav setting and the corresponding value should be set for scorm/nav.
      Show
      Enter a SCORM as a student and make sure no new JS errors occur (ignore any JS errors that previously existed) from the YUI3 code Test the new settings that are added - scorm/nav - Module level setting - Controls wether the SCORM navigation panel is disabled, under content or floating. No - when set to 'No' the navigation panel should not be displayed. Under content - when set to 'Under content' the navigation panel should be displayed below the SCORM content in the center. Floating - when set to 'Floating' the navigation panel should appear at position specified by the settings - scorm/navpositionleft and scorm/navpositiontop . The navigation panel when set to floating can be dragged around anywhere in the window. scorm/navpositionleft and scorm/navpositiontop should be disabled when scorm/nav is not set to 'Floating'. scorm/collapsetocwinsize - Site level setting - set a value and enter a SCORM package. Reduce the size (width) of the browser window and notice the TOC should collapse automatically below the corresponding window size value. Increase the width of window and the TOC should re-appear. TOC toggle button - It should appear next to title of the SCORM package. On pressing the toggle button in state < - will collapse the TOC. When pressed in state > - will expand the TOC. When the TOC is forced collapsed by pressing the toggle button the TOC should not re-appear on increasing/decreasing the width of the browser window. Once TOC is un-collapsed by pressing the TOC toggle button > , the TOC should again collapse when the window size is reduced below scorm/collapsetocwinsize value. When TOC is disabled the TOC toggle button should not be visible. Responsive - The TOC and content should adjust to the available window size (width). The TOC will be in collapsed state by default on small screen devices like iPads and mobile phones, if scorm/collapsetocwinsize is appropriately set. TOC Tree - The TOC should be correctly rendered with YUI3 sm-treeview. Any clickable nodes of the tree should navigate to the corresponding content. The active content item should be highlighted in the TOC tree, also while navigating with navigation buttons. TOC can be resized as before. Navigation buttons - the Next/previous navigation buttons should be enabled/disabled when content can/cannot be navigated forward/backward respectively. Content should not be trimmed at the bottom - specially on iOS devices (Safari) and should be scrollable with touch gestures. Upgrading - while upgrading the new scorm/nav setting should replace the scorm/hidenav setting and the corresponding value should be set for scorm/nav .
    • Affected Branches:
      MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull Master Branch:
      master_MDL-39910

      Description

      GSOC 2013 project
      The SCORM player currently uses YUI2 code - it should be converted to YUI3 and we should add an HTML5 version of the player that can be used as a replacement for compatible browsers - especially to improve display on mobile devices.

        Gliffy Diagrams

        1. screenshot.png
          54 kB

          Issue Links

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            Hi, Mayank.

            Normally we don't put a real version number in the fixVersion field until the issue is ready for integration. At triage we can put a backlog (FRONTEND or BACKEND) or leave it blank if it's work for someone outside of HQ's teams.

            Show
            salvetore Michael de Raadt added a comment - Hi, Mayank. Normally we don't put a real version number in the fixVersion field until the issue is ready for integration. At triage we can put a backlog (FRONTEND or BACKEND) or leave it blank if it's work for someone outside of HQ's teams.
            Hide
            danmarsden Dan Marsden added a comment -

            Hi Andrew - any chance you could help to review Mayanks code this week? - particularly the js changes in module.js

            Show
            danmarsden Dan Marsden added a comment - Hi Andrew - any chance you could help to review Mayanks code this week? - particularly the js changes in module.js
            Hide
            danmarsden Dan Marsden added a comment -

            ..might be useful for you to know - we've removed a lot of the browser sniffing and moved to using iframe instead of object in the master branch already.

            Show
            danmarsden Dan Marsden added a comment - ..might be useful for you to know - we've removed a lot of the browser sniffing and moved to using iframe instead of object in the master branch already.
            Hide
            danmarsden Dan Marsden added a comment -

            ping - Andrew, any chance you could help out here this week? - I really need some help reviewing that JS - please?

            Show
            danmarsden Dan Marsden added a comment - ping - Andrew, any chance you could help out here this week? - I really need some help reviewing that JS - please?
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Sorry - I'm away at present. Here are a few comments though:

            I think it would probably be too much to ask you do convert this to a
            shifted YUI module at this time so don't worry about that (we'll get there
            eventually).

            Could you run the JS through jshint (with our cnfig) and correct any lines
            you're already changing. The ones that stand out obviously are things like
            non-typed equality checks ((nav_display != 0) instead of (nav_display !== 0)).

            Also, I'm not ovely familiar with Tree yet, so please excuse any schoolboy
            questions!

            mod/scorm/module.js:

            • lines 57-59: indentation
            • lines 63-69: Is there any benefit to writing this using tree.children and then
              testing each returned child against Node.canHaveChildren instead? Your
              way may be more efficient, but I'm not sure.
            • 71: Can you raise a bug for us to upgrade to a newer version of YUI and
              make a TODO in the code to remove this when we have done so
            • 85: Assume that srcNode is not already a Y.Node - why not (this is
              probably valid just want to check).
            • 86: We typically use SELECTORS and I'd prefer we keep things consisent, but sourceSelectors will suffice
            • 93: We do we do this additional assignment and duplicate line 86?
            • 102: For scoid, shoudl we be using data Attributes (getData handles this
              in YUI across older browsers)
            • 172: No need to call Y.one as a constructor - Y.one will suffice (e.g.
              cut the new)
            • 204: Can we add a class rather than set style? This would mean that
              themers can make changes which you hadn't envisaged
            • 234-235: Same as 204
            • 245: Please use M.util.get_string()
            • 246-...: Liek the SELECTORS var above, can we add a CLASSES object which
              contains these. I see you're using YUI grids, and these class names may
              confuse people so a more descriptive variable name with comment above may
              be particularly beneficical here
            • 394: AIUI, you can just use .children (and drop rootNode)

            Got to crack on with something else for a bit - will come back to this
            shortly.
            The more I look at some of this, the more I wonder whether it wouldn't be
            worth investing some time to move some of it to shifted modules. Things
            like lines 726 - 773 feel wrong - calling Y.use() in a callback feel very
            wrong. Aside from the fact you're just returning the same Y that you're
            using to generate it, and it's generating an unnecessary function in
            a function with no obvious benefit.

            Will try and look at this some more in a few hours.

            Show
            dobedobedoh Andrew Nicols added a comment - Sorry - I'm away at present. Here are a few comments though: I think it would probably be too much to ask you do convert this to a shifted YUI module at this time so don't worry about that (we'll get there eventually). Could you run the JS through jshint (with our cnfig) and correct any lines you're already changing. The ones that stand out obviously are things like non-typed equality checks ((nav_display != 0) instead of (nav_display !== 0)). Also, I'm not ovely familiar with Tree yet, so please excuse any schoolboy questions! mod/scorm/module.js: lines 57-59: indentation lines 63-69: Is there any benefit to writing this using tree.children and then testing each returned child against Node.canHaveChildren instead? Your way may be more efficient, but I'm not sure. 71: Can you raise a bug for us to upgrade to a newer version of YUI and make a TODO in the code to remove this when we have done so 85: Assume that srcNode is not already a Y.Node - why not (this is probably valid just want to check). 86: We typically use SELECTORS and I'd prefer we keep things consisent, but sourceSelectors will suffice 93: We do we do this additional assignment and duplicate line 86? 102: For scoid, shoudl we be using data Attributes (getData handles this in YUI across older browsers) 172: No need to call Y.one as a constructor - Y.one will suffice (e.g. cut the new) 204: Can we add a class rather than set style? This would mean that themers can make changes which you hadn't envisaged 234-235: Same as 204 245: Please use M.util.get_string() 246-...: Liek the SELECTORS var above, can we add a CLASSES object which contains these. I see you're using YUI grids, and these class names may confuse people so a more descriptive variable name with comment above may be particularly beneficical here 394: AIUI, you can just use .children (and drop rootNode) Got to crack on with something else for a bit - will come back to this shortly. The more I look at some of this, the more I wonder whether it wouldn't be worth investing some time to move some of it to shifted modules. Things like lines 726 - 773 feel wrong - calling Y.use() in a callback feel very wrong. Aside from the fact you're just returning the same Y that you're using to generate it, and it's generating an unnecessary function in a function with no obvious benefit. Will try and look at this some more in a few hours.
            Hide
            danmarsden Dan Marsden added a comment -

            thanks heaps Andrew - that's a great start, Mayank does that all make sense to you?

            Show
            danmarsden Dan Marsden added a comment - thanks heaps Andrew - that's a great start, Mayank does that all make sense to you?
            Hide
            mayank_gupta2005 Mayank Gupta added a comment - - edited

            Thanks a lot Andrew for the feedback.
            As for -

            • 63-69: If I got it right then - I think using this would be probably more efficient than looping with tree.children - in cases when there is a larger number of children and relatively smaller subset of children nodes that canHaveChildren. Also it needs to be a recursive function when using tree.children testing each child for its children and so on.
            • 85: The treeviews and other modules (sm-menu, etc) in YUI Gallery take the source node selector in the constructor. sm-treeview does not render a tree from markup so while adding the function to parse the markup I decided to use the source selector as well. Will update it to Y.node in the function call instead.

            Dan: Yes, definitely it does.

            Show
            mayank_gupta2005 Mayank Gupta added a comment - - edited Thanks a lot Andrew for the feedback. As for - 63-69: If I got it right then - I think using this would be probably more efficient than looping with tree.children - in cases when there is a larger number of children and relatively smaller subset of children nodes that canHaveChildren. Also it needs to be a recursive function when using tree.children testing each child for its children and so on. 85: The treeviews and other modules (sm-menu, etc) in YUI Gallery take the source node selector in the constructor. sm-treeview does not render a tree from markup so while adding the function to parse the markup I decided to use the source selector as well. Will update it to Y.node in the function call instead. Dan: Yes, definitely it does.
            Hide
            dobedobedoh Andrew Nicols added a comment -
            • 63-69: totally agree - thanks for the explanation. I hadn't considered the subchildren
            • 85: thanks

            Just another comment while I was looking through:

            • 244-254: You could make use of chaining to reduce calls to Y.one and make it clearer what you're doing. E.g. :

              Y.one('#scorm_toc_toggle_btn').
                  setHTML('&gt;').
                  set('title', M.str.moodle.show);
              Y.one('#scorm_content').
                  removeClass('yui3-u-3-4').
                  addClass('yui3-u-1');

              (obviously taking on the M.util.get_string comments, and use of a SELECTORS var)

            • line 283: Missing radix in parseInt (If you had a width of 080 or similar, then that it would be interpretted as being in base 8) - unlikely to be an issue, but you never know what bizarre browser bug is around the corner

            Another comment, and Mary Evans or others (Frédéric Massart?) may have more pertinent views on it, but I see you've added some CSS styles for the YUI grid patterns. I wonder if perhaps we shouldn't do so and should have appropriately descriptive class names instead.
            Also, the styles should be broken into multiple lines for future git blamability, and readability.

            If it's okay with you Mayank, I'm going to pull this out of Peer Review for the moment while you look at the comments I've raised already as I'm covering ground I've already covered the further I get (get_string, etc). When you're ready, put it up for peer review again and I'll take a further look.

            Thanks,

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - 63-69: totally agree - thanks for the explanation. I hadn't considered the subchildren 85: thanks Just another comment while I was looking through: 244-254: You could make use of chaining to reduce calls to Y.one and make it clearer what you're doing. E.g. : Y.one('#scorm_toc_toggle_btn'). setHTML('&gt;'). set('title', M.str.moodle.show); Y.one('#scorm_content'). removeClass('yui3-u-3-4'). addClass('yui3-u-1'); (obviously taking on the M.util.get_string comments, and use of a SELECTORS var) line 283: Missing radix in parseInt (If you had a width of 080 or similar, then that it would be interpretted as being in base 8) - unlikely to be an issue, but you never know what bizarre browser bug is around the corner Another comment, and Mary Evans or others ( Frédéric Massart ?) may have more pertinent views on it, but I see you've added some CSS styles for the YUI grid patterns. I wonder if perhaps we shouldn't do so and should have appropriately descriptive class names instead. Also, the styles should be broken into multiple lines for future git blamability, and readability. If it's okay with you Mayank, I'm going to pull this out of Peer Review for the moment while you look at the comments I've raised already as I'm covering ground I've already covered the further I get (get_string, etc). When you're ready, put it up for peer review again and I'll take a further look. Thanks, Andrew
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Sorry - apparently I forgot to hit the finish button

            Show
            dobedobedoh Andrew Nicols added a comment - Sorry - apparently I forgot to hit the finish button
            Hide
            lazydaisy Mary Evans added a comment -

            Regarding the YUI grid CSS I think using the predefined YUI grid class selectors is acceptable, but of course there is nothing stopping you adding semantic class names to them if you think it necessary for the reason you give Andrew.

            Show
            lazydaisy Mary Evans added a comment - Regarding the YUI grid CSS I think using the predefined YUI grid class selectors is acceptable, but of course there is nothing stopping you adding semantic class names to them if you think it necessary for the reason you give Andrew.
            Hide
            mayank_gupta2005 Mayank Gupta added a comment -

            Thanks Mary. For now - leaving the YUI grid classes as-is in style.css.

            Andrew: As per your suggestion, I have added the cssclasses object that helps in somewhat describing what these classes are used for.
            I have also updated the branch and made changes according to your other feedback.
            style.css would be updated using block style as a part of MDL-41216.
            It would be great if you could look at it again.

            The diff for the new changes can be viewed here -
            https://github.com/mayankgupta/moodle/compare/46cb169724...1b6081430e

            Thanks!

            Show
            mayank_gupta2005 Mayank Gupta added a comment - Thanks Mary. For now - leaving the YUI grid classes as-is in style.css. Andrew: As per your suggestion, I have added the cssclasses object that helps in somewhat describing what these classes are used for. I have also updated the branch and made changes according to your other feedback. style.css would be updated using block style as a part of MDL-41216 . It would be great if you could look at it again. The diff for the new changes can be viewed here - https://github.com/mayankgupta/moodle/compare/46cb169724...1b6081430e Thanks!
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hi Mayank,

            Just wanted to let you know, I've not forgotten about this. Just been away over the weekend. Hoping to look again tomorrow.

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Hi Mayank, Just wanted to let you know, I've not forgotten about this. Just been away over the weekend. Hoping to look again tomorrow. Andrew
            Hide
            danmarsden Dan Marsden added a comment -

            ping - Hey Andrew - any chance you could look at this again this week? - thanks heaps!

            Show
            danmarsden Dan Marsden added a comment - ping - Hey Andrew - any chance you could look at this again this week? - thanks heaps!
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hi Mayank,

            Sorry for the delay. I've been off work recently.

            This looks great - just a few minor minor things and it should be good for integration:

            • scorm_content.setStyle('width', ''); - I think it should be null rather than '' if you're trying to unset. '' may work too, but I feel that null is probably clearer

            So looks like just one thing actually.

            I haven't had a chance to run the code as I don't have any SCORM content, and there aren't testing instructions, but the code looks good in theory to me.

            Cheers,

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Hi Mayank, Sorry for the delay. I've been off work recently. This looks great - just a few minor minor things and it should be good for integration: scorm_content.setStyle('width', ''); - I think it should be null rather than '' if you're trying to unset. '' may work too, but I feel that null is probably clearer So looks like just one thing actually. I haven't had a chance to run the code as I don't have any SCORM content, and there aren't testing instructions, but the code looks good in theory to me. Cheers, Andrew
            Hide
            danmarsden Dan Marsden added a comment -

            Thanks Andrew! - Mayank can you please tidy that last bit up, add testing instructions, create a new branch in your repo that rebases this work into a single commit (you will need to rebase ontop of latest master as well)

            thanks!

            Show
            danmarsden Dan Marsden added a comment - Thanks Andrew! - Mayank can you please tidy that last bit up, add testing instructions, create a new branch in your repo that rebases this work into a single commit (you will need to rebase ontop of latest master as well) thanks!
            Hide
            matteo Matteo Scaramuccia added a comment -

            Hi Mayank,
            regarding with https://github.com/mayankgupta/moodle/compare/master_MDL-40803_combine_hidenav_navpositiontype_as_nav_setting#L3R109 , is -100 the appropriate value? Guessing for a typo. The same for navpositiontop.

            @Dan: I hadn't the time for a run yet.

            HTH,
            Matteo

            Show
            matteo Matteo Scaramuccia added a comment - Hi Mayank, regarding with https://github.com/mayankgupta/moodle/compare/master_MDL-40803_combine_hidenav_navpositiontype_as_nav_setting#L3R109 , is -100 the appropriate value? Guessing for a typo. The same for navpositiontop . @Dan: I hadn't the time for a run yet. HTH, Matteo
            Hide
            danmarsden Dan Marsden added a comment -

            good spotting - yeah the default should be the same as in install.xml - Mayank can you please tidy that up too?

            Show
            danmarsden Dan Marsden added a comment - good spotting - yeah the default should be the same as in install.xml - Mayank can you please tidy that up too?
            Hide
            danmarsden Dan Marsden added a comment -

            how are you going Mayank - would be great if we could get this into integration review before Monday so it can be included in next weeks review!

            Show
            danmarsden Dan Marsden added a comment - how are you going Mayank - would be great if we could get this into integration review before Monday so it can be included in next weeks review!
            Hide
            mayank_gupta2005 Mayank Gupta added a comment -

            Thanks Andrew - updated it to null.

            Matteo - Thanks for pointing out. The default value should be -100 for scorm/navpositiontop and scorm/navpositionleft as the default value for scorm/nav is undercontent - missed it while updating it through XMLDB editor.

            Dan - Apologies for the delay, rebased the branch on the latest master and squashed the commits.

            Attaching a SCORM package for testing.

            Thanks,
            Mayank

            Show
            mayank_gupta2005 Mayank Gupta added a comment - Thanks Andrew - updated it to null. Matteo - Thanks for pointing out. The default value should be -100 for scorm/navpositiontop and scorm/navpositionleft as the default value for scorm/nav is undercontent - missed it while updating it through XMLDB editor. Dan - Apologies for the delay, rebased the branch on the latest master and squashed the commits. Attaching a SCORM package for testing. Thanks, Mayank
            Hide
            damyon Damyon Wiese added a comment -

            Thanks for working on this Mayank.

            It will be a good improvement to the module and this code is almost there. It just needs a few things fixed before it can be integrated.

            Here is the list of things I spotted while reviewing this patch:

            Upgrade code:

            This step:

            +        $scorms = $DB->get_recordset('scorm');
            +        foreach($scorms as $scorm) {
            +            if ($scorm->hidenav == 1) { // Update nav setting to disable navigation buttons.
            +                $scorm->nav = 0;
            +            } else { // Update nav setting to show floating navigation buttons under TOC.
            +                $scorm->nav = 2;
            +                $scorm->navpositionleft = 215;
            +                $scorm->navpositiontop = 300;
            +            }
            +            $DB->update_record('scorm', $scorm);
            +        }
            +        $scorms->close();
            

            Could be done in 3 $DB->update_field calls for all records in the database. This will perform much better on large installs with thousands of scorms.
            This same upgrade code should be checking if the "hidenav" column exists before running. (We like to be ultra-cautious with upgrade code).

            Coding style:
            All comments should start with a capital letter and end with a punctuation mark.
            There is trailing whitespace in "mod/scorm/module.js"
            There is trailing whitespace in "mod/scorm/styles.css"

            Accessibility:
            All click handlers should also allow keyboard navigation (e.g. key handlers for space/enter).

            CSS:
            All module style rules need to be prefixed with the page id (#page-mod-scorm-player) otherwise they will interfere with other pages in Moodle. Especially the non-specific style rules like this: ".yui3-skin-sam .yui3-treeview-icon "

            So - not too much to fix. I'm reopening this for this week, please resubmit once these issues have been addressed.

            Thanks, Damyon

            Show
            damyon Damyon Wiese added a comment - Thanks for working on this Mayank. It will be a good improvement to the module and this code is almost there. It just needs a few things fixed before it can be integrated. Here is the list of things I spotted while reviewing this patch: Upgrade code: This step: + $scorms = $DB->get_recordset('scorm'); + foreach($scorms as $scorm) { + if ($scorm->hidenav == 1) { // Update nav setting to disable navigation buttons. + $scorm->nav = 0; + } else { // Update nav setting to show floating navigation buttons under TOC. + $scorm->nav = 2; + $scorm->navpositionleft = 215; + $scorm->navpositiontop = 300; + } + $DB->update_record('scorm', $scorm); + } + $scorms->close(); Could be done in 3 $DB->update_field calls for all records in the database. This will perform much better on large installs with thousands of scorms. This same upgrade code should be checking if the "hidenav" column exists before running. (We like to be ultra-cautious with upgrade code). Coding style: All comments should start with a capital letter and end with a punctuation mark. There is trailing whitespace in "mod/scorm/module.js" There is trailing whitespace in "mod/scorm/styles.css" Accessibility: All click handlers should also allow keyboard navigation (e.g. key handlers for space/enter). CSS: All module style rules need to be prefixed with the page id (#page-mod-scorm-player) otherwise they will interfere with other pages in Moodle. Especially the non-specific style rules like this: ".yui3-skin-sam .yui3-treeview-icon " So - not too much to fix. I'm reopening this for this week, please resubmit once these issues have been addressed. Thanks, Damyon
            Hide
            cibot CiBoT added a comment -

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

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            mayank_gupta2005 Mayank Gupta added a comment -

            Hi Damyon,

            Thanks for pointing out the issues.
            I have fixed these issues as a part of commit - https://github.com/mayankgupta/moodle/commit/5f4ca3b47220a2bdc6a4656e41221e71d9e5a464
            (assuming you meant - $DB->set_field instead of $DB->update_field)

            Please do let me know if I am missing something.

            Thanks,
            Mayank

            Show
            mayank_gupta2005 Mayank Gupta added a comment - Hi Damyon, Thanks for pointing out the issues. I have fixed these issues as a part of commit - https://github.com/mayankgupta/moodle/commit/5f4ca3b47220a2bdc6a4656e41221e71d9e5a464 (assuming you meant - $DB->set_field instead of $DB->update_field) Please do let me know if I am missing something. Thanks, Mayank
            Hide
            danmarsden Dan Marsden added a comment -

            Thanks Mayank - lets push this through integration again next week.

            Show
            danmarsden Dan Marsden added a comment - Thanks Mayank - lets push this through integration again next week.
            Hide
            danmarsden Dan Marsden added a comment -

            branch rebased on current integration master to make integrators life easier (to avoid conflicts with other patches already landed in integration branch) - this is a single commit and can be cherry-picked if easier. Thanks.

            Show
            danmarsden Dan Marsden added a comment - branch rebased on current integration master to make integrators life easier (to avoid conflicts with other patches already landed in integration branch) - this is a single commit and can be cherry-picked if easier. Thanks.
            Hide
            damyon Damyon Wiese added a comment -

            Thanks for the changes.

            What are the css changes to letter-spacing/word-spacing for? For me (chrome) they make all the text in the navigation unreadable.

            I'll add a screenshot.

            Show
            damyon Damyon Wiese added a comment - Thanks for the changes. What are the css changes to letter-spacing/word-spacing for? For me (chrome) they make all the text in the navigation unreadable. I'll add a screenshot.
            Hide
            danmarsden Dan Marsden added a comment -

            Thanks Damyon, please re-open this and we'll do some more cross browser testing and put it in again next week

            Show
            danmarsden Dan Marsden added a comment - Thanks Damyon, please re-open this and we'll do some more cross browser testing and put it in again next week
            Hide
            damyon Damyon Wiese added a comment -

            Thanks Dan, reopening.

            Show
            damyon Damyon Wiese added a comment - Thanks Dan, reopening.
            Hide
            cibot CiBoT added a comment -

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

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            danmarsden Dan Marsden added a comment -

            this appears to be a problem only with the "clean" theme - standard theme (which I was using for testing) works fine. Looking at it now.

            Show
            danmarsden Dan Marsden added a comment - this appears to be a problem only with the "clean" theme - standard theme (which I was using for testing) works fine. Looking at it now.
            Hide
            danmarsden Dan Marsden added a comment -

            pushing back up for integration - issues found by Damyon were because we hadn't tested this with the clean theme - tidied up the css selectors to make it work as expected.

            Note: - the default value for collapsetocwinsize isn't really small enough for the clean theme but as this is adjustable by the admin in this version I think that's ok.

            Also important to note that CSS file doesn't comply with coding guidelines but to make it easier to review specific changes we will fix css coding style separately in MDL-41216 - thanks!

            Show
            danmarsden Dan Marsden added a comment - pushing back up for integration - issues found by Damyon were because we hadn't tested this with the clean theme - tidied up the css selectors to make it work as expected. Note: - the default value for collapsetocwinsize isn't really small enough for the clean theme but as this is adjustable by the admin in this version I think that's ok. Also important to note that CSS file doesn't comply with coding guidelines but to make it easier to review specific changes we will fix css coding style separately in MDL-41216 - thanks!
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks guys this has been integrated now

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks guys this has been integrated now
            Hide
            danmarsden Dan Marsden added a comment -

            awesome - thanks Sam - great work Mayank!

            Show
            danmarsden Dan Marsden added a comment - awesome - thanks Sam - great work Mayank!
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Drat - there is a bug here. The database comparison of an upgraded site and a fresh install triggered a failure.

            Show
            samhemelryk Sam Hemelryk added a comment - Drat - there is a bug here. The database comparison of an upgraded site and a fresh install triggered a failure.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Ok having talked to Dan I've quickly made a commit to change not null = false in install.xml which should resolve the issue.

            Show
            samhemelryk Sam Hemelryk added a comment - Ok having talked to Dan I've quickly made a commit to change not null = false in install.xml which should resolve the issue.
            Hide
            dmonllao David Monllaó added a comment -

            Hi, good work Mayank

            Most of it working as expected, but:

            • With "Display course structure in player" disabled I see a small button with id="scorm_top_toggle_btn"
            • Not sure if navigation buttons behaviour is correct, I attached the scorm package I used (211.facts.SCO - Multiscoes .zip) I only see the next button, but not the previous one, it does not matter the page I am in.
            • With "Hide navigation buttons" site setting to 'Yes', after upgrading to last integration 'Show Navigation' is set to 'Under content' when, according to the testing instructions, I understand that should be 'No'

            IMO it should fail; waiting for fix/es or comments to retest or pass.

            Show
            dmonllao David Monllaó added a comment - Hi, good work Mayank Most of it working as expected, but: With "Display course structure in player" disabled I see a small button with id="scorm_top_toggle_btn" Not sure if navigation buttons behaviour is correct, I attached the scorm package I used (211.facts.SCO - Multiscoes .zip) I only see the next button, but not the previous one, it does not matter the page I am in. With "Hide navigation buttons" site setting to 'Yes', after upgrading to last integration 'Show Navigation' is set to 'Under content' when, according to the testing instructions, I understand that should be 'No' IMO it should fail; waiting for fix/es or comments to retest or pass.
            Hide
            danmarsden Dan Marsden added a comment -

            Thanks David - did you check that scorm package on the previous version of the player with navigation to see if it operated the same way?

            Show
            danmarsden Dan Marsden added a comment - Thanks David - did you check that scorm package on the previous version of the player with navigation to see if it operated the same way?
            Hide
            danmarsden Dan Marsden added a comment -

            good thorough testing thanks David - I've pushed fixes for course structure disabled problem and the hide nav setting upgrade stuff - the upgrade steps were in the wrong order and were overriding themselves. looking at navigation buttons next.

            Show
            danmarsden Dan Marsden added a comment - good thorough testing thanks David - I've pushed fixes for course structure disabled problem and the hide nav setting upgrade stuff - the upgrade steps were in the wrong order and were overriding themselves. looking at navigation buttons next.
            Hide
            danmarsden Dan Marsden added a comment -

            confirming a problem with navigation buttons...

            Show
            danmarsden Dan Marsden added a comment - confirming a problem with navigation buttons...
            Hide
            dmonllao David Monllaó added a comment -

            yes, navigation problem seems new I was going to comment about it now

            Show
            dmonllao David Monllaó added a comment - yes, navigation problem seems new I was going to comment about it now
            Hide
            mayank_gupta2005 Mayank Gupta added a comment -

            Thanks Dan, David and Sam.

            The issues brought up during testing have been fixed and are ready to be tested again.
            The commits are:

            Thanks,
            Mayank

            Show
            mayank_gupta2005 Mayank Gupta added a comment - Thanks Dan, David and Sam. The issues brought up during testing have been fixed and are ready to be tested again. The commits are: https://github.com/mayankgupta/moodle/commit/6db676bfa144ba2348167f9b7a4a9395496e3a98 https://github.com/mayankgupta/moodle/commit/bb22cd80b7c3b5724f808d1e22070927495b91ac Thanks, Mayank
            Hide
            danmarsden Dan Marsden added a comment -

            thanks Mayank - I can confirm with your patch that the navigation now works as it used to in the old code... it's still a bit stupid sometimes - when you're at the end of the list of items in a sco you can't hit the next arrow to enter the next sco (this isn't a regression, it's existing behaviour that we should look into at a different point in future - I think there's a bug in the tracker for it somewhere)

            Show
            danmarsden Dan Marsden added a comment - thanks Mayank - I can confirm with your patch that the navigation now works as it used to in the old code... it's still a bit stupid sometimes - when you're at the end of the list of items in a sco you can't hit the next arrow to enter the next sco (this isn't a regression, it's existing behaviour that we should look into at a different point in future - I think there's a bug in the tracker for it somewhere)
            Hide
            jm.andonegi José Miguel Andonegi Martínez added a comment -

            Hi Dan and Mayank:

            I think this issue is about that navigation problem: https://tracker.moodle.org/browse/MDL-39551 It's a problem for multi-sco SCORMs, like the ones generated by eXeLearning (http://exelearning.net/)

            Looking forward to see the new viewer.

            Show
            jm.andonegi José Miguel Andonegi Martínez added a comment - Hi Dan and Mayank: I think this issue is about that navigation problem: https://tracker.moodle.org/browse/MDL-39551 It's a problem for multi-sco SCORMs, like the ones generated by eXeLearning ( http://exelearning.net/ ) Looking forward to see the new viewer.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks guys, Mayank I've pulling in your two new commits now and pushed them to integration.

            Sending this back to testing.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks guys, Mayank I've pulling in your two new commits now and pushed them to integration. Sending this back to testing.
            Hide
            dmonllao David Monllaó added a comment -

            Fails. Same exact problem I already reported:

            With "Hide navigation buttons" site setting to 'Yes', after upgrading to last integration 'Show Navigation' is set to 'Under content' when, according to the testing instructions, I understand that should be 'No'

            Show
            dmonllao David Monllaó added a comment - Fails. Same exact problem I already reported: With "Hide navigation buttons" site setting to 'Yes', after upgrading to last integration 'Show Navigation' is set to 'Under content' when, according to the testing instructions, I understand that should be 'No'
            Hide
            danmarsden Dan Marsden added a comment -

            Hi David - just confirming you ran through the upgrade again? - the updated patch won't fix any scorms that went through the bad upgrade code.

            Show
            danmarsden Dan Marsden added a comment - Hi David - just confirming you ran through the upgrade again? - the updated patch won't fix any scorms that went through the bad upgrade code.
            Hide
            dmonllao David Monllaó added a comment -

            Hi Dan,

            Yes, I restarted the test with last weekly master and upgraded again to last integration. The activities settings are not the problem, is the site setting.

            Show
            dmonllao David Monllaó added a comment - Hi Dan, Yes, I restarted the test with last weekly master and upgraded again to last integration. The activities settings are not the problem, is the site setting.
            Hide
            danmarsden Dan Marsden added a comment -

            thanks David - completely missed you were referring to the site level setting... this should fix it: https://github.com/danmarsden/moodle/commit/cf1fd64d235c5e1bf98cfad5e1438f552d04e641

            Eloy is cherry-picking as I write this.

            Show
            danmarsden Dan Marsden added a comment - thanks David - completely missed you were referring to the site level setting... this should fix it: https://github.com/danmarsden/moodle/commit/cf1fd64d235c5e1bf98cfad5e1438f552d04e641 Eloy is cherry-picking as I write this.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Fix picked and applied to integration.git. Moving back to testing...

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Fix picked and applied to integration.git. Moving back to testing...
            Hide
            samhemelryk Sam Hemelryk added a comment -

            I'll test this issue this morning but will leave David down as tester for his efforts here.

            Show
            samhemelryk Sam Hemelryk added a comment - I'll test this issue this morning but will leave David down as tester for his efforts here.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            After one more final tweak this has been passed
            Many thanks Dan

            Show
            samhemelryk Sam Hemelryk added a comment - After one more final tweak this has been passed Many thanks Dan
            Hide
            dmonllao David Monllaó added a comment -

            Hi, sorry guys, good to see that it passed

            Show
            dmonllao David Monllaó added a comment - Hi, sorry guys, good to see that it passed
            Hide
            marina Marina Glancy added a comment -

            And THANK YOU again for making Moodle better every day!

            Another weekly release has been released.

            Show
            marina Marina Glancy added a comment - And THANK YOU again for making Moodle better every day! Another weekly release has been released.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hi Dan Marsden & Mayank Gupta,

            I notice that this closed issue still has 12 open subtasks. Are they complete or to be completed still?

            Show
            dobedobedoh Andrew Nicols added a comment - Hi Dan Marsden & Mayank Gupta , I notice that this closed issue still has 12 open subtasks. Are they complete or to be completed still?
            Hide
            danmarsden Dan Marsden added a comment -

            Thanks Andrew - yes those have been closed now.

            Show
            danmarsden Dan Marsden added a comment - Thanks Andrew - yes those have been closed now.
            Hide
            marycooch Mary Cooch added a comment -

            (Housekeeping) somewhat late in the day but these improvements are now documented in https://docs.moodle.org/26/en/SCORM_settings and https://docs.moodle.org/27/en/SCORM_settings, so removing docs_required label.

            Show
            marycooch Mary Cooch added a comment - (Housekeeping) somewhat late in the day but these improvements are now documented in https://docs.moodle.org/26/en/SCORM_settings and https://docs.moodle.org/27/en/SCORM_settings , so removing docs_required label.

              People

              • Votes:
                3 Vote for this issue
                Watchers:
                14 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  18/Nov/13