1046485 - Move search bar out of SearchFragment

Status: VERIFIED FIXED

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

Description

[:Margaret Leibovic]

I started exploring this refactor to see if it would make our transitions easier.

Comment 1

[:Margaret Leibovic]

WIP here: https://github.com/ericedens/FirefoxSearch/pull/43

The main reason I started down this road was to try to deactivate the edit text before kicking off the search card transition, but I think this overall makes the transition logic less fragile (e.g. I could get rid of the onAnimationEnd listener).

Comment 2

[:Margaret Leibovic]

Attachment: Move search bar out of SearchFragment

As a follow-up, I think we should revisit how we're using fragments, since this patch basically makes an assumption that the fragments will not be unloaded while the main activity is running.

Also, I'll file a bug about using a ViewSwitcher for the main view area.

Comment 3

[Eric Edens [:eedens]]

Comment on attachment 8467418 [details] [diff] [review]
Move search bar out of SearchFragment

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

I like the overall structure!

I don't know how to benchmark this, but I think this introduced a regression in the search-suggestion animation. The animation from history->results appears unchanged, however the animation from suggestions->results has a stutter about halfway through.

From comparing nightly to this patch, I *think* this is related to hiding the keyboard. On nightly, the text animates, and then the keyboard hides. In this version, the the text animates while the keyboard is hiding.

::: mobile/android/search/java/org/mozilla/search/MainActivity.java
@@ +80,5 @@
>  
>          queryHandler = new AsyncQueryHandler(getContentResolver()) {};
>  
> +        // Dismiss edit mode when the user taps outside of the suggestions.
> +        findViewById(R.id.search_activity_main).setOnClickListener(new View.OnClickListener() {

Clicking on the background isn't dismissing suggestions on my local build. :/

@@ +271,5 @@
> +        suggestions.setVisibility(editState == EditState.EDITING ? View.VISIBLE : View.INVISIBLE);
> +
> +        // Cache a reference to the suggestions fragment when entering editing mode.
> +        suggestionsFragment = (editState != EditState.EDITING) ? null :
> +                (SuggestionsFragment) getSupportFragmentManager().findFragmentById(R.id.suggestions);

Can this assignment be moved into onCreate so that it only happens once?

::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
@@ +80,5 @@
>      private void setUrl(String url) {
>          // Only load URLs if they're different than what's already
>          // loaded in the webview.
>          if (!TextUtils.equals(webview.getUrl(), url)) {
> +            webview.loadUrl("about:blank");

Rebase this against https://bugzil.la/1043522

::: mobile/android/search/res/layout/search_activity_main.xml
@@ +1,5 @@
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
> +<FrameLayout

I think we'll want to keep the merge.

From my understanding, inflating this in an activity will give us a root FrameLayout for free; therefore, by using merge, we avoid having a redundant FrameLayout in the view hierarchy. You can grab that root FrameLayout with:

findViewById(android.R.id.content)

Ref: http://android-developers.blogspot.com/2009/03/android-layout-tricks-3-optimize-by.html

@@ +16,5 @@
> +        android:layout_height="@dimen/search_bar_height"
> +        android:paddingTop="10dp"
> +        android:paddingBottom="10dp"
> +        android:paddingLeft="@dimen/card_background_padding_x"
> +        android:paddingRight="@dimen/card_background_padding_x"/>

For consistency, I'd recommend having all of the padding values either be inline, or put into a resource file.

@@ +41,2 @@
>          android:layout_width="match_parent"
> +        android:layout_height="wrap_content"

When there are a small number of suggestions (eg: 1 or 2), you can also see the search history items. I don't think this is intended?

::: mobile/android/search/res/layout/search_auto_complete.xml
@@ +1,5 @@
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
> +<ListView

Perhaps rename this file to indicate that it's only doing suggestions now?

Comment 4

[:Margaret Leibovic]

(In reply to Eric Edens [:eedens] from comment #3)
> Comment on attachment 8467418 [details] [diff] [review]
> Move search bar out of SearchFragment
> 
> Review of attachment 8467418 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like the overall structure!
> 
> I don't know how to benchmark this, but I think this introduced a regression
> in the search-suggestion animation. The animation from history->results
> appears unchanged, however the animation from suggestions->results has a
> stutter about halfway through.
> 
> From comparing nightly to this patch, I *think* this is related to hiding
> the keyboard. On nightly, the text animates, and then the keyboard hides. In
> this version, the the text animates while the keyboard is hiding.

Gr, this is frustrating, but good catch. This was an intentional change I made, since the animation as designed is supposed to deactivate the search bar immediately, then expand the card/float the suggestion. I'll just revert this for now, and we can refine the animation in a separate bug.

> @@ +271,5 @@
> > +        suggestions.setVisibility(editState == EditState.EDITING ? View.VISIBLE : View.INVISIBLE);
> > +
> > +        // Cache a reference to the suggestions fragment when entering editing mode.
> > +        suggestionsFragment = (editState != EditState.EDITING) ? null :
> > +                (SuggestionsFragment) getSupportFragmentManager().findFragmentById(R.id.suggestions);
> 
> Can this assignment be moved into onCreate so that it only happens once?

The reason I did this is to avoid holding onto a reference to the fragment for longer than we need to. However, since we're already holding onto references to views inside the fragment, we might as well just move this into onCreate.

This goes back to what I said before about how we're essentially treating these fragments as views, since we're never dynamically attaching/detaching them. But we can revisit this issue at some other point.

> ::: mobile/android/search/res/layout/search_activity_main.xml
> @@ +1,5 @@
> >  <!-- This Source Code Form is subject to the terms of the Mozilla Public
> >     - License, v. 2.0. If a copy of the MPL was not distributed with this
> >     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> >  
> > +<FrameLayout
> 
> I think we'll want to keep the merge.
> 
> From my understanding, inflating this in an activity will give us a root
> FrameLayout for free; therefore, by using merge, we avoid having a redundant
> FrameLayout in the view hierarchy. You can grab that root FrameLayout with:
> 
> findViewById(android.R.id.content)
> 
> Ref:
> http://android-developers.blogspot.com/2009/03/android-layout-tricks-3-
> optimize-by.html

I added the FrameLayout because I needed an id for the view in order to add the click listener. However, given some of the other problems we're experiencing, I might need to add a separate "backdrop" view like we used to have.

Comment 5

[:Margaret Leibovic]

Attachment: (v2) Move search bar out of SearchFragment

Here's a patch that addresses your feedback.

I don't really like that I had to wrap the suggestions dropdown in a separate view to capture clicks outside of the suggsestions, but I couldn't think of a better way to do this. The reason that a click handler on the root view didn't work is because the PreSearchFragment actually takes up the whole height of the screen.

Apologies for the diff not using hg rename... I think that's an unfortunate part of our grunt export flow.

Comment 6

[Eric Edens [:eedens]]

Comment on attachment 8468017 [details] [diff] [review]
(v2) Move search bar out of SearchFragment

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

Looks good! One comment plus:

- Apply fix for margin on gingerbread

::: mobile/android/search/java/org/mozilla/search/MainActivity.java
@@ +132,5 @@
>          super.onDestroy();
>          queryHandler = null;
> +        editText = null;
> +        preSearch = null;
> +        postSearch = null;

suggestionsContainer is also init'd in onCreate

Comment 7

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/abedc0067813
https://github.com/ericedens/FirefoxSearch/commit/59c99e79073619bbf2af8c7b070485a4b661d174

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/abedc0067813

Comment 9

[Flaviu Cos]

Verified as fixed in build 34.0a1 (2014-08-07);
Devices: Samsung Galaxy R (Android 2.3.4) and Samsung Galaxy Nexus (Android 4.2.1)