> On Dec. 21, 2013, 7 p.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.
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~ > On Dec. 21, 2013, 7 p.m., Frank R. wrote: > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java, line 228 > > <https://reviews.apache.org/r/16322/diff/6/?file=401693#file401693line228> > > > > Yes. This method is only invoked for such case. It's extracted only to > > make it easy to read the comments where it's invoked. > > Ali Lown wrote: > The issue is that this whole " private void search(String creator, > boolean searchAllowed, String query, Blip blip, int start)" method doesn't > make much sense because: > a) It is a on overload of "private void search(String query, String > creator, Blip outputBlip)", yet has extra side effects (string manipulation > stuff) that are completely unpredictable from the extra parameter > 'searchAllowed'. > b) It is only called and (due to the string manipulation stuff) will only > ever make sense to call from a single location. > > So, I would much rather you either a) inline this whole method back in to > onDocumentChanged, or b) take the rest of the string manipulation stuff (so > from line 190) out of onDocumentChanged and call the method something > different from 'search'. ('maybeProcessSearch'?) Agreed. I'll put it back. > On Dec. 21, 2013, 7 p.m., Frank R. wrote: > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java, line 55 > > <https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line55> > > > > Does this comply with the code style reinforced here? > > > > http://maven.apache.org/developers/conventions/code.html > > > > I don't think this is respected. > > Blocks: Always enclose with a new line brace. > > > > I used to have http://maven.apache.org/plugins/maven-checkstyle-plugin/ > > > > I don't think some rules make sense at all. > > > > Anyway, where can I read about the rules? Thanks. > > > > p.s. sorry for the delay in response. I'm still a learner to the review > > site. > > Yuri Zelikov wrote: > I think you are right, we need to decide on code style for Wave. > Basically we follow the standard Eclipse 2.1 built in style conventions with > following additions: > Use 2 spaces for indentation(not tabs) > Max line length is 100. > > Maybe some more but I can't recall the rest right now. > > Also it is advised to enable the 'Save Action" in Eclipse with following > options: > Format source code (edited lines) > Organize imports > Additional actions -> add final modifier to private fields > > Ali Lown wrote: > Yuri's comment here is referring to the trailing whitespace, which should > not be here. > > We don't have a single document stating the coding convention (and the > codebase is not 100% self consistent). > The best reference is the "style" of the code in the surrounding parts of > the file being edited. That little trailing space is introduced by eclipse whenever I press CTRL+SHIF+F. I do that more frequently than CTRL+S :) I'll try to tweak my Java Formatter profile as suggested by Yuri. I did try to find where to disable the trailing space, but I had no luck. It seems eclipse can adopt itself to the existing code at least with the indention of two spaces. Eclipse used to reduce successive newlines into one on formatting. Don't when that behavior changed to preserving two. --- I found the settings here, .settings/org.eclipse.jdt.core.prefs when I was trying to creating a formatter profile, and config the save actions. This should be changed to 1. Right? org.eclipse.jdt.core.formatter.number_of_empty_lines_to_preserve=2 Settings Yuri talked about: org.eclipse.jdt.core.formatter.tabulation.char=space org.eclipse.jdt.core.formatter.tabulation.size=2 org.eclipse.jdt.core.formatter.lineSplit=100 Save Action goes here, .settings/org.eclipse.jdt.ui.prefs, which doesn't include the desired settings: sp_cleanup.format_source_code=true sp_cleanup.format_source_code_changes_only=true sp_cleanup.organize_imports=true sp_cleanup.make_private_fields_final=true There are more settings added when I set the rules up in Project Properties. Format source code (edited lines) Organize imports Additional actions -> add final modifier to private fields (I cleared other default actions.) Can you, Ali and Yuri, submit an updated .settings/org.eclipse.jdt.core.prefs and .settings/org.eclipse.jdt.ui.prefs to match the rules you in your mind? Thanks~ - 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. > >
