Closed Bug 1033686 Opened 5 years ago Closed 5 years ago

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

Categories

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

All
Android
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 33

People

(Reporter: eedens, Assigned: eedens)

References

Details

Attachments

(1 file, 5 obsolete files)

Attached image Screenshot_2014-07-02-14-37-32.png (obsolete) —
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
Blocks: search
Attached patch bug-1033686-fix.diff (obsolete) — Splinter Review
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
Attachment #8449903 - Flags: review?(wjohnston)
Assignee: nobody → eedens
Comment on attachment 8449903 [details] [diff] [review]
bug-1033686-fix.diff

Need to do some github cleanup first..
Attachment #8449903 - Flags: review?(wjohnston)
Attached patch bug-1033686-fix.patch (obsolete) — Splinter Review
- 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
Attachment #8449903 - Attachment is obsolete: true
Attachment #8451755 - Flags: review?(nalexander)
The associated pull request is https://github.com/ericedens/FirefoxSearch/pull/17
Attached patch bug-1033686-fix.patch (obsolete) — Splinter Review
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
Attachment #8449756 - Attachment is obsolete: true
Attachment #8452542 - Flags: review?(nalexander)
Attachment #8451755 - Attachment is obsolete: true
Attachment #8451755 - Flags: review?(nalexander)
Attached patch bug-1033686-fix.4.patch (obsolete) — Splinter Review
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
Attachment #8452542 - Attachment is obsolete: true
Attachment #8452542 - Flags: review?(nalexander)
Attachment #8453429 - Flags: feedback?(lucasr.at.mozilla)
(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
Attachment #8453429 - Flags: feedback?(lucasr.at.mozilla)
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!
Attachment #8453429 - Flags: review+
Fresh patch, grunt exported from https://github.com/ericedens/FirefoxSearch/pull/17
Attachment #8453429 - Attachment is obsolete: true
Attachment #8454566 - Flags: review?(nalexander)
https://hg.mozilla.org/mozilla-central/rev/b5ce59bda9b9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
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.
Attachment #8454566 - Flags: review?(nalexander)
Blocks: 1040416
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.