View level support for Features and Locales

Patch to support View level features and locales which is an approved proposal for OpenSocial 2.0.

http://docs.opensocial.org/display/OSD/View+Level+Features+Proposal

Patch includes updated JUnit tests and new EndToEnd test.

https://issues.apache.org/jira/browse/SHINDIG-1492

[zhoresh, 2011-01-26 01:45:19]

Some preliminary comments

http://codereview.appspot.com/4077043/diff/14001/java/gadgets/src/main/java/o...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java (right):

http://codereview.appspot.com/4077043/diff/14001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java:141:
if(directFeatureDeps == null){
Please fix indentation (use spaces)

http://codereview.appspot.com/4077043/diff/14001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java:153:
initializeFeatureDeps();
same

http://codereview.appspot.com/4077043/diff/14001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/MessageBundleFactory.java
(right):

http://codereview.appspot.com/4077043/diff/14001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/MessageBundleFactory.java:46:
MessageBundle getBundle(GadgetSpec spec, Locale locale, boolean ignoreCache,
String container, String view)
Did you miss DefaultMessageBundleFactory class?

http://codereview.appspot.com/4077043/diff/14001/java/gadgets/src/main/java/o...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java
(right):

http://codereview.appspot.com/4077043/diff/14001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java:496:
public Map<String,Map<Locale, LocaleSpec>> getLocales() {
I would to leave the interface here as before, map locale to spec. The view will
be part of the spec. (similar to features).
This interface propagate down to the json interface. Which is simple enough to
add to it, but change structure will be harder to push.

[mgmarum, 2011-01-26 19:00:19]

Made some updates based upon your comments.

I added a comment about the JSON interface.  Not sure if the Locales are
surfaced currently?

http://codereview.appspot.com/4077043/diff/14001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/MessageBundleFactory.java
(right):

http://codereview.appspot.com/4077043/diff/14001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/MessageBundleFactory.java:46:
MessageBundle getBundle(GadgetSpec spec, Locale locale, boolean ignoreCache,
String container, String view)
On 2011/01/26 01:45:20, zhoresh wrote:
> Did you miss DefaultMessageBundleFactory class?

It is in the change set, first entry.  :)

http://codereview.appspot.com/4077043/diff/14001/java/gadgets/src/main/java/o...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java
(right):

http://codereview.appspot.com/4077043/diff/14001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java:496:
public Map<String,Map<Locale, LocaleSpec>> getLocales() {
On 2011/01/26 01:45:20, zhoresh wrote:
> I would to leave the interface here as before, map locale to spec. The view
will
> be part of the spec. (similar to features).
> This interface propagate down to the json interface. Which is simple enough to
> add to it, but change structure will be harder to push.

I've made the suggested change, but when I dug down into it I'm not sure if
LocaleSpecs are currently available via the APIs.

I'm looking at...
org.apache.shindig.gadgets.servlet.GadgetsHandlerApi.ModulePrefs
org.apache.shindig.gadgets.servlet.JsonRpcHandler.Job.getGadgetJson(Gadget,
GadgetSpec)

[mgmarum, 2011-01-28 15:53:04]

http://codereview.appspot.com/4077043/diff/14001/java/gadgets/src/main/java/o...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java (right):

http://codereview.appspot.com/4077043/diff/14001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java:141:
if(directFeatureDeps == null){
On 2011/01/26 01:45:20, zhoresh wrote:
> Please fix indentation (use spaces)

Done.

http://codereview.appspot.com/4077043/diff/14001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java:153:
initializeFeatureDeps();
On 2011/01/26 01:45:20, zhoresh wrote:
> same

Done.

[zhoresh, 2011-01-28 18:00:00]

Looks pretty good, just couple more comments.

http://codereview.appspot.com/4077043/diff/14001/java/gadgets/src/main/java/o...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java
(right):

http://codereview.appspot.com/4077043/diff/14001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java:496:
public Map<String,Map<Locale, LocaleSpec>> getLocales() {
On 2011/01/26 19:00:19, mgmarum wrote:
> On 2011/01/26 01:45:20, zhoresh wrote:
> > I would to leave the interface here as before, map locale to spec. The view
> will
> > be part of the spec. (similar to features).
> > This interface propagate down to the json interface. Which is simple enough
to
> > add to it, but change structure will be harder to push.
> 
> I've made the suggested change, but when I dug down into it I'm not sure if
> LocaleSpecs are currently available via the APIs.
> 
> I'm looking at...
> org.apache.shindig.gadgets.servlet.GadgetsHandlerApi.ModulePrefs
> org.apache.shindig.gadgets.servlet.JsonRpcHandler.Job.getGadgetJson(Gadget,
> GadgetSpec)

True, but only because I was lazy...
The problem with shindig is that almost any class is a an "interface" since we
have many implementations that enhance the code and use internal classes, so it
better not to break them if possible

http://codereview.appspot.com/4077043/diff/43001/java/gadgets/src/main/java/o...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java (right):

http://codereview.appspot.com/4077043/diff/43001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java:176: view =
spec.getView(GadgetSpec.DEFAULT_VIEW);
just to be safe, instead of doing spec.getView(DEFAULT_VIEW).getName(), just use
DEFAULT_VIEW
(like in line 132-136)

http://codereview.appspot.com/4077043/diff/43001/java/gadgets/src/main/java/o...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java
(right):

http://codereview.appspot.com/4077043/diff/43001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java:140:
locales = prefs.locales;
calculate allLocales
(and add a test :-)

http://codereview.appspot.com/4077043/diff/43001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java:824:
Map<Locale, LocaleSpec> viewLocaleSpecs;
This is repeated code, can probably be improved

http://codereview.appspot.com/4077043/diff/43001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java:838:
moduleprefs.locales = ImmutableMap.copyOf(locales);
no need to copy

http://codereview.appspot.com/4077043/diff/43001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java:839:
for(String view : locales.keySet()){
just iterate values

[mgmarum, 2011-01-28 21:32:20]

http://codereview.appspot.com/4077043/diff/14001/java/gadgets/src/main/java/o...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java
(right):

http://codereview.appspot.com/4077043/diff/14001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java:496:
public Map<String,Map<Locale, LocaleSpec>> getLocales() {
On 2011/01/28 18:00:00, zhoresh wrote:
> On 2011/01/26 19:00:19, mgmarum wrote:
> > On 2011/01/26 01:45:20, zhoresh wrote:
> > > I would to leave the interface here as before, map locale to spec. The
view
> > will
> > > be part of the spec. (similar to features).
> > > This interface propagate down to the json interface. Which is simple
enough
> to
> > > add to it, but change structure will be harder to push.
> > 
> > I've made the suggested change, but when I dug down into it I'm not sure if
> > LocaleSpecs are currently available via the APIs.
> > 
> > I'm looking at...
> > org.apache.shindig.gadgets.servlet.GadgetsHandlerApi.ModulePrefs
> > org.apache.shindig.gadgets.servlet.JsonRpcHandler.Job.getGadgetJson(Gadget,
> > GadgetSpec)
> 
> True, but only because I was lazy...
> The problem with shindig is that almost any class is a an "interface" since we
> have many implementations that enhance the code and use internal classes, so
it
> better not to break them if possible

Good to know.  Thanks.

http://codereview.appspot.com/4077043/diff/43001/java/gadgets/src/main/java/o...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java (right):

http://codereview.appspot.com/4077043/diff/43001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java:176: view =
spec.getView(GadgetSpec.DEFAULT_VIEW);
On 2011/01/28 18:00:01, zhoresh wrote:
> just to be safe, instead of doing spec.getView(DEFAULT_VIEW).getName(), just
use
> DEFAULT_VIEW
> (like in line 132-136)

Done.

http://codereview.appspot.com/4077043/diff/43001/java/gadgets/src/main/java/o...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java
(right):

http://codereview.appspot.com/4077043/diff/43001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java:140:
locales = prefs.locales;
On 2011/01/28 18:00:01, zhoresh wrote:
> calculate allLocales
> (and add a test :-)

Fixed & new test added to ModulePrefsTest.

http://codereview.appspot.com/4077043/diff/43001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java:824:
Map<Locale, LocaleSpec> viewLocaleSpecs;
On 2011/01/28 18:00:01, zhoresh wrote:
> This is repeated code, can probably be improved

Extracted a method.  Done.

http://codereview.appspot.com/4077043/diff/43001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java:838:
moduleprefs.locales = ImmutableMap.copyOf(locales);
On 2011/01/28 18:00:01, zhoresh wrote:
> no need to copy

Done.

http://codereview.appspot.com/4077043/diff/43001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java:839:
for(String view : locales.keySet()){
On 2011/01/28 18:00:01, zhoresh wrote:
> just iterate values

Done.

[zhoresh, 2011-02-01 17:19:45]

Nice work, LGTM

[mgmarum, 2011-03-22 18:37:39]

Refresh patch with latest

[Paul Lindner, 2011-03-28 09:51:17]

looking good.  Just a few stylistic things and import ordering.

Adding fargo as a reviewer.

Is this still consistent with the spec proposal?

http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/main/java/o...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java (right):

http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java:20: import
java.util.Collection;
import flux.

http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/main/java/o...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/Feature.java
(right):

http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/Feature.java:168:
this.views = ImmutableSet.copyOf(viewNames.trim().split("\\s*,+\\s*"));
Try this instead of a regex:

Splitter.on(',').omitEmptyStrings().trimResults().split(viewNames);

http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/main/java/o...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/LocaleSpec.java
(right):

http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/LocaleSpec.java:65:
this.views = ImmutableSet.copyOf(viewNames.trim().split("\\s*,+\\s*"));
Use splitter again.  You also don't need to special case the empty string then.

http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/main/java/o...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java
(left):

http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java:19: 
import flux

http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/test/java/o...
File java/gadgets/src/test/java/org/apache/shindig/gadgets/GadgetTest.java
(right):

http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/GadgetTest.java:23: 
import flux

[mgmarum, 2011-03-28 14:23:48]

Yes, this is still consistent with spec proposal.

On 2011/03/28 09:51:17, Paul Lindner wrote:
> looking good.  Just a few stylistic things and import ordering.
> 
> Adding fargo as a reviewer.
> 
> Is this still consistent with the spec proposal?
> 
>
http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/main/java/o...
> File java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java
(right):
> 
>
http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/main/java/o...
> java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java:20: import
> java.util.Collection;
> import flux.
> 
>
http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/main/java/o...
> File java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/Feature.java
> (right):
> 
>
http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/main/java/o...
> java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/Feature.java:168:
> this.views = ImmutableSet.copyOf(viewNames.trim().split("\\s*,+\\s*"));
> Try this instead of a regex:
> 
> Splitter.on(',').omitEmptyStrings().trimResults().split(viewNames);
> 
>
http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/main/java/o...
> File
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/LocaleSpec.java
> (right):
> 
>
http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/main/java/o...
> java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/LocaleSpec.java:65:
> this.views = ImmutableSet.copyOf(viewNames.trim().split("\\s*,+\\s*"));
> Use splitter again.  You also don't need to special case the empty string
then.
> 
>
http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/main/java/o...
> File
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java
> (left):
> 
>
http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/main/java/o...
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java:19: 
> import flux
> 
>
http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/test/java/o...
> File java/gadgets/src/test/java/org/apache/shindig/gadgets/GadgetTest.java
> (right):
> 
>
http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/test/java/o...
> java/gadgets/src/test/java/org/apache/shindig/gadgets/GadgetTest.java:23: 
> import flux

[mgmarum, 2011-03-28 14:24:08]

Updated patch.

http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/main/java/o...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java (right):

http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java:20: import
java.util.Collection;
On 2011/03/28 09:51:17, Paul Lindner wrote:
> import flux.

Done.

http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/main/java/o...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/Feature.java
(right):

http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/Feature.java:168:
this.views = ImmutableSet.copyOf(viewNames.trim().split("\\s*,+\\s*"));
On 2011/03/28 09:51:17, Paul Lindner wrote:
> Try this instead of a regex:
> 
> Splitter.on(',').omitEmptyStrings().trimResults().split(viewNames);

Done.

http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/main/java/o...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/LocaleSpec.java
(right):

http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/LocaleSpec.java:65:
this.views = ImmutableSet.copyOf(viewNames.trim().split("\\s*,+\\s*"));
On 2011/03/28 09:51:17, Paul Lindner wrote:
> Use splitter again.  You also don't need to special case the empty string
then.

Done.

http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/main/java/o...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java
(left):

http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java:19: 
On 2011/03/28 09:51:17, Paul Lindner wrote:
> import flux

Done.

http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/test/java/o...
File java/gadgets/src/test/java/org/apache/shindig/gadgets/GadgetTest.java
(right):

http://codereview.appspot.com/4077043/diff/67001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/GadgetTest.java:23: 
On 2011/03/28 09:51:17, Paul Lindner wrote:
> import flux

Done.

[mgmarum, 2011-04-02 03:09:53]

Fixed test failure on Sun JRE