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

      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

          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: