Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-35655

Generalize navigation object assignment and renderer assignment in block navigation to allow more code reuse in specialization of the block

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.2
    • Fix Version/s: 2.4
    • Component/s: Navigation
    • Labels:
    • Testing Instructions:
      Hide

      These a basic, backend changes. There should be no visible difference.

      1. Without logging in check the navigation block is displayed without errors.
      2. Log in as an administrator.
      3. Use the navigation block a little and just make sure it is still working as expected.
      Show
      These a basic, backend changes. There should be no visible difference. Without logging in check the navigation block is displayed without errors. Log in as an administrator. Use the navigation block a little and just make sure it is still working as expected.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      The global navigation object is generated in the get_content method of the navigation block. This could be extracted to a protected method get_method that could be overridden in specialized navigation block plugins to generate alternate navigation from the global navigation or from scratch. One example is a block to display in a course only the branch of the global navigation that pertains to that course.

          /**
           * Returns the navigation
           *
           * @return navigation_node The navigation object to display
           */
          protected function get_navigation() {
              // Initialise (only actually happens if it hasn't already been done yet
              $this->page->navigation->initialise();
              return clone($this->page->navigation);
          }

      The renderer of the navigation block is fetched by a string

      $renderer = $this->page->get_renderer('block_navigation');

      but could be fetched by reference to the block name

      $renderer = $this->page->get_renderer($this->blockname);

      to allow specialization of the block by redefining the renderer.

        Gliffy Diagrams

          Activity

          Hide
          itamart Itamar Tzadok added a comment -

          Sam, could you please comment?

          Show
          itamart Itamar Tzadok added a comment - Sam, could you please comment?
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Hi Itamar,

          Having read through the description and looked at the proposed patch this gets a +1 from me.
          The end change isn't a big one, it's 100% safe, and obviously it's a useful change (as you're written the code yourself, thanks!).
          Feel free to put this up for integration, or if you're unable to let me know and I'll put it up for you

          Many thanks for both the issue and the patch.
          Sam

          Show
          samhemelryk Sam Hemelryk added a comment - Hi Itamar, Having read through the description and looked at the proposed patch this gets a +1 from me. The end change isn't a big one, it's 100% safe, and obviously it's a useful change (as you're written the code yourself, thanks!). Feel free to put this up for integration, or if you're unable to let me know and I'll put it up for you Many thanks for both the issue and the patch. Sam
          Hide
          itamart Itamar Tzadok added a comment -

          Thanks Sam! Please send to integration for me (I should write the patch that fixes the workflow issue and allows me to send patches to integration ).

          Show
          itamart Itamar Tzadok added a comment - Thanks Sam! Please send to integration for me (I should write the patch that fixes the workflow issue and allows me to send patches to integration ).
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Cool thanks Itamar, I've tidied up the details in the issue and have put up for integration. Hopefully it'll be in master next week

          Show
          samhemelryk Sam Hemelryk added a comment - Cool thanks Itamar, I've tidied up the details in the issue and have put up for integration. Hopefully it'll be in master next week
          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
          itamart Itamar Tzadok added a comment -

          I added a condition on the navigation object retrieved in get_content since specialized blocks may have nothing to show in which case get_content needs to return null. For the global navigation this condition is transparent because the the object must exist.

                  // Get the navigation object or don't display the block if none provided.
                  if (!$navigation = $this->get_navigation()) {
                      return null;
                  }

          The change to the fix is minimal and hopefully could be reviewed in no time (sorry for the hassle).

          Show
          itamart Itamar Tzadok added a comment - I added a condition on the navigation object retrieved in get_content since specialized blocks may have nothing to show in which case get_content needs to return null. For the global navigation this condition is transparent because the the object must exist. // Get the navigation object or don't display the block if none provided. if (!$navigation = $this->get_navigation()) { return null; } The change to the fix is minimal and hopefully could be reviewed in no time (sorry for the hassle).
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Looks good thanks Itamar, putting this back up for integration now.

          Show
          samhemelryk Sam Hemelryk added a comment - Looks good thanks Itamar, putting this back up for integration now.
          Hide
          poltawski Dan Poltawski added a comment -

          Integrated to master. Thanks Itamar!

          Show
          poltawski Dan Poltawski added a comment - Integrated to master. Thanks Itamar!
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Thanks Itamar,

          Works well for me, no problem found in navigation.

          Show
          rajeshtaneja Rajesh Taneja added a comment - Thanks Itamar, Works well for me, no problem found in navigation.
          Hide
          nebgor Aparup Banerjee added a comment -

          Your issue has dug up some gold.
          It works great i've been told.
          Go forth, be brave, be bold.

          yay! "All your thoughts are belong to everyone."

          Thanks and ciao!

          Show
          nebgor Aparup Banerjee added a comment - Your issue has dug up some gold. It works great i've been told. Go forth, be brave, be bold. yay! "All your thoughts are belong to everyone." Thanks and ciao!

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                3/Dec/12