On my trial installation of Moodle
I use a theme with $THEME->custompix = true; so, taken from theme config:
/// If true, then this theme must have a "pix"
/// subdirectory that contains copies of all
/// files from the moodle/pix directory, plus a
/// "pix/mod" directory containing all the icons
/// for all the activity modules.
I have some missing icon into my moodle/theme/<<myTheme>>/pix/ folder so my moodle pages, sometimes, are not nice to see.
My question is:
Why moodle doesn't provide me with the "spare" icon taken from moodle/pix folder (as if I had $THEME->custompix = false;) if the expected correct icon is not found in the right moodle/theme/<<myTheme>>/pix/ folder?
Why this so strong restricion?
Is it a performance problem?
I don't know why but I guess the original idea has been that all themes have similar structure and those theme designers who select custompix = true will check that all icons do exist. It may be a big problem if you have not designed your theme, upgrade moodle and don't know that you need to add some new icons to custom theme pix subfolders.
Of course it would be possible to change for example code of weblib.php so that existence of each icon could be checked (subfolders of theme pix folder). Also upgrading scripts could check theme subfolders if new core icons are added/needed. Now image paths are set by
/// Set up image paths
if(isset($CFG->smartpix) && $CFG->smartpix==1) {
if($CFG->slasharguments) { // Use this method if possible for better caching
$extra='';
} else {
$extra='?file=';
}
$CFG->pixpath = $CFG->wwwroot. '/pix/smartpix.php'.$extra.'/'.$theme;
$CFG->modpixpath = $CFG->wwwroot .'/pix/smartpix.php'.$extra.'/'.$theme.'/mod';
} else if (empty($THEME->custompix)) { // Could be set in the above file
$CFG->pixpath = $CFG->wwwroot .'/pix';
$CFG->modpixpath = $CFG->wwwroot .'/mod';
} else {
$CFG->pixpath = $CFG->themewww .'/'. $theme .'/pix';
$CFG->modpixpath = $CFG->themewww .'/'. $theme .'/pix/mod';
}
-------------
For example code for module icons in weblib.php could be changed from
$menustyle[$url] = 'style="background-image: url('.$CFG->modpixpath.'/'.$mod->modname.'/icon.gif);"';
to
if (file_exists($CFG->modpixpath.'/'.$mod->modname.'/icon.gif')) {
$moduleicon = $CFG->modpixpath.'/'.$mod->modname.'/icon.gif';
} else $moduleicon = $CFG->wwwroot .'/mod/'.$mod->modname.'/icon.gif';
$menustyle[$url] = 'style="background-image: url('.$moduleicon.');"';
---------------
You only need to add new custom icons once but if weblib does this kind of checks for all icons it may be a tiny performance problem. And you may also select smartpix...
Thank you Mauno.
As always, you are clear, correct, comprehensive and fast.
Really thank you.
I think it is a good improvement.
I think this is matter for a new feature request!
I try to ask for it.
At the end it is matter of two more rows in weblib.
As always, you are clear, correct, comprehensive and fast.
Really thank you.
I think it is a good improvement.
I think this is matter for a new feature request!
I try to ask for it.
At the end it is matter of two more rows in weblib.
You also need to change in course/lib.php:
$icon = "$CFG->modpixpath/$mod->modname/icon.gif";
to
if (file_exists($CFG->modpixpath.'/'.$mod->modname.'/icon.gif')) {
$icon = $CFG->modpixpath.'/'.$mod->modname.'/icon.gif';
} else $icon = $CFG->wwwroot .'/mod/'.$mod->modname.'/icon.gif';
and in admin/modules.php:
$icon = "<img src=\"$CFG->modpixpath/$module->name/icon.gif\" class=\"icon\" alt=\"\" />";
to
if (file_exists($CFG->modpixpath.'/'.$module->name.'/icon.gif')) {
$icon = '<img src="'.$CFG->modpixpath.'/'.$module->name.'/icon.gif" class="icon" alt="" />';
} else $icon = '<img src="'.$CFG->wwwroot.'/mod/'.$module->name.'/icon.gif" class="icon" alt="" />';
(or something similar - I had to test it because you sent this to tracker - and it works ok 
- still there are some other icons too that may miss)
To be continued: at least in blocks/block_activity_modules
$this->content->icons[] = '<img src="'.$CFG->modpixpath.'/'.$modname.'/icon.gif" class="icon" alt="" />';
could be
if (file_exists($CFG->modpixpath.'/'.$modname.'/icon.gif')) {
$this->content->icons[] = '<img src="'.$CFG->modpixpath.'/'.$modname.'/icon.gif" class="icon" alt="" />';
} else $this->content->icons[] = '<img src="'.$CFG->wwwroot .'/mod/'.$modname.'/icon.gif" class="icon" alt="" />';
... and definitely there are some more places that should have check code (blocks and other custom icons than module icons).
...and number of rows is growing - one more: in course/modedit.php
$icon = '<img src="'.$CFG->modpixpath.'/'.$module->name.'/icon.gif" alt=""/>';
could be
if (file_exists($CFG->modpixpath.'/'.$module->name.'/icon.gif')) {
$icon = '<img src="'.$CFG->modpixpath.'/'.$module->name.'/icon.gif" alt="" />';
} else $icon = '<img src="'.$CFG->wwwroot.'/mod/'.$module->name.'/icon.gif" alt="" />';
Is it still worth of changing or should we just copy new custompix to custom themes, Daniele?
One note:
It is better to copy those files than use file existence checking because if those files do not exist your server error logs get unnecessary errors from missing files every time you visit such page. In busy sites error logs may grow this way really huge.
Although this kind of checks are administrator friendly I suggest closing the feature request in tracker - some kind of a note after upgrading or in environment check could be better. When administrator sees an empty box at the place of missing icon he/she at least knows that some icons should be copied.
You are really right, Mauno.
Maybe a warning at upgrading time will close this simple problem that, otherwise, is going to be too cumbersome.
Thank you for your support and continuous suggestions.
In the feature request is present a link to this thread so, Urs or whoever will read the issue, will read your comments too.
Thank you again.
Maybe a warning at upgrading time will close this simple problem that, otherwise, is going to be too cumbersome.
Thank you for your support and continuous suggestions.
In the feature request is present a link to this thread so, Urs or whoever will read the issue, will read your comments too.
Thank you again.
Reviving this old discussion. I think what you want is already implemented as
Administration ► Miscellaneous ► Experimental: Smart pix search.
(You can thank sam marshall of the OU for that.) MDL-8216
Administration ► Miscellaneous ► Experimental: Smart pix search.
(You can thank sam marshall of the OU for that.) MDL-8216
Thank you for digging up these old discussions, Tim. In my first reply http://moodle.org/mod/forum/discuss.php?d=93141#p411116 I said
... And you may also select smartpix...
so Daniele knew smartpix already. The problem exists only if smartpix is not enabled and it is not enabled by default.
Are there any plans for getting Smartpix a default setting from experimental features menu?
IIRC, it's expensive -- it loads the whole moodle infra for each pix and it has to check for the existence of the file.
If we could make it extremely cheap, it'd be reasonable to change the default.
Ideas -
- Instead of reading config.php and triggering the whole moodle infra, make it read a small file that only has the the data it needs. put that file in moodledata. (Still - how do we find moodledata without reading setup.php? Can we teach setup.php to "return" early?)
- The fallback strategy that smartpix implements can be done with apache config tricks, using 404 error documents for the fallback img. Post a howto in the wiki and be done with it
If the "smartpix" feature handles per-course or per-user themes then it gets a whole lot trickier...
If we could make it extremely cheap, it'd be reasonable to change the default.
Ideas -
- Instead of reading config.php and triggering the whole moodle infra, make it read a small file that only has the the data it needs. put that file in moodledata. (Still - how do we find moodledata without reading setup.php? Can we teach setup.php to "return" early?)
- The fallback strategy that smartpix implements can be done with apache config tricks, using 404 error documents for the fallback img. Post a howto in the wiki and be done with it
If the "smartpix" feature handles per-course or per-user themes then it gets a whole lot trickier...
Hey! I get to tell Martin L to RTFC 
http://cvs.moodle.org/moodle/pix/smartpix.php?view=markup
When Sam implemented this, he thought about performance. It does file_get_contents('config.php'), then greps for $CFG->dirroot, etc. All the needed functions are in that file, and so on.
When Sam implemented this, he thought about performance. It does file_get_contents('config.php'), then greps for $CFG->dirroot, etc. All the needed functions are in that file, and so on.
Ha ha ha. You are mean. And you are right - Sam's implementation is doing the nasty-but-right thing. It probably breaks if people are playing games with config.php, but playing such games is always risky.
Was there an earlier version of this? I (mis?)remember talking about a smartpix thing with Penny, long long time ago. And she said: the code sucks! reads config.php and whatnot, I implemented a ninja-powered Apache config trick using ErrorDocument that is Oh So Fast.
It would have been around 1.4/1.5 timeframe, but that smartpix.php in CVS is much newer, first commit is in 2007. Maybe we had a SoC or someone @ catalyst prototyping a smartpix implementation?
Was there an earlier version of this? I (mis?)remember talking about a smartpix thing with Penny, long long time ago. And she said: the code sucks! reads config.php and whatnot, I implemented a ninja-powered Apache config trick using ErrorDocument that is Oh So Fast.
It would have been around 1.4/1.5 timeframe, but that smartpix.php in CVS is much newer, first commit is in 2007. Maybe we had a SoC or someone @ catalyst prototyping a smartpix implementation?