Re: [xwiki-devs] [xwiki-notifications] r21811 - in platform: core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/api core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/doc core/trunk/xwiki-core/src/test/java/com/xpn/xwiki/api core/trunk/xwiki-rendering/
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
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()) b- we add SyntaFactory.getAvailableOutputSyntaxes() (or getAvailableRendererSyntaxes()) c- (optional) we add a Renderer.getSyntax() interface method (for later use when renderers will be components) d- we rename Document/XWikiDocument/XWiki methods for getting syntaxes to use Input/Ouput (or Parser/Renderer) in their names. 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. 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
participants (2)
-
Thomas Mortagne -
Vincent Massol