On Sat, Jul 11, 2009 at 10:09, Vincent Massol<[email protected]> wrote: > Summarizing (I've now understood): > - We have input syntaxes (parser syntaxes) and output syntaxes > (renderer syntaxes) > > Proposal > ======= > > a- we rename SyntaxFactory.getAvailableSyntaxes() in > getAvailableInputSyntaxes() (or getAvailableParserSyntaxes())
+1 , since it's in rendering module we should always use same vocabulary so +1 for getAvailableParserSyntaxes > b- we add SyntaFactory.getAvailableOutputSyntaxes() (or > getAvailableRendererSyntaxes()) +1 for getAvailableRendererSyntaxes > c- (optional) we add a Renderer.getSyntax() interface method (for > later use when renderers will be components) let's wait for when we will make renderers componenents since it depends on how we do that exactly > d- we rename Document/XWikiDocument/XWiki methods for getting syntaxes > to use Input/Ouput (or Parser/Renderer) in their names. +1 > > Note that b) means that we'll hardcode the list of available renderer > syntaxes but that's ok for now (this is hat is currently done in > PrintRendererFactory). This will be made dynamic when Renderer are > made components. The advantage of putting this in PrintRendererFactory is that you have less chance to forget to update the list when you add a new renderer but i doubt we will add one before making renderers real components actually so i guess it ok. > > WDYT? > > Thanks > -Vincent > > On Jul 10, 2009, at 6:49 PM, Thomas Mortagne wrote: > >> On Fri, Jul 10, 2009 at 09:41, Vincent Massol<[email protected]> >> wrote: >>> >>> On Jul 4, 2009, at 2:44 PM, tmortagne (SVN) wrote: >>> >>>> Author: tmortagne >>>> Date: 2009-07-04 14:44:30 +0200 (Sat, 04 Jul 2009) >>>> New Revision: 21811 >>>> >>>> Modified: >>>> platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/api/ >>>> Document.java >>>> platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/api/ >>>> XWiki.java >>>> platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/doc/ >>>> XWikiDocument.java >>>> platform/core/trunk/xwiki-core/src/test/java/com/xpn/xwiki/api/ >>>> XWikiTest.java >>>> platform/core/trunk/xwiki-rendering/xwiki-rendering-api/pom.xml >>>> platform/core/trunk/xwiki-rendering/xwiki-rendering-api/src/main/ >>>> java/org/xwiki/rendering/internal/renderer/ >>>> DefaultPrintRendererFactory.java >>>> platform/core/trunk/xwiki-rendering/xwiki-rendering-api/src/main/ >>>> java/org/xwiki/rendering/parser/Syntax.java >>>> platform/core/trunk/xwiki-rendering/xwiki-rendering-api/src/main/ >>>> java/org/xwiki/rendering/parser/SyntaxType.java >>>> platform/core/trunk/xwiki-rendering/xwiki-rendering-api/src/main/ >>>> java/org/xwiki/rendering/renderer/PrintRendererFactory.java >>>> platform/web/trunk/standard/src/main/webapp/templates/ >>>> contentview.vm >>>> platform/web/trunk/standard/src/main/webapp/templates/plain.vm >>>> Log: >>>> XWIKI-4042: Add a way to choose the renderer when viewing a page >>> >>>> >>>> Modified: platform/core/trunk/xwiki-core/src/main/java/com/xpn/ >>>> xwiki/ >>>> api/Document.java >>>> =================================================================== >>>> --- platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/api/ >>>> Document.java 2009-07-04 12:42:14 UTC (rev 21810) >>>> +++ platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/api/ >>>> Document.java 2009-07-04 12:44:30 UTC (rev 21811) >>>> @@ -37,6 +37,7 @@ >>>> import org.suigeneris.jrcs.diff.DifferentiationFailedException; >>>> import org.suigeneris.jrcs.diff.delta.Delta; >>>> import org.suigeneris.jrcs.rcs.Version; >>>> +import org.xwiki.rendering.parser.Syntax; >>>> >>>> import com.xpn.xwiki.XWiki; >>>> import com.xpn.xwiki.XWikiContext; >>>> @@ -473,6 +474,16 @@ >>>> } >>>> >>>> /** >>>> + * @param targetSyntax the syntax in which render the document >>>> content >>>> + * @return the rendered content >>>> + * @throws XWikiException error when rendering content >>>> + */ >>>> + public String getRenderedContent(Syntax targetSyntax) throws >>>> XWikiException >>>> + { >>>> + return this.doc.getRenderedContent(targetSyntax, >>>> getXWikiContext()); >>>> + } >>> >>> Since this is for Velocity, isn't it better to use a syntax Id String >>> instead of a Syntax object which is hard to construct from velocity? >> >> The problem is that it was not easy to find a right signature since >> there is already getRenderedContent taking string fo the parser >> syntax. Since anyway it's necessary to give a supported syntax by >> looking at existing one it was easier to not have to directly give the >> found valid syntax object. >> >>> >>>> + >>>> + /** >>>> * return a escaped version of the content of this document >>>> */ >>>> public String getEscapedContent() throws XWikiException >>>> >>>> Modified: platform/core/trunk/xwiki-core/src/main/java/com/xpn/ >>>> xwiki/ >>>> api/XWiki.java >>>> =================================================================== >>>> --- platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/api/ >>>> XWiki.java 2009-07-04 12:42:14 UTC (rev 21810) >>>> +++ platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/api/ >>>> XWiki.java 2009-07-04 12:44:30 UTC (rev 21811) >>>> @@ -30,6 +30,8 @@ >>>> import org.apache.commons.logging.LogFactory; >>>> import org.suigeneris.jrcs.diff.delta.Chunk; >>>> import org.xwiki.query.QueryManager; >>>> +import org.xwiki.rendering.parser.Syntax; >>>> +import org.xwiki.rendering.renderer.PrintRendererFactory; >>>> >>>> import com.xpn.xwiki.XWikiContext; >>>> import com.xpn.xwiki.XWikiException; >>>> @@ -2685,4 +2687,42 @@ >>>> { >>>> return this.xwiki.getDefaultDocumentSyntax(); >>>> } >>>> + >>>> + /** >>>> + * Find the corresponding available renderer syntax. >>>> + * <p> >>>> + * If <code>syntaxVersion</code> is null the last version of >>>> the available provided syntax type is returned. >>>> + * >>>> + * @param syntaxType the syntax type >>>> + * @param syntaxVersion the syntax version >>>> + * @return the available corresponding {...@link Syntax}. Null if >>>> no available renderer can be found. >>>> + */ >>>> + public Syntax getAvailableRendererSyntax(String syntaxType, >>>> String syntaxVersion) >>>> + { >>>> + Syntax syntax = null; >>>> + >>>> + PrintRendererFactory printRendererFactory = >>>> + (PrintRendererFactory) >>>> Utils.getComponent(PrintRendererFactory.class); >>>> + >>>> + List<Syntax> availableSyntaxes = >>>> printRendererFactory.getAvailableSyntaxes(); >>> >>> We already have the notion of available syntaxes in the >>> SyntaxFactory. >> >> No, we have the notion of available parsers, the method name is just >> wrong in SyntaxFactory... The only place where we know the available >> renderers is the PrintRendererFactory since it's impossible to get all >> renderers otherwise until renderer are made components. >> >>> I don't understand why we need to have them in the >>> PrintRendererFactory (which is not supposed to return syntaxes BTW, >>> it's only supposed to return PrintRenderer(s). >>> >>>> + >>>> + for (Syntax availableSyntax : availableSyntaxes) { >>> >>> Why not instead do: >>> SyntaxType syntaxType = SyntaxType.getSyntaxType(syntaxTypeAsString); >>> >>> And then (when syntaxVersion != null): >>> Syntax syntax = new Syntax(syntaxType, syntaxVersion); >>> >>> And then: >>> syntaxFatory.getAvailableSyntaxes().contains(syntax) >> >> It's not that simple since we have to support the use case where >> syntaxVersion is not given (which is most of the time). >> >>> >>> hmmm.... Actually thinking more about it, I believe this whole code >>> should be moved out of XWikiDocument and instead go in the rendering >>> module in SyntaxFactory. >> >> Again in SyntaxFactory you have no idea what are the available >> renderers syntaxes and here we don't care about parsers. >> Until we have clean renderer componet my goal was to put as less as >> possible temporary code in rendering module. >> >>> >>>> + if (syntaxVersion != null) { >>>> + if >>>> (availableSyntax.getType().toIdString().equalsIgnoreCase(syntaxType) >>>> + && >>>> availableSyntax.getVersion().equals(syntaxVersion)) { >>>> + syntax = availableSyntax; >>>> + break; >>>> + } >>>> + } else { >>>> + // TODO: improve version comparaison since it does >>>> not work when comparing 2.0 and 10.0 for example. We >>>> + // should have a Version which implements >>>> Comparable like we have SyntaxId in Syntax >>>> + if >>>> (availableSyntax.getType().toIdString().equalsIgnoreCase(syntaxType) >>>> + && (syntax == null || >>>> availableSyntax.getVersion().compareTo(syntax.getVersion()) > 0)) { >>>> + syntax = availableSyntax; >>>> + } >>>> + } >>>> + } >>>> + >>>> + return syntax; >>>> + } >>>> } >>> >>> [snip] >>>> >>>> /** >>>> + * @return the syntax of the document >>>> + */ >>>> + private Syntax getSyntax() >>>> + { >>>> + Syntax syntax = null; >>>> + >>>> + String syntaxId = getDefaultDocumentSyntax(); >>>> + try { >>>> + syntax = >>>> this.syntaxFactory.createSyntaxFromIdString(getSyntaxId()); >>> >>> Shouldn't we save the Syntax object instead of syntaxId in >>> XWikiDocument so that we don't perform the conversion every time >>> getSyntax is called? >> >> I did not planed to refactor the whole XWikiDocument, i just done what >> was needed but at some point yes we should have Syntax and XDOM >> instead of the syntax/content strings :) >> >>> >>>> + } catch (ParseException e) { >>>> + LOG.error("Failed to genrate Syntax object for syntax >>>> identifier [" + syntaxId + "] in page [" >>> >>> Typo: generate >>> >>>> + + getDocumentName() + "]", e); >>>> + >>>> + syntaxId = getDefaultDocumentSyntax(); >>> >>> I don't understand this line above, it's the same as the one a few >>> lines above. >>> >>>> + try { >>>> + syntax = >>>> this >>>> .syntaxFactory.createSyntaxFromIdString(getDefaultDocumentSyntax()); >>> >>> It seems you're not using the syntaxId variable. >> >> Yes it's an error, I added the variables when added the error message >> and forgot to change the "real" code properly. >> >>> >>>> + } catch (ParseException e1) { >>>> + LOG.error("Failed to genrate default Syntax object. >>>> The defautlt syntax id in [" + syntaxId + "]", e); >>> >>> same typo >>> >>> [snip] >>> >>>> Modified: platform/core/trunk/xwiki-rendering/xwiki-rendering-api/ >>>> pom.xml >>>> =================================================================== >>>> --- platform/core/trunk/xwiki-rendering/xwiki-rendering-api/pom.xml >>>> 2009-07-04 12:42:14 UTC (rev 21810) >>>> +++ platform/core/trunk/xwiki-rendering/xwiki-rendering-api/pom.xml >>>> 2009-07-04 12:44:30 UTC (rev 21811) >>>> @@ -187,7 +187,9 @@ >>>> <!-- Excludes for the 2.0 M2 release. Once it's >>>> released remove them. --> >>>> <exclude>org/xwiki/rendering/macro/MacroManager</ >>>> exclude> >>>> <exclude>org/xwiki/rendering/macro/MacroSource</ >>>> exclude> >>>> - <exclude>org/xwiki/rendering/macro/ >>>> AbstractMacroSource</exclude> >>>> + <exclude>org/xwiki/rendering/macro/ >>>> AbstractMacroSource</exclude> >>>> + <!-- Seems clirr plugin consider adding new API as >>>> an error (which I don't understand). --> >>> >>> It doesn't normally (it' considered as an INFO level) unless it >>> breaks >>> binary compatibility. But I don't know why in your case (would need >>> to >>> check more the code, i'm only looking at the svn diff). >> >> If you remove this the plugin clearly say that there is an error and >> it also clearly say that the error is that there is new api in >> PrintRendererFactory. Maybe the error message is wrong but i check and >> rechecked and this is the only difference. >> >>> >>>> + <exclude>org/xwiki/rendering/renderer/ >>>> PrintRendererFactory</exclude> >>>> </excludes> >>>> </configuration> >>>> </execution> >>>> >>>> Modified: platform/core/trunk/xwiki-rendering/xwiki-rendering-api/ >>>> src/main/java/org/xwiki/rendering/internal/renderer/ >>>> DefaultPrintRendererFactory.java >>>> =================================================================== >>>> --- platform/core/trunk/xwiki-rendering/xwiki-rendering-api/src/ >>>> main/ >>>> java/org/xwiki/rendering/internal/renderer/ >>>> DefaultPrintRendererFactory.java 2009-07-04 12:42:14 UTC (rev >>>> 21810) >>>> +++ platform/core/trunk/xwiki-rendering/xwiki-rendering-api/src/ >>>> main/ >>>> java/org/xwiki/rendering/internal/renderer/ >>>> DefaultPrintRendererFactory.java 2009-07-04 12:44:30 UTC (rev >>>> 21811) >>>> @@ -19,6 +19,10 @@ >>>> */ >>>> package org.xwiki.rendering.internal.renderer; >>>> >>>> +import java.util.Arrays; >>>> +import java.util.Collections; >>>> +import java.util.List; >>>> + >>>> import org.xwiki.component.annotation.Component; >>>> import org.xwiki.component.annotation.Requirement; >>>> import org.xwiki.rendering.parser.Syntax; >>>> @@ -43,6 +47,10 @@ >>>> @Component >>>> public class DefaultPrintRendererFactory implements >>>> PrintRendererFactory >>>> { >>>> + private static final List<Syntax> AVAILABLE_SYNTAXES = >>>> + >>>> Collections.unmodifiableList(Arrays.asList(Syntax.XHTML_1_0, >>>> Syntax.XWIKI_2_0, Syntax.EVENT_1_0, >>>> + Syntax.TEX_1_0, Syntax.PLAIN_1_0)); >>> >>> I don't like this, see above. >> >> Again it's a factory, it's the only place where we really now what are >> the renderers. I don't like this either but since renderer are not >> component there is no choice. >> >>> >>>> + >>>> /** >>>> * Factory to easily create an XHTML Image and Link Renderers. >>>> */ >>>> @@ -55,6 +63,16 @@ >>>> /** >>>> * {...@inheritdoc} >>>> * >>>> + * @see >>>> org >>>> .xwiki >>>> .rendering.renderer.PrintRendererFactory#getAvailableSyntaxes() >>>> + */ >>>> + public List<Syntax> getAvailableSyntaxes() >>>> + { >>>> + return AVAILABLE_SYNTAXES; >>>> + } >>>> + >>> >>> I don't like this, see above. >>> >>> [snip] >>> >>> Thanks >>> -Vincent >>> >>> _______________________________________________ >>> devs mailing list >>> [email protected] >>> http://lists.xwiki.org/mailman/listinfo/devs >>> >> >> >> >> -- >> Thomas Mortagne >> _______________________________________________ >> devs mailing list >> [email protected] >> http://lists.xwiki.org/mailman/listinfo/devs > > _______________________________________________ > devs mailing list > [email protected] > http://lists.xwiki.org/mailman/listinfo/devs > -- Thomas Mortagne _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
