Custom icons in dataroot

Details

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.

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.

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.

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
added a comment - 22/Mar/12 11:53 AM 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.

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
added a comment - 30/Mar/12 11:32 AM - 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.

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?

Mary Evans
added a comment - 05/Apr/12 6:53 AM - 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?

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.

Eric Merrill
added a comment - 05/Apr/12 6:59 AM 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.

Mary Evans
added a comment - 05/Apr/12 7:20 AM 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?

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.

Eric Merrill
added a comment - 05/Apr/12 7:21 AM 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.

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...

Eric Merrill
added a comment - 05/Apr/12 7:30 AM 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...

Mary Evans
added a comment - 05/Apr/12 9:45 AM 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

Eric Merrill
added a comment - 05/Apr/12 10:00 AM - 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.

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.

Aparup Banerjee
added a comment - 06/Apr/12 8:33 PM 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

Sam Hemelryk
added a comment - 10/Apr/12 6:31 AM 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

Dan Poltawski
added a comment - 12/Apr/12 11:28 PM 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!