1033686 - Pre-search and post-search are visible at the same time

Status: RESOLVED FIXED

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

Description

[Eric Edens [:eedens]]

Attachment: Screenshot_2014-07-02-14-37-32.png

When MainActivity is restored, sometimes both the pre-search view and the post-search view are visible. 

Repro steps:
 1. Search for something
 2. Background the app
 3. Wait for Android to destroy the ap
 4. Re-open the activity

A possible cause is that although the stateBundle is null, the AutoCompleteFragment is not, and it is being re-added the already visible geckoview.

https://github.com/ericedens/FirefoxSearch/blob/master/app/src/main/java/org/mozilla/search/MainActivity.java#L42

Comment 1

[Eric Edens [:eedens]]

Attachment: bug-1033686-fix.diff

Pull Request: https://github.com/ericedens/FirefoxSearch/pull/16

MainActivity:
  Check whether fragments are created or in the tree before being
  created or added to the tree. 
 
MainActivityTest:
  Checks whether the proper fragments are visible after initial
  load, search, and having the activity recreated.



Renaming caused a lot of churn in this patch; the only substantive changes are:
  org/mozilla/search/MainActivity.java
  org/mozilla/search/tests/MainActivityTest.java

Comment 2

[Eric Edens [:eedens]]

Comment on attachment 8449903 [details] [diff] [review]
bug-1033686-fix.diff

Need to do some github cleanup first..

Comment 3

[Eric Edens [:eedens]]

Attachment: bug-1033686-fix.patch

- This is now an MC patch instead of a diff against the github repo
- Much of the changes are renaming; the substantial changes are:
     MainActivity.java
     MainActivityTest.java

Comment 4

[Eric Edens [:eedens]]

The associated pull request is https://github.com/ericedens/FirefoxSearch/pull/17

Comment 5

[Eric Edens [:eedens]]

Attachment: bug-1033686-fix.patch

MC patch built using new grunt export from https://github.com/ericedens/FirefoxSearch/pull/18

The associated PR on github is https://github.com/ericedens/FirefoxSearch/pull/17

Comment 6

[Eric Edens [:eedens]]

Attachment: bug-1033686-fix.4.patch

Hey Lucas,

I was hoping to get some of your Android expertise on this :)

There are three fragments, and they need to be hidden or shown depending on the state of the application:
1. The search bar, which is always shown; 
2. The presearch view, which is shown when you first load; and
3. The postsearch webview, which is shown after a search.

Does this seem like a sensible approach to manage the fragments? Is there a different approach you'd recommend?

The patch is huge because some files were renamed. This is the portion with the actual changes: https://github.com/ericedens/FirefoxSearch/commit/e295d8a92a153d3672094550bad173d96da8b387

Comment 7

[Lucas Rocha (:lucasr)]

(In reply to Eric Edens [:eedens] from comment #6)
> Created attachment 8453429 [details] [diff] [review]
> bug-1033686-fix.4.patch
> 
> Hey Lucas,
> 
> I was hoping to get some of your Android expertise on this :)
> 
> There are three fragments, and they need to be hidden or shown depending on
> the state of the application:
> 1. The search bar, which is always shown; 
> 2. The presearch view, which is shown when you first load; and
> 3. The postsearch webview, which is shown after a search.
> 
> Does this seem like a sensible approach to manage the fragments? Is there a
> different approach you'd recommend?

You're creating the fragments via the layout file. This means the fragments will be unconditionally created on search startup, independently of the app states described above. I recommend creating the fragments dynamically in the activity code so that you have more control over the fragment's life-cycle and their associated UI transitions, and the back stack within the app.

In case you've never done this before, have a look at the fragment transactions API:
http://developer.android.com/guide/components/fragments.html#Transactions

Comment 8

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8453429 [details] [diff] [review]
bug-1033686-fix.4.patch

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

Make sure it's fresh and bombs away!

Comment 9

[Eric Edens [:eedens]]

Attachment: bug-1033686-fix.5.patch

Fresh patch, grunt exported from https://github.com/ericedens/FirefoxSearch/pull/17

Comment 10

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/b5ce59bda9b9

Comment 11

[Carsten Book [:Tomcat]]

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

Comment 12

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8454566 [details] [diff] [review]
bug-1033686-fix.5.patch

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

A version of this has landed.