882185 - Restore opt-in search suggestions UI

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P1)

Description

[Lucas Rocha (:lucasr)]

This hasn't been re-implement in the new about:home code yet.

Comment 1

[Brian Nicholson (:bnicholson)]

Just a remember to also restore the animations with the prompt. The three animations that should be working are:

1) The prompt slides off the screen to the right after the user selects yes or no
2) When the suggestions appear, they should fade in one at a time from the left
3) If the size of the row changes as the user types, there should be an animation of the row resizing

Comment 2

[Brian Nicholson (:bnicholson)]

(In reply to Brian Nicholson (:bnicholson) from comment #1)
> Just a remember to also restore the animations with the prompt. The three

s/remember/reminder/

Comment 3

[Brian Nicholson (:bnicholson)]

Attachment: Restore opt-in search suggestions UI

As usual, there's a couple of non-obvious changes that I think need explaining. The first is that I moved the SearchEngines:Get message from onCreate() to onCreateView(). The reason is to avoid a race between the SearchEngines:Data response and the view being created. After we get the search engine data, we then show a prompt, and showing the prompt requires the fragment's view to exist. This simplest way to do that seemed to be to just make sure things happen in a deterministic order.

The second change is the fact that the list of search engines are no longer cached, and the SearchEngines:Data is retrieved every time the fragment is shown (like we do in the current Nightly). I think we should do this is because search engines can change in Gecko if we add/remove/disable them, and it would require extra work to keep them in sync. Caching the engines can also lead to subtle bugs -- for example, there's currently a bug in fig where private tabs will show search suggestions after they've been shown in a normal tab. Or if the user changes their search suggestion enabled pref, it won't work since the pref isn't refetched. And, for this patch in particular, we'd have even more complexity since we'd need to maintain more state; for example, we'd need something like a mShouldShowPrompt to store the result of the JSON "prompted" boolean (since we may show the prompt after multiple fragment reattachments, but the value is only fetched once). Fetching the search engines every time and nulling out these fields on destroy helps to avoid all of these issues (and reduces memory consumption).

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: Restore opt-in search suggestions UI, v2

Made mSuggestClient volatile and fixed a race that could lead to a NPE when using mSuggestClient on the background thread.

Comment 5

[Lucas Rocha (:lucasr)]

Comment on attachment 780227 [details] [diff] [review]
Restore opt-in search suggestions UI, v2

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

Looks great. Just want to see questions answered and suggested fixes in before giving r+.

::: mobile/android/base/home/BrowserSearch.java
@@ +192,5 @@
>          // If the style of the list changes, inflate it from an XML.
> +        mView = (LinearLayout) inflater.inflate(R.layout.browser_search, container, false);
> +        mList = (ListView) mView.findViewById(R.id.home_list_view);
> +
> +        registerEventListener("SearchEngines:Data");

We've been doing this kind of thing (i.e. setting up view-related bits) in onViewCreated(). I'd these search engine bits onViewCreated() for consistency. Not a big deal though.

@@ +257,5 @@
>          if (event.equals("SearchEngines:Data")) {
>              ThreadUtils.postToUiThread(new Runnable() {
>                  @Override
>                  public void run() {
> +                    if (mView != null) {

Maybe this check belongs to setSearchEngines? Add it as an early return in there.

@@ +401,5 @@
> +        TextView promptText = (TextView) mSuggestionsOptInPrompt.findViewById(R.id.suggestions_prompt_title);
> +        promptText.setText(getResources().getString(R.string.suggestions_prompt, mSearchEngines.get(0).name));
> +
> +        final View yesButton = mSuggestionsOptInPrompt.findViewById(R.id.suggestions_prompt_yes);
> +        final View noButton = mSuggestionsOptInPrompt.findViewById(R.id.suggestions_prompt_no);

nit: add empty line here.

@@ +402,5 @@
> +        promptText.setText(getResources().getString(R.string.suggestions_prompt, mSearchEngines.get(0).name));
> +
> +        final View yesButton = mSuggestionsOptInPrompt.findViewById(R.id.suggestions_prompt_yes);
> +        final View noButton = mSuggestionsOptInPrompt.findViewById(R.id.suggestions_prompt_no);
> +        OnClickListener listener = new OnClickListener() {

final

@@ +411,5 @@
> +                noButton.setOnClickListener(null);
> +
> +                setSuggestionsEnabled(v == yesButton);
> +            }
> +        };

nit: add empty line here.

@@ +434,5 @@
> +                if (client != null) {
> +                    client.query(mSearchTerm);
> +                }
> +            }
> +        });

Why not simply calling filterSuggestions() here? It would use the same code path through the AsyncLoader. It's also safer in terms of fragment life cycle.

@@ +437,5 @@
> +            }
> +        });
> +
> +        // Pref observer in gecko will also set prompted = true
> +        PrefsHelper.setPref("browser.search.suggest.enabled", enabled);

I assume this is safe to call in the UI thread.

@@ +655,5 @@
>  
>                  row.setSearchTerm(mSearchTerm);
>  
>                  final SearchEngine engine = mSearchEngines.get(getEngineIndex(position));
> +                final boolean doAnimation = (mAnimateSuggestions && engine.suggestions.size() > 0);

Ditto. Rename to 'animate'.

@@ +657,5 @@
>  
>                  final SearchEngine engine = mSearchEngines.get(getEngineIndex(position));
> +                final boolean doAnimation = (mAnimateSuggestions && engine.suggestions.size() > 0);
> +                row.updateFromSearchEngine(engine, doAnimation);
> +                if (doAnimation) {

I guess it's safe to always set mAnimateSuggestions to false after updateFromSearchEngine is called. No need to check if it's true.

::: mobile/android/base/home/SearchEngineRow.java
@@ +132,5 @@
>      public void setOnEditSuggestionListener(OnEditSuggestionListener listener) {
>          mEditSuggestionListener = listener;
>      }
>  
> +    public void updateFromSearchEngine(SearchEngine searchEngine, boolean doAnimation) {

Maybe 'animate' instead of 'doAnimation' sounds simpler?

Comment 6

[Brian Nicholson (:bnicholson)]

Attachment: Restore opt-in search suggestions UI, v3

(In reply to Lucas Rocha (:lucasr) from comment #5)
> @@ +434,5 @@
> > +                if (client != null) {
> > +                    client.query(mSearchTerm);
> > +                }
> > +            }
> > +        });
> 
> Why not simply calling filterSuggestions() here? It would use the same code
> path through the AsyncLoader. It's also safer in terms of fragment life
> cycle.

That's a good idea, but mSuggestionsEnabled is false until the animation has finished, so filterSuggestions() would do nothing. Even if we fixed that, I think we'll have an issue with the callbacks used in filterSuggestions(). When the query completes, it calls setSuggestions(), and that does a notifyDataSetChanged() which refreshes the ListView. That would disrupt the fade-in animations.

> @@ +437,5 @@
> > +            }
> > +        });
> > +
> > +        // Pref observer in gecko will also set prompted = true
> > +        PrefsHelper.setPref("browser.search.suggest.enabled", enabled);
> 
> I assume this is safe to call in the UI thread.

If you mean thread-safe, I think it's fine since setPref() is a static method and doesn't act on any instance variables. If you mean efficient, that method only does some very minor JSON building before sending a message to Gecko, so I don't think that's an issue either.

> @@ +657,5 @@
> >  
> >                  final SearchEngine engine = mSearchEngines.get(getEngineIndex(position));
> > +                final boolean doAnimation = (mAnimateSuggestions && engine.suggestions.size() > 0);
> > +                row.updateFromSearchEngine(engine, doAnimation);
> > +                if (doAnimation) {
> 
> I guess it's safe to always set mAnimateSuggestions to false after
> updateFromSearchEngine is called. No need to check if it's true.

Note that doAnimation is true only if mAnimateSuggestions is true *and* engine.suggestions.size() > 0. It's possible for mAnimateSuggestions to be true if we just opted in, but we may not have received any suggestions yet for our query. I don't think we'd want to set mAnimateSuggestions to false in that case since we'd lose the animation whenever the suggestions finally come in.

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 780584 [details] [diff] [review]
Restore opt-in search suggestions UI, v3

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

Excellent.

Comment 8

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/projects/fig/rev/2097a11d8902

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2097a11d8902