Feature Proposal: Introduce+Foswiki->{skin}
Motivation
As getSkin() is called several times, by Foswiki intern routines ( render etc ) and plugins quite a number of times, it makes sense to cache the result
in a variable Foswiki->{skin} and return this one when getSkin is called, if it has been calculated before.
Description and Documentation
Problem cases are, when a plugin or similar are setting Skin Preferences after Foswiki->{skin} has been set/calculated, which is quite common.
In that case, the setPreferenceValue is advanced, that it checks for the pref-name "SKIN" and if it is used, clears the Foswiki::{skin} value accordingly, so the next time
Foswiki::getSkin is called, you get the correct result as we are used to it right now.
The next problem case is the URL parameter skin, which i dont know where to fix. Just the same fix as with the Prefs.pm, if a skin URL parameter exist, cleare Foswiki->{skin} so tha the next geSkin call recalculates the skin.
Examples
Impact
Implementation
--
Contributors: EugenMayer - 20 Jan 2009
Discussion
By only reading this topic I guess that the wanted goal is performance improvement.
The implementation of
getSkin
, as of svn revision 2083 is:
sub getSkin {
my $this = shift;
my $skinpath = $this->{prefs}->getPreferencesValue('SKIN') || '';
if ( $this->{request} ) {
my $resurface = $this->{request}->param('skin');
$skinpath = $resurface if $resurface;
}
my $epidermis = $this->{prefs}->getPreferencesValue('COVER');
$skinpath = $epidermis . ',' . $skinpath if $epidermis;
if ( $this->{request} ) {
$epidermis = $this->{request}->param('cover');
$skinpath = $epidermis . ',' . $skinpath if $epidermis;
}
return $skinpath;
}
There are 2
prefs->getPreferencesValue
and 2
request->param
. Both methods are pretty fast, so is
getSkin
, then this cache doesn't add great performance improvements.
Indeed, I have some concerns about this specific cache:
- It doesn't have a great impact on performance
- It adds some code complexity
- The provided patches assume that the only way to update preferences stack is by
setPreferencesValue
, but there are some other methods like pushPreferences
and restore
. The check about SKIN
pref could be moved to Foswiki::Prefs::PrefsCache::insert
(or ThinPrefs equivalent method), solving the problem with pushPreferences
, but the restore
would need to be patched yet...
- The
SKIN
check by the preferences stack has two side effects: the prefs mechanism must be aware of the internal structure of the session object (we loose encapsulation) and it makes harder to have pluggable prefs back-ends (all of them should be aware of this check)
According to
this conversation, the main reason to have this is to add a
setSkin
method, that overwrites both preferences and parameter settings. But there is the
COVER
setting that already does that...
The
setSkin
method itself adds one more issue: there should be a way to know that the current
$session->{skin}
was set by that method, so it isn't updated anymore: probably a flag (another field in the session object) and more checks into the preferences mechanism.
Eugen, please add more information about what you want to finally achieve, then we can find a way that doesn't make the code even more complex
--
GilmarSantosJr - 21 Jan 2009
Actually the finally achieve should be a performance boost, even if its a small one. I cant see any complexity added to the code. Adding ~8lines of simplistic perl shouldnt be that complex.
The only thing missing on my to patches is the url parameter skin. We have exactly 2 ways of changin the skin:
- Prefrences: Default / Site / Web / User Preferences
- URL GET parameter skin
There are no more ways to do this. The api calls through Func::setPrefrenceValue, which could be considered as the 3rd way, are actually case 1, as thats exactly the way Prefs are setted int he core. This is covered by the Pref.pm_patch. all we need is a patch for case 2.
--
EugenMayer - 22 Jan 2009
After having a discussion with Gilmar he totaly convinced me that there are several more special cases. That would actually ad a good portition of complexity and the extra check would waste the performance boost anyway. So rejecting the proposal as its useless