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:

1. Prefrences: Default / Site / Web / User Preferences
2. 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