1210534 - Search suggestions are not refreshed after single character entered into search field

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Sergej Kravcenko]

Steps to reproduce.

1. Press address bar on home screen
2. Enter any single letter and observe search results based on that letter
3. Hit backspace and delete that letter
4. Enter any other single letter and observe same search suggestions from step 2

Expected: Search suggestions refresh.

Comment 1

[:Margaret Leibovic]

Thanks for reporting. Sergej, do you want to look into writing a patch for this?

Comment 2

[Sergej Kravcenko]

I'll take a look

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I expect this is another bug 1186683 regression.

Comment 4

[Sergej Kravcenko]

Attachment: MozReview Request: Bug 1210534 - Search suggestions are not refreshed after single character entered into search field

Bug 1210534 - Search suggestions are not refreshed after single character entered into search field

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Sergej, sorry for the delay – I'll get to this tomorrow.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8670339 [details]
MozReview Request: Bug 1210534 - Search suggestions are not refreshed after single character entered into search field

https://reviewboard.mozilla.org/r/21361/#review19489

I investigated and found the issue to be `BrowserSearch.filter` is called as expected but `isVisible()` [1] unexpectedly returns false (presumably because the `showBrowserSearch` call hasn't had time to propagate yet.

Your solution seems overly complex given the analysis above – is it possible to do something simpler? Maybe we can recall `BrowserSearch.filter` with the previously saved `mSearchTerm` in `onHiddenChanged` (assuming `add` and the other requirements for `isVisible` have taken affect). Or we can add a different condition to replace `isVisible`.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BrowserSearch.java?rev=aa701b608df5#795

Comment 7

[Sergej Kravcenko]

Comment on attachment 8670339 [details]
MozReview Request: Bug 1210534 - Search suggestions are not refreshed after single character entered into search field

Bug 1210534 - Search suggestions are not refreshed after single character entered into search field

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8670339 [details]
MozReview Request: Bug 1210534 - Search suggestions are not refreshed after single character entered into search field

https://reviewboard.mozilla.org/r/21361/#review19769

::: mobile/android/base/home/BrowserSearch.java:195
(Diff revision 2)
> +        mCursorLoaderCallbacks = new CursorLoaderCallbacks();

Nice cleanup!

::: mobile/android/base/home/BrowserSearch.java:798
(Diff revision 2)
> -        if (isVisible()) {
> +        if (mAdapter != null) {

I find just this change fixes my issues – what problems do the other changes solve?

Comment 9

[Sergej Kravcenko]

It solves initial load when fragment is created. Now it's working because on first load, fragment requests search engines from gecko and resend query. But Bug 927692 will fix this, and there is possibility that filter method will be called before adapter is available.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I see. In order to prevent confusion, we generally try to scope different fixes/features into new bugs (e.g. bug 927692) with the exception to small cleanups (e.g. whitespace & readability). Unfortunately, someone else is assigned to the preload search engines bug so we should probably let them work through it. It's worth noting that your solution looks quite elegant, however. :)

Do you think you can trim down this patch to just refresh the search engines after a single character is typed?

Comment 11

[Sergej Kravcenko]

Comment on attachment 8670339 [details]
MozReview Request: Bug 1210534 - Search suggestions are not refreshed after single character entered into search field

Bug 1210534 - Search suggestions are not refreshed after single character entered into search field

Comment 12

[Sergej Kravcenko]

Here you go. I think we must leave comment in bug 927692, to warn that adapter can be null, when fragment is created and filter method runs first before onActivityCreated.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8670339 [details]
MozReview Request: Bug 1210534 - Search suggestions are not refreshed after single character entered into search field

https://reviewboard.mozilla.org/r/21361/#review21107

Sorry for the delay on this Sergej – this looks good to me.

Merge got moved to tomorrow so do you mind flagging this for uplift to aurora?

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Sergej Kravcenko from comment #12)
> Here you go. I think we must leave comment in bug 927692, to warn that
> adapter can be null, when fragment is created and filter method runs first
> before onActivityCreated.

Would you like to go ahead and do this? It might be good to add a comment somewhere for this too.

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/0930afdb8a61cd130ffe7d6276d2cbd0c1c2ca4d
Bug 1210534 - Search suggestions are not refreshed after single character entered into search field

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Didn't realize the push in comment 15 didn't have an `r=` – I reviewed it.

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8670339 [details]
MozReview Request: Bug 1210534 - Search suggestions are not refreshed after single character entered into search field

Approval Request Comment
[Feature/regressing bug #]: bug 1186683.
[User impact if declined]:
  Users who open the toolbar search, hide the search screen, and enter a single character to show it again will get old results.

[Describe test coverage new/current, TreeHerder]: Tested locally.
[Risks and why]: Low. Before we updated the search screen only when it was visible – now we do it as long as the memory we need to access is not null.

[String/UUID change made/needed]: None

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0930afdb8a61

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/0930afdb8a61

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8670339 [details]
MozReview Request: Bug 1210534 - Search suggestions are not refreshed after single character entered into search field

This seems like a safe fix, let's uplift to Aurora44.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/75355c970c73

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/75355c970c73

Comment 23

[Teodora Vermesan (:TeoVermesan)]

After you enter a single letter, delete it and enter another single letter, search suggestions are refreshed.

Verified as fixed using:
Device: Xiaomi Mi i4 (Android 5.0.2)
Build: Firefox for Android 45.0a1 (2015-11-11)

Comment 24

[Teodora Vermesan (:TeoVermesan)]

After you enter a single letter, delete it and enter another single letter, search suggestions are refreshed.

Verified as fixed using:
Device: Moto X (Android 4.4)
Build: Firefox for Android 44.0a2 (2015-11-23)