> On Dec. 21, 2013, 11 a.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. > > Frank R. wrote: > 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).
https://reviews.apache.org/r/16435/ does the required work. You will now find everything useful in AnnotationConstants. > On Dec. 21, 2013, 11 a.m., Frank R. wrote: > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java, line 178 > > <https://reviews.apache.org/r/16322/diff/6/?file=401693#file401693line178> > > > > Thanks for showing me the constants. I'll change their scope from > > private to public. > > > > Can you explain more what you mean by "these"? Thanks > > Ali Lown wrote: > Rather than just changing the visibility of the constants in > wave.client.doodad.selection.SelectionAnnotationHandler, and then referring > to them from there (which would be pretty ugly). > > I was requesting that you move the constants out of > wave.client.doodad.selection.SelectionAnnotationHandler, in to a new file in > the wave.model.conversation namespace, and then update both your code, and > SelectionAnnotationHandler to refer to the constants from there. > > Frank R. wrote: > I was trying to make as little impact as possible to existing code > written by others. I can do that some day. Now, I would suggest someone like > you with more experience in wave to go ahead and make the changes with a > bigger picture in mind. Thanks~ > > Ali Lown wrote: > Fine. I will do this moving, and fix the BACKGROUND_COLOUR at the same > time. I should create a review request within the next half hour. Created. See https://reviews.apache.org/r/16435/. > On Dec. 21, 2013, 11 a.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. > > Frank R. wrote: > 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. Yep, create an issue, (make a note in it that it depends on this review request), then remove the searchAsync stuff from here for the moment. - Ali ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16322/#review30770 ----------------------------------------------------------- On Dec. 20, 2013, 5:12 p.m., Frank R. wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16322/ > ----------------------------------------------------------- > > (Updated Dec. 20, 2013, 5:12 p.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. > >
