1052563 - Refine interaction for exiting editing mode

Status: RESOLVED FIXED

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

Description

[:Margaret Leibovic]

Splitting this off from bug 1045655.

Comment 1

[:Margaret Leibovic]

Attachment: Don't capture clicks to dismiss search bar

This patch removed the main view click handler, as well as the suggestions container.

Because the click handler is gone, I had to add a focus change listener to make sure to exit editing mode when the search bar loses focus.

The edit state in MainActivity still keeps track of whether or not the suggestions fragment is shown, but the fragment itself makes sure to only show the list of suggestions when there are actually items in it. Without this check, the empty suggestions fragment view would cover up whatever is underneath it, preventing the user from doing things like clicking on search history terms while the search bar is active.

This patch also addresses bug 1051983 by updating the suggestions fragment to take up the whole screen.

Comment 3

[Eric Edens [:eedens]]

Attachment: NPE - onLoaderReset

This was thrown from a local build (against fx-team).

Repro steps:
  - Launch search activity
  - Click on settings button

Comment 4

[:Margaret Leibovic]

Comment on attachment 8473372 [details] [diff] [review]
Don't capture clicks to dismiss search bar

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

::: mobile/android/search/java/org/mozilla/search/autocomplete/SuggestionsFragment.java
@@ +176,5 @@
>  
>          @Override
>          public void onLoaderReset(Loader<List<Suggestion>> loader) {
>              autoCompleteAdapter.update(null);
> +            suggestionsList.setVisibility(View.INVISIBLE);

Oh, autoCompleteAdapter and suggestionsList have different lifecycles.

I think I can just get rid of this call, since suggestionsList is destroyed in this case anyway.

Comment 5

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #4)
> Comment on attachment 8473372 [details] [diff] [review]
> Don't capture clicks to dismiss search bar
> 
> Review of attachment 8473372 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> mobile/android/search/java/org/mozilla/search/autocomplete/
> SuggestionsFragment.java
> @@ +176,5 @@
> >  
> >          @Override
> >          public void onLoaderReset(Loader<List<Suggestion>> loader) {
> >              autoCompleteAdapter.update(null);
> > +            suggestionsList.setVisibility(View.INVISIBLE);
> 
> Oh, autoCompleteAdapter and suggestionsList have different lifecycles.
> 
> I think I can just get rid of this call, since suggestionsList is destroyed
> in this case anyway.

Actually, this is wrong. In fact, calling autoCompleteAdapter.update(null) is also wrong. onLoaderReset is just called when the loader wants to release its data. In other places in the code where we use CursorLoaders, we need to clean up references to the Cursor, but in our case, we're not actually holding onto the loader data anywhere, so we can remove the entire body of this method.

Comment 6

[:Margaret Leibovic]

Attachment: Don't capture clicks to dismiss search bar

Updated to address loader issue.

Comment 7

[Eric Edens [:eedens]]

Comment on attachment 8473966 [details] [diff] [review]
Don't capture clicks to dismiss search bar

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

LGTM

Comment 8

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/35343a2e0abd
https://github.com/mozilla/fennec-search/commit/d975f187ccccce07bced660df1c3ebf72bf842d9

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/35343a2e0abd