Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.2, 2.3
    • Fix Version/s: 2.3
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide

      Base Moodle install. All icons should appear normally in all themes.
      Create course and place a forum in the course. Edit icons and Forum icon should appear normally.

      Enable themedesignermode - This just forces moodle to re-check the icon locations instead of using its cache

      In dataroot, create pix and pix_plugins

      • For core icons
        In dataroot/pix, create a folder 'i'
        Copy dirroot/pix/i/risk_spam.gif to dataroot/pix/i/edit.gif
        Load a course, you should now see an yellow triangle next to the course editing link.
        All other icons should stay the same.
      • For plugin icons
        In dataroot/pix_plugins, create mod/forum.
        Copy dirroot/pix/i/risk_xss.gif to dataroot/pix_plugins/mod/forum/icon.gif
        Reload the page. You should now see a red triangle next to the forum.
      • To verify that theme override takes precedence.
        Switch theme to afterburner.
        Reload the course. The edit and forum icons should appear as the normal afterburner icons for those items, since afterburner includes its own versions.
      Show
      Base Moodle install. All icons should appear normally in all themes. Create course and place a forum in the course. Edit icons and Forum icon should appear normally. Enable themedesignermode - This just forces moodle to re-check the icon locations instead of using its cache In dataroot, create pix and pix_plugins For core icons In dataroot/pix, create a folder 'i' Copy dirroot/pix/i/risk_spam.gif to dataroot/pix/i/edit.gif Load a course, you should now see an yellow triangle next to the course editing link. All other icons should stay the same. For plugin icons In dataroot/pix_plugins, create mod/forum. Copy dirroot/pix/i/risk_xss.gif to dataroot/pix_plugins/mod/forum/icon.gif Reload the page. You should now see a red triangle next to the forum. To verify that theme override takes precedence. Switch theme to afterburner. Reload the course. The edit and forum icons should appear as the normal afterburner icons for those items, since afterburner includes its own versions.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
    • Rank:
      38866

      Description

      I feel like this has to have come up before, but I've looked through ~400 tickets and haven't found it.

      It would be great if there was a pix store in dataroot that would just override base. Much the same way dataroot/lang overrides the base language translations.

      E.G.: We have an icon that we want to replace a base icon in all themes (that don't override it themselves). Right now, as far as I can tell, we have to replace the icon in dirroot/pix/. We like to make as few changes in the dirroot as possible for hopefully obvious reasons.

      I would be happy to work on the patch for this, but if it's too likely to get reject on some principle, I won't bother, so I want to run it by the people that know the most about this area.

        Issue Links

          Activity

          Eric Merrill created issue -
          Hide
          Michael de Raadt added a comment -

          Thanks for suggesting this, Eric.

          I'm happy to leave this open for discussion.

          It would be good if you could provide a patch. We don't always integrate patches, but even if we don't, the fix is still available to people who want to use it. And a patched improvement issue has a far greater chance of getting attention.

          Show
          Michael de Raadt added a comment - Thanks for suggesting this, Eric. I'm happy to leave this open for discussion. It would be good if you could provide a patch. We don't always integrate patches, but even if we don't, the fix is still available to people who want to use it. And a patched improvement issue has a far greater chance of getting attention.
          Michael de Raadt made changes -
          Field Original Value New Value
          Fix Version/s STABLE backlog [ 10463 ]
          Labels triaged
          Michael de Raadt made changes -
          Fix Version/s DEV backlog [ 10464 ]
          Fix Version/s STABLE backlog [ 10463 ]
          Hide
          Tim Hunt added a comment -

          Well, the key method is resolve_image_location in lib/outputlib.php. That is probably the only place you have to change.

          Show
          Tim Hunt added a comment - Well, the key method is resolve_image_location in lib/outputlib.php. That is probably the only place you have to change.
          Eric Merrill made changes -
          Assignee Patrick Malley [ ptrkmkl ] Eric Merrill [ emerrill ]
          Hide
          Eric Merrill added a comment -

          Curious what you all think.

          Sorry if I'm doing this wrong - first time using the new git process (and I blame Tim's Blog )

          -Eric

          Show
          Eric Merrill added a comment - Curious what you all think. Sorry if I'm doing this wrong - first time using the new git process (and I blame Tim's Blog ) -Eric
          Eric Merrill made changes -
          Status Open [ 1 ] Waiting for peer review [ 10012 ]
          Pull Master Diff URL https://github.com/merrill-oakland/moodle/compare/master...MDL-32143
          Pull Master Branch MDL-32143
          Eric Merrill made changes -
          Affects Version/s 2.3 [ 10657 ]
          Eric Merrill made changes -
          Hide
          Eric Merrill added a comment - - edited

          More specifics about the change I made: Normal heirarchy is followed for icons, except before it would check $CFG->dirroot/pix or $CFG->dirroot/type/plugin/pix, it checks $CFG->dataroot/pix and $CFG->dirroot/type/plugin/pix respectively.

          Effectively this means that icons added to $CFG->dataroot/ paths will override the very base level icons included in Moodle (or the a plugin), but are lower in priority than icons specified by themes.

          Show
          Eric Merrill added a comment - - edited More specifics about the change I made: Normal heirarchy is followed for icons, except before it would check $CFG->dirroot/pix or $CFG->dirroot/type/plugin/pix, it checks $CFG->dataroot/pix and $CFG->dirroot/type/plugin/pix respectively. Effectively this means that icons added to $CFG->dataroot/ paths will override the very base level icons included in Moodle (or the a plugin), but are lower in priority than icons specified by themes.
          Eric Merrill made changes -
          Fix Version/s 2.3 [ 10657 ]
          Eric Merrill made changes -
          Testing Instructions Base Moodle install. All icons should appear normally in all themes.
          Create course and place a forum in the course. Edit icons and Forum icon should appear normally.

          In dataroot, create pix and pix_plugins

          - For core icons
          In dataroot/pix, create a folder 'i'
          Copy dirroot/pix/i/risk_spam.gif to dataroot/pix/i/edit.gif
          Load a course, you should now see an yellow triangle next to the course editing link.
          All other icons should stay the same.

          - For plugin icons
          In dataroot/pix_plugins, create mod/forum.
          Copy dirroot/pix/i/risk_xss.gif to dataroot/pix_plugins/mod/forum/icon.gif
          Reload the page. You should now see a red triangle next to the forum.

          - To verify that theme override takes precedence.
          Switch theme to afterburner.
          Reload the course. The edit and forum icons should appear as the normal afterburner icons for those items, since afterburner includes its own versions.
          moodle.com made changes -
          Peer reviewer phalacee
          Eric Merrill made changes -
          Testing Instructions Base Moodle install. All icons should appear normally in all themes.
          Create course and place a forum in the course. Edit icons and Forum icon should appear normally.

          In dataroot, create pix and pix_plugins

          - For core icons
          In dataroot/pix, create a folder 'i'
          Copy dirroot/pix/i/risk_spam.gif to dataroot/pix/i/edit.gif
          Load a course, you should now see an yellow triangle next to the course editing link.
          All other icons should stay the same.

          - For plugin icons
          In dataroot/pix_plugins, create mod/forum.
          Copy dirroot/pix/i/risk_xss.gif to dataroot/pix_plugins/mod/forum/icon.gif
          Reload the page. You should now see a red triangle next to the forum.

          - To verify that theme override takes precedence.
          Switch theme to afterburner.
          Reload the course. The edit and forum icons should appear as the normal afterburner icons for those items, since afterburner includes its own versions.
          Base Moodle install. All icons should appear normally in all themes.
          Create course and place a forum in the course. Edit icons and Forum icon should appear normally.

          Enable themedesignermode - This just forces moodle to re-check the icon locations instead of using its cache

          In dataroot, create pix and pix_plugins

          - For core icons
          In dataroot/pix, create a folder 'i'
          Copy dirroot/pix/i/risk_spam.gif to dataroot/pix/i/edit.gif
          Load a course, you should now see an yellow triangle next to the course editing link.
          All other icons should stay the same.

          - For plugin icons
          In dataroot/pix_plugins, create mod/forum.
          Copy dirroot/pix/i/risk_xss.gif to dataroot/pix_plugins/mod/forum/icon.gif
          Reload the page. You should now see a red triangle next to the forum.

          - To verify that theme override takes precedence.
          Switch theme to afterburner.
          Reload the course. The edit and forum icons should appear as the normal afterburner icons for those items, since afterburner includes its own versions.
          Hide
          Mary Evans added a comment - - edited

          We do this in themes...over ride the parent theme using specific icons that are in pix_core or pix_plugins.

          Oops sorry I did not read the whole of this discussion. Do we really need what you are advocating Eric? I have not read your code yet but will, and see where you are coming from.

          Also, how easy would you consider this to be achieved by an Admin in a school or college setting? How would they create a directory in Moodledata? How would they upload the images? I mean what seems like a piece of cake to you may be beyond others?

          Show
          Mary Evans added a comment - - edited We do this in themes...over ride the parent theme using specific icons that are in pix_core or pix_plugins. Oops sorry I did not read the whole of this discussion. Do we really need what you are advocating Eric? I have not read your code yet but will, and see where you are coming from. Also, how easy would you consider this to be achieved by an Admin in a school or college setting? How would they create a directory in Moodledata? How would they upload the images? I mean what seems like a piece of cake to you may be beyond others?
          Hide
          Eric Merrill added a comment -

          Yes, but you can't replace the default icon for all themes that don't include the icon without modifying dirroot/pix (or even worse dirroot/mod/plugin/pix/x). In our case we replace most the the default icons with more stylized ones, and it makes quite the mess of the code base.

          Themes that have an icon (or that their parents have the icon) take precedence over this patch, but this is one level over the base icon installs.

          Show
          Eric Merrill added a comment - Yes, but you can't replace the default icon for all themes that don't include the icon without modifying dirroot/pix (or even worse dirroot/mod/plugin/pix/x). In our case we replace most the the default icons with more stylized ones, and it makes quite the mess of the code base. Themes that have an icon (or that their parents have the icon) take precedence over this patch, but this is one level over the base icon installs.
          Hide
          Mary Evans added a comment -

          If what you are saying works, as I think it would, since the code you have submitted is easy enough to understand, then why has this not been done before?

          Show
          Mary Evans added a comment - If what you are saying works, as I think it would, since the code you have submitted is easy enough to understand, then why has this not been done before?
          Hide
          Eric Merrill added a comment -

          Woops, I responded before you edited

          Personally, I'm all for keeping mods out of install code. When I look at the changes we have made in our core install, I see this mess of icons that are hard to track in comparison to other things.

          To me it is already very similar in principle the the lang overrides that are stored in dataroot/lang.

          It is also a very brief and simple patch (6 lines of code).

          As for complexity of use, I would have a few thoughts:
          First off I need to/would write a doc page for this that fully explains how to do it.
          Second, I don't think that using this is any more complex than the process for replacing icons or install themes now (the old way or new way you will need to use ssh or ftp) - it mainly allows you place them outside your code base and better maintain changes you make.

          Separately I can envision a interface that would allow you to do this from the UI, but that would obviously be more involved, and I would need to figure out how to actually implement it.

          Show
          Eric Merrill added a comment - Woops, I responded before you edited Personally, I'm all for keeping mods out of install code. When I look at the changes we have made in our core install, I see this mess of icons that are hard to track in comparison to other things. To me it is already very similar in principle the the lang overrides that are stored in dataroot/lang. It is also a very brief and simple patch (6 lines of code). As for complexity of use, I would have a few thoughts: First off I need to/would write a doc page for this that fully explains how to do it. Second, I don't think that using this is any more complex than the process for replacing icons or install themes now (the old way or new way you will need to use ssh or ftp) - it mainly allows you place them outside your code base and better maintain changes you make. Separately I can envision a interface that would allow you to do this from the UI, but that would obviously be more involved, and I would need to figure out how to actually implement it.
          Hide
          Eric Merrill added a comment -

          Ha, not sure if that is a "clearly there is no need for it because it hasn't been done before" or a "that's so simple this should have been done ages ago"

          Honestly I'm not sure why it hasn't been done before. Our custom icons have been a thorn in my side for a long time, this was just the first time I could come up with a simple solution to it. The idea came to me because of the language customizations now being available without code modifications.

          I did also have another developer check it and confirm it works, although he seems to have forgotten to post...

          Show
          Eric Merrill added a comment - Ha, not sure if that is a "clearly there is no need for it because it hasn't been done before" or a "that's so simple this should have been done ages ago" Honestly I'm not sure why it hasn't been done before. Our custom icons have been a thorn in my side for a long time, this was just the first time I could come up with a simple solution to it. The idea came to me because of the language customizations now being available without code modifications. I did also have another developer check it and confirm it works, although he seems to have forgotten to post...
          Hide
          Charles Fulton added a comment -

          Oops, that was me. I verified that this worked as advertised on a clean 2.3 install.

          Show
          Charles Fulton added a comment - Oops, that was me. I verified that this worked as advertised on a clean 2.3 install.
          Hide
          Mary Evans added a comment -

          In that case I'll test it all tomorrow.

          Thanks Eric...I personally would like to see this added, as I see it could pave the way for what we would like to make themes to be more user friendly.

          We badly need a mechanism that can upload an image to a theme pix folder, but if your idea was implemented this would mean we could upload an image to the dataroot/pix folder instead...am I right?

          either way this gets +1 from me
          Cheers
          Mary

          Show
          Mary Evans added a comment - In that case I'll test it all tomorrow. Thanks Eric...I personally would like to see this added, as I see it could pave the way for what we would like to make themes to be more user friendly. We badly need a mechanism that can upload an image to a theme pix folder, but if your idea was implemented this would mean we could upload an image to the dataroot/pix folder instead...am I right? either way this gets +1 from me Cheers Mary
          Hide
          Eric Merrill added a comment - - edited

          It depends on the specifics of what image you want to add.

          The patch as it doesn't work for images that are considered 'theme' images (like the logo), because themes take priority over this particular mod.

          But, based on the same logic, you could (in the same function) do:

                  } else if ($component === 'theme') { //exception
          +            if ($imagefile = $this->image_exists("$CFG->dataroot/pix_themes/$this->name/$image")) {
          +                return $imagefile;
          +            }
                      if ($image === 'favicon') {
                          return "$this->dir/pix/favicon.ico";
                      }
                      if ($imagefile = $this->image_exists("$this->dir/pix/$image")) {
                          return $imagefile;
                      }
                      foreach (array_reverse($this->parent_configs) as $parent_config) { // base first, the immediate parent last
          +                if ($imagefile = $this->image_exists("$CFG->dataroot/pix_themes/$parent_config->name/$image")) {
          +                    return $imagefile;
          +                }
                          if ($imagefile = $this->image_exists("$parent_config->dir/pix/$image")) {
                              return $imagefile;
                          }
                      }
                      return null;
          
                  } else {
          

          This would allow a similar concept, but allow uploading to replace a logo or fav icon.

          I think that would be a different ticket, but the basic concept it there.

          Show
          Eric Merrill added a comment - - edited It depends on the specifics of what image you want to add. The patch as it doesn't work for images that are considered 'theme' images (like the logo), because themes take priority over this particular mod. But, based on the same logic, you could (in the same function) do: } else if ($component === 'theme') { //exception + if ($imagefile = $ this ->image_exists( "$CFG->dataroot/pix_themes/$ this ->name/$image" )) { + return $imagefile; + } if ($image === 'favicon') { return "$ this ->dir/pix/favicon.ico" ; } if ($imagefile = $ this ->image_exists( "$ this ->dir/pix/$image" )) { return $imagefile; } foreach (array_reverse($ this ->parent_configs) as $parent_config) { // base first, the immediate parent last + if ($imagefile = $ this ->image_exists( "$CFG->dataroot/pix_themes/$parent_config->name/$image" )) { + return $imagefile; + } if ($imagefile = $ this ->image_exists( "$parent_config->dir/pix/$image" )) { return $imagefile; } } return null ; } else { This would allow a similar concept, but allow uploading to replace a logo or fav icon. I think that would be a different ticket, but the basic concept it there.
          Jason Fowler made changes -
          Original Estimate 0 minutes [ 0 ]
          Remaining Estimate 0 minutes [ 0 ]
          Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
          Hide
          Jason Fowler added a comment -

          Looks good Eric.
          Do you need this pushed for integration, or is that something you can do yourself?

          Show
          Jason Fowler added a comment - Looks good Eric. Do you need this pushed for integration, or is that something you can do yourself?
          Jason Fowler made changes -
          Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
          Hide
          Eric Merrill added a comment -

          Jason,

          If you could push for integration that would be great. Pretty sure I don't have the permission

          -eric

          Show
          Eric Merrill added a comment - Jason, If you could push for integration that would be great. Pretty sure I don't have the permission -eric
          Hide
          Jason Fowler added a comment -

          Pushing for integration on behalf of Eric

          Show
          Jason Fowler added a comment - Pushing for integration on behalf of Eric
          Jason Fowler made changes -
          Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
          Hide
          Aparup Banerjee added a comment -

          The main moodle.git repository has just been updated (yesterday) 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
          Aparup Banerjee added a comment - The main moodle.git repository has just been updated (yesterday) 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
          Eric Merrill added a comment -

          The patch is rebased on latest.

          Show
          Eric Merrill added a comment - The patch is rebased on latest.
          Hide
          Petr Škoda added a comment -

          great idea +1

          Show
          Petr Škoda added a comment - great idea +1
          Sam Hemelryk made changes -
          Currently in integration Yes [ 10041 ]
          Sam Hemelryk made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          Integrator samhemelryk
          Hide
          Sam Hemelryk added a comment -

          Awesome thanks Eric, this has been integrated now and is up for testing on Wednesday.

          This feature should be documented somewhere now as well (where ever we document images I suppose). I'll add a label so that it doesn't get forgotten.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Awesome thanks Eric, this has been integrated now and is up for testing on Wednesday. This feature should be documented somewhere now as well (where ever we document images I suppose). I'll add a label so that it doesn't get forgotten. Cheers Sam
          Sam Hemelryk made changes -
          Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
          Fix Version/s DEV backlog [ 10464 ]
          Sam Hemelryk made changes -
          Labels triaged docs_required triaged
          Michael de Raadt made changes -
          Tester phalacee
          Michael de Raadt made changes -
          Tester phalacee rajeshtaneja
          Rajesh Taneja made changes -
          Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
          Hide
          Rajesh Taneja added a comment -

          Thanks for awesome patch Eric
          Works as expected.

          Show
          Rajesh Taneja added a comment - Thanks for awesome patch Eric Works as expected.
          Rajesh Taneja made changes -
          Status Testing in progress [ 10011 ] Tested [ 10006 ]
          Hide
          Dan Poltawski added a comment -

          Jolly good show!

          Your changes have made it into the Moodle release - its time to celebrate! I suggest a hot cup of English tea (with milk, no sugar) or a hoppy English ale.

          Tally-ho!

          Show
          Dan Poltawski added a comment - Jolly good show! Your changes have made it into the Moodle release - its time to celebrate! I suggest a hot cup of English tea (with milk, no sugar) or a hoppy English ale. Tally-ho!
          Dan Poltawski made changes -
          Status Tested [ 10006 ] Closed [ 6 ]
          Resolution Fixed [ 1 ]
          Currently in integration Yes [ 10041 ]
          Integration date 12/Apr/12
          Helen Foster made changes -
          Labels docs_required triaged dev_docs_required triaged
          Mary Evans made changes -
          Link This issue has a non-specific relationship to MDL-35434 [ MDL-35434 ]
          Hide
          Michael de Raadt added a comment -

          I've made a change in the Dev docs to document this new feature. I invite you to review the change and alter it if necessary.

          Show
          Michael de Raadt added a comment - I've made a change in the Dev docs to document this new feature. I invite you to review the change and alter it if necessary.
          Michael de Raadt made changes -
          Documentation link http://docs.moodle.org/dev/Themes_overview#Files_and_folders
          Labels dev_docs_required triaged triaged

            People

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

              Dates

              • Created:
                Updated:
                Resolved: