On Wed, Aug 26, 2009 at 2:56 PM, Shaopeng Jia (贾少鹏) <[email protected]>wrote:
> Thanks for the reply John! My comments inlined. > > On Wed, Aug 26, 2009 at 7:57 PM, John Hjelmstad <[email protected]> wrote: > >> Hi Shaopeng: >> As I see it, there are two ways to implement this that fit with current >> Shindig extension style. The main issue w/ the patch you've implemented is >> that it embeds feature-sensitivity into a core piece of rendering logic. We >> can use one of the existing mechanisms: >> >> 1) feature.xml and the JsLibrary/JsFeatureLoader/GadgetFeature system. >> We've been considering the addition of language and country-sensitivity to >> this mechanism for some time, and this might be the right place to do it. >> The net result would be a feature.xml including blocks such as: >> <gadget lang="en" country="us"> >> <script src="data/DateTimeConstants_en_us.js"/> >> <script src="..."/> >> >> This implementation approach is generic and reusable, both reasons I >> prefer it, assuming this would work for the i18n library (perhaps you can >> say as well as I; a perusal of the DateTimeConstants*.js files doesn't >> appear to map cleanly to lang/country... is there other selection logic >> needed?). It involves changes to GadgetFeature.java, JsFeatureLoader.java, >> JsLibrary.java, and obviously i18n/feature.xml. >> > > I agree this is a better approach. The i18n library almost map cleanly to > lang/country. There are some cases where fallbacks are needed though. For > example, there is no en_CN (English as used in China) data file, and this > needs to fall back to the en data file. > Any such implementation will need to include lang/country fallback selection logic, eg. "prefer lang match over country when feature.xml has an entry with both but not only one". Complications like these bolster arguments in favor of implementation approach #2. > > However, this sounds like a pretty significant change, and probably won't > be in production until OpenSocial 1.0. That might be too late for developers > (like Rajiv) who needs to use this feature now. Probably short term we > should provide a quick fix, and go for this approach long term. > Well, given that opensocial-i18n is really the only library (of which I'm aware) that uses this right now, since you have a strong need for this in the short term (and it's not especially clear what the "generic" rules ought to be) I'm fine w/ #2 right now. This and other uses cases will hopefully accrue to give us a better sense for what we'd need generically. > > >> >> 2) A GadgetRewriter. Essentially, write an OpensocialI18nGadgetRewriter >> implementing GadgetRewriter, which does: >> - Loads data/DateTimeConstants*.js files. >> - In rewriter, injects the appropriate DTC.js when the gadget's features >> include "opensocial-i18n". >> > > This is quite similar to what I have been doing in my patch. The difference > is instead of extending GadgetRewritter, I changed it directly. My only > problem now is how to find the location of the data directory during > rendering time. I saw the comments from Paul advising me loading the file > directly from the classpath. I will give that a try tomorrow first thing > when I get to the office. Hopefully this could work. > Yes, sounds like it'll work. But please do convert your implementation to a GadgetRewriter, as doing so is much more modular, easier to maintain, and removable by implementations in case problems are found. I'll be happy to do the review. Preferred process: 1. Write patch. 2. Upload patch to codereview.appspot.com (SVN base: Shindig - *trunk* - Real Trunk, Base: http://svn.apache.org/repos/asf/incubator/shindig/trunk/, Reviewers: [email protected]). 3. Annotate JIRA issue w/ uploaded patch and codereview.appspot reference. We iterate from there and a committer (probably me, maybe Paul) will commit your code when it looks ready. Cheers, John > > >> >> Thoughts? Other Shindig folks? >> >> --John >> >> On Wed, Aug 26, 2009 at 12:41 AM, Shaopeng Jia (贾少鹏) < >> [email protected]> wrote: >> >>> Hi shindig-dev, >>> As per this bug: >>> >>> https://issues.apache.org/jira/browse/SHINDIG-1159 >>> >>> <https://issues.apache.org/jira/browse/SHINDIG-1159>it seems >>> gadgets.i18n.DateTimeConstants.js are not outputted during rendering. I did >>> some investigation and located the problem is at >>> >>> >>> java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingContentRewriter.java >>> >>> I have attached a diff which illustrate how to fix the bug, but it is >>> loading the DateTimeConstants from my local directory right now. Is there a >>> way to get the location of the feature directory on the fly? Or there are >>> better ways to load the js code? >>> >>> Thanks, >>> >>> Shaopeng >>> >>> >>> -- >>> Shaopeng Jia >>> 贾少鹏 >>> Software Engineer - Îñţérñåţîöñåļîžåţîöñ >>> Google Switzerland GmbH |Identifikationsnummer: CH-020.4.028.116-1 >>> >> >> > > > -- > Shaopeng Jia > 贾少鹏 > Software Engineer - Îñţérñåţîöñåļîžåţîöñ > Google Switzerland GmbH |Identifikationsnummer: CH-020.4.028.116-1 >
