1214811 - unbold all instances of mUserSearchTerm in search suggestions

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Allison Naaktgeboren :ally]

Bug 1207845  introduced selective formatting of search suggestions, and unbolds only the first instance of mUserSearchTerm. In this bug, we'd like to unbold all instances. In other words, bold all text between multiple instances of mUserSearchTerm

For example, for the search term 'dog', 
suggestion 'dog eat dog world' is formatted as 'dog <b>eat dog world</b>'
we'd like to change that to 'dog <b>eat</b> dog <b>world</b>
suggestion 'frogdogdogmoosedog' is formatted as '<b>frog</b>dog<b>dogmoosedog</b>'  we'd like to change that to '<b>frog</b>dogdog<b>moose</b>dog<b>frog</b>'

setSuggestionOnView() in SearchEngineRow.java is where the magic happens. Note that you'll need a new stylespan object for each span you want to bold, or SpannableStringBuilder will interpret that as you moving the pre-existing span around.

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

maybe this should block the awesomescreen meta?

Comment 2

[Varun Joshi (:vjoshi)]

Hi, I'm a newcomer and I'd like to take this up.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to varunj.1011 from comment #2)
> Hi, I'm a newcomer and I'd like to take this up.

Go for it! We'll assign you to the bug once you post a patch. Be sure to let :ally know if you have any questions.

Comment 4

[Varun Joshi (:vjoshi)]

Attachment: Unbolds all instances of mUserSearchTerm

Tested on search terms like "hello", "waka" and "duck"

Comment 5

[:Margaret Leibovic]

Comment on attachment 8682961 [details] [diff] [review]
Unbolds all instances of mUserSearchTerm

Oops, I must not have been clear enough on IRC... you want to set the review? flag to an appropriate reviewer. Then they will get back to you with review comments.

Comment 6

[Varun Joshi (:vjoshi)]

(In reply to :Margaret Leibovic from comment #5)
> Comment on attachment 8682961 [details] [diff] [review]
> Unbolds all instances of mUserSearchTerm
> 
> Oops, I must not have been clear enough on IRC... you want to set the
> review? flag to an appropriate reviewer. Then they will get back to you with
> review comments.

Ah, I couldn't find where I needed to specify the mentor. It's clear now. Thanks!

Comment 7

[Allison Naaktgeboren :ally]

Comment on attachment 8682961 [details] [diff] [review]
Unbolds all instances of mUserSearchTerm

Review of attachment 8682961 [details] [diff] [review]:
-----------------------------------------------------------------

Thank for picking up a bug and joining our community.

You've made good progress, there's just a bit further to go... The most important of which is getting rid of the special case spans. We shouldn't need those anymore.

feel free to ping us on IRC. I'm :ally.

::: mobile/android/base/home/SearchEngineRow.java
@@ +157,5 @@
>      }
>  
> +    private List<Integer> findAllOccurencesOf (String substring, String string) {
> +        List<Integer> occurences = new ArrayList<Integer>();
> +

remove this newline

@@ +158,5 @@
>  
> +    private List<Integer> findAllOccurencesOf (String substring, String string) {
> +        List<Integer> occurences = new ArrayList<Integer>();
> +
> +        int index = 0;

these need better names if possible. index of what? how about indexOfMatch for example? currentOffset likewise.

@@ +160,5 @@
> +        List<Integer> occurences = new ArrayList<Integer>();
> +
> +        int index = 0;
> +        int currentOffset = 0;
> +

remove this newline

@@ +163,5 @@
> +        int currentOffset = 0;
> +
> +        while(index != -1) {
> +            index = string.indexOf(substring);
> +

You're a little heavy on the whitespace to match this file. Keep the one that separate groups of steps.

@@ +165,5 @@
> +        while(index != -1) {
> +            index = string.indexOf(substring);
> +
> +            if(index != -1 && index < string.length()) {
> +

remove this newline

@@ +166,5 @@
> +            index = string.indexOf(substring);
> +
> +            if(index != -1 && index < string.length()) {
> +
> +                occurences.add(index + currentOffset);

This concerns me. indexOf() returns the 0-based index of the first match, which is sufficient to find that specific match later. What does adding the current offset

@@ +167,5 @@
> +
> +            if(index != -1 && index < string.length()) {
> +
> +                occurences.add(index + currentOffset);
> +

remove this newline

@@ +168,5 @@
> +            if(index != -1 && index < string.length()) {
> +
> +                occurences.add(index + currentOffset);
> +
> +                string = string.substring(index + substring.length(), string.length());

substring.length() is not going to change, so these function invocations are needless. store this up top as a final int.

@@ +169,5 @@
> +
> +                occurences.add(index + currentOffset);
> +
> +                string = string.substring(index + substring.length(), string.length());
> +                currentOffset += index + substring.length();

CurrentOffset really needs better name. You're using it to ensure the value you record for the match is relative to the original string, so I'd want to see a name that reflect some of that. How about something like offsetOfOriginalString?

My colleagues have a better gift for naming than I do. :/

@@ +172,5 @@
> +                string = string.substring(index + substring.length(), string.length());
> +                currentOffset += index + substring.length();
> +            }
> +        }
> +

remove this newline

@@ +187,5 @@
>          }
>  
>          final TextView suggestionText = (TextView) v.findViewById(R.id.suggestion_text);
>          final String searchTerm = getSuggestionTextFromView(mUserEnteredView);
> +        // If there is more than one copy of mUserSearchTerm, all are not bolded

I don't think we need this comment anymore.

@@ +195,2 @@
>              final SpannableStringBuilder sb = new SpannableStringBuilder(suggestion);
> +

remove this newline

@@ +201,5 @@
> +            StyleSpan priorToFirst = new StyleSpan(Typeface.BOLD);
> +            StyleSpan afterLastOccurence = new StyleSpan(Typeface.BOLD);
> +
> +            sb.setSpan(priorToFirst, 0, firstOccurenceIndex, Spannable.SPAN_INCLUSIVE_INCLUSIVE);
> +            int nextStartSpanIndex = firstOccurenceIndex + searchTerm.length();

as above, the length of the searchterm is not going to change. store searchTerm.length() in a final int please.

@@ +203,5 @@
> +
> +            sb.setSpan(priorToFirst, 0, firstOccurenceIndex, Spannable.SPAN_INCLUSIVE_INCLUSIVE);
> +            int nextStartSpanIndex = firstOccurenceIndex + searchTerm.length();
> +
> +            for(int i = 1; i < occurences.size(); i++) {

So since you've got this nice loop, you should rework this code so we don't need the specific spans for priorToFirst and afterLastOccurence.

Comment 8

[Varun Joshi (:vjoshi)]

(In reply to Allison Naaktgeboren :ally from comment #7)
> Comment on attachment 8682961 [details] [diff] [review]
> Unbolds all instances of mUserSearchTerm
> 
> Review of attachment 8682961 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank for picking up a bug and joining our community.
> 
> You've made good progress, there's just a bit further to go... The most
> important of which is getting rid of the special case spans. We shouldn't
> need those anymore.
> 
> feel free to ping us on IRC. I'm :ally.
> 
> ::: mobile/android/base/home/SearchEngineRow.java
> @@ +157,5 @@
> >      }
> >  
> > +    private List<Integer> findAllOccurencesOf (String substring, String string) {
> > +        List<Integer> occurences = new ArrayList<Integer>();
> > +
> 
> remove this newline
> 
> @@ +158,5 @@
> >  
> > +    private List<Integer> findAllOccurencesOf (String substring, String string) {
> > +        List<Integer> occurences = new ArrayList<Integer>();
> > +
> > +        int index = 0;
> 
> these need better names if possible. index of what? how about indexOfMatch
> for example? currentOffset likewise.
> 
> @@ +160,5 @@
> > +        List<Integer> occurences = new ArrayList<Integer>();
> > +
> > +        int index = 0;
> > +        int currentOffset = 0;
> > +
> 
> remove this newline
> 
> @@ +163,5 @@
> > +        int currentOffset = 0;
> > +
> > +        while(index != -1) {
> > +            index = string.indexOf(substring);
> > +
> 
> You're a little heavy on the whitespace to match this file. Keep the one
> that separate groups of steps.
> 
> @@ +165,5 @@
> > +        while(index != -1) {
> > +            index = string.indexOf(substring);
> > +
> > +            if(index != -1 && index < string.length()) {
> > +
> 
> remove this newline
> 
> @@ +166,5 @@
> > +            index = string.indexOf(substring);
> > +
> > +            if(index != -1 && index < string.length()) {
> > +
> > +                occurences.add(index + currentOffset);
> 
> This concerns me. indexOf() returns the 0-based index of the first match,
> which is sufficient to find that specific match later. What does adding the
> current offset
> 
> @@ +167,5 @@
> > +
> > +            if(index != -1 && index < string.length()) {
> > +
> > +                occurences.add(index + currentOffset);
> > +
> 
> remove this newline
> 
> @@ +168,5 @@
> > +            if(index != -1 && index < string.length()) {
> > +
> > +                occurences.add(index + currentOffset);
> > +
> > +                string = string.substring(index + substring.length(), string.length());
> 
> substring.length() is not going to change, so these function invocations are
> needless. store this up top as a final int.
> 
> @@ +169,5 @@
> > +
> > +                occurences.add(index + currentOffset);
> > +
> > +                string = string.substring(index + substring.length(), string.length());
> > +                currentOffset += index + substring.length();
> 
> CurrentOffset really needs better name. You're using it to ensure the value
> you record for the match is relative to the original string, so I'd want to
> see a name that reflect some of that. How about something like
> offsetOfOriginalString?
> 
> My colleagues have a better gift for naming than I do. :/
> 
> @@ +172,5 @@
> > +                string = string.substring(index + substring.length(), string.length());
> > +                currentOffset += index + substring.length();
> > +            }
> > +        }
> > +
> 
> remove this newline
> 
> @@ +187,5 @@
> >          }
> >  
> >          final TextView suggestionText = (TextView) v.findViewById(R.id.suggestion_text);
> >          final String searchTerm = getSuggestionTextFromView(mUserEnteredView);
> > +        // If there is more than one copy of mUserSearchTerm, all are not bolded
> 
> I don't think we need this comment anymore.
> 
> @@ +195,2 @@
> >              final SpannableStringBuilder sb = new SpannableStringBuilder(suggestion);
> > +
> 
> remove this newline
> 
> @@ +201,5 @@
> > +            StyleSpan priorToFirst = new StyleSpan(Typeface.BOLD);
> > +            StyleSpan afterLastOccurence = new StyleSpan(Typeface.BOLD);
> > +
> > +            sb.setSpan(priorToFirst, 0, firstOccurenceIndex, Spannable.SPAN_INCLUSIVE_INCLUSIVE);
> > +            int nextStartSpanIndex = firstOccurenceIndex + searchTerm.length();
> 
> as above, the length of the searchterm is not going to change. store
> searchTerm.length() in a final int please.
> 
> @@ +203,5 @@
> > +
> > +            sb.setSpan(priorToFirst, 0, firstOccurenceIndex, Spannable.SPAN_INCLUSIVE_INCLUSIVE);
> > +            int nextStartSpanIndex = firstOccurenceIndex + searchTerm.length();
> > +
> > +            for(int i = 1; i < occurences.size(); i++) {
> 
> So since you've got this nice loop, you should rework this code so we don't
> need the specific spans for priorToFirst and afterLastOccurence.

Thanks for reviewing my patch :).
I really do need to work on my naming. I'll make these changes and upload another patch soon.

Comment 9

[Varun Joshi (:vjoshi)]

Attachment: Unbolds all instaces of mUserSearchTerm

Changed findAllOccurrences to use another form of string.indexOf(), reducing a lot of complexity. 
Removed the special spans for the beginning and end of the suggestion.
Reduced whitespace and changed variable names.

Comment 10

[Allison Naaktgeboren :ally]

Comment on attachment 8684682 [details] [diff] [review]
Unbolds all instaces of mUserSearchTerm

Review of attachment 8684682 [details] [diff] [review]:
-----------------------------------------------------------------

You are just a couple keystrokes from an r+! Just need to re-work the comments a bit before final landing.

It behaves as desired on all my test cases.

::: mobile/android/base/home/SearchEngineRow.java
@@ +155,5 @@
>          final TextView suggestionText = (TextView) v.findViewById(R.id.suggestion_text);
>          return suggestionText.getText().toString();
>      }
>  
> +    private List<Integer> findAllOccurrencesOf(String substring, String string) {

This deserves its own function comment block at the top.

Please rename substring to pattern. and lengthOfSubstring to patternLength.

@@ +183,5 @@
>          final TextView suggestionText = (TextView) v.findViewById(R.id.suggestion_text);
>          final String searchTerm = getSuggestionTextFromView(mUserEnteredView);
> +        final int searchTermLength = searchTerm.length();
> +        final List<Integer> occurrences = findAllOccurrencesOf(searchTerm, suggestion);
> +        // Sometimes the suggestion does not contain mUserSearchTerm at all, in which case, bold nothing

I think it is time for this comment to move up to a function comment block, as this function has too many and this is more a statement of desired behavior than an explanation of an unusual implementation detail.

@@ +189,2 @@
>              final SpannableStringBuilder sb = new SpannableStringBuilder(suggestion);
> +            // Styles for text in a suggestion 'button' that is not part of the first instance of mUserSearchTerm

This comment no longer makes sense. the first instance of the searchterm in a suggestion is now no different from any other.

@@ +189,4 @@
>              final SpannableStringBuilder sb = new SpannableStringBuilder(suggestion);
> +            // Styles for text in a suggestion 'button' that is not part of the first instance of mUserSearchTerm
> +            // Even though they're the same style, SpannableStringBuilder will interpret there as being only one Span present if we re-use a StyleSpan
> +            int nextStartSpanIndex = 0;

move this to right above the for() loop. This declaration in the middle of comments that are not related to the nextStartSpanIndex makes it more confusing than it needs to be.

@@ +189,5 @@
>              final SpannableStringBuilder sb = new SpannableStringBuilder(suggestion);
> +            // Styles for text in a suggestion 'button' that is not part of the first instance of mUserSearchTerm
> +            // Even though they're the same style, SpannableStringBuilder will interpret there as being only one Span present if we re-use a StyleSpan
> +            int nextStartSpanIndex = 0;
> +            // So that the text after the last suggestion is bold

This is a thing that deserves a comment, but this one isn't quite clear enough, and will be hard for non english speakers to parse.

@@ +191,5 @@
> +            // Even though they're the same style, SpannableStringBuilder will interpret there as being only one Span present if we re-use a StyleSpan
> +            int nextStartSpanIndex = 0;
> +            // So that the text after the last suggestion is bold
> +            occurrences.add(suggestion.length());
> +            for(int occurrence : occurrences) {

much better. :)

@@ +192,5 @@
> +            int nextStartSpanIndex = 0;
> +            // So that the text after the last suggestion is bold
> +            occurrences.add(suggestion.length());
> +            for(int occurrence : occurrences) {
> +                // Styles for text between two search terms

this comment should be removed and put this one there

// Even though they're the same style, SpannableStringBuilder will interpret there as being only one Span present if we re-use a StyleSpan

Comment 11

[Varun Joshi (:vjoshi)]

Attachment: Unbolds all instaces of mUserSearchTerm

Added comments and renamed variables as suggested.

Comment 12

[Allison Naaktgeboren :ally]

Comment on attachment 8686633 [details] [diff] [review]
Unbolds all instaces of mUserSearchTerm

Review of attachment 8686633 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with 

- whitespace removed.
- bug title added. You can use hg qref -m "Bug # - Bug Title.r=reviewer"

I'll do that for you this time since you've worked really hard and the patch is otherwise good.

::: mobile/android/base/home/SearchEngineRow.java
@@ +155,5 @@
>      private String getSuggestionTextFromView(View v) {
>          final TextView suggestionText = (TextView) v.findViewById(R.id.suggestion_text);
>          return suggestionText.getText().toString();
>      }
> +    

whitespace. Note that you can look at your patch in the reviewing tool, and it will highlight any trailing whitespace like this.

Comment 13

[Allison Naaktgeboren :ally]

https://hg.mozilla.org/integration/fx-team/rev/c3cb49558709e719781152e44013378d706b79ae
Bug 1214811 - unbold all instances of mUserSearchTerm in search suggestions.r=ally

Comment 14

[Varun Joshi (:vjoshi)]

(In reply to Allison Naaktgeboren :ally from comment #12)
> Comment on attachment 8686633 [details] [diff] [review]
> Unbolds all instaces of mUserSearchTerm
> 
> Review of attachment 8686633 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with 
> 
> - whitespace removed.
> - bug title added. You can use hg qref -m "Bug # - Bug Title.r=reviewer"
> 
> I'll do that for you this time since you've worked really hard and the patch
> is otherwise good.
> 
> ::: mobile/android/base/home/SearchEngineRow.java
> @@ +155,5 @@
> >      private String getSuggestionTextFromView(View v) {
> >          final TextView suggestionText = (TextView) v.findViewById(R.id.suggestion_text);
> >          return suggestionText.getText().toString();
> >      }
> > +    
> 
> whitespace. Note that you can look at your patch in the reviewing tool, and
> it will highlight any trailing whitespace like this.

Thank you! I'll make sure I keep those issues in mind whenever I submit a patch. Is there anything else that needs to be done before closing this bug?

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c3cb49558709