You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by John Hjelmstad <fa...@google.com> on 2010/11/08 23:41:19 UTC
Re: svn commit: r1031332 - in /shindig/trunk/java/gadgets/src:
main/java/org/apache/shindig/gadgets/render/ main/java/org/apache/shindig/gadgets/rewrite/
test/java/org/apache/shindig/gadgets/render/
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, <li...@apache.org> 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);
>
> }
>
>
>
Re: svn commit: r1031332 - in /shindig/trunk/java/gadgets/src:
main/java/org/apache/shindig/gadgets/render/ main/java/org/apache/shindig/gadgets/rewrite/
test/java/org/apache/shindig/gadgets/render/
Posted by John Hjelmstad <fa...@google.com>.
I guess it gets to philosophy then, notably that I remain nervous that Set
"really" means OrderedSet, and always will. It's an implicit dependency on
the particulars of the implementation rather than the contract of the
interfaces involved.
I suspect this discussion could devolve into a referendum on using Set in
Multibindings overall, but I'll assume the Guice folks have come up w/ a
good reason for that. Which I hope isn't "it's a hedge, we can turn it into
an unordered Set later" :)
--j
On Mon, Nov 8, 2010 at 2:52 PM, Paul Lindner <li...@inuus.com> 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 <fa...@google.com> 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, <li...@apache.org> 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 -- lindner@inuus.com -- linkedin.com/in/plindner
>
Re: svn commit: r1031332 - in /shindig/trunk/java/gadgets/src:
main/java/org/apache/shindig/gadgets/render/ main/java/org/apache/shindig/gadgets/rewrite/
test/java/org/apache/shindig/gadgets/render/
Posted by Sam Berlin <sb...@gmail.com>.
On Tue, Nov 9, 2010 at 1:28 AM, Paul Lindner <li...@inuus.com> wrote:
> Thanks Sam.
>
> Any suggestions on the best way to support arbitrary ordered plugin modules
> using multibindings? So far proposed solutions include:
>
> * Use separate bindings for separate "phases"
> * Add a getPhase() method that returns an integer, sort the Multibinding
> Set based on this.
> * implement Comparable interface and have each class compare against the
> list of dependent classes. Sort.
>
> Thanks!
>
The last one sounds the least appealing to me.. each class would have to
know about each other class. The middle one is OK, but then an arbitrary
plugin could still insert itself in the wrong place. Separate bindings
sounds nice, because core/internal stuff can bind using core/internal
binding annotations and prevent external stuff from being inserted.
sam
>
>
>
> On Mon, Nov 8, 2010 at 8:46 PM, Sam Berlin <sb...@gmail.com> wrote:
>
>> It's unlikely that we'll add an OrderedSet or UniqueList into Guice
>> itself. Guava may have something like this (or may consider adding it), but
>> we've been hesitant to put a full Guava dependency in Guice since Guice is
>> intended to be a small/lightweight library, and Guava is fairly big.
>>
>> sam
>>
>>
>> On Mon, Nov 8, 2010 at 6:12 PM, Paul Lindner <li...@inuus.com> wrote:
>>
>>> +sberlin
>>>
>>> On Mon, Nov 8, 2010 at 3:08 PM, John Hjelmstad <fa...@google.com> 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 <henry.saputra@gmail.com
>>>> >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 <li...@inuus.com>
>>>> 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 <fa...@google.com>
>>>> 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, <li...@apache.org> 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 -- lindner@inuus.com -- linkedin.com/in/plindner
>>>> > >
>>>> >
>>>> >
>>>> >
>>>> > --
>>>> > Thanks,
>>>> > Henry
>>>> >
>>>>
>>>
>>>
>>>
>>> --
>>> Paul Lindner -- lindner@inuus.com -- linkedin.com/in/plindner
>>>
>>
>>
>
>
> --
> Paul Lindner -- lindner@inuus.com -- linkedin.com/in/plindner
>
Re: svn commit: r1031332 - in /shindig/trunk/java/gadgets/src:
main/java/org/apache/shindig/gadgets/render/ main/java/org/apache/shindig/gadgets/rewrite/
test/java/org/apache/shindig/gadgets/render/
Posted by Paul Lindner <li...@inuus.com>.
Thanks Sam.
Any suggestions on the best way to support arbitrary ordered plugin modules
using multibindings? So far proposed solutions include:
* Use separate bindings for separate "phases"
* Add a getPhase() method that returns an integer, sort the Multibinding Set
based on this.
* implement Comparable interface and have each class compare against the
list of dependent classes. Sort.
Thanks!
On Mon, Nov 8, 2010 at 8:46 PM, Sam Berlin <sb...@gmail.com> wrote:
> It's unlikely that we'll add an OrderedSet or UniqueList into Guice
> itself. Guava may have something like this (or may consider adding it), but
> we've been hesitant to put a full Guava dependency in Guice since Guice is
> intended to be a small/lightweight library, and Guava is fairly big.
>
> sam
>
>
> On Mon, Nov 8, 2010 at 6:12 PM, Paul Lindner <li...@inuus.com> wrote:
>
>> +sberlin
>>
>> On Mon, Nov 8, 2010 at 3:08 PM, John Hjelmstad <fa...@google.com> 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 <henry.saputra@gmail.com
>>> >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 <li...@inuus.com>
>>> 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 <fa...@google.com>
>>> 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, <li...@apache.org> 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 -- lindner@inuus.com -- linkedin.com/in/plindner
>>> > >
>>> >
>>> >
>>> >
>>> > --
>>> > Thanks,
>>> > Henry
>>> >
>>>
>>
>>
>>
>> --
>> Paul Lindner -- lindner@inuus.com -- linkedin.com/in/plindner
>>
>
>
--
Paul Lindner -- lindner@inuus.com -- linkedin.com/in/plindner
Re: svn commit: r1031332 - in /shindig/trunk/java/gadgets/src:
main/java/org/apache/shindig/gadgets/render/ main/java/org/apache/shindig/gadgets/rewrite/
test/java/org/apache/shindig/gadgets/render/
Posted by Sam Berlin <sb...@gmail.com>.
It's unlikely that we'll add an OrderedSet or UniqueList into Guice itself.
Guava may have something like this (or may consider adding it), but we've
been hesitant to put a full Guava dependency in Guice since Guice is
intended to be a small/lightweight library, and Guava is fairly big.
sam
On Mon, Nov 8, 2010 at 6:12 PM, Paul Lindner <li...@inuus.com> wrote:
> +sberlin
>
> On Mon, Nov 8, 2010 at 3:08 PM, John Hjelmstad <fa...@google.com> 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 <henry.saputra@gmail.com
>> >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 <li...@inuus.com> 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 <fa...@google.com>
>> 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, <li...@apache.org> 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 -- lindner@inuus.com -- linkedin.com/in/plindner
>> > >
>> >
>> >
>> >
>> > --
>> > Thanks,
>> > Henry
>> >
>>
>
>
>
> --
> Paul Lindner -- lindner@inuus.com -- linkedin.com/in/plindner
>
Re: svn commit: r1031332 - in /shindig/trunk/java/gadgets/src:
main/java/org/apache/shindig/gadgets/render/ main/java/org/apache/shindig/gadgets/rewrite/
test/java/org/apache/shindig/gadgets/render/
Posted by Kai Feng Zhang <kf...@gmail.com>.
Thank you Ziv, it makes sense.
Thanks,
Kevin Zhang (凯峰)
Gtalk: kf.zhang@gmail.com
Blog: http://www.zhangkf.com
Twitter: http://twitter.com/zhangkf
On Wed, Nov 10, 2010 at 12:31 PM, Ziv Horesh <zh...@gmail.com> wrote:
> On Tue, Nov 9, 2010 at 5:29 PM, Kai Feng Zhang <kf...@gmail.com> 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 <zh...@gmail.com> 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 <li...@inuus.com>
> wrote:
> > >
> > > > +sberlin
> > > >
> > > > On Mon, Nov 8, 2010 at 3:08 PM, John Hjelmstad <fa...@google.com>
> > 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 <
> > henry.saputra@gmail.com
> > > > > >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 <li...@inuus.com>
> > > > 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 <
> fargo@google.com
> > >
> > > > > 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, <li...@apache.org> 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 -- lindner@inuus.com -- linkedin.com/in/plindner
> > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Thanks,
> > > > > > Henry
> > > > > >
> > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > Paul Lindner -- lindner@inuus.com -- linkedin.com/in/plindner
> > > >
> > >
> >
>
Re: svn commit: r1031332 - in /shindig/trunk/java/gadgets/src:
main/java/org/apache/shindig/gadgets/render/ main/java/org/apache/shindig/gadgets/rewrite/
test/java/org/apache/shindig/gadgets/render/
Posted by Ziv Horesh <zh...@gmail.com>.
On Tue, Nov 9, 2010 at 5:29 PM, Kai Feng Zhang <kf...@gmail.com> 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 <zh...@gmail.com> 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 <li...@inuus.com> wrote:
> >
> > > +sberlin
> > >
> > > On Mon, Nov 8, 2010 at 3:08 PM, John Hjelmstad <fa...@google.com>
> 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 <
> henry.saputra@gmail.com
> > > > >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 <li...@inuus.com>
> > > 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 <fargo@google.com
> >
> > > > 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, <li...@apache.org> 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 -- lindner@inuus.com -- linkedin.com/in/plindner
> > > > > >
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Thanks,
> > > > > Henry
> > > > >
> > > >
> > >
> > >
> > >
> > > --
> > > Paul Lindner -- lindner@inuus.com -- linkedin.com/in/plindner
> > >
> >
>
Re: svn commit: r1031332 - in /shindig/trunk/java/gadgets/src:
main/java/org/apache/shindig/gadgets/render/ main/java/org/apache/shindig/gadgets/rewrite/
test/java/org/apache/shindig/gadgets/render/
Posted by Kai Feng Zhang <kf...@gmail.com>.
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?
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?
Thank you.
Kevin.
On Tue, Nov 9, 2010 at 7:25 AM, Ziv Horesh <zh...@gmail.com> 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 <li...@inuus.com> wrote:
>
> > +sberlin
> >
> > On Mon, Nov 8, 2010 at 3:08 PM, John Hjelmstad <fa...@google.com> 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 <henry.saputra@gmail.com
> > > >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 <li...@inuus.com>
> > 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 <fa...@google.com>
> > > 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, <li...@apache.org> 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 -- lindner@inuus.com -- linkedin.com/in/plindner
> > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > Thanks,
> > > > Henry
> > > >
> > >
> >
> >
> >
> > --
> > Paul Lindner -- lindner@inuus.com -- linkedin.com/in/plindner
> >
>
Re: svn commit: r1031332 - in /shindig/trunk/java/gadgets/src:
main/java/org/apache/shindig/gadgets/render/ main/java/org/apache/shindig/gadgets/rewrite/
test/java/org/apache/shindig/gadgets/render/
Posted by Henry Saputra <he...@gmail.com>.
+1
- Henry
On Mon, Nov 8, 2010 at 11:25 PM, Ziv Horesh <zh...@gmail.com> 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 <li...@inuus.com> wrote:
>
>> +sberlin
>>
>> On Mon, Nov 8, 2010 at 3:08 PM, John Hjelmstad <fa...@google.com> 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 <henry.saputra@gmail.com
>> > >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 <li...@inuus.com>
>> 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 <fa...@google.com>
>> > 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, <li...@apache.org> 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 -- lindner@inuus.com -- linkedin.com/in/plindner
>> > > >
>> > >
>> > >
>> > >
>> > > --
>> > > Thanks,
>> > > Henry
>> > >
>> >
>>
>>
>>
>> --
>> Paul Lindner -- lindner@inuus.com -- linkedin.com/in/plindner
>>
>
--
Thanks,
Henry
Re: svn commit: r1031332 - in /shindig/trunk/java/gadgets/src:
main/java/org/apache/shindig/gadgets/render/ main/java/org/apache/shindig/gadgets/rewrite/
test/java/org/apache/shindig/gadgets/render/
Posted by John Hjelmstad <fa...@google.com>.
SGTM
On Mon, Nov 8, 2010 at 3:25 PM, Ziv Horesh <zh...@gmail.com> 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 <li...@inuus.com> wrote:
>
> > +sberlin
> >
> > On Mon, Nov 8, 2010 at 3:08 PM, John Hjelmstad <fa...@google.com> 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 <henry.saputra@gmail.com
> > > >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 <li...@inuus.com>
> > 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 <fa...@google.com>
> > > 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, <li...@apache.org> 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 -- lindner@inuus.com -- linkedin.com/in/plindner
> > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > Thanks,
> > > > Henry
> > > >
> > >
> >
> >
> >
> > --
> > Paul Lindner -- lindner@inuus.com -- linkedin.com/in/plindner
> >
>
Re: svn commit: r1031332 - in /shindig/trunk/java/gadgets/src:
main/java/org/apache/shindig/gadgets/render/ main/java/org/apache/shindig/gadgets/rewrite/
test/java/org/apache/shindig/gadgets/render/
Posted by Ziv Horesh <zh...@gmail.com>.
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 <li...@inuus.com> wrote:
> +sberlin
>
> On Mon, Nov 8, 2010 at 3:08 PM, John Hjelmstad <fa...@google.com> 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 <henry.saputra@gmail.com
> > >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 <li...@inuus.com>
> 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 <fa...@google.com>
> > 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, <li...@apache.org> 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 -- lindner@inuus.com -- linkedin.com/in/plindner
> > > >
> > >
> > >
> > >
> > > --
> > > Thanks,
> > > Henry
> > >
> >
>
>
>
> --
> Paul Lindner -- lindner@inuus.com -- linkedin.com/in/plindner
>
Re: svn commit: r1031332 - in /shindig/trunk/java/gadgets/src:
main/java/org/apache/shindig/gadgets/render/ main/java/org/apache/shindig/gadgets/rewrite/
test/java/org/apache/shindig/gadgets/render/
Posted by Paul Lindner <li...@inuus.com>.
+sberlin
On Mon, Nov 8, 2010 at 3:08 PM, John Hjelmstad <fa...@google.com> 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 <henry.saputra@gmail.com
> >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 <li...@inuus.com> 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 <fa...@google.com>
> 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, <li...@apache.org> 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 -- lindner@inuus.com -- linkedin.com/in/plindner
> > >
> >
> >
> >
> > --
> > Thanks,
> > Henry
> >
>
--
Paul Lindner -- lindner@inuus.com -- linkedin.com/in/plindner
Re: svn commit: r1031332 - in /shindig/trunk/java/gadgets/src:
main/java/org/apache/shindig/gadgets/render/ main/java/org/apache/shindig/gadgets/rewrite/
test/java/org/apache/shindig/gadgets/render/
Posted by John Hjelmstad <fa...@google.com>.
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 <he...@gmail.com>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 <li...@inuus.com> 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 <fa...@google.com> 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, <li...@apache.org> 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 -- lindner@inuus.com -- linkedin.com/in/plindner
> >
>
>
>
> --
> Thanks,
> Henry
>
Re: svn commit: r1031332 - in /shindig/trunk/java/gadgets/src:
main/java/org/apache/shindig/gadgets/render/ main/java/org/apache/shindig/gadgets/rewrite/
test/java/org/apache/shindig/gadgets/render/
Posted by Henry Saputra <he...@gmail.com>.
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 <li...@inuus.com> 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 <fa...@google.com> 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, <li...@apache.org> 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 -- lindner@inuus.com -- linkedin.com/in/plindner
>
--
Thanks,
Henry
Re: svn commit: r1031332 - in /shindig/trunk/java/gadgets/src:
main/java/org/apache/shindig/gadgets/render/ main/java/org/apache/shindig/gadgets/rewrite/
test/java/org/apache/shindig/gadgets/render/
Posted by Paul Lindner <li...@inuus.com>.
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 <fa...@google.com> 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, <li...@apache.org> 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 -- lindner@inuus.com -- linkedin.com/in/plindner