Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-35672 META JavaScript performance issues
  3. MDL-36023

Adjust chooserdialogue to only generate and render the dialogue when it's required

    Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 2.3.2
    • Fix Version/s: 2.3.3
    • Component/s: JavaScript
    • Labels:
    • Testing Instructions:
      Hide

      With the console open:

      • [-] Turn editing on
      • [ ] Confirm that there were no errors in the JS console
      • [ ] Open the activity chooser
      • [ ] Confirm that there were no errors in the JS console
      • [ ] Confirm that the activity chooser appears correctly and functions as expected
      • [-] Open the inspector and find the Activity Chooser in the DOM
      • [ ] Confirm that the chooser HTML appears as expected
      • [ ] Confirm that the chooser HTML appears only once within the DOM
      Show
      With the console open: [-] Turn editing on [ ] Confirm that there were no errors in the JS console [ ] Open the activity chooser [ ] Confirm that there were no errors in the JS console [ ] Confirm that the activity chooser appears correctly and functions as expected [-] Open the inspector and find the Activity Chooser in the DOM [ ] Confirm that the chooser HTML appears as expected [ ] Confirm that the chooser HTML appears only once within the DOM
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-36023-master

      Description

      At present, the chooser dialogue generates M.core.dialogue (Y.overlay) in the setup_chooser_dialogue function. This is called in the child classes initializer even if the chooser is never displayed.

      I propose we modify this such that we still call setup_chooser_dialogue from the initializer, but when we actually display the dialogue for the first time, we actually instantiate the dialogue. This should reduce load time further.

      It should also make it easier to modify in the future to handle asynchronous loading of the dialogue's contents.

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            dobedobedoh Andrew Nicols added a comment -

            Raising the level of this to critical because it will affect performance (in a positive manner)

            Show
            dobedobedoh Andrew Nicols added a comment - Raising the level of this to critical because it will affect performance (in a positive manner)
            Hide
            paul.n Paul Nicholls added a comment - - edited

            Hi Andrew,
            Looks good to me - though the branch you've submitted seems to also contain the patch for MDL-35836. The removal of the chunk of code from modchooser.js which makes the doc links appear in popups also seems to have slipped from the MDL-35836 commit into the commit for this issue, somehow. It'd probably be a good idea to tidy that up, so this branch can be merged cleanly.

            -Paul

            Show
            paul.n Paul Nicholls added a comment - - edited Hi Andrew, Looks good to me - though the branch you've submitted seems to also contain the patch for MDL-35836 . The removal of the chunk of code from modchooser.js which makes the doc links appear in popups also seems to have slipped from the MDL-35836 commit into the commit for this issue, somehow. It'd probably be a good idea to tidy that up, so this branch can be merged cleanly. -Paul
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Cheers Paul,

            To simplify things in this patch (and reduce the potential for rebase conflicts), I based this patch on MDL-35836 which is in for integration review already. That said, I somehow screwed up the rebase slightly so I've corrected it.

            Thanks for the review, submitting for IR.

            Show
            dobedobedoh Andrew Nicols added a comment - Cheers Paul, To simplify things in this patch (and reduce the potential for rebase conflicts), I based this patch on MDL-35836 which is in for integration review already. That said, I somehow screwed up the rebase slightly so I've corrected it. Thanks for the review, submitting for IR.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Reviewed by Paul Nicholls out of Workflow. See comments above.

            Show
            dobedobedoh Andrew Nicols added a comment - Reviewed by Paul Nicholls out of Workflow. See comments above.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Added branch for MOODLE_23_STABLE too (or you can cherry-pick cleanly if you prefer).
            Both of these commits are based on MDL-35836 which is currently in IR.

            Show
            dobedobedoh Andrew Nicols added a comment - Added branch for MOODLE_23_STABLE too (or you can cherry-pick cleanly if you prefer). Both of these commits are based on MDL-35836 which is currently in IR.
            Hide
            poltawski Dan Poltawski 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
            poltawski Dan Poltawski 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
            dobedobedoh Andrew Nicols added a comment -

            Rebased and ready for you

            Show
            dobedobedoh Andrew Nicols added a comment - Rebased and ready for you
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks Andrew, i've integrated that to 2.3 and master.

            Show
            poltawski Dan Poltawski added a comment - Thanks Andrew, i've integrated that to 2.3 and master.
            Hide
            dmonllao David Monllaó added a comment -

            Hi,

            In 2.3 I'm experiencing an issue when trying to add an activity in the latest Moodle 2.3 integration, I see a JS crash in the JS console regarding the modchooser

            Uncaught TypeError: Cannot call method 'one' of null 
            course/yui/modchooser/modchooser.js Line 31: this.jumplink = this.container.one('#jump');

            I haven't dig a lot into the problem, but this.container is filled in moodle-core-chooserdialogue->prepare_chooser while moodle-course-modchooser is trying to use it in it's initializer, changing from this.container.one to Y.one will solve the problem

            Show
            dmonllao David Monllaó added a comment - Hi, In 2.3 I'm experiencing an issue when trying to add an activity in the latest Moodle 2.3 integration, I see a JS crash in the JS console regarding the modchooser Uncaught TypeError: Cannot call method 'one' of null course/yui/modchooser/modchooser.js Line 31: this.jumplink = this.container.one('#jump'); I haven't dig a lot into the problem, but this.container is filled in moodle-core-chooserdialogue->prepare_chooser while moodle-course-modchooser is trying to use it in it's initializer, changing from this.container.one to Y.one will solve the problem
            Hide
            dmonllao David Monllaó added a comment -

            I've been able to reproduce the problem with linux + firefox, linux + chrome and win7 + IE9, not tested in other environments.

            In case Andrew is not available this days the fix seems to be straight forward according to the patch and the comment above.

            Show
            dmonllao David Monllaó added a comment - I've been able to reproduce the problem with linux + firefox, linux + chrome and win7 + IE9, not tested in other environments. In case Andrew is not available this days the fix seems to be straight forward according to the patch and the comment above.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Bah - I must have tested this with a cached version.

            The jumplink setting can actually be removed from modchooser.js as it's actually set in chooserdialogue.js::display_chooser() already.

            I've put a fix into:
            https://git.luns.net.uk/moodle.git/shortlog/refs/heads/MDL-36023-MOODLE_23_STABLE-fix

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Bah - I must have tested this with a cached version. The jumplink setting can actually be removed from modchooser.js as it's actually set in chooserdialogue.js::display_chooser() already. I've put a fix into: https://git.luns.net.uk/moodle.git/shortlog/refs/heads/MDL-36023-MOODLE_23_STABLE-fix Andrew
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks Andrew, i've integrated that fix.

            ps.If you want to make life really easy for the integrator, the easiest thing is to provide branch like:
            git://git.luns.net.uk/moodle.git MDL-36023-MOODLE_23_STABLE-fix

            Then I can copy and paste it easily rather than doing it in multiple steps! (Or add it to existing branch)

            Show
            poltawski Dan Poltawski added a comment - Thanks Andrew, i've integrated that fix. ps.If you want to make life really easy for the integrator, the easiest thing is to provide branch like: git://git.luns.net.uk/moodle.git MDL-36023 -MOODLE_23_STABLE-fix Then I can copy and paste it easily rather than doing it in multiple steps! (Or add it to existing branch)
            Hide
            dobedobedoh Andrew Nicols added a comment -

            will do boss

            Show
            dobedobedoh Andrew Nicols added a comment - will do boss
            Hide
            dmonllao David Monllaó added a comment -

            It passes. Tested in master and 23

            Show
            dmonllao David Monllaó added a comment - It passes. Tested in master and 23
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Amazed. Inspired. Grateful. That’s how your generosity makes me feel.

            (not really)

            Closing, thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Amazed. Inspired. Grateful. That’s how your generosity makes me feel. (not really) Closing, thanks!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  12/Nov/12