incubator-wave-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ali Lown" <...@lown.me.uk>
Subject Re: Review Request 16322: full text search (with known issue of solr-bot output highlight)
Date Sat, 21 Dec 2013 12:41:02 GMT


> On Dec. 21, 2013, 11 a.m., Frank R. wrote:
> >

These are really hard to read in context, because you seem to have posted it as a new review?
Please use the "Add comment" button to put them in-line with the appropriate parts of the
appropriate reviews in future.


> On Dec. 21, 2013, 11 a.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

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.


> 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

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.


> On Dec. 21, 2013, 11 a.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.

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'?)


> 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.

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.


> 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.

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.


- 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.
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message