[
https://issues.apache.org/jira/browse/WAVE-319?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13228643#comment-13228643
]
[email protected] commented on WAVE-319:
----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3960/#review5884
-----------------------------------------------------------
You did really great work!
Just some comments below.
Regarding your questions:
1. I think it's OK to put in the root folder.
2. Probably it would be better to cache the json in the memory. For example we
can use ComputingMap with expireOnWrite() option set to 5 minutes.
src/org/waveprotocol/box/server/rpc/GadgetProviderServlet.java
<https://reviews.apache.org/r/3960/#comment12825>
Most server side code uses the standard Java Logger for logging.
src/org/waveprotocol/box/server/rpc/GadgetProviderServlet.java
<https://reviews.apache.org/r/3960/#comment12826>
Maybe we need to extract this code into a method and add it to the
FileUtils class.
src/org/waveprotocol/box/server/rpc/GadgetProviderServlet.java
<https://reviews.apache.org/r/3960/#comment12827>
Probably it would be better to use StringBuilder for line variable instead
of String.
src/org/waveprotocol/box/server/rpc/GadgetProviderServlet.java
<https://reviews.apache.org/r/3960/#comment12828>
It seems like Exception is too general, can we replace it with just
IOException?
src/org/waveprotocol/box/server/rpc/GadgetProviderServlet.java
<https://reviews.apache.org/r/3960/#comment12829>
Please wrap "+" with spaces
src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetInfoProvider.java
<https://reviews.apache.org/r/3960/#comment12831>
Can we use more meaningful name for the category type instead of "value"?
How about "type"?
src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetInfoProvider.java
<https://reviews.apache.org/r/3960/#comment12830>
Maybe just use equalsIgnoreCase() here?
src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetInfoProvider.java
<https://reviews.apache.org/r/3960/#comment12832>
static public -> public static
src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetInfoProvider.java
<https://reviews.apache.org/r/3960/#comment12833>
Can we change the access modifier for the GadgetInfo from public to
private? If we want to access the fields - we would better provide getters.
src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetInfoProvider.java
<https://reviews.apache.org/r/3960/#comment12835>
Please terminate the comments with full stop.
src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetInfoProvider.java
<https://reviews.apache.org/r/3960/#comment12836>
Also here and below
src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetInfoProvider.java
<https://reviews.apache.org/r/3960/#comment12837>
Can we use more meaningful name instead of "myFilter"? Or maybe just use
equalsIgnoreCase()? Also, please access the gadgetInfo fields via corresponding
getters.
src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetInfoProvider.java
<https://reviews.apache.org/r/3960/#comment12838>
Please move the constructor up.
It should be declared just below the fields
src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetSelectorWidget.java
<https://reviews.apache.org/r/3960/#comment12839>
These new variables have default access modifier, can we change it to
private?
src/org/waveprotocol/wave/client/widget/popup/CenterPopupPositioner.java
<https://reviews.apache.org/r/3960/#comment12840>
I had long discussion about the value of the offset with David Hearnden and
his opinion was that the most natural value for offset is 0. I ll try to find
it and send you the link.
src/org/waveprotocol/wave/client/widget/popup/PositionUtil.java
<https://reviews.apache.org/r/3960/#comment12841>
Restrict -> Restricts
Also, please terminate the comments in javadoc with full stop.
test/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetInfoProviderTest.java
<https://reviews.apache.org/r/3960/#comment12842>
Please add @author
test/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetInfoProviderTest.java
<https://reviews.apache.org/r/3960/#comment12843>
Do we really need GWTTestCase here? Can we do with just regular JUnit test?
- Yuri
On 2012-03-11 17:31:31, rocklund wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/3960/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-03-11 17:31:31)
bq.
bq.
bq. Review request for wave.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. * Added more gadgets to the gadget list
bq. * Made the gadget list scrollable and filterable through a text box and a
drop down box for categories.
bq. * The filtering looks at both the name of the gadget and its description.
Author could also be added as a searchable property (?)
bq. * Filter the result directly in the scrollable gadget list
bq. * Marking the top filtered search as selected with gray background and
made it possible to choose that gadget by pressing enter
bq. * Change the default focus to the filter box to allow the user to quickly
select a gadget from the list by filter it out and pressing enter
bq.
bq.
bq. This addresses bug wave-319.
bq. https://issues.apache.org/jira/browse/wave-319
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. jsongadgets.json PRE-CREATION
bq. src/org/waveprotocol/box/server/ServerMain.java 13a2d55
bq. src/org/waveprotocol/box/server/rpc/GadgetProviderServlet.java
PRE-CREATION
bq.
src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetInfoProvider.java
PRE-CREATION
bq.
src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetInfoWidget.java
97611b4
bq.
src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetInfoWidget.ui.xml
c8b7a81
bq.
src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetSelectorWidget.java
ccbcdae
bq.
src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetSelectorWidget.ui.xml
cc6b73e
bq. src/org/waveprotocol/wave/client/widget/popup/CenterPopupPositioner.java
555f32e
bq. src/org/waveprotocol/wave/client/widget/popup/PositionUtil.java 5e3bab4
bq.
test/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetInfoProviderTest.java
PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/3960/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. Tested locally.
bq. Successfully run all junit test (Except
PerUserWaveViewSubscriberTest::testGetPerUserWaveView and
WaveServerTest::testWaveletNotification that fails even before this patch)
bq. Tested all included gadgets
bq.
bq.
bq. Thanks,
bq.
bq. rocklund
bq.
bq.
> Improve the "Add gadget" popup
> -------------------------------
>
> Key: WAVE-319
> URL: https://issues.apache.org/jira/browse/WAVE-319
> Project: Wave
> Issue Type: Improvement
> Components: Web Client
> Reporter: Yuri Zelikov
> Assignee: Olof
> Priority: Minor
> Labels: StarterProject
>
> Improve the "Add gadget" popup. Currently it contains only a handful of
> gadgets. Add more gadget definitions, make the panel scrollable, add ability
> to filter the gadgets list and or add suggestion box -
> http://gwt.google.com/samples/Showcase/Showcase.html#!CwSuggestBox
> The gadget definitions can be taken from http://goo.gl/nEem2
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
