----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3960/#review6128 -----------------------------------------------------------
Thanks for the effort - the patch is almost done! Just some more comments. src/org/waveprotocol/box/server/persistence/file/FileUtils.java <https://reviews.apache.org/r/3960/#comment13165> Open -> Opens src/org/waveprotocol/box/server/rpc/GadgetProviderServlet.java <https://reviews.apache.org/r/3960/#comment13166> A servlet -> the servlet. src/org/waveprotocol/box/server/rpc/GadgetProviderServlet.java <https://reviews.apache.org/r/3960/#comment13167> LOGGER -> LOG The standard name for logger in WIAB is LOG. src/org/waveprotocol/box/server/rpc/GadgetProviderServlet.java <https://reviews.apache.org/r/3960/#comment13168> Please also include the actual exception in the log. -> LOG.log(Level.WARNING, Error while loading gadgets json", e); src/org/waveprotocol/box/webclient/client/WebClient.java <https://reviews.apache.org/r/3960/#comment13169> After thinking a bit more about the injection, it seems like the more natural place to inject the actual implementation would in EditToolbar. Instead of: GadgetSelectorWidget selector = new GadgetSelectorWidget(); -> GadgetSelectorWidget selector = new GadgetSelectorWidget(new GadgetInfoProviderImpl()); Sorry, for the mess. src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetInfoProvider.java <https://reviews.apache.org/r/3960/#comment13176> Please make an interface. I mean there should be an interface, and a concrete implementation. No need for abstract class here. src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetInfoProviderImpl.java <https://reviews.apache.org/r/3960/#comment13170> Why trace? Probably warning or error is more appropriate. src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetInfoProviderImpl.java <https://reviews.apache.org/r/3960/#comment13171> Same here. src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetSelectorWidget.java <https://reviews.apache.org/r/3960/#comment13173> We should inject the instance in the constructor. src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetSelectorWidget.java <https://reviews.apache.org/r/3960/#comment13174> Line too long? src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetSelectorWidget.java <https://reviews.apache.org/r/3960/#comment13175> Note -> TODO test/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetInfoProviderTest.java <https://reviews.apache.org/r/3960/#comment13178> Are you sure we really need GWTTestCase here? - Yuri On 2012-03-17 20:25:34, rocklund wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3960/ > ----------------------------------------------------------- > > (Updated 2012-03-17 20:25:34) > > > 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 > ----- > > > 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 > > 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/GadgetInfoProviderImpl.java > PRE-CREATION > src/org/waveprotocol/box/server/ServerMain.java 13a2d55 > src/org/waveprotocol/box/server/persistence/file/FileUtils.java 538e6e3 > src/org/waveprotocol/box/server/rpc/GadgetProviderServlet.java PRE-CREATION > src/org/waveprotocol/box/webclient/client/WebClient.java c87b7cc > > src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/gadget/GadgetInfoProvider.java > PRE-CREATION > jsongadgets.json 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 > >
