kolibri-discuss team mailing list archive

[Anders Steinlein <anders@xxxxxxxxxxxx>]

On Wed, 2008-12-17 at 11:25 +0100, Frode Danielsen wrote:
> On Dec 17, 2008, at 11:03 AM, Anders Steinlein wrote:
> 
> > While going through the framework hunting for trigger_error()s to be
> > replaced by exceptions, one thing struck me in XslTransformer as it
> > relates to XmlGenerator in XsltResult. XslTransformer's addXml()  
> > method
> > currently only accepts XML as a string, which is loaded into a
> > DOMDocument internally. This seems rather odd considering we use a
> > DOMDucument internally in XmlGenerator as well, before it is dumped  
> > to a
> > string. I suggest this should be improved during the current  
> > rewrite of
> > XmlGenerator, to support, say, buildToDom() and buildToString(), with
> > matching methods in XslTransformer.
> 
> Good idea to brush up on XslTransformer as well. I've already changed  
> XmlGenerator's public interface, which no longer uses a build()  
> method. It's been replaced by a getXml() method which returns an XML  
> string, and thus a similar getDom() method would be easy to provide.

Agreed.

> It's definitely unnecessary to jump between DOMDocument -> XML string  
> -> DOMDocument to use the output of XmlGenerator in XslTransformer.  
> Since they're both core utility classes I'd suggest XslTransformer  
> supports working directly with XmlGenerator (ie. using getDom())?

I don't think XslTransformer should have any dependency on XmlGenerator,
in order to allow it to be used with any plain DOMDocument from other
potential sources. Sending the result of getDom() to the transformer
instead of the generator instance itself seems good to me.

> Another "weirdness" in XslTransformer as it stands now is that the  
> addXml() method really should be called setXml() considering how it  
> works right now. You can't incrementally add XML strings to  
> XslTransformer as it will just overwrite the previous XML. I guess  
> some improvement on this could be done as well, using the internal  
> DOMDocument more properly.

I don't really see the need to append/add several XML sources to the
transformer, but I agree that set is not the best of names. I suggest
loadXml() and loadDom().

-- 
Regards,
 \Anders