Thank you Ziv, it makes sense. Thanks,
Kevin Zhang (凯峰) Gtalk: [email protected] Blog: http://www.zhangkf.com Twitter: http://twitter.com/zhangkf On Wed, Nov 10, 2010 at 12:31 PM, Ziv Horesh <[email protected]> wrote: > 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 > > > > > > > > > >
