I think I will change the renderer contract back to list, and in the RewriteModule bind the list provider to the multi bind set. This way the contract is still an ordered list if someone overwrite it, but it also multi bind if someone uses the shindig RewriteModule and just want to add.
-Ziv On Mon, Nov 8, 2010 at 3:12 PM, Paul Lindner <[email protected]> wrote: > +sberlin > > On Mon, Nov 8, 2010 at 3:08 PM, John Hjelmstad <[email protected]> wrote: > > > If I were the Guice folks I would have proposed creating an OrderedSet, > or > > perhaps UniquifiedList (if that's the point of Set here) interface in > > com.google.inject :) > > > > On Mon, Nov 8, 2010 at 3:03 PM, Henry Saputra <[email protected] > > >wrote: > > > > > Well, the concern is the definition of Set itself which is/should not > > > guarantee sequential/ordering of the contents. > > > > > > The default implementation does return an OrderedSet (via Guice module > > > based on the individual call to bind it) but the contract API change > > > from List to Set which does not guarantee ordering. > > > > > > > > > - Henry > > > > > > On Mon, Nov 8, 2010 at 2:52 PM, Paul Lindner <[email protected]> > wrote: > > > > The nice part about the patch is that MultiBinding results in an > > Ordered > > > Set > > > > based on the order of the individual bind statements. > > > > > > > > So this means a third party module can only add a rewriter at the > > > beginning > > > > of the pipeline (by using a Guice Module that loads before the > > > > RewriterModule) or by adding a rewriter at the end of the pipeline > (by > > > > adding a Guice Module that runs after the Rewrite Module) > > > > > > > > I suspect that one could also rejigger the order of the rewriters > using > > > > Modules.override(), but that remains to be seen. > > > > > > > > On Mon, Nov 8, 2010 at 2:41 PM, John Hjelmstad <[email protected]> > > wrote: > > > > > > > >> Hi Paul: > > > >> > > > >> Fundamental problem here is that rewriter ordering is important: a > Set > > > >> obviously doesn't offer that guarantee. > > > >> > > > >> I'd recommend we rejigger this implementation so that the > Set-provided > > > >> rewriters come as an addendum to those provided as "core" by > Shindig. > > If > > > >> relative ordering of *those* Rewriters is important, we can have > > > pre-core > > > >> and post-core Sets. > > > >> > > > >> Apologies for not having seen this in JIRA; my mailbox has gotten so > > > >> overstuffed of late that some of the messages get lost in the > aether. > > > >> > > > >> --j > > > >> > > > >> On Thu, Nov 4, 2010 at 3:58 PM, <[email protected]> wrote: > > > >> > > > >> > Author: lindner > > > >> > Date: Thu Nov 4 22:58:54 2010 > > > >> > New Revision: 1031332 > > > >> > > > > >> > URL: http://svn.apache.org/viewvc?rev=1031332&view=rev > > > >> > Log: > > > >> > SHINDIG-1456 | Patch from Kai Feng Zhang | Allow modules to add > new > > > >> > rewriters > > > >> > > > > >> > Modified: > > > >> > > > > >> > > > > >> > > > > > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/GadgetRewritersProvider.java > > > >> > > > > >> > > > > >> > > > > > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java > > > >> > > > > >> > > > > >> > > > > > > > shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/render/HtmlRendererTest.java > > > >> > > > > >> > Modified: > > > >> > > > > >> > > > > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/GadgetRewritersProvider.java > > > >> > URL: > > > >> > > > > >> > > > > > > http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/GadgetRewritersProvider.java?rev=1031332&r1=1031331&r2=1031332&view=diff > > > >> > > > > >> > > > > >> > > > > > > ============================================================================== > > > >> > --- > > > >> > > > > >> > > > > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/GadgetRewritersProvider.java > > > >> > (original) > > > >> > +++ > > > >> > > > > >> > > > > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/GadgetRewritersProvider.java > > > >> > Thu Nov 4 22:58:54 2010 > > > >> > @@ -19,13 +19,13 @@ > > > >> > > > > >> > package org.apache.shindig.gadgets.render; > > > >> > > > > >> > -import com.google.inject.Inject; > > > >> > -import com.google.inject.name.Named; > > > >> > +import java.util.Set; > > > >> > > > > >> > import org.apache.shindig.gadgets.GadgetContext; > > > >> > import org.apache.shindig.gadgets.rewrite.GadgetRewriter; > > > >> > > > > >> > -import java.util.List; > > > >> > +import com.google.inject.Inject; > > > >> > +import com.google.inject.name.Named; > > > >> > > > > >> > /** > > > >> > * Class to provide list of rewriters according to gadget request. > > > >> > @@ -34,15 +34,14 @@ import java.util.List; > > > >> > * @since 2.0.0 > > > >> > */ > > > >> > public class GadgetRewritersProvider { > > > >> > - private final List<GadgetRewriter> renderRewriters; > > > >> > + private final Set<GadgetRewriter> renderRewriters; > > > >> > > > > >> > @Inject > > > >> > - public GadgetRewritersProvider( > > > >> > - @Named("shindig.rewriters.gadget") List<GadgetRewriter> > > > >> > renderRewriters) { > > > >> > + public > GadgetRewritersProvider(@Named("shindig.rewriters.gadget") > > > >> > Set<GadgetRewriter> renderRewriters) { > > > >> > this.renderRewriters = renderRewriters; > > > >> > } > > > >> > > > > >> > - public List<GadgetRewriter> getRewriters(GadgetContext context) > { > > > >> > + public Set<GadgetRewriter> getRewriters(GadgetContext context) > { > > > >> > return renderRewriters; > > > >> > } > > > >> > } > > > >> > > > > >> > Modified: > > > >> > > > > >> > > > > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java > > > >> > URL: > > > >> > > > > >> > > > > > > http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java?rev=1031332&r1=1031331&r2=1031332&view=diff > > > >> > > > > >> > > > > >> > > > > > > ============================================================================== > > > >> > --- > > > >> > > > > >> > > > > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java > > > >> > (original) > > > >> > +++ > > > >> > > > > >> > > > > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java > > > >> > Thu Nov 4 22:58:54 2010 > > > >> > @@ -18,12 +18,8 @@ > > > >> > */ > > > >> > package org.apache.shindig.gadgets.rewrite; > > > >> > > > > >> > -import com.google.common.collect.ImmutableList; > > > >> > -import com.google.inject.AbstractModule; > > > >> > -import com.google.inject.Provides; > > > >> > -import com.google.inject.Singleton; > > > >> > -import com.google.inject.name.Named; > > > >> > -import com.google.inject.name.Names; > > > >> > +import java.util.List; > > > >> > + > > > >> > import org.apache.shindig.gadgets.parse.GadgetHtmlParser; > > > >> > import org.apache.shindig.gadgets.render.CajaResponseRewriter; > > > >> > import > > > org.apache.shindig.gadgets.render.OpenSocialI18NGadgetRewriter; > > > >> > @@ -33,7 +29,13 @@ import org.apache.shindig.gadgets.render > > > >> > import > org.apache.shindig.gadgets.rewrite.image.BasicImageRewriter; > > > >> > import org.apache.shindig.gadgets.servlet.CajaContentRewriter; > > > >> > > > > >> > -import java.util.List; > > > >> > +import com.google.common.collect.ImmutableList; > > > >> > +import com.google.inject.AbstractModule; > > > >> > +import com.google.inject.Provides; > > > >> > +import com.google.inject.Singleton; > > > >> > +import com.google.inject.multibindings.Multibinder; > > > >> > +import com.google.inject.name.Named; > > > >> > +import com.google.inject.name.Names; > > > >> > > > > >> > /** > > > >> > * Guice bindings for the rewrite package. > > > >> > @@ -45,25 +47,23 @@ public class RewriteModule extends Abstr > > > >> > bind(ResponseRewriterRegistry.class) > > > >> > > > > >> > > > > >> > > > > > > .annotatedWith(Names.named("shindig.accelerate.response.rewriter.registry")) > > > >> > .to(AccelResponseRewriterRegistry.class); > > > >> > + > > > >> > + configureRewriters(); > > > >> > + > > > >> > } > > > >> > > > > >> > - @Provides > > > >> > - @Singleton > > > >> > - @Named("shindig.rewriters.gadget") > > > >> > - protected List<GadgetRewriter> provideGadgetRewriters( > > > >> > - PipelineDataGadgetRewriter pipelineRewriter, > > > >> > - TemplateRewriter templateRewriter, > > > >> > - AbsolutePathReferenceRewriter absolutePathRewriter, > > > >> > - StyleTagExtractorContentRewriter styleTagExtractorRewriter, > > > >> > - StyleAdjacencyContentRewriter styleAdjacencyRewriter, > > > >> > - ProxyingContentRewriter proxyingRewriter, > > > >> > - CajaContentRewriter cajaRewriter, > > > >> > - SanitizingGadgetRewriter sanitizedRewriter, > > > >> > - RenderingGadgetRewriter renderingRewriter, > > > >> > - OpenSocialI18NGadgetRewriter i18nRewriter) { > > > >> > - return ImmutableList.of(pipelineRewriter, templateRewriter, > > > >> > - absolutePathRewriter, styleTagExtractorRewriter, > > > >> > styleAdjacencyRewriter, proxyingRewriter, > > > >> > - cajaRewriter, sanitizedRewriter, renderingRewriter, > > > >> i18nRewriter); > > > >> > + private void configureRewriters() { > > > >> > + Multibinder<GadgetRewriter> multibinder = > > > >> > Multibinder.newSetBinder(binder(), GadgetRewriter.class, > > > >> > Names.named("shindig.rewriters.gadget")); > > > >> > + > multibinder.addBinding().to(PipelineDataGadgetRewriter.class); > > > >> > + multibinder.addBinding().to(TemplateRewriter.class); > > > >> > + > > multibinder.addBinding().to(AbsolutePathReferenceRewriter.class); > > > >> > + > > > multibinder.addBinding().to(StyleTagExtractorContentRewriter.class); > > > >> > + > > multibinder.addBinding().to(StyleAdjacencyContentRewriter.class); > > > >> > + multibinder.addBinding().to(ProxyingContentRewriter.class); > > > >> > + multibinder.addBinding().to(CajaContentRewriter.class); > > > >> > + multibinder.addBinding().to(SanitizingGadgetRewriter.class); > > > >> > + multibinder.addBinding().to(RenderingGadgetRewriter.class); > > > >> > + > > multibinder.addBinding().to(OpenSocialI18NGadgetRewriter.class); > > > >> > } > > > >> > > > > >> > @Provides > > > >> > > > > >> > Modified: > > > >> > > > > >> > > > > > > shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/render/HtmlRendererTest.java > > > >> > URL: > > > >> > > > > >> > > > > > > http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/render/HtmlRendererTest.java?rev=1031332&r1=1031331&r2=1031332&view=diff > > > >> > > > > >> > > > > >> > > > > > > ============================================================================== > > > >> > --- > > > >> > > > > >> > > > > > > shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/render/HtmlRendererTest.java > > > >> > (original) > > > >> > +++ > > > >> > > > > >> > > > > > > shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/render/HtmlRendererTest.java > > > >> > Thu Nov 4 22:58:54 2010 > > > >> > @@ -42,6 +42,7 @@ import java.util.Collection; > > > >> > import java.util.concurrent.Callable; > > > >> > > > > >> > import com.google.common.collect.ImmutableList; > > > >> > +import com.google.common.collect.ImmutableSet; > > > >> > > > > >> > /** > > > >> > * Tests for HtmlRenderer > > > >> > @@ -84,7 +85,7 @@ public class HtmlRendererTest { > > > >> > @Before > > > >> > public void setUp() throws Exception { > > > >> > renderer = new HtmlRenderer(preloaderService, proxyRenderer, > > > >> > - new > > GadgetRewritersProvider(ImmutableList.of((GadgetRewriter) > > > >> > captureRewriter)), > > > >> > + new > > GadgetRewritersProvider(ImmutableSet.of((GadgetRewriter) > > > >> > captureRewriter)), > > > >> > null); > > > >> > > > > >> > } > > > >> > > > > >> > > > > >> > > > > >> > > > > > > > > > > > > > > > > -- > > > > Paul Lindner -- [email protected] -- linkedin.com/in/plindner > > > > > > > > > > > > > > > > -- > > > Thanks, > > > Henry > > > > > > > > > -- > Paul Lindner -- [email protected] -- linkedin.com/in/plindner >
