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
