Hi John and shindig-dev, I have created and tested the fix and uploaded it for code review:
http://codereview.appspot.com/109114/show Regarding your comments on converting the implementation to a GadgetRewritter, I would like to discuss more with you. It seems right now the RenderingGadgetRewriter is handling everything to produce a valid HTML doc for the gadget output. It seems a little weird to have a standalone OpenSocialI18NGadgetRewritter at this moment. I understand it is a future plan to break up RenderingGadgetRewritter to multiple rewritters, and it might result in less repeated code written if we do the refactoring all-together later. Also, with a standalone OpenSocialI18NGadgetRewritter, it will most likely be loaded from RewriteModule.GadgetRewritterProvider<http://svn.apache.org/repos/asf/incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java> in parallel with RenderingGadgetRewritter. It would be quite likely to forget about the I18N rewritter when someone refactors RenderingGadgetRewriter later, as there won't be any mention of the I18N rewritter there. On the ease of removal in case of problem note, I made my patch a private injectI18NConstants function. In case any problem is found, commenting out the only call to this function will disable the change. Let me know what you think :) On Thu, Aug 27, 2009 at 12:10 AM, Shaopeng Jia (贾少鹏) <[email protected] > wrote: > > > On Thu, Aug 27, 2009 at 12:04 AM, John Hjelmstad <[email protected]> wrote: > >> 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. >> > > Agreed. > > >> >> >>> >>> 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. >> > > Yes, that sounds good. > > >> >> >>> >>> >>>> >>>> 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. >> > > Sure, I will do that tomorrow and hopefully upload the patch when you guys > get to office tomorrow. > > Thanks for the great tips and help! :) > > Cheers, > > >> >> 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 >>> >> >> > > > -- > 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
