+1 - Henry
On Mon, Nov 8, 2010 at 11:25 PM, 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 >> > -- Thanks, Henry
