You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by "Shaopeng Jia (贾少鹏)" <sh...@google.com> on 2009/08/26 09:41:58 UTC
Not able to access gadgets.i18n.DateTimeConstants
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
Re: Not able to access gadgets.i18n.DateTimeConstants
Posted by "Shaopeng Jia (贾少鹏)" <sh...@google.com>.
Thanks for the swift review John! Comments inlined:
On Fri, Aug 28, 2009 at 7:51 PM, John Hjelmstad <fa...@google.com> wrote:
> Hi Shaopeng:
> Comments inline.
>
> On Fri, Aug 28, 2009 at 3:25 AM, Shaopeng Jia (贾少鹏) <
> shaopengjia@google.com> 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 (贾少鹏) <
>> shaopengjia@google.com> wrote:
>>
>>>
>>>
>>> On Thu, Aug 27, 2009 at 12:04 AM, John Hjelmstad <fa...@google.com>wrote:
>>>
>>>> On Wed, Aug 26, 2009 at 2:56 PM, Shaopeng Jia (贾少鹏) <
>>>> shaopengjia@google.com> wrote:
>>>>
>>>>> Thanks for the reply John! My comments inlined.
>>>>>
>>>>> On Wed, Aug 26, 2009 at 7:57 PM, John Hjelmstad <fa...@google.com>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:
>>>> shindig.remailer@gmail.com).
>>>> 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 (贾少鹏) <
>>>>>> shaopengjia@google.com> 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
Re: Not able to access gadgets.i18n.DateTimeConstants
Posted by John Hjelmstad <fa...@google.com>.
Hi Shaopeng:
Comments inline.
On Fri, Aug 28, 2009 at 3:25 AM, Shaopeng Jia (贾少鹏)
<sh...@google.com>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 (贾少鹏) <
> shaopengjia@google.com> wrote:
>
>>
>>
>> On Thu, Aug 27, 2009 at 12:04 AM, John Hjelmstad <fa...@google.com>wrote:
>>
>>> On Wed, Aug 26, 2009 at 2:56 PM, Shaopeng Jia (贾少鹏) <
>>> shaopengjia@google.com> wrote:
>>>
>>>> Thanks for the reply John! My comments inlined.
>>>>
>>>> On Wed, Aug 26, 2009 at 7:57 PM, John Hjelmstad <fa...@google.com>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:
>>> shindig.remailer@gmail.com).
>>> 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 (贾少鹏) <
>>>>> shaopengjia@google.com> 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
>
Re: Not able to access gadgets.i18n.DateTimeConstants
Posted by "Shaopeng Jia (贾少鹏)" <sh...@google.com>.
On Thu, Aug 27, 2009 at 12:04 AM, John Hjelmstad <fa...@google.com> wrote:
> On Wed, Aug 26, 2009 at 2:56 PM, Shaopeng Jia (贾少鹏) <
> shaopengjia@google.com> wrote:
>
>> Thanks for the reply John! My comments inlined.
>>
>> On Wed, Aug 26, 2009 at 7:57 PM, John Hjelmstad <fa...@google.com> 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: shindig.remailer@gmail.com).
> 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 (贾少鹏) <
>>> shaopengjia@google.com> 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
Re: Not able to access gadgets.i18n.DateTimeConstants
Posted by John Hjelmstad <fa...@google.com>.
On Wed, Aug 26, 2009 at 2:56 PM, Shaopeng Jia (贾少鹏)
<sh...@google.com>wrote:
> Thanks for the reply John! My comments inlined.
>
> On Wed, Aug 26, 2009 at 7:57 PM, John Hjelmstad <fa...@google.com> 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: shindig.remailer@gmail.com).
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 (贾少鹏) <
>> shaopengjia@google.com> 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
>
Re: Not able to access gadgets.i18n.DateTimeConstants
Posted by "Shaopeng Jia (贾少鹏)" <sh...@google.com>.
Thanks for the reply John! My comments inlined.
On Wed, Aug 26, 2009 at 7:57 PM, John Hjelmstad <fa...@google.com> 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.
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.
>
> 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.
>
> Thoughts? Other Shindig folks?
>
> --John
>
> On Wed, Aug 26, 2009 at 12:41 AM, Shaopeng Jia (贾少鹏) <
> shaopengjia@google.com> 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
Re: Not able to access gadgets.i18n.DateTimeConstants
Posted by John Hjelmstad <fa...@google.com>.
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.
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".
Thoughts? Other Shindig folks?
--John
On Wed, Aug 26, 2009 at 12:41 AM, Shaopeng Jia (贾少鹏) <shaopengjia@google.com
> 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
>