Hi Shaopeng: Comments inline. On Fri, Aug 28, 2009 at 3:25 AM, Shaopeng Jia (贾少鹏) <[email protected]>wrote:
> 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. > This is par for the course when it comes to rewriters. We could always use more complete documentation, both regarding the function of existing rewriters as well as their intended order. Meanwhile, RenderingGadgetRewriter's existing factoring is reasonably clear in the sense that it performs all doc-generation functions required to accommodate the spec's core tags and associated functionality. OpenSocialI18N is a feature extension, and isn't core to rendering a gadget spec, so should be factored as such. I don't mean to be difficult, just want to avoid the overextension of our core classes. I'll go ahead and review the code with this presumed factoring in mind. It's totally fair, that is to say, that you assume that RenderingContentRewriter has already done its work before your rewriter comes into the picture. Cheers, John > > 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 >
