You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by Charl van Niekerk <c....@utwente.nl> on 2013/07/17 12:05:28 UTC
Review Request 12647: Replacement for iGoogle gadgetsettings
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12647/
-----------------------------------------------------------
Review request for shindig and Ryan Baxter.
Bugs: SHINDIG-1188
https://issues.apache.org/jira/browse/SHINDIG-1188
Repository: shindig
Description
-------
In Shindig 2.0.x there is a JavaScript that is being loaded from Google. iGoogle is shutting down in November and the expectation is that this JavaScript will no longer be available.
This patch contains a servlet that is supposed to replace iGoogle's JavaScript by implementing the equivalent functionality.
Diffs
-----
http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetSettingsException.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/GadgetSettings.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetSettingsServlet.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/server/src/main/webapp/WEB-INF/web.xml 1504060
Diff: https://reviews.apache.org/r/12647/diff/
Testing
-------
Thanks,
Charl van Niekerk
Re: Review Request 12647: Replacement for iGoogle gadgetsettings
Posted by Ryan Baxter <rb...@gmail.com>.
> On July 18, 2013, 8:48 p.m., Ryan Baxter wrote:
> > In general I understand what is happening here but I feel like there should be more reuse of the code from Shindig. I am less familiar with then 2.0.x stream so maybe some of this doesn't exist there....
> >
> > One place where I would expect some reuse is when you are fetching the gadget xml. At the very least I would expect you could use the HttpFetcher class that is part of Shindig instead of using the HttpClient directly. However I feel like there are probably classes at a higher level that will fetch, and parse the gadget XML so your code does not have to do that either. Have you looked around in Shindig to see if you can reuse any of that?
> >
> > General code cleanup comment...
> > Anywhere you see red in the diff on review board mean there are white spaces there, you should remove that.
> > You should probably import the Eclipse code formatting templates from trunk https://svn.apache.org/repos/asf/shindig/trunk/etc/eclipse/. There are ones for the Java and Javascript editors in Eclipse. I found this simple article that gives some details on how to import them http://www.avajava.com/tutorials/lessons/how-do-i-share-my-code-formatting-settings-with-another-user.html
>
> Charl van Niekerk wrote:
> I have not (yet). The code was originally designed to be able to function separately from Shindig as I did not want to fork the Shindig 2.0.x branch on my own. It's a pity making a proper patch did not occur to me earlier.
>
> I took a quick look at HttpFetcher and it should be fairly easy to implement. As for the rest, I have very limited experience with Shindig 2.0.x myself so would appreciate any pointers.
>
> Regarding the code cleanup... I should have checked for trailing whitespaces before I created the diff, my bad. Corrected immediately with a simple vim command.
>
> Thanks for the pointer regarding Eclipse; I (think) I managed to get the code formatted properly now. Earlier I tried to import the code as a project, but failed. Now I only opened the individual code files and formatted them, but this seems to be sufficient.
The formatting changes look great! Thanks for doing that.
I would take a look at GadgetHandlerService and GadgetHandler. In 2.5.0 these classes handle requests from the client to get gadget metadata, which returns back information about gadget preferences among other things. They should be there in 2.0.2 as well.
https://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java
- Ryan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12647/#review23438
-----------------------------------------------------------
On July 22, 2013, 12:19 p.m., Charl van Niekerk wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12647/
> -----------------------------------------------------------
>
> (Updated July 22, 2013, 12:19 p.m.)
>
>
> Review request for shindig and Ryan Baxter.
>
>
> Bugs: SHINDIG-1188
> https://issues.apache.org/jira/browse/SHINDIG-1188
>
>
> Repository: shindig
>
>
> Description
> -------
>
> In Shindig 2.0.x there is a JavaScript that is being loaded from Google. iGoogle is shutting down in November and the expectation is that this JavaScript will no longer be available.
>
> This patch contains a servlet that is supposed to replace iGoogle's JavaScript by implementing the equivalent functionality.
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetSettingsException.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/GadgetSettings.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetSettingsServlet.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/server/src/main/webapp/WEB-INF/web.xml 1505652
>
> Diff: https://reviews.apache.org/r/12647/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Charl van Niekerk
>
>
Re: Review Request 12647: Replacement for iGoogle gadgetsettings
Posted by Charl van Niekerk <c....@utwente.nl>.
> On July 18, 2013, 8:48 p.m., Ryan Baxter wrote:
> > In general I understand what is happening here but I feel like there should be more reuse of the code from Shindig. I am less familiar with then 2.0.x stream so maybe some of this doesn't exist there....
> >
> > One place where I would expect some reuse is when you are fetching the gadget xml. At the very least I would expect you could use the HttpFetcher class that is part of Shindig instead of using the HttpClient directly. However I feel like there are probably classes at a higher level that will fetch, and parse the gadget XML so your code does not have to do that either. Have you looked around in Shindig to see if you can reuse any of that?
> >
> > General code cleanup comment...
> > Anywhere you see red in the diff on review board mean there are white spaces there, you should remove that.
> > You should probably import the Eclipse code formatting templates from trunk https://svn.apache.org/repos/asf/shindig/trunk/etc/eclipse/. There are ones for the Java and Javascript editors in Eclipse. I found this simple article that gives some details on how to import them http://www.avajava.com/tutorials/lessons/how-do-i-share-my-code-formatting-settings-with-another-user.html
I have not (yet). The code was originally designed to be able to function separately from Shindig as I did not want to fork the Shindig 2.0.x branch on my own. It's a pity making a proper patch did not occur to me earlier.
I took a quick look at HttpFetcher and it should be fairly easy to implement. As for the rest, I have very limited experience with Shindig 2.0.x myself so would appreciate any pointers.
Regarding the code cleanup... I should have checked for trailing whitespaces before I created the diff, my bad. Corrected immediately with a simple vim command.
Thanks for the pointer regarding Eclipse; I (think) I managed to get the code formatted properly now. Earlier I tried to import the code as a project, but failed. Now I only opened the individual code files and formatted them, but this seems to be sufficient.
- Charl
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12647/#review23438
-----------------------------------------------------------
On July 22, 2013, 12:19 p.m., Charl van Niekerk wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12647/
> -----------------------------------------------------------
>
> (Updated July 22, 2013, 12:19 p.m.)
>
>
> Review request for shindig and Ryan Baxter.
>
>
> Bugs: SHINDIG-1188
> https://issues.apache.org/jira/browse/SHINDIG-1188
>
>
> Repository: shindig
>
>
> Description
> -------
>
> In Shindig 2.0.x there is a JavaScript that is being loaded from Google. iGoogle is shutting down in November and the expectation is that this JavaScript will no longer be available.
>
> This patch contains a servlet that is supposed to replace iGoogle's JavaScript by implementing the equivalent functionality.
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetSettingsException.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/GadgetSettings.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetSettingsServlet.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/server/src/main/webapp/WEB-INF/web.xml 1505652
>
> Diff: https://reviews.apache.org/r/12647/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Charl van Niekerk
>
>
Re: Review Request 12647: Replacement for iGoogle gadgetsettings
Posted by Charl van Niekerk <c....@utwente.nl>.
> On July 18, 2013, 8:48 p.m., Ryan Baxter wrote:
> > In general I understand what is happening here but I feel like there should be more reuse of the code from Shindig. I am less familiar with then 2.0.x stream so maybe some of this doesn't exist there....
> >
> > One place where I would expect some reuse is when you are fetching the gadget xml. At the very least I would expect you could use the HttpFetcher class that is part of Shindig instead of using the HttpClient directly. However I feel like there are probably classes at a higher level that will fetch, and parse the gadget XML so your code does not have to do that either. Have you looked around in Shindig to see if you can reuse any of that?
> >
> > General code cleanup comment...
> > Anywhere you see red in the diff on review board mean there are white spaces there, you should remove that.
> > You should probably import the Eclipse code formatting templates from trunk https://svn.apache.org/repos/asf/shindig/trunk/etc/eclipse/. There are ones for the Java and Javascript editors in Eclipse. I found this simple article that gives some details on how to import them http://www.avajava.com/tutorials/lessons/how-do-i-share-my-code-formatting-settings-with-another-user.html
>
> Charl van Niekerk wrote:
> I have not (yet). The code was originally designed to be able to function separately from Shindig as I did not want to fork the Shindig 2.0.x branch on my own. It's a pity making a proper patch did not occur to me earlier.
>
> I took a quick look at HttpFetcher and it should be fairly easy to implement. As for the rest, I have very limited experience with Shindig 2.0.x myself so would appreciate any pointers.
>
> Regarding the code cleanup... I should have checked for trailing whitespaces before I created the diff, my bad. Corrected immediately with a simple vim command.
>
> Thanks for the pointer regarding Eclipse; I (think) I managed to get the code formatted properly now. Earlier I tried to import the code as a project, but failed. Now I only opened the individual code files and formatted them, but this seems to be sufficient.
>
> Ryan Baxter wrote:
> The formatting changes look great! Thanks for doing that.
>
> I would take a look at GadgetHandlerService and GadgetHandler. In 2.5.0 these classes handle requests from the client to get gadget metadata, which returns back information about gadget preferences among other things. They should be there in 2.0.2 as well.
>
> https://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java
I should be able to construct a MetadataRequest and then perform a call to getMetadata. After that I can do a getUserPrefs on the returned MetadataResponse. This returns a map of UserPref objects where I can get the necessary values using getName, getDisplayName, getDefaultValue and getDataType.
I'll have to spend some more time figuring this out precisely. When I get the MetadataResponse, the rest should be simple.
- Charl
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12647/#review23438
-----------------------------------------------------------
On July 22, 2013, 12:19 p.m., Charl van Niekerk wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12647/
> -----------------------------------------------------------
>
> (Updated July 22, 2013, 12:19 p.m.)
>
>
> Review request for shindig and Ryan Baxter.
>
>
> Bugs: SHINDIG-1188
> https://issues.apache.org/jira/browse/SHINDIG-1188
>
>
> Repository: shindig
>
>
> Description
> -------
>
> In Shindig 2.0.x there is a JavaScript that is being loaded from Google. iGoogle is shutting down in November and the expectation is that this JavaScript will no longer be available.
>
> This patch contains a servlet that is supposed to replace iGoogle's JavaScript by implementing the equivalent functionality.
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetSettingsException.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/GadgetSettings.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetSettingsServlet.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/server/src/main/webapp/WEB-INF/web.xml 1505652
>
> Diff: https://reviews.apache.org/r/12647/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Charl van Niekerk
>
>
Re: Review Request 12647: Replacement for iGoogle gadgetsettings
Posted by Ryan Baxter <rb...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12647/#review23438
-----------------------------------------------------------
In general I understand what is happening here but I feel like there should be more reuse of the code from Shindig. I am less familiar with then 2.0.x stream so maybe some of this doesn't exist there....
One place where I would expect some reuse is when you are fetching the gadget xml. At the very least I would expect you could use the HttpFetcher class that is part of Shindig instead of using the HttpClient directly. However I feel like there are probably classes at a higher level that will fetch, and parse the gadget XML so your code does not have to do that either. Have you looked around in Shindig to see if you can reuse any of that?
General code cleanup comment...
Anywhere you see red in the diff on review board mean there are white spaces there, you should remove that.
You should probably import the Eclipse code formatting templates from trunk https://svn.apache.org/repos/asf/shindig/trunk/etc/eclipse/. There are ones for the Java and Javascript editors in Eclipse. I found this simple article that gives some details on how to import them http://www.avajava.com/tutorials/lessons/how-do-i-share-my-code-formatting-settings-with-another-user.html
http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetSettingsServlet.java
<https://reviews.apache.org/r/12647/#comment47313>
Documentation of what the request URL looks like and which parameters are part of the URL would be helpful.
- Ryan Baxter
On July 17, 2013, 10:05 a.m., Charl van Niekerk wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12647/
> -----------------------------------------------------------
>
> (Updated July 17, 2013, 10:05 a.m.)
>
>
> Review request for shindig and Ryan Baxter.
>
>
> Bugs: SHINDIG-1188
> https://issues.apache.org/jira/browse/SHINDIG-1188
>
>
> Repository: shindig
>
>
> Description
> -------
>
> In Shindig 2.0.x there is a JavaScript that is being loaded from Google. iGoogle is shutting down in November and the expectation is that this JavaScript will no longer be available.
>
> This patch contains a servlet that is supposed to replace iGoogle's JavaScript by implementing the equivalent functionality.
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetSettingsException.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/GadgetSettings.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetSettingsServlet.java PRE-CREATION
> http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/server/src/main/webapp/WEB-INF/web.xml 1504060
>
> Diff: https://reviews.apache.org/r/12647/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Charl van Niekerk
>
>
Re: Review Request 12647: Replacement for iGoogle gadgetsettings
Posted by Charl van Niekerk <c....@utwente.nl>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12647/
-----------------------------------------------------------
(Updated July 29, 2013, 2:38 p.m.)
Review request for shindig and Ryan Baxter.
Changes
-------
Documentation of what the request URL looks like and which parameters are part of the URL
Bugs: SHINDIG-1188
https://issues.apache.org/jira/browse/SHINDIG-1188
Repository: shindig
Description
-------
In Shindig 2.0.x there is a JavaScript that is being loaded from Google. iGoogle is shutting down in November and the expectation is that this JavaScript will no longer be available.
This patch contains a servlet that is supposed to replace iGoogle's JavaScript by implementing the equivalent functionality.
Diffs (updated)
-----
http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetSettingsException.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/GadgetSettings.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetSettingsServlet.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/server/src/main/webapp/WEB-INF/web.xml 1505652
Diff: https://reviews.apache.org/r/12647/diff/
Testing
-------
Thanks,
Charl van Niekerk
Re: Review Request 12647: Replacement for iGoogle gadgetsettings
Posted by Charl van Niekerk <c....@utwente.nl>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12647/
-----------------------------------------------------------
(Updated July 22, 2013, 12:19 p.m.)
Review request for shindig and Ryan Baxter.
Changes
-------
Formatted code according to shindig-eclipse-codestyle_2.xml
Bugs: SHINDIG-1188
https://issues.apache.org/jira/browse/SHINDIG-1188
Repository: shindig
Description
-------
In Shindig 2.0.x there is a JavaScript that is being loaded from Google. iGoogle is shutting down in November and the expectation is that this JavaScript will no longer be available.
This patch contains a servlet that is supposed to replace iGoogle's JavaScript by implementing the equivalent functionality.
Diffs (updated)
-----
http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetSettingsException.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/GadgetSettings.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetSettingsServlet.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/server/src/main/webapp/WEB-INF/web.xml 1505652
Diff: https://reviews.apache.org/r/12647/diff/
Testing
-------
Thanks,
Charl van Niekerk
Re: Review Request 12647: Replacement for iGoogle gadgetsettings
Posted by Charl van Niekerk <c....@utwente.nl>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12647/
-----------------------------------------------------------
(Updated July 22, 2013, 11:46 a.m.)
Review request for shindig and Ryan Baxter.
Changes
-------
Stripped out unnecessary spacing.
Bugs: SHINDIG-1188
https://issues.apache.org/jira/browse/SHINDIG-1188
Repository: shindig
Description
-------
In Shindig 2.0.x there is a JavaScript that is being loaded from Google. iGoogle is shutting down in November and the expectation is that this JavaScript will no longer be available.
This patch contains a servlet that is supposed to replace iGoogle's JavaScript by implementing the equivalent functionality.
Diffs (updated)
-----
http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetSettingsException.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/GadgetSettings.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetSettingsServlet.java PRE-CREATION
http://svn.apache.org/repos/asf/shindig/branches/2.0.x/java/server/src/main/webapp/WEB-INF/web.xml 1505652
Diff: https://reviews.apache.org/r/12647/diff/
Testing
-------
Thanks,
Charl van Niekerk