----------------------------------------------------------- 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: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3960/ > ----------------------------------------------------------- > > (Updated 2012-03-11 17:31:31) > > > Review request for wave. > > > Summary > ------- > > * Added more gadgets to the gadget list > * Made the gadget list scrollable and filterable through a text box and a > drop down box for categories. > * The filtering looks at both the name of the gadget and its description. > Author could also be added as a searchable property (?) > * Filter the result directly in the scrollable gadget list > * Marking the top filtered search as selected with gray background and made > it possible to choose that gadget by pressing enter > * 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 > > > This addresses bug wave-319. > https://issues.apache.org/jira/browse/wave-319 > > > Diffs > ----- > > jsongadgets.json PRE-CREATION > src/org/waveprotocol/box/server/ServerMain.java 13a2d55 > src/org/waveprotocol/box/server/rpc/GadgetProviderServlet.java PRE-CREATION > > src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetInfoProvider.java > PRE-CREATION > > src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetInfoWidget.java > 97611b4 > > src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetInfoWidget.ui.xml > c8b7a81 > > src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetSelectorWidget.java > ccbcdae > > src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetSelectorWidget.ui.xml > cc6b73e > src/org/waveprotocol/wave/client/widget/popup/CenterPopupPositioner.java > 555f32e > src/org/waveprotocol/wave/client/widget/popup/PositionUtil.java 5e3bab4 > > test/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetInfoProviderTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/3960/diff > > > Testing > ------- > > Tested locally. > Successfully run all junit test (Except > PerUserWaveViewSubscriberTest::testGetPerUserWaveView and > WaveServerTest::testWaveletNotification that fails even before this patch) > Tested all included gadgets > > > Thanks, > > rocklund > >
