----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7683/#review13037 -----------------------------------------------------------
This is a great patch! Thanks. See the comments below. src/org/waveprotocol/box/common/Snippets.java <https://reviews.apache.org/r/7683/#comment28055> I am not sure why do we need those changes in renderSnippets. I think those additional checks on titleBlip and firstId are not needed. I tried to replace the class with HEAD version and it still worked fine (after fixing updateSnippet). src/org/waveprotocol/box/webclient/search/SearchPanelWidget.java <https://reviews.apache.org/r/7683/#comment28056> Empty line src/org/waveprotocol/box/webclient/search/SearchPresenter.java <https://reviews.apache.org/r/7683/#comment28057> Can we rename the boolean variable to start with "is"? Like isRenderingInProgress. src/org/waveprotocol/box/webclient/search/SearchPresenter.java <https://reviews.apache.org/r/7683/#comment28058> I think the comment here is unnecessary. src/org/waveprotocol/box/webclient/search/SearchPresenter.java <https://reviews.apache.org/r/7683/#comment28059> Same here src/org/waveprotocol/box/webclient/search/SearchPresenter.java <https://reviews.apache.org/r/7683/#comment28060> Can we extract this code that looks for the DigestView to remove into a separate method? src/org/waveprotocol/box/webclient/search/SearchPresenter.java <https://reviews.apache.org/r/7683/#comment28061> Same here, please extract this code into a separate method. src/org/waveprotocol/box/webclient/search/SimpleSearch.java <https://reviews.apache.org/r/7683/#comment28062> I think the javadoc for this method should be updated. src/org/waveprotocol/box/webclient/search/SimpleSearch.java <https://reviews.apache.org/r/7683/#comment28063> Why fireOnTotalChanged(total) is invoked here instead inside of "if" block that checks whether total did change? src/org/waveprotocol/box/webclient/search/WaveBasedDigest.java <https://reviews.apache.org/r/7683/#comment28064> Please add spaces snippet=null; -> snippet = null; src/org/waveprotocol/box/webclient/search/WaveBasedDigest.java <https://reviews.apache.org/r/7683/#comment28054> I think this is correct, if there's no conversational document - there's no need to update anything. src/org/waveprotocol/box/webclient/search/WaveBasedDigest.java <https://reviews.apache.org/r/7683/#comment28066> Please put the return statement inside a block. return -> { return; } src/org/waveprotocol/box/webclient/search/WaveBasedDigest.java <https://reviews.apache.org/r/7683/#comment28065> Please replace "140" with a constant. There's already such constant DIGEST_SNIPPET_LENGTH in WaveDigester class. I think it would be better to move the constant into Snippets class and make it public. Also, as already mentioned, please check if we need to modify the Snippets.renderSnippet - I think we don't. - Yuri Zelikov On Oct. 20, 2012, 4:49 p.m., rocklund wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7683/ > ----------------------------------------------------------- > > (Updated Oct. 20, 2012, 4:49 p.m.) > > > Review request for wave. > > > Description > ------- > > Fix for "SimpleSearch.DigestProxy should undate only the opened wave on > changes". Also updated so that the active digest update its title and snippet > in real time to make it feel more responsive and live. > > There might still be some code tweaks I should do and I still have two TODOs > in my code that I need to fix. > > Any feedback on the patch are welcome. Thanks! > > > This addresses bug WAVE-354. > https://issues.apache.org/jira/browse/WAVE-354 > > > Diffs > ----- > > src/org/waveprotocol/box/common/Snippets.java 099ce55 > src/org/waveprotocol/box/webclient/search/SearchPanelView.java e922fd4 > src/org/waveprotocol/box/webclient/search/SearchPanelWidget.java 4402b8e > src/org/waveprotocol/box/webclient/search/SearchPresenter.java b136c88 > src/org/waveprotocol/box/webclient/search/SimpleSearch.java 5b018d4 > src/org/waveprotocol/box/webclient/search/WaveBasedDigest.java 8d3cb92 > src/org/waveprotocol/wave/concurrencycontrol/wave/CcBasedWavelet.java > e87b1e0 > src/org/waveprotocol/wave/model/wave/Wavelet.java a463d45 > src/org/waveprotocol/wave/model/wave/opbased/OpBasedWavelet.java e99067c > > Diff: https://reviews.apache.org/r/7683/diff/ > > > Testing > ------- > > It would be good with some unit tests for the TitleHelper and the Snippet > classes but I don't think there currently are any good mock classes to use > for that. > > I tested manually on a locally running server. > > > Thanks, > > rocklund > >
