[dev] [commits] Horde branch master updated. 89bca7f749db1780dca80363d6f3d8c89f899df4

[Michael M Slusarz]

Quoting Jan Schneider <jan at horde.org>:

> Zitat von Michael M Slusarz <slusarz at horde.org>:
>
>> commit 97fd203dcaaf5ddf7c9c2252eb44ac76e8d84846
>> Author: Michael M Slusarz <slusarz at curecanti.org>
>> Date:   Sun Mar 7 20:15:31 2010 -0700
>>
>>    Add Horde_Themes:: library.
>>
>>    Move all themes related code to a single place (includes both image and
>>    sound).
>>
>>    Horde::img(), through Horde_Themes::img(), will now attempt to auto
>>    determine a graphic location. It looks in the following directories with
>>    this priority:
>>    1. App theme
>>    2. Horde theme
>>    3. App default
>>    4. Horde default
>>    This prevents us from having to copy all over all graphics for a theme -
>>    now, only need the graphics that differ from defaults (caveat: if
>>    loading graphics from CSS, copies of graphics need to continue to live
>>    in theme directory).
>
> Please revert, at least the new autodetection code. Using  
> themed_graphics and caching it in the session is absolutely  
> necessary for perfomance reasons. Without that we are filestat'ing  
> for directories and images in *every* img() call, which puts a huge  
> burden on the filesystem IO, compared to the once-per-session  
> stat'ing of themed_graphics.
> This was added after benchmarking massive performance degration when  
> introducing themes with graphics.

I was not aware of the crippling IO issue.  My next job was going to  
be to save the image data via Horde_Cache (similar to how we cache CSS  
scripts).  As mentioned in the past, it has already become almost  
mandatory to have a Horde_Cache driver installed for various Horde  
functions for any sort of decent performance, so this is not a  
dealbreaker.

But I am open to other ways of making this behavior better.  But not  
returning to the old behavior, which was defective in many ways:
1) Absolute overkill when it comes to having to copy *every* graphic  
to *every* theme that wants to add 1 graphic.
2) #2 makes it impossible to reliably determine which graphics have  
been overriden in a theme.
3) #2 makes it virtually impossible to keep themes up-to-date.
4) It was impossible to override a graphic via an app theme if the  
Horde::img() call specifically referenced a horde graphic (which sort  
of defeats the whole point of themes).
5) There is no concept of default graphics imported from horde.  This  
is counter-intuitive as compared with the way CSS files work.   
However, I'll admit that there are powerful incentives to require  
*all* graphics used by an app to have a physical copy of the app (i.e.  
an app never loads graphics from horde).  First, this makes the  
applications more self-contained.  More importantly, there is no  
practical way to load graphics from CSS files outside of making it an  
absolute requirement that apps MUST live under Horde.

Brainstorming, I have the following ideas:
1) Caching as described above.
Pros: automatically determined (themes are drop-in replaceable).
Cons: adds a bit of overhead the first time a graphic is loaded;  
fallback to file stat() calls on Horde_Cache.  As mentioned  
previously, the latter issue is not critical IMHO.
2) Add theme definition script.  The idea: after changing theme  
graphics, this script is run and it produces a PHP data file that  
contains the graphics list for the theme (i.e. which location each  
graphic of the theme is located.)
Pro: Arguably identical overhead than the old method - instead of  
doing a file_exists() call, we simply include 1 or 2 PHP files per  
page load.
Cons: This script would have to be run manually whenever a graphic is  
changed. However, themes/graphics theoretically won't be changed all  
that often, and this script could be called as part of the build  
process.  Additionally, this is no more burdensome than the previous  
method (requiring calling a script to determine which graphics are  
missing from a particular theme).

michael

-- 
___________________________________
Michael Slusarz [slusarz at horde.org]