1029682 - For autocomplete suggestions, show which part is matched and which part is suggested

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Search Activity, defect)

Description

[Eric Edens [:eedens]]

Attachment: firefox-search-match-style.png

As we display autocomplete suggestions, the user should receive feedback on which part is a suggestion and which part is matched from their query.

If we add this as a method on the row's view, eg: rowView.highlight(String usersQuery), then it should be easy to support additional views in the future.

Comment 1

[:Margaret Leibovic]

I started cleaning up the suggestions layout, and I can work on this as part of that.

Comment 2

[:Margaret Leibovic]

WIP PR here:
https://github.com/ericedens/FirefoxSearch/pull/28

Comment 3

[:Margaret Leibovic]

Attachment: (Part 1) Remove AutoCompleteRowView

Simplifying things a bit.

Comment 4

[:Margaret Leibovic]

Attachment: (Part 2) Style search term match in search suggestions

I addressed your feedback on the pull request to be more careful about highlighting the search term match. Even with the yahoo suggestions, I found that sometimes the suggestion won't contain the search term at all, so this is much safer.

Comment 5

[Eric Edens [:eedens]]

Comment on attachment 8459795 [details] [diff] [review]
(Part 2) Style search term match in search suggestions

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

This is looking good! It definitely adds polish to the autocomplete experience :) There are a few items below.

::: mobile/android/search/java/org/mozilla/search/autocomplete/SearchFragment.java
@@ +51,5 @@
>      // Maximum number of results returned by the suggestion client
>      private static final int SUGGESTION_MAX = 5;
>  
> +    // Color of search term match in search suggestion
> +    private static final int SUGGESTION_HIGHLIGHT_COLOR = 0xFF999999;

Can this go into a resource file?

@@ +266,5 @@
> +            this.value = value;
> +
> +            display = new SpannableString(value);
> +
> +            final int start = value.indexOf(searchTerm);

This addresses substring matching, and spell check suggestions :)  How about mixed-case matches?

@@ +268,5 @@
> +            display = new SpannableString(value);
> +
> +            final int start = value.indexOf(searchTerm);
> +            if (start >= 0) {
> +                display.setSpan(new ForegroundColorSpan(SUGGESTION_HIGHLIGHT_COLOR), start, searchTerm.length(), 0);

Can ForegroundColorSpan be initialized once as static final, and then be reused here every time?

@@ +280,1 @@
>              // suggestClient is set to null in onDestroyView(), so using it

It looks like suggestClient is nullified in onDetach -- is the reasoning in this comment still valid?

@@ +288,1 @@
>              autoCompleteAdapter.update(suggestions);

Why does onLoaderReset perform a null check of autoCompleteAdapter, but this method does not?

@@ +311,5 @@
>          @Override
> +        public List<Suggestion> loadInBackground() {
> +            final List<String> values = suggestClient.query(searchTerm);
> +
> +            final List<Suggestion> suggestions = new ArrayList<Suggestion>(values.size());

It's a bit confusing that this local variable `suggestions` has the same name as the instance variable `suggestions` -- the reader may wonder whether it was an unintentional masking (which I don't think it is).

Comment 6

[Eric Edens [:eedens]]

Comment on attachment 8459792 [details] [diff] [review]
(Part 1) Remove AutoCompleteRowView

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

LGTM

Comment 7

[:Margaret Leibovic]

(In reply to Eric Edens [:eedens] from comment #5)
> Comment on attachment 8459795 [details] [diff] [review]
> (Part 2) Style search term match in search suggestions
> 
> Review of attachment 8459795 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is looking good! It definitely adds polish to the autocomplete
> experience :) There are a few items below.
>
> :::
> mobile/android/search/java/org/mozilla/search/autocomplete/SearchFragment.
> java
> @@ +51,5 @@
> >      // Maximum number of results returned by the suggestion client
> >      private static final int SUGGESTION_MAX = 5;
> >  
> > +    // Color of search term match in search suggestion
> > +    private static final int SUGGESTION_HIGHLIGHT_COLOR = 0xFF999999;
> 
> Can this go into a resource file?

Unfortunately we're using Suggestion in a static context, so we can't access resource files. I agree it would be better in a color resource file, but I don't know a way to work around this limitation.

> @@ +266,5 @@
> > +            this.value = value;
> > +
> > +            display = new SpannableString(value);
> > +
> > +            final int start = value.indexOf(searchTerm);
> 
> This addresses substring matching, and spell check suggestions :)  How about
> mixed-case matches?

Good call, I'll add support for this.

> @@ +268,5 @@
> > +            display = new SpannableString(value);
> > +
> > +            final int start = value.indexOf(searchTerm);
> > +            if (start >= 0) {
> > +                display.setSpan(new ForegroundColorSpan(SUGGESTION_HIGHLIGHT_COLOR), start, searchTerm.length(), 0);
> 
> Can ForegroundColorSpan be initialized once as static final, and then be
> reused here every time?

Good call, I'll do that.

> @@ +280,1 @@
> >              // suggestClient is set to null in onDestroyView(), so using it
> 
> It looks like suggestClient is nullified in onDetach -- is the reasoning in
> this comment still valid?

Hm... I copied this comment along with the logic I stole from BrowserSearch, where SuggestClient has a different lifecycle. So I think we can just get rid of this comment.

> @@ +288,1 @@
> >              autoCompleteAdapter.update(suggestions);
> 
> Why does onLoaderReset perform a null check of autoCompleteAdapter, but this
> method does not?

I think this was necessary when we were nullifying autoCompleteAdapter in onDestroyView, so it was possible for the view to be destroyed before onLoaderReset was called. This doesn't seem to be an issue anymore, so I'll remove this check.

> @@ +311,5 @@
> >          @Override
> > +        public List<Suggestion> loadInBackground() {
> > +            final List<String> values = suggestClient.query(searchTerm);
> > +
> > +            final List<Suggestion> suggestions = new ArrayList<Suggestion>(values.size());
> 
> It's a bit confusing that this local variable `suggestions` has the same
> name as the instance variable `suggestions` -- the reader may wonder whether
> it was an unintentional masking (which I don't think it is).

Good catch. I'll rename this local variable.

Comment 8

[:Margaret Leibovic]

Attachment: (Part 2 v2) Style search term match in search suggestions

Comment 9

[Eric Edens [:eedens]]

Comment on attachment 8459876 [details] [diff] [review]
(Part 2 v2) Style search term match in search suggestions

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

Looks good!

::: mobile/android/search/java/org/mozilla/search/autocomplete/SearchFragment.java
@@ +51,5 @@
>      // Maximum number of results returned by the suggestion client
>      private static final int SUGGESTION_MAX = 5;
>  
> +    // Color of search term match in search suggestion
> +    private static final int SUGGESTION_HIGHLIGHT_COLOR = 0xFF999999;

I think you're right -- moving this into resources might be more trouble that it's worth. How about adding a comment in the colors file that points to this?

Comment 10

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/2a75a86479ba
https://hg.mozilla.org/integration/fx-team/rev/1f7bb307f01e

I also updated the PR, so you can merge that!

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2a75a86479ba
https://hg.mozilla.org/mozilla-central/rev/1f7bb307f01e

Comment 12

[Flaviu Cos]

Verified as fixed in build 34.0a1 (2014-08-11);
Device: Samsung Galaxy Nexus (Android 4.2.1).