> On Dec. 21, 2013, 7 p.m., Frank R. wrote: > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java, line 246 > > <https://reviews.apache.org/r/16322/diff/6/?file=401693#file401693line246> > > > > It's feature to be implemented. If the TODO was to be removed, who else > > in the world would know that? The comments are here for a reason - at > > least, you'll know how you can help to improve the code around them, or, > > why they are what they are, and what problem other should avoid when they > > collaborate with me. > > > > I'll make my Java comments more readable. Please give me feedback when > > you've actually read them. Thanks. > > Ali Lown wrote: > This comment may be marking where an improvement can be made, but all I > can see here is dead LOC. (You even supressed the unused warning on the > searchAsync function!). > Either a) Make searchAsync work, b) remove the comment, > commented-out-code, and the searchAsync method, and create a single JIRA > issue instead.
You were suggesting to create an issue for something pending on review, and not in the code repository yet. I didn't know I could do that. Shall I? The dead LOC will be uncommented once searchAsync function will be implemented. It's not straight forward to me why searchAsync doesn't work. So, it was commented out. > On Dec. 21, 2013, 7 p.m., Frank R. wrote: > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java, line 268 > > <https://reviews.apache.org/r/16322/diff/6/?file=401693#file401693line268> > > > > Would it be more readable? > > BundledAnnotation.listOf(Annotation.BACKGROUND_COLOR, > > ERROR_BACKGROUND_COLOR, > > Annotation.COLOR, ERROR_COLOR), message); > > > > Why isn't "style/backgroundColor" defined in a single place, > > com.google.wave.api.Annotation.BACKGROUND_COLOR? > > > > See > > org.waveprotocol.wave.client.editor.content.misc.StyleAnnotationHandler.KEYS, > > and > > org.waveprotocol.wave.model.richtext.RichTextMutationBuilder.STYLE_KEY_BG_COLOR. > > Ali Lown wrote: > Yes it is more readable. (And certainly easier to find and change in > future, when it is not buried in the middle of over code). > > There is no particular reason it isn't defined in a single place already. > (Probably because whoever wrote it didn't realize it was already defined > elsewhere). You are welcome to fix this in a different patch. Like wave.client.doodad.selection.SelectionAnnotationHandler, I would suggest someone else to do the global refactor. Though, I've already added a constant, com.google.wave.api.Annotation.WAVE_LINK for "link/wave". I'll update the diff (patch). - Frank ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16322/#review30770 ----------------------------------------------------------- On Dec. 21, 2013, 1:12 a.m., Frank R. wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16322/ > ----------------------------------------------------------- > > (Updated Dec. 21, 2013, 1:12 a.m.) > > > Review request for wave, Ali Lown, Vicente J. Ruiz Jurado, and Yuri Zelikov. > > > Bugs: WAVE-311 > https://issues.apache.org/jira/browse/WAVE-311 > > > Repository: wave > > > Description > ------- > > For details (issues and commits): > https://github.com/renfeng/wave > > > Diffs > ----- > > run-export.sh d2cddb7 > run-import.sh 45fff8a > server.config.example 19ba8b2 > src/org/waveprotocol/box/server/SearchModule.java 2de0ef9 > src/org/waveprotocol/box/server/ServerMain.java b50454d > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java > PRE-CREATION > src/org/waveprotocol/box/server/robots/agent/welcome/WelcomeRobot.java > 2735940 > src/org/waveprotocol/box/server/waveserver/SimpleSearchProviderImpl.java > ee7093f > src/org/waveprotocol/box/server/waveserver/SolrSearchProviderImpl.java > PRE-CREATION > src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java > PRE-CREATION > src/org/waveprotocol/box/server/waveserver/WaveDigester.java b103bbb > > Diff: https://reviews.apache.org/r/16322/diff/ > > > Testing > ------- > > tests on search box > > * in:inbox > * (empty) for all, including waves shared in the domain > * with:@ > * (free texts) > > tests on solr-bot > > * single word > * phrase (quoted with double quotation marks) > * syntax applicable to search box > > > Thanks, > > Frank R. > >
