Thanks for the swift review John! Comments inlined: On Fri, Aug 28, 2009 at 7:51 PM, John Hjelmstad <[email protected]> wrote:
> 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. > When you talked about making it more modular and easy to remove in case of problem last time, I thought you were talking about the java code itself. After realizing you also meant the generated html/js code, I think it is a good idea to put the generated constants into a separate <script> section. I have uploaded a new patch set which creates the OpenSocialI18NGadgetRewriter. I have tested it and the data constants are generated correctly in a separate <script> section in the resultant html file. Please take a look at the change and let me know your feedback: http://codereview.appspot.com/109114/show Thanks, Shaopeng > 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 >> > > -- Shaopeng Jia 贾少鹏 Software Engineer - Îñţérñåţîöñåļîžåţîöñ Google Switzerland GmbH |Identifikationsnummer: CH-020.4.028.116-1
