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

    • 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
    • Rank:
      44783

      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.

        Activity

        Hide
        Andrew Nicols added a comment -

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

        Show
        Andrew Nicols added a comment - Raising the level of this to critical because it will affect performance (in a positive manner)
        Hide
        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 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
        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
        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
        Andrew Nicols added a comment -

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

        Show
        Andrew Nicols added a comment - Reviewed by Paul Nicholls out of Workflow. See comments above.
        Hide
        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
        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
        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
        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
        Andrew Nicols added a comment -

        Rebased and ready for you

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

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

        Show
        Dan Poltawski added a comment - Thanks Andrew, i've integrated that to 2.3 and master.
        Hide
        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
        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
        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
        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
        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
        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
        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
        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
        Andrew Nicols added a comment -

        will do boss

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

        It passes. Tested in master and 23

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

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

        (not really)

        Closing, thanks!

        Show
        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: