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 16:02:03 GMT


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


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