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

Wed Mar 10 10:10:59 UTC 2010

Zitat von Michael M Slusarz <slusarz at horde.org>:

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

I don't see this is much of an issue. Disk space is not an argument,  
this is a one-time action when adding a new icon, and it only requires  
some discipline for the developers/designers. I never felt like this  
was really annoying.

> 2) #2 makes it impossible to reliably determine which graphics have  
> been overriden in a theme.

That's true, but I fail to see why this is a problem. For a theme  
designer it's not important to know which icons are really overriden  
or just copies from the original icon theme. They only have to be  
happy with their own icon theme. And actually it might be easier for  
them to verify that everything is in place if they have the *complete*  
icon set in one directory.

> 3) #2 makes it virtually impossible to keep themes up-to-date.

See 1).

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

Yes, true, but if you don't want to use the Horde icon, why would you  
point to it? Maybe I'm missing something, if I understood you  
correctly, you are talking about the case where an application (like  
IMP) is using a Horde icon, but an application view (like DIMP) should  
be using a different one? Beside that this seems a bit obscure, you  
can still point to the sub-application's icon from the application  
view template.

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

That's an interesting approach. It would produce even more duplicate  
icons, but makes it much clearer where the icons are coming from, i.e.  
where to look for them. We could still keep often used icons in Horde,  
so that application developers have a repository to pick (and copy)  
from. Or we could outsource them to a separate Git module.
Especially the increasing use of graphics in CSS would benefit from that.

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

Unless I'm missing something, you would either have to manually  
invalidate the cache if any icon is changed, or we still have to stat  
each icon to see if has changed, which wouldn't give us anything.

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

I can't help it, but this seems more work and overhead than what you  
called overkill in your very first point.
I still think that the original approach with themed_graphics was  
pretty smart and solid. If inheritation and loading of Horde icons  
from CSS files is an issue, we could require each application to keep  
a copy of *any* icon it needs. In the case of sub-application I still  
might be missing the point, maybe you could make this clearer with an  
example.

Jan.

-- 
Do you need professional PHP or Horde consulting?
http://horde.org/consulting/