Closed Bug 1029682 Opened 7 years ago Closed 7 years ago

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

Categories

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

ARM
Android
defect
Not set
normal

Tracking

(firefox34 verified)

VERIFIED FIXED
Firefox 34
Tracking Status
firefox34 --- verified

People

(Reporter: eedens, Assigned: Margaret)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
I started cleaning up the suggestions layout, and I can work on this as part of that.
Assignee: nobody → margaret.leibovic
Simplifying things a bit.
Attachment #8459792 - Flags: review?(eedens)
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.
Attachment #8459795 - Flags: review?(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).
Attachment #8459795 - Flags: review?(eedens) → feedback+
Comment on attachment 8459792 [details] [diff] [review]
(Part 1) Remove AutoCompleteRowView

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

LGTM
Attachment #8459792 - Flags: review?(eedens) → review+
(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.
Attachment #8459795 - Attachment is obsolete: true
Attachment #8459876 - Flags: review?(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?
Attachment #8459876 - Flags: review?(eedens) → review+
https://hg.mozilla.org/mozilla-central/rev/2a75a86479ba
https://hg.mozilla.org/mozilla-central/rev/1f7bb307f01e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
See Also: → 1043624
Verified as fixed in build 34.0a1 (2014-08-11);
Device: Samsung Galaxy Nexus (Android 4.2.1).
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.