If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Restore opt-in search suggestions UI

RESOLVED FIXED in Firefox 26

Status

()

Firefox for Android
Awesomescreen
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: lucasr, Assigned: bnicholson)

Tracking

unspecified
Firefox 26
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: abouthome-hackathon, fixed-fig)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
This hasn't been re-implement in the new about:home code yet.
(Assignee)

Comment 1

4 years ago
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
(Assignee)

Comment 2

4 years ago
(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/

Updated

4 years ago
Priority: -- → P1
(Reporter)

Updated

4 years ago
Whiteboard: abouthome-hackathon
(Reporter)

Updated

4 years ago
Assignee: lucasr.at.mozilla → nobody
(Assignee)

Updated

4 years ago
Assignee: nobody → bnicholson
Status: NEW → ASSIGNED
(Assignee)

Comment 3

4 years ago
Created attachment 780221 [details] [diff] [review]
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).
Attachment #780221 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 4

4 years ago
Created attachment 780227 [details] [diff] [review]
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.
Attachment #780221 - Attachment is obsolete: true
Attachment #780221 - Flags: review?(lucasr.at.mozilla)
Attachment #780227 - Flags: review?(lucasr.at.mozilla)
(Reporter)

Comment 5

4 years ago
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?
Attachment #780227 - Flags: review?(lucasr.at.mozilla) → review-
(Assignee)

Comment 6

4 years ago
Created attachment 780584 [details] [diff] [review]
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.
Attachment #780227 - Attachment is obsolete: true
Attachment #780584 - Flags: review?(lucasr.at.mozilla)
(Reporter)

Comment 7

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

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

Excellent.
Attachment #780584 - Flags: review?(lucasr.at.mozilla) → review+
(Assignee)

Comment 8

4 years ago
http://hg.mozilla.org/projects/fig/rev/2097a11d8902
Whiteboard: abouthome-hackathon → abouthome-hackathon, fixed-fig
(Assignee)

Updated

4 years ago
Depends on: 898219

Updated

4 years ago
Depends on: 898424

Comment 9

4 years ago
https://hg.mozilla.org/mozilla-central/rev/2097a11d8902
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.