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

[Chuck Hagenbuch]

Quoting Jan Schneider <jan at horde.org>:

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

I agree with Jan's summary. I don't think duplicating images is a big  
deal. And for theme designers, if we want a tool to say which images  
have been overridden, we can write something that uses md5sums to  
check. Lots of overhead, but not a big deal in a theme-designing  
specific tool.

I'm also fine with requiring applications to contain all of their own  
graphics. Does seem like it would simplify a lot of things.

-chuck