PDA

View Full Version : Re: WIP plugins web interface patch



Vidur Apparao
2004-04-21, 13:51
Robert Moser wrote:

>First off, good work, keep it coming. Don't take the rest of this the
>wrong way.
>
>
Not at all. I posted with the hope of getting feedback.

>I'm not sure why most of the pageFunctions entries don't have the
>leading ^. I may have to test out adding that to see if it causes some
>harm that isn't springing to mind. I'd much rather fix pageFunctions to
>allow plugin stuff there than have a separate hash for them.
>
>
I agree. If no one comes up with a reason for omitting the leading ^ ,
we can definitely merge the two.

>In your patch to Plugins/DateTime.pm you are some questionable things in
>the handleHTTP function:
>
> Hardcoding HTML in the perl goes against the skinning idea.
> Performing a s/// on a stringified phrase is not going to work for
>languages other than english.
>
>
The changes to Plugins/DateTime.pm were meant as a quick way for people
testing the patch to see it in use. I agree that the changes themselves
are questionable and probaby shouldn't be checked in as-is.

>I don't like the changes to newSkinTemplate. Specifically, having a
>separate template object for every skin/plugin combination seems
>wasteful (and I didn't like having separate ones for each skin in the
>first place but couldn't think of anything better). Also, I feel that
>templates in the plugins directories should come last in the search
>heirarchy, not first (ie, use push and not unshift). And if a plugin
>isn't doing anything web based, we shouldn't ever have to look in its
>directory.
>
>
Well...from what I can tell, having a separate template object doesn't
create a separate compiled version of the templates. And I don't believe
the in-memory cost of the object is great. I can live with the templates
in the server directory coming before those in the plugin directory. But
I don't think that lumping all plugin template directories together in a
single include path for a single template object is a great idea. The
potential for collisions between plugins would then exist. If the cost
of creating separate skin spaces is reasonable, why not do it? I can see
there being room for optimization - if a plugin doesn't have its own
HTML directories, it can use the base template object. If, as you
suggest, the cost is significant, I'll back off this point.

>Either all plugins should dump their templates in a single location (an
>HTML directory under the plugins directory), or they should register
>their HTML directory with Slim::Web::HTTP so that it can be added to the
>list returned by HTMLTemplateDirs().
>
>
I'm a fan of allowing plugins to be self-contained (easier
installation), so the idea of a common HTML directory doesn't sit well.
Also, rather than explicit registration by the plugin, code in
Slim::Buttons::Plugins could probably detect the presence of an HTML
directory and register it...you may have meant that.

>What I would have done, and what I was planning on doing, but got
>sidetracked, would be to have a function in Slim::Web::HTTP to add
>entries to the pageFunctions hash. There would be a webPages (or
>similar) function that would be implemented in plugins desiring to have
>web components. This would be invoked similar to addSetupGroup and
>addDefaultMaps (Slim::Buttons::Plugins calls webPages to get the hash
>entries to add, then calls the Slim::Web::HTTP::registerPages function
>to add them to the pageFunctions hash.
>
>I never contemplated having a virtual /plugins/ directory on the server,
>with pages under that. My thought was that (for example), the DateTime
>plugin would register datetime.html in the virtual web root, with some
>function that would handle that. Each plugin could register additional
>pages as needed, but first come first served as far as name collisions
>go. I'm not sure which way is better, free-for-all or
>/plugins/$plugin/somepage.html.
>
>
The idea of having a webPages-like method to return a map of pages to
handlers, rather than a single handleHTTP method, makes sense. I'm a fan
of enforced separation of namespace (created by having the virtual
/plugins/$plugin/ directory prefix to a plugin page) rather than a
free-for-all. Again, why create the potential for collisions when it can
be avoided?

Good feedback -thanks! If we can resolve the single vs. per-plugin
template object issue, I can post a revised patch that incorporates most
of your suggestions.

--Vidur