On Tue, Nov 9, 2010 at 5:29 PM, Kai Feng Zhang <[email protected]> wrote:
> Hey Ziv, > > Thank you for the new patch. > > As per your new patch, now if we want to append any rewriter in custom > module, we need to multibind it to @Named "shindig.rewriters.gadget.set", > but not "shindig.rewriters.gadget", is it true? > Yes if you use shindig RewriteModule and you want to add rewriter you need to multibind the name "shindig.rewriters.gadget.set" > I am a little confused why we need to provide it this way, keeping gadget > rewirters provided as List to provider? What's the difference between the > new patch and my old patch? > The difference is in the api specification. When you use list in the provider it is clear that order is important. And it is relevant in cases that you override RewriterModule and replace the list of rewriters with your own list. > > Thank you. > > Kevin. > > > On Tue, Nov 9, 2010 at 7:25 AM, Ziv Horesh <[email protected]> wrote: > > > 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 > > > > > >
